linux-kernel - Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240903143718.GX6858@google.com>
Date: Tue, 3 Sep 2024 15:37:18 +0100
From: Lee Jones <lee@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] mfd: bd96801: Add ERRB IRQ

On Fri, 30 Aug 2024, Matti Vaittinen wrote:

> On 8/30/24 10:28, Lee Jones wrote:
> > On Mon, 26 Aug 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.
> 
> Thanks for the review Lee! :)
> 
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> > > ---
> > > Revision history:
> > > New series (only ERRB addition)
> > > v1:
> > > 	- use devm allocation for regulator_res
> > > 	- use goto skip_errb instead of an if (errb)
> > > 	- constify immutable structs
> > > 
> > > Old series (All BD96801 functionality + irqdomain and regmap changes)
> > > v2 => v3:
> > > 	- No changes
> > > v1 => v2:
> > > 	- New patch
> > > 
> > > mfd: constify structs
> > > ---
> > >   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;
> > 
> > Nit: Not sure why the smaller data elements have been placed at the top.
> 
> Because some people have told me it's easier for them to read the local
> variable declarations when the code is formatted to "reverse xmas-tree"
> -style. I suppose I've tried to follow that here.
> 
> > They were better down where they were.
> 
> My old personal preference has just been to have 'simple' integer types
> first, then structs, and the pointers last. I don't think having xmas-tree
> (reversed or not) plays a role in my code-reading ability...
> 
> I won't re-spin the series just for this, if this is just a 'nit'. I will
> try to remember the comment if I need to rebase / respin this later though
> :)

Please leave them were they are.

> > > +	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)
> > > @@ -213,6 +364,23 @@ 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 = devm_kcalloc(&i2c->dev, num_regu_irqs,
> > > +				     sizeof(*regulator_res), GFP_KERNEL);
> > > +	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),
> > > @@ -226,16 +394,54 @@ static int bd96801_i2c_probe(struct i2c_client *i2c)
> > >   				       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");
> > > +		return dev_err_probe(&i2c->dev, ret, "Failed to add INTB irq_chip\n");

Kernel logs are user facing.  The previous message was better.

Please drop this change.

> > >   	intb_domain = regmap_irq_get_domain(intb_irq_data);
> > > -	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO,
> > > -				   bd96801_cells,
> > > -				   ARRAY_SIZE(bd96801_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.
> > > +	 */
> > 
> > Do one or more of the subdevices consume both domains?
> 
> I believe the regulators consume both.
> 
> > If not, why not call devm_mfd_add_devices() twice?
> 
> Thanks for this suggestion :) It didn't occur to me I could do that. Well,
> here I need both domains for regulators so it probably wouldn't work - but
> maybe I will remember this is a viable option for future designs! Thanks!

That's a shame.  This all looks quite messy.

-- 
Lee Jones [李琼斯]