Re: [PATCH v2] mfd: axp20x: Generalise handling without interrupt

From: Jernej Škrabec
Date: Tue Sep 05 2023 - 17:03:54 EST

Dne torek, 05. september 2023 ob 22:46:42 CEST je Andre Przywara napisal(a):
> On Tue, 05 Sep 2023 22:28:10 +0200
> Jernej Škrabec <jernej.skrabec@xxxxxxxxx> wrote:
>
> Hi Jernej,
>
> > On Monday, August 28, 2023 11:32:29 PM CEST Andre Przywara wrote:
> > > At the moment we allow the AXP15060 and the AXP806 PMICs to omit the
> > > interrupt line to the SoC, and we skip registering the PEK (power key)
> > > driver in this case, since that crashes when no IRQ is described in the
> > > DT node.
> > > The IRQ pin potentially not being connected to anything does affect more
> > > PMICs, though, and the PEK driver is not the only one requiring an
> > > interrupt: at least the AC power supply driver crashes in a similar
> > > fashion.
> > >
> > > Generalise the handling of AXP MFD devices when the platform tables
> > > describe no interrupt, by allowing each device to specify an alternative
> > > MFD list for this case. If no specific alternative is specified, we go
> > > with the safe default of "just the regulators", which matches the
> > > current
> > > situation.
> > >
> > > This enables new devices using the AXP313a PMIC, but not connecting the
> > > IRQ pin.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > ---
> > > Changelog v2 .. v1:
> > > - drop reordering approach, use separate cell lists
> > >
> > > drivers/mfd/axp20x.c | 44 ++++++++++++++++++++++++--------------------
> > > 1 file changed, 24 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > > index c03bc5cda080a..239e7f18956ae 100644
> > > --- a/drivers/mfd/axp20x.c
> > > +++ b/drivers/mfd/axp20x.c
> > > @@ -1133,6 +1133,8 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > >
> > > struct device *dev = axp20x->dev;
> > > const struct acpi_device_id *acpi_id;
> > > const struct of_device_id *of_id;
> > >
> > > + const struct mfd_cell *cells_no_irq = NULL;
> > > + int nr_cells_no_irq = 0;
> > >
> > > if (dev->of_node) {
> > >
> > > of_id = of_match_device(dev->driver->of_match_table,
> >
> > dev);
> >
> > > @@ -1207,14 +1209,15 @@ int axp20x_match_device(struct axp20x_dev
> > > *axp20x)
> > >
> > > * if there is no interrupt line.
> > > */
> > >
> > > if (of_property_read_bool(axp20x->dev->of_node,
> > >
> > > - "x-powers,self-
> >
> > working-mode") &&
> >
> > > - axp20x->irq > 0) {
> > > + "x-powers,self-
> >
> > working-mode")) {
> >
> > > axp20x->nr_cells =
> >
> > ARRAY_SIZE(axp806_self_working_cells);
> >
> > > axp20x->cells = axp806_self_working_cells;
> > >
> > > } else {
> > >
> > > axp20x->nr_cells =
ARRAY_SIZE(axp806_cells);
> > > axp20x->cells = axp806_cells;
> > >
> > > }
> > >
> > > + nr_cells_no_irq = ARRAY_SIZE(axp806_cells);
> > > + cells_no_irq = axp806_cells;
> > >
> > > axp20x->regmap_cfg = &axp806_regmap_config;
> > > axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
> > > break;
> > >
> > > @@ -1238,24 +1241,8 @@ int axp20x_match_device(struct axp20x_dev
> > > *axp20x)
> > >
> > > axp20x->regmap_irq_chip = &axp803_regmap_irq_chip;
> > > break;
> > >
> > > case AXP15060_ID:
> > > - /*
> > > - * Don't register the power key part if there is no
> >
> > interrupt
> >
> > > - * line.
> > > - *
> > > - * Since most use cases of AXP PMICs are Allwinner
> >
> > SOCs, board
> >
> > > - * designers follow Allwinner's reference design and
> >
> > connects
> >
> > > - * IRQ line to SOC, there's no need for those variants
> >
> > to deal
> >
> > > - * with cases that IRQ isn't connected. However,
> >
> > AXP15660 is
> >
> > > - * used by some other vendors' SOCs that didn't
connect
> >
> > IRQ
> >
> > > - * line, we need to deal with this case.
> > > - */
> > > - if (axp20x->irq > 0) {
> > > - axp20x->nr_cells =
> >
> > ARRAY_SIZE(axp15060_cells);
> >
> > > - axp20x->cells = axp15060_cells;
> > > - } else {
> > > - axp20x->nr_cells =
> >
> > ARRAY_SIZE(axp_regulator_only_cells);
> >
> > > - axp20x->cells = axp_regulator_only_cells;
> > > - }
> > > + axp20x->nr_cells = ARRAY_SIZE(axp15060_cells);
> > > + axp20x->cells = axp15060_cells;
> > >
> > > axp20x->regmap_cfg = &axp15060_regmap_config;
> > > axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
> > > break;
> > >
> > > @@ -1263,6 +1250,23 @@ int axp20x_match_device(struct axp20x_dev
> > > *axp20x)
> > >
> > > dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x-
> > >
> > >variant);
> > >
> > > return -EINVAL;
> > >
> > > }
> > >
> > > +
> > > + /*
> > > + * Use an alternative cell array when no interrupt line is
> >
> > connected,
> >
> > > + * since IRQs are required by some drivers.
> > > + * The default is the safe "regulator-only", as this works fine
> >
> > without
> >
> > > + * an interrupt specified.
> > > + */
> > > + if (axp20x->irq <= 0) {
> > > + if (cells_no_irq) {
> > > + axp20x->nr_cells = nr_cells_no_irq;
> > > + axp20x->cells = cells_no_irq;
> > > + } else {
> > > + axp20x->nr_cells =
> >
> > ARRAY_SIZE(axp_regulator_only_cells);
> >
> > > + axp20x->cells = axp_regulator_only_cells;
> >
> > axp806_cells (old value for AXP806_ID without irq) and
> > axp_regulator_only_cells differs in id field. Is that an issue?
>
> Yes, an annoying one, which led me to introduce the ability to let each
> PMIC override this default. So the case AXP806_ID: above sets the local
> variable cells_no_irq to axp806_cells, and the check above sees that it
> has been changed from the NULL initialisation and uses that instead of
> the default. At least that was my plan, let me know if I messed that
> up somehow, or if it deserves more comments.

Ah, yes. It looks ok, but it's not obvious. It's fine.
Reviewed-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>

Best regards,
Jernej

>
> Cheers,
> Andre
>
> > Best regards,
> > Jernej
> >
> > > + }
> > > + }
> > > +
> > >
> > > dev_info(dev, "AXP20x variant %s found\n",
> > >
> > > axp20x_model_names[axp20x->variant]);