[PATCH] mfd: axp20x: Generalise handling without interrupts

[PATCH] mfd: axp20x: Generalise handling without interrupts

[Andre Przywara]

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(-)

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

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

[Chen-Yu Tsai]

On Mon, Aug 7, 2023 at 9:39 PM Andre Przywara <andre.przywara@arm.com> 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>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

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

[Lee Jones]

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.

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

-- 
Lee Jones [李琼斯]

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

[Andre Przywara]

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