[<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 [李琼斯]
Powered by blists - more mailing lists