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

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

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@gmail.com> 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@arm.com>
> > > ---
> > > 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@gmail.com>

Best regards,
Jernej

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