Date: Fri, 14 Jun 2024 11:01:52 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Wim Van Sebroeck <wim@...ux-watchdog.org>, Guenter Roeck
 <linux@...ck-us.net>, Thomas Gleixner <tglx@...utronix.de>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-watchdog@...r.kernel.org
Subject: Re: [PATCH v3 09/10] mfd: bd96801: Add ERRB IRQ

On 6/14/24 10:50, Lee Jones wrote:
> On Fri, 14 Jun 2024, Matti Vaittinen wrote:
> 
>> On 6/13/24 19:32, Lee Jones wrote:
>>> On Tue, 04 Jun 2024, Matti Vaittinen wrote:
>>>
>>>> The ROHM BD96801 "scalable PMIC" provides two physical IRQs. The ERRB
>>>> handling can in many cases be omitted because it is used to inform fatal
>>>> IRQs, which usually kill the power from the SOC.
>>>>
>>>> There may however be use-cases where the SOC has a 'back-up' emergency
>>>> power source which allows some very short time of operation to try to
>>>> gracefully shut down sensitive hardware. Furthermore, it is possible the
>>>> processor controlling the PMIC is not powered by the PMIC. In such cases
>>>> handling the ERRB IRQs may be beneficial.
>>>>
>>>> Add support for ERRB IRQs.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>>>> ---
>>>> Revision history:
>>>> v2 =>:
>>>> 	- No changes
>>>> v1 => v2:
>>>> 	- New patch
>>>> ---
>>>>    drivers/mfd/rohm-bd96801.c | 291 ++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 253 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
>>>> index 1c2a9591be7b..b7f073318873 100644
>>>> --- a/drivers/mfd/rohm-bd96801.c
>>>> +++ b/drivers/mfd/rohm-bd96801.c
>>>> @@ -5,13 +5,9 @@
>>>>     * ROHM BD96801 PMIC driver
>>>>     *
>>>>     * This version of the "BD86801 scalable PMIC"'s driver supports only very
>>>> - * basic set of the PMIC features. Most notably, there is no support for
>>>> - * the ERRB interrupt and the configurations which should be done when the
>>>> - * PMIC is in STBY mode.
>>>> - *
>>>> - * Supporting the ERRB interrupt would require dropping the regmap-IRQ
>>>> - * usage or working around (or accepting a presense of) a naming conflict
>>>> - * in debugFS IRQs.
>>>
>>> Why bother adding all that blurb in the first place?
>>
>> Because, I assume there are users who would like to have the ERRB in use.
>> The main purpose of this comment is that any such users could
>> 	a) see this version does not support ERRB.
>> 	b) can find the original RFC with ERRB supportn and a workaround.
>> 	c) know why this version does not work with ERRB and thus fix this
>>
>> It seems this ERRB support may be missing from upstream for a while, hence I
>> think having this note is worthy until (if) this ERRB patch lands in
>> upstream.
> 
> What I mean is - you're adding all of these extra lines in patch 3 and
> removing them in patch 9.
> 

True. This is because I had a feeling the irqdomain changes might not 
get merged that fast as it seemed like something that is not completely 
trivial. This comment is useful if patches 7-10 aren't merged together 
with 1-6 - which I now also hope is the case XD

>>>> + * basic set of the PMIC features.
>>>> + * Most notably, there is no support for the configurations which should
>>>> + * be done when the PMIC is in STBY mode.
>>>>     *
>>>>     * Being able to reliably do the configurations like changing the
>>>>     * regulator safety limits (like limits for the over/under -voltages, over
>>>> @@ -23,16 +19,14 @@
>>>>     * be the need to configure these safety limits. Hence it's not simple to
>>>>     * come up with a generic solution.
>>>>     *
>>>> - * Users who require the ERRB handling and STBY state configurations can
>>>> - * have a look at the original RFC:
>>>> + * Users who require the STBY state configurations can  have a look at the
>>>> + * original RFC:
>>>>     * https://lore.kernel.org/all/cover.1712920132.git.mazziesaccount@gmail.com/
>>>> - * which implements a workaround to debugFS naming conflict and some of
>>>> - * the safety limit configurations - but leaves the state change handling
>>>> - * and synchronization to be implemented.
>>>> + * which implements some of the safety limit configurations - but leaves the
>>>> + * state change handling and synchronization to be implemented.
>>>>     *
>>>>     * It would be great to hear (and receive a patch!) if you implement the
>>>> - * STBY configuration support or a proper fix to the debugFS naming
>>>> - * conflict in your downstream driver ;)
>>>> + * STBY configuration support or a proper fix in your downstream driver ;)
>>>>     */
>>
>> ...
>>
>> Thanks for comments Lee. Reworking this will have to wait for the irqdomain
>> name suffix, which I will continue after Hervé has done his part of the
>> irqdomain changes. I will omit this patch from the next re-spin of the
>> series.
> 
> I'm in no rush. :)

Well, glad to hear ;) I still usually try to avoid delaying sending the 
follow-up patches. I am under impression it is easier to review the new 
revision if the previous revision was not reviewed too long ago... ;)

I feel it is polite to tell the reviewers there will be some delay when 
I know it.

Yours,
	-- 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