Date: Fri, 14 Jun 2024 10:33:21 +0100
From: Lee Jones <lee@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
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 Fri, 14 Jun 2024, Matti Vaittinen wrote:

> 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'm used to it.  Old reviews are cached locally.

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

I wouldn't lose any sleep over it. :)

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists