Re: [PATCH] mfd: axp20x: Generalise handling without interrupts

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 17 Aug 2023 12:55:29 +0100
Lee Jones <lee@kernel.org> wrote:

Hi Lee,

> On Mon, 07 Aug 2023, 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, for instance.
> > 
> > Generalise the handling of AXP MFD devices when the platform tables
> > describe no interrupt, by putting devices requiring an IRQ *last* in
> > the MFD cell array. We then can easily cut short the number of devices
> > to be registered in this case.
> > 
> > This patch just enables that for three PMIC models for now: the two
> > already handled, plus the AXP313a, for which we now have mulitple examples
> > of boards without the IRQ pin connected.
> > To stay consistent with the current behaviour, we still (try to) register
> > all devices for the other PMICs, even though this will probably crash
> > without an interrupt specified, if those problematic drivers are loaded.
> > But this new approach can now easily be extended to other PMICs with more
> > devices, should the need arise: currently all in-tree users are fine.
> > 
> > This fixes operation on the first boards using the AXP313a, which do not
> > bother to connect the PMIC's IRQ pin.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Reported-by: Shengyu Qu <wiagn233@outlook.com>
> > ---
> >  drivers/mfd/axp20x.c | 48 +++++++++++++++++---------------------------
> >  1 file changed, 18 insertions(+), 30 deletions(-)  
> 
> Relying on the ordering of static struct elements is fragile.
> 
> Please separate into new structs.

Well, I was hoping to avoid this churn, since the IRQ-less lists are
purely a subset of the original structs. At first I thought about some
filtering (marking IRQ-required cells somehow), but looking at the
current scope (just needed for three simple PMICs at the moment), I
found an easier solution, which is both simple (avoids specifying both
lists for *all* PMICs), but also flexible.
Will tests this and post ASAP.

Cheers,
Andre

> 
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index c03bc5cda080a..c9623604bf6c1 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -1031,12 +1031,12 @@ static const struct mfd_cell axp803_cells[] = {
> >  };
> >  
> >  static const struct mfd_cell axp806_self_working_cells[] = {
> > +	{	.name		= "axp20x-regulator" },
> >  	{
> >  		.name		= "axp221-pek",
> >  		.num_resources	= ARRAY_SIZE(axp806_pek_resources),
> >  		.resources	= axp806_pek_resources,
> >  	},
> > -	{	.name		= "axp20x-regulator" },
> >  };
> >  
> >  static const struct mfd_cell axp806_cells[] = {
> > @@ -1090,19 +1090,11 @@ static const struct mfd_cell axp813_cells[] = {
> >  };
> >  
> >  static const struct mfd_cell axp15060_cells[] = {
> > +	{	.name		= "axp20x-regulator", },
> >  	{
> >  		.name		= "axp221-pek",
> >  		.num_resources	= ARRAY_SIZE(axp15060_pek_resources),
> >  		.resources	= axp15060_pek_resources,
> > -	}, {
> > -		.name		= "axp20x-regulator",
> > -	},
> > -};
> > -
> > -/* For boards that don't have IRQ line connected to SOC. */
> > -static const struct mfd_cell axp_regulator_only_cells[] = {
> > -	{
> > -		.name		= "axp20x-regulator",
> >  	},
> >  };
> >  
> > @@ -1133,6 +1125,7 @@ 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;
> > +	int nr_cells_no_irq = 0;
> >  
> >  	if (dev->of_node) {
> >  		of_id = of_match_device(dev->driver->of_match_table, dev);
> > @@ -1191,6 +1184,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >  		break;
> >  	case AXP313A_ID:
> >  		axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > +		nr_cells_no_irq = 1;
> >  		axp20x->cells = axp313a_cells;
> >  		axp20x->regmap_cfg = &axp313a_regmap_config;
> >  		axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > @@ -1207,14 +1201,14 @@ 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 = 1;
> >  		axp20x->regmap_cfg = &axp806_regmap_config;
> >  		axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
> >  		break;
> > @@ -1238,24 +1232,9 @@ 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);
> > +		nr_cells_no_irq = 1;
> > +		axp20x->cells = axp15060_cells;
> >  		axp20x->regmap_cfg = &axp15060_regmap_config;
> >  		axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
> >  		break;
> > @@ -1263,6 +1242,15 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >  		dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> >  		return -EINVAL;
> >  	}
> > +
> > +	/*
> > +	 * Skip registering some MFD cells if there is no interrupt
> > +	 * line, as IRQs might be required by some drivers.
> > +	 * Those components must be the last in the cell array.
> > +	 */
> > +	if (axp20x->irq <= 0)
> > +		axp20x->nr_cells -= nr_cells_no_irq;
> > +
> >  	dev_info(dev, "AXP20x variant %s found\n",
> >  		 axp20x_model_names[axp20x->variant]);
> >  
> > -- 
> > 2.25.1
> >   
>