Messages in this thread | | | Date | Wed, 3 Apr 2024 10:38:36 +0300 | Subject | Re: [RFC PATCH 4/6] regulator: bd96801: ROHM BD96801 PMIC regulators | From | Matti Vaittinen <> |
| |
Hi dee Ho Krzysztof,
Heading to the Seattle? If so - Enjoy! It's a bummer I'm not able to share a beer with you in ELC this time.
On 4/2/24 19:14, Krzysztof Kozlowski wrote: > On 02/04/2024 15:10, Matti Vaittinen wrote: >> The ROHM BD96801 "Scalable PMIC" is an automotive grade PMIC which can >> scale to different applications by allowing chaining of PMICs. The PMIC >> also supports various protection features which can be configured either >> to fire IRQs - or to shut down power outputs when failure is detected. >> > > ... > >> + >> +static int initialize_pmic_data(struct device *dev, >> + struct bd96801_pmic_data *pdata) >> +{ >> + int r, i; >> + >> + *pdata = bd96801_data; >> + >> + /* >> + * Allocate and initialize IRQ data for all of the regulators. We >> + * wish to modify IRQ information independently for each driver >> + * instance. >> + */ >> + for (r = 0; r < BD96801_NUM_REGULATORS; r++) { >> + const struct bd96801_irqinfo *template; >> + struct bd96801_irqinfo *new; >> + int num_infos; >> + >> + template = pdata->regulator_data[r].irq_desc.irqinfo; >> + num_infos = pdata->regulator_data[r].irq_desc.num_irqs; >> + >> + new = devm_kzalloc(dev, num_infos * sizeof(*new), GFP_KERNEL); > > Aren't you open coding devm_kcalloc?
I think yes. Thanks.
>> + if (!new) >> + return -ENOMEM; >> + >> + pdata->regulator_data[r].irq_desc.irqinfo = new; >> + >> + for (i = 0; i < num_infos; i++) >> + new[i] = template[i]; >> + } >> + >> + return 0; >> +} >> + > > > ... > >> +static int bd96801_probe(struct platform_device *pdev) >> +{ >> + struct device *parent; >> + int i, ret, irq; >> + void *retp; >> + struct regulator_config config = {}; >> + struct bd96801_regulator_data *rdesc; >> + struct bd96801_pmic_data *pdata; >> + struct regulator_dev *ldo_errs_rdev_arr[BD96801_NUM_LDOS]; >> + int ldo_errs_arr[BD96801_NUM_LDOS]; >> + int temp_notif_ldos = 0; >> + struct regulator_dev *all_rdevs[BD96801_NUM_REGULATORS]; >> + bool use_errb; >> + >> + parent = pdev->dev.parent; >> + >> + pdata = devm_kzalloc(&pdev->dev, sizeof(bd96801_data), GFP_KERNEL); > > This and assignment in initialize_pmic_data() could be probably > devm_kmemdup() which would be a bit more obvious for the reader.
I think you're right.
>> + if (!pdata) >> + return -ENOMEM; >> + >> + if (initialize_pmic_data(&pdev->dev, pdata)) >> + return -ENOMEM; >> + >> + pdata->regmap = dev_get_regmap(parent, NULL); >> + if (!pdata->regmap) { >> + dev_err(&pdev->dev, "No register map found\n"); >> + return -ENODEV; >> + } >> + >> + rdesc = &pdata->regulator_data[0]; >> + >> + config.driver_data = pdata; >> + config.regmap = pdata->regmap; >> + config.dev = parent; >> + >> + ret = of_property_match_string(pdev->dev.parent->of_node, >> + "interrupt-names", "errb"); > This does not guarantee that interrupts are properly set up.
Hmm. Yes, you're right. I'm not sure if I did think of this.
> Don't you > have some state shared between parent and this device where you could > mark that interrupts are OK?
There is currently no need to share/allocate any private data from the MFD. We get the regmap using dev_get_regmap, and interrupts using the platform_get_irq_byname(). Nothing else is shared between the MFD and sub-devices.
Considering the use of platform_get_irq_byname() - and how failures to get 'errb' IRQs are silently ignored in bd96801_global_errb_irqs() and in bd96801_rdev_errb_irqs() - this check is just a slight optimization to not even try registering the errb IRQs if they're not found from the device tree. So, I think things do not really go south even if we go to "errb route" when the "errb" IRQs aren't successfully registered.
Whether this warrants a comment, or if this check is just unnecessarily complex can be pondered. Personally I think the purpose is pretty clear and thus the complexity is not added that much - but yes, a comment above call(s) to the platform_get_irq_byname() saying errb IRQs are not guaranteed to be populated might be justified.
> >> + if (ret < 0) >> + use_errb = false; >> + else >> + use_errb = true; >> + > > ... > >> + >> + if (use_errb) >> + return bd96801_global_errb_irqs(pdev, all_rdevs, >> + ARRAY_SIZE(all_rdevs)); >> + >> + return 0; >> +} >> + >> +static struct platform_driver bd96801_regulator = { >> + .driver = { >> + .name = "bd96801-pmic" >> + }, >> + .probe = bd96801_probe, >> +}; >> + >> +module_platform_driver(bd96801_regulator); >> + >> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); >> +MODULE_DESCRIPTION("BD96801 voltage regulator driver"); >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("platform:bd96801-pmic"); > > Just add platform device ID table with MODULE_DEVICE_TABLE(). You should > not need MODULE_ALIAS() in normal cases. MODULE_ALIAS() is not a > substitute for incomplete ID table.
I guess I have something to learn here. Thanks. :)
Take care -- Matti
-- Matti Vaittinen Linux kernel developer at ROHM Semiconductors Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
| |