Message-ID: <e0d760d7-5408-47b8-b204-7900bcd9c608@gmail.com>
Date: Wed, 3 Apr 2024 10:38:36 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Lee Jones <lee@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 4/6] regulator: bd96801: ROHM BD96801 PMIC regulators

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

Powered by blists - more mailing lists