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

[Matti Vaittinen]

On 9/3/24 17:37, Lee Jones wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > > 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.

I suppose this is a request to leave them where they are in-tree code 
now - not to leave them where they are in this patch :) I'll re-spin 
this anyways for the print below. So, I might as well revert the local 
variable formatting here :)

> > > > +	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.

Good catch. This has been an unintended change. Not sure where this 
originated from.


Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~