Re: [RFC PATCH 4/6] regulator: bd96801: ROHM BD96801 PMIC regulators

Matti Vaittinen
2024-04-03T07:38:39Z

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! ~~