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

Krzysztof Kozlowski
2024-04-03T14:24:55Z

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