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

[Krzysztof Kozlowski <krzk@kernel.org>]

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