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

[Krzysztof Kozlowski]

On 03/04/2024 09:38, Matti Vaittinen wrote:
> 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.

Second chance, I hope, will be Vienna in September.


..

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

Fine with me.

Best regards,
Krzysztof