Messages in this thread | | | Date | Tue, 2 Apr 2024 18:14:33 +0200 | Subject | Re: [RFC PATCH 4/6] regulator: bd96801: ROHM BD96801 PMIC regulators | From | Krzysztof Kozlowski <> |
| |
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
| |