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

Krzysztof Kozlowski
2024-04-02T16:14:39Z

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?

> + 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.

> + 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. Don't you
have some state shared between parent and this device where you could
mark that interrupts are OK?

> + 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.

Best regards,
Krzysztof