Received-SPF: pass (google.com: domain of linux-kernel+bounces-214478-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 14 Jun 2024 08:50:04 +0100
From: Lee Jones <lee@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Thomas Gleixner <tglx@linutronix.de>, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v3 09/10] mfd: bd96801: Add ERRB IRQ
Message-ID: <20240614075004.GB2561462@google.com>
References: <cover.1717486682.git.mazziesaccount@gmail.com>
 <332a2d2429e2ba3c96afd28c1ccc18efc38e1fd3.1717486682.git.mazziesaccount@gmail.com>
 <20240613163249.GN2561462@google.com>
 <21a468c2-7d8f-459a-a5a9-53d8694c3f38@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <21a468c2-7d8f-459a-a5a9-53d8694c3f38@gmail.com>

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@gmail.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.

> > > + * 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 ;)
> > >    */
> 
> ...
> 
> > >   static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   {
> > > -	struct regmap_irq_chip_data *intb_irq_data;
> > > +	int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> > > +	int wdg_irq_no;
> > > +	struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> > > +	struct irq_domain *intb_domain, *errb_domain;
> > > +	struct resource wdg_irq;
> > >   	const struct fwnode_handle *fwnode;
> > > -	struct irq_domain *intb_domain;
> > > +	struct resource *regulator_res;
> > >   	struct regmap *regmap;
> > > -	int ret, intb_irq;
> > >   	fwnode = dev_fwnode(&i2c->dev);
> > >   	if (!fwnode)
> > > @@ -212,10 +364,28 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   	if (intb_irq < 0)
> > >   		return dev_err_probe(&i2c->dev, intb_irq, "INTB IRQ not configured\n");
> > > +	num_intb =  ARRAY_SIZE(regulator_intb_irqs);
> > > +
> > > +	/* ERRB may be omitted if processor is powered by the PMIC */
> > > +	errb_irq = fwnode_irq_get_byname(fwnode, "errb");
> > > +	if (errb_irq < 0)
> > > +		errb_irq = 0;
> > > +
> > > +	if (errb_irq)
> > > +		num_errb = ARRAY_SIZE(regulator_errb_irqs);
> > > +
> > > +	num_regu_irqs = num_intb + num_errb;
> > > +
> > > +	regulator_res = kcalloc(num_regu_irqs, sizeof(*regulator_res), GFP_KERNEL);
> > 
> > Why not devm_* and omit the kfree()?
> 
> I used kcalloc() because this memory is only temporarily needed. It is not
> needed after devm_mfd_add_devices() returns.
> 
> Sure the devm_* would simplify the error paths... Thanks!
> 
> > 
> > > +	if (!regulator_res)
> > > +		return -ENOMEM;
> > > +
> > >   	regmap = devm_regmap_init_i2c(i2c, &bd96801_regmap_config);
> > > -	if (IS_ERR(regmap))
> > > -		return dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > > +	if (IS_ERR(regmap)) {
> > > +		ret = dev_err_probe(&i2c->dev, PTR_ERR(regmap),
> > >   				    "Regmap initialization failed\n");
> > > +		goto free_out;
> > > +	}
> > >   	ret = regmap_write(regmap, BD96801_LOCK_REG, BD96801_UNLOCK);
> > >   	if (ret)
> > > @@ -224,18 +394,63 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   	ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, intb_irq,
> > >   				       IRQF_ONESHOT, 0, &bd96801_irq_chip_intb,
> > >   				       &intb_irq_data);
> > > -	if (ret)
> > > -		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB IRQ chip\n");
> > > +	if (ret) {
> > > +		dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");
> > > +		goto free_out;
> > > +	}
> > >   	intb_domain = regmap_irq_get_domain(intb_irq_data);
> > > -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > > -				   bd96801_mfd_cells,
> > > -				   ARRAY_SIZE(bd96801_mfd_cells), NULL, 0,
> > > -				   intb_domain);
> > > -
> > > +	/*
> > > +	 * MFD core code is built to handle only one IRQ domain. BD96801
> > > +	 * has two domains so we do IRQ mapping here and provide the
> > > +	 * already mapped IRQ numbers to sub-devices.
> > > +	 */
> > > +	for (i = 0; i < num_intb; i++) {
> > > +		struct resource *res = &regulator_res[i];
> > > +
> > > +		*res = regulator_intb_irqs[i];
> > > +		res->start = res->end = irq_create_mapping(intb_domain,
> > > +							    res->start);
> > > +	}
> > > +
> > > +	wdg_irq_no = irq_create_mapping(intb_domain, BD96801_WDT_ERR_STAT);
> > > +	wdg_irq = DEFINE_RES_IRQ_NAMED(wdg_irq_no, "bd96801-wdg");
> > > +	bd96801_mfd_cells[WDG_CELL].resources = &wdg_irq;
> > > +	bd96801_mfd_cells[WDG_CELL].num_resources = 1;
> > > +
> > > +	if (num_errb) {
> > 
> > 	if (!num_errb)
> > 		goto skip_errb;
> 
> Ok, can do.
> 
> > 
> > > +		ret = devm_regmap_add_irq_chip(&i2c->dev, regmap, errb_irq,
> > > +					       IRQF_ONESHOT, 0,
> > > +					       &bd96801_irq_chip_errb,
> > > +					       &errb_irq_data);
> > > +		if (ret) {
> > > +			dev_err_probe(&i2c->dev, ret,
> > > +				      "Failed to add ERRB (%d) irq_chip\n",
> > > +				      errb_irq);
> > > +			goto free_out;
> > > +		}
> > > +		errb_domain = regmap_irq_get_domain(errb_irq_data);
> > > +
> > > +		for (i = 0; i < num_errb; i++) {
> > > +			struct resource *res = &regulator_res[num_intb + i];
> > > +
> > > +			*res = regulator_errb_irqs[i];
> > > +			res->start = res->end = irq_create_mapping(errb_domain,
> > > +								   res->start);
> > > +		}
> > > +	}
> > 
> > skip_errb:
> 
> ...
> 
> 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. :)

-- 
Lee Jones [李琼斯]