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

[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@xxxxxxxxxxxxxxxxx>");
> > +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! ~~