[PATCH 0/5] regulator: add X-Powers AXP323 support

[PATCH 0/5] regulator: add X-Powers AXP323 support

[Andre Przywara]

The X-Powers AXP323 is a close sibling to the AXP313a PMIC, only that it
allows to dual-phase the first two DC/DC converters. This is controlled
via a new register. On the first glance that would sound like a
compatible extension, but any random AXP313a driver would not know about
the potential dual-phase nature of the second DCDC rail, so might want
to turn that off, spoiling the whole setup. So this patchset introduces
a new compatible string, without any fallbacks.

Patch 1 adds the DT binding documentation, patch 2 fixes some fragile
connection between PMIC IDs and an array. Patch 3 allows multiple AXP
chips, since the AXP323 seems to be often paired with the AXP717, and
there is some sysfs naming clash with the current code.
Patch 4 then adds the MFD bits, to introduce the new device type and that
extra register, while patch 4 eventually adds the new regulator device,
and takes care about the proper poly-phase detection.

One note: so far the poly-phased AXP PMICs had that setting already
enabled at reset time, so we just detected it and were good. However the
AXP323 on my board does not, so it requires enabling the dual-phase bit at
boot time. The BSP kernel does that in their boot0 (SPL) early boot code,
and the plan would be to do this either in U-Boot or TF-A for mainline.
But should we actually expose this in the DT, as some new property, to
give kernels a chance to set it? The rails in question power the secondary
cluster, so it's not strictly required at boot time, but it's probably too
late for the kernel anyway, given that SMP bringup is much earlier than
drivers? I would appreciate any thoughts here.

Thanks,
Andre

Andre Przywara (5):
  dt-bindings: mfd: x-powers,axp152: Document AXP323
  mfd: axp20x: ensure relationship between IDs and model names
  mfd: axp20x: Allow multiple regulators
  mfd: axp20x: Add support for AXP323
  regulator: axp20x: add support for the AXP323

 .../bindings/mfd/x-powers,axp152.yaml         |  3 +
 drivers/mfd/axp20x-i2c.c                      |  1 +
 drivers/mfd/axp20x.c                          | 58 ++++++++++++++-----
 drivers/regulator/axp20x-regulator.c          | 13 ++++-
 include/linux/mfd/axp20x.h                    |  4 +-
 5 files changed, 61 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH 1/5] dt-bindings: mfd: x-powers,axp152: Document AXP323

[Andre Przywara]

The X-Powers AXP323 is a PMIC used on some newer Allwinner devices.
It is almost the same as the AXP313, but supports dual-phasing the first
two DC/DC converters. A pure AXP313 driver wouldn't know about this, and
might turn the linked DCDC2 regulator off, as it does not seem to be
used. This makes the AXP323 incompatible to the AXP313a.

Add the new compatible string, and treat it like the AXP313a.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
index 14ab367fc887..3f7661bdd202 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -71,6 +71,7 @@ allOf:
                 - x-powers,axp15060
                 - x-powers,axp305
                 - x-powers,axp313a
+                - x-powers,axp323
 
     then:
       required:
@@ -82,6 +83,7 @@ allOf:
           contains:
             enum:
               - x-powers,axp313a
+              - x-powers,axp323
               - x-powers,axp15060
               - x-powers,axp717
 
@@ -100,6 +102,7 @@ properties:
           - x-powers,axp221
           - x-powers,axp223
           - x-powers,axp313a
+          - x-powers,axp323
           - x-powers,axp717
           - x-powers,axp803
           - x-powers,axp806
-- 
2.25.1

[PATCH 2/5] mfd: axp20x: ensure relationship between IDs and model names

[Andre Przywara]

At the moment there is an implicit relationship between the AXP model
IDs and the order of the strings in the axp20x_model_names[] array.
This is fragile, and makes adding IDs in the middle error prone.

Make this relationship official by changing the ID type to the actual
enum used, and using indexed initialisers for the string list.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mfd/axp20x.c                 | 30 ++++++++++++++--------------
 drivers/regulator/axp20x-regulator.c |  2 +-
 include/linux/mfd/axp20x.h           |  2 +-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 4051551757f2..5ceea359289f 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -34,20 +34,20 @@
 #define AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE	BIT(4)
 
 static const char * const axp20x_model_names[] = {
-	"AXP152",
-	"AXP192",
-	"AXP202",
-	"AXP209",
-	"AXP221",
-	"AXP223",
-	"AXP288",
-	"AXP313a",
-	"AXP717",
-	"AXP803",
-	"AXP806",
-	"AXP809",
-	"AXP813",
-	"AXP15060",
+	[AXP152_ID] = "AXP152",
+	[AXP192_ID] = "AXP192",
+	[AXP202_ID] = "AXP202",
+	[AXP209_ID] = "AXP209",
+	[AXP221_ID] = "AXP221",
+	[AXP223_ID] = "AXP223",
+	[AXP288_ID] = "AXP288",
+	[AXP313A_ID] = "AXP313a",
+	[AXP717_ID] = "AXP717",
+	[AXP803_ID] = "AXP803",
+	[AXP806_ID] = "AXP806",
+	[AXP809_ID] = "AXP809",
+	[AXP813_ID] = "AXP813",
+	[AXP15060_ID] = "AXP15060",
 };
 
 static const struct regmap_range axp152_writeable_ranges[] = {
@@ -1345,7 +1345,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
 		break;
 	default:
-		dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
+		dev_err(dev, "unsupported AXP20X ID %u\n", axp20x->variant);
 		return -EINVAL;
 	}
 
diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index a8e91d9d028b..3ba76dbd0fb9 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -1597,7 +1597,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 		nregulators = AXP15060_REG_ID_MAX;
 		break;
 	default:
-		dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
+		dev_err(&pdev->dev, "Unsupported AXP variant: %d\n",
 			axp20x->variant);
 		return -EINVAL;
 	}
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index f4dfc1871a95..79ecaaaa2070 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -959,7 +959,7 @@ struct axp20x_dev {
 	unsigned long			irq_flags;
 	struct regmap			*regmap;
 	struct regmap_irq_chip_data	*regmap_irqc;
-	long				variant;
+	enum axp20x_variants		variant;
 	int                             nr_cells;
 	const struct mfd_cell           *cells;
 	const struct regmap_config	*regmap_cfg;
-- 
2.25.1

[PATCH 3/5] mfd: axp20x: Allow multiple regulators

[Andre Przywara]

At the moment trying to register a second AXP chip makes the probe fail,
as some sysfs registration fails due to a duplicate name:

...
[    3.688215] axp20x-i2c 0-0035: AXP20X driver loaded
[    3.695610] axp20x-i2c 0-0036: AXP20x variant AXP323 found
[    3.706151] sysfs: cannot create duplicate filename '/bus/platform/devices/axp20x-regulator'
[    3.714718] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc1-00026-g50bf2e2c079d-dirty #192
[    3.724020] Hardware name: Avaota A1 (DT)
[    3.728029] Call trace:
[    3.730477]  dump_backtrace+0x94/0xec
[    3.734146]  show_stack+0x18/0x24
[    3.737462]  dump_stack_lvl+0x80/0xf4
[    3.741128]  dump_stack+0x18/0x24
[    3.744444]  sysfs_warn_dup+0x64/0x80
[    3.748109]  sysfs_do_create_link_sd+0xf0/0xf8
[    3.752553]  sysfs_create_link+0x20/0x40
[    3.756476]  bus_add_device+0x64/0x104
[    3.760229]  device_add+0x310/0x760
[    3.763717]  platform_device_add+0x10c/0x238
[    3.767990]  mfd_add_device+0x4ec/0x5c8
[    3.771829]  mfd_add_devices+0x88/0x11c
[    3.775666]  axp20x_device_probe+0x70/0x184
[    3.779851]  axp20x_i2c_probe+0x9c/0xd8
...

This is because we use PLATFORM_DEVID_NONE for the mfd_add_devices()
call, which would number the child devices in the same 0-based way, even
for the second (or any other) instance.

Use PLATFORM_DEVID_AUTO instead, which automatically assigns
non-conflicting device numbers.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mfd/axp20x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 5ceea359289f..bc08ae433260 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -1419,7 +1419,7 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
 		}
 	}
 
-	ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
+	ret = mfd_add_devices(axp20x->dev, PLATFORM_DEVID_AUTO, axp20x->cells,
 			      axp20x->nr_cells, NULL, 0, NULL);
 
 	if (ret) {
-- 
2.25.1

[PATCH 4/5] mfd: axp20x: Add support for AXP323

[Andre Przywara]

The X-Powers AXP323 is a very close sibling of the AXP313A. The only
difference seems to be the ability to dual-phase the first two DC/DC
converter, which adds another register.

Add the required boilerplate to introduce a new PMIC to the AXP MFD
driver. Where possible, this just maps into the existing structs defined
for the AXP313A, only deviating where needed.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mfd/axp20x-i2c.c   |  1 +
 drivers/mfd/axp20x.c       | 26 ++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h |  2 ++
 3 files changed, 29 insertions(+)

diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
index 791a0b4cb64b..5c93136f977e 100644
--- a/drivers/mfd/axp20x-i2c.c
+++ b/drivers/mfd/axp20x-i2c.c
@@ -65,6 +65,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
 	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
 	{ .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
 	{ .compatible = "x-powers,axp313a", .data = (void *)AXP313A_ID },
+	{ .compatible = "x-powers,axp323", .data = (void *)AXP323_ID },
 	{ .compatible = "x-powers,axp717", .data = (void *)AXP717_ID },
 	{ .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
 	{ .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index bc08ae433260..8d90962b56d9 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -42,6 +42,7 @@ static const char * const axp20x_model_names[] = {
 	[AXP223_ID] = "AXP223",
 	[AXP288_ID] = "AXP288",
 	[AXP313A_ID] = "AXP313a",
+	[AXP323_ID] = "AXP323",
 	[AXP717_ID] = "AXP717",
 	[AXP803_ID] = "AXP803",
 	[AXP806_ID] = "AXP806",
@@ -193,6 +194,10 @@ static const struct regmap_range axp313a_writeable_ranges[] = {
 	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
 };
 
+static const struct regmap_range axp323_writeable_ranges[] = {
+	regmap_reg_range(AXP313A_ON_INDICATE, AXP323_DCDC_MODE_CTRL2),
+};
+
 static const struct regmap_range axp313a_volatile_ranges[] = {
 	regmap_reg_range(AXP313A_SHUTDOWN_CTRL, AXP313A_SHUTDOWN_CTRL),
 	regmap_reg_range(AXP313A_IRQ_STATE, AXP313A_IRQ_STATE),
@@ -203,6 +208,11 @@ static const struct regmap_access_table axp313a_writeable_table = {
 	.n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
 };
 
+static const struct regmap_access_table axp323_writeable_table = {
+	.yes_ranges = axp323_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(axp323_writeable_ranges),
+};
+
 static const struct regmap_access_table axp313a_volatile_table = {
 	.yes_ranges = axp313a_volatile_ranges,
 	.n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
@@ -433,6 +443,15 @@ static const struct regmap_config axp313a_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
+static const struct regmap_config axp323_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.wr_table = &axp323_writeable_table,
+	.volatile_table = &axp313a_volatile_table,
+	.max_register = AXP323_DCDC_MODE_CTRL2,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_config axp717_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -1221,6 +1240,7 @@ static int axp20x_power_off(struct sys_off_data *data)
 	unsigned int shutdown_reg;
 
 	switch (axp20x->variant) {
+	case AXP323_ID:
 	case AXP313A_ID:
 		shutdown_reg = AXP313A_SHUTDOWN_CTRL;
 		break;
@@ -1289,6 +1309,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_cfg = &axp313a_regmap_config;
 		axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
 		break;
+	case AXP323_ID:
+		axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
+		axp20x->cells = axp313a_cells;
+		axp20x->regmap_cfg = &axp323_regmap_config;
+		axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
+		break;
 	case AXP717_ID:
 		axp20x->nr_cells = ARRAY_SIZE(axp717_cells);
 		axp20x->cells = axp717_cells;
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 79ecaaaa2070..c3df0e615fbf 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -19,6 +19,7 @@ enum axp20x_variants {
 	AXP223_ID,
 	AXP288_ID,
 	AXP313A_ID,
+	AXP323_ID,
 	AXP717_ID,
 	AXP803_ID,
 	AXP806_ID,
@@ -113,6 +114,7 @@ enum axp20x_variants {
 #define AXP313A_SHUTDOWN_CTRL		0x1a
 #define AXP313A_IRQ_EN			0x20
 #define AXP313A_IRQ_STATE		0x21
+#define AXP323_DCDC_MODE_CTRL2		0x22
 
 #define AXP717_ON_INDICATE		0x00
 #define AXP717_PMU_STATUS_2		0x01
-- 
2.25.1

[PATCH 5/5] regulator: axp20x: add support for the AXP323

[Andre Przywara]

The X-Powers AXP323 is a very close sibling of the AXP313A. The only
difference seems to be the ability to dual-phase the first two DC/DC
converters.

Place the new AXP323 ID next to the existing AXP313A checks, to let
them share most code.
The only difference is the poly-phase detection code, which gets
extended to check the respective bit in a newly used register.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/regulator/axp20x-regulator.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 3ba76dbd0fb9..e3cc59b82ea6 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -1341,6 +1341,7 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 		step = 150;
 		break;
 	case AXP313A_ID:
+	case AXP323_ID:
 	case AXP717_ID:
 	case AXP15060_ID:
 		/* The DCDC PWM frequency seems to be fixed to 3 MHz. */
@@ -1527,6 +1528,15 @@ static bool axp20x_is_polyphase_slave(struct axp20x_dev *axp20x, int id)
 		}
 		break;
 
+	case AXP323_ID:
+		regmap_read(axp20x->regmap, AXP323_DCDC_MODE_CTRL2, &reg);
+
+		switch (id) {
+		case AXP313A_DCDC2:
+			return !!(reg & BIT(1));
+		}
+		break;
+
 	default:
 		return false;
 	}
@@ -1565,6 +1575,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 						  "x-powers,drive-vbus-en");
 		break;
 	case AXP313A_ID:
+	case AXP323_ID:
 		regulators = axp313a_regulators;
 		nregulators = AXP313A_REG_ID_MAX;
 		break;
-- 
2.25.1

Re: [PATCH 5/5] regulator: axp20x: add support for the AXP323

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

On Thu, Oct 03, 2024 at 12:14:44PM +0100, Andre Przywara wrote:
> The X-Powers AXP323 is a very close sibling of the AXP313A. The only
> difference seems to be the ability to dual-phase the first two DC/DC
> converters.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] dt-bindings: mfd: x-powers,axp152: Document AXP323

[Chen-Yu Tsai]

On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The X-Powers AXP323 is a PMIC used on some newer Allwinner devices.
> It is almost the same as the AXP313, but supports dual-phasing the first
> two DC/DC converters. A pure AXP313 driver wouldn't know about this, and
> might turn the linked DCDC2 regulator off, as it does not seem to be
> used. This makes the AXP323 incompatible to the AXP313a.
>
> Add the new compatible string, and treat it like the AXP313a.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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

> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> index 14ab367fc887..3f7661bdd202 100644
> --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> @@ -71,6 +71,7 @@ allOf:
>                  - x-powers,axp15060
>                  - x-powers,axp305
>                  - x-powers,axp313a
> +                - x-powers,axp323
>
>      then:
>        required:
> @@ -82,6 +83,7 @@ allOf:
>            contains:
>              enum:
>                - x-powers,axp313a
> +              - x-powers,axp323
>                - x-powers,axp15060
>                - x-powers,axp717
>
> @@ -100,6 +102,7 @@ properties:
>            - x-powers,axp221
>            - x-powers,axp223
>            - x-powers,axp313a
> +          - x-powers,axp323
>            - x-powers,axp717
>            - x-powers,axp803
>            - x-powers,axp806
> --
> 2.25.1
>

Re: [PATCH 2/5] mfd: axp20x: ensure relationship between IDs and model names

[Chen-Yu Tsai]

On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> At the moment there is an implicit relationship between the AXP model
> IDs and the order of the strings in the axp20x_model_names[] array.
> This is fragile, and makes adding IDs in the middle error prone.
>
> Make this relationship official by changing the ID type to the actual
> enum used, and using indexed initialisers for the string list.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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

> ---
>  drivers/mfd/axp20x.c                 | 30 ++++++++++++++--------------
>  drivers/regulator/axp20x-regulator.c |  2 +-
>  include/linux/mfd/axp20x.h           |  2 +-
>  3 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 4051551757f2..5ceea359289f 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -34,20 +34,20 @@
>  #define AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE    BIT(4)
>
>  static const char * const axp20x_model_names[] = {
> -       "AXP152",
> -       "AXP192",
> -       "AXP202",
> -       "AXP209",
> -       "AXP221",
> -       "AXP223",
> -       "AXP288",
> -       "AXP313a",
> -       "AXP717",
> -       "AXP803",
> -       "AXP806",
> -       "AXP809",
> -       "AXP813",
> -       "AXP15060",
> +       [AXP152_ID] = "AXP152",
> +       [AXP192_ID] = "AXP192",
> +       [AXP202_ID] = "AXP202",
> +       [AXP209_ID] = "AXP209",
> +       [AXP221_ID] = "AXP221",
> +       [AXP223_ID] = "AXP223",
> +       [AXP288_ID] = "AXP288",
> +       [AXP313A_ID] = "AXP313a",
> +       [AXP717_ID] = "AXP717",
> +       [AXP803_ID] = "AXP803",
> +       [AXP806_ID] = "AXP806",
> +       [AXP809_ID] = "AXP809",
> +       [AXP813_ID] = "AXP813",
> +       [AXP15060_ID] = "AXP15060",
>  };
>
>  static const struct regmap_range axp152_writeable_ranges[] = {
> @@ -1345,7 +1345,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>                 axp20x->regmap_irq_chip = &axp15060_regmap_irq_chip;
>                 break;
>         default:
> -               dev_err(dev, "unsupported AXP20X ID %lu\n", axp20x->variant);
> +               dev_err(dev, "unsupported AXP20X ID %u\n", axp20x->variant);
>                 return -EINVAL;
>         }
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index a8e91d9d028b..3ba76dbd0fb9 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -1597,7 +1597,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>                 nregulators = AXP15060_REG_ID_MAX;
>                 break;
>         default:
> -               dev_err(&pdev->dev, "Unsupported AXP variant: %ld\n",
> +               dev_err(&pdev->dev, "Unsupported AXP variant: %d\n",
>                         axp20x->variant);
>                 return -EINVAL;
>         }
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index f4dfc1871a95..79ecaaaa2070 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -959,7 +959,7 @@ struct axp20x_dev {
>         unsigned long                   irq_flags;
>         struct regmap                   *regmap;
>         struct regmap_irq_chip_data     *regmap_irqc;
> -       long                            variant;
> +       enum axp20x_variants            variant;
>         int                             nr_cells;
>         const struct mfd_cell           *cells;
>         const struct regmap_config      *regmap_cfg;
> --
> 2.25.1
>

Re: [PATCH 3/5] mfd: axp20x: Allow multiple regulators

[Chen-Yu Tsai]

On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> At the moment trying to register a second AXP chip makes the probe fail,
> as some sysfs registration fails due to a duplicate name:
>
> ...
> [    3.688215] axp20x-i2c 0-0035: AXP20X driver loaded
> [    3.695610] axp20x-i2c 0-0036: AXP20x variant AXP323 found
> [    3.706151] sysfs: cannot create duplicate filename '/bus/platform/devices/axp20x-regulator'
> [    3.714718] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc1-00026-g50bf2e2c079d-dirty #192
> [    3.724020] Hardware name: Avaota A1 (DT)
> [    3.728029] Call trace:
> [    3.730477]  dump_backtrace+0x94/0xec
> [    3.734146]  show_stack+0x18/0x24
> [    3.737462]  dump_stack_lvl+0x80/0xf4
> [    3.741128]  dump_stack+0x18/0x24
> [    3.744444]  sysfs_warn_dup+0x64/0x80
> [    3.748109]  sysfs_do_create_link_sd+0xf0/0xf8
> [    3.752553]  sysfs_create_link+0x20/0x40
> [    3.756476]  bus_add_device+0x64/0x104
> [    3.760229]  device_add+0x310/0x760
> [    3.763717]  platform_device_add+0x10c/0x238
> [    3.767990]  mfd_add_device+0x4ec/0x5c8
> [    3.771829]  mfd_add_devices+0x88/0x11c
> [    3.775666]  axp20x_device_probe+0x70/0x184
> [    3.779851]  axp20x_i2c_probe+0x9c/0xd8
> ...
>
> This is because we use PLATFORM_DEVID_NONE for the mfd_add_devices()
> call, which would number the child devices in the same 0-based way, even
> for the second (or any other) instance.
>
> Use PLATFORM_DEVID_AUTO instead, which automatically assigns
> non-conflicting device numbers.

That's weird... I don't remember running into this when working on the A80,
which had two albeit different AXP chips. That was a long time ago though.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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

> ---
>  drivers/mfd/axp20x.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 5ceea359289f..bc08ae433260 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -1419,7 +1419,7 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
>                 }
>         }
>
> -       ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> +       ret = mfd_add_devices(axp20x->dev, PLATFORM_DEVID_AUTO, axp20x->cells,
>                               axp20x->nr_cells, NULL, 0, NULL);
>
>         if (ret) {
> --
> 2.25.1
>

Re: [PATCH 4/5] mfd: axp20x: Add support for AXP323

[Chen-Yu Tsai]

On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The X-Powers AXP323 is a very close sibling of the AXP313A. The only
> difference seems to be the ability to dual-phase the first two DC/DC
> converter, which adds another register.
>
> Add the required boilerplate to introduce a new PMIC to the AXP MFD
> driver. Where possible, this just maps into the existing structs defined
> for the AXP313A, only deviating where needed.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/mfd/axp20x-i2c.c   |  1 +
>  drivers/mfd/axp20x.c       | 26 ++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h |  2 ++
>  3 files changed, 29 insertions(+)
>
> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> index 791a0b4cb64b..5c93136f977e 100644
> --- a/drivers/mfd/axp20x-i2c.c
> +++ b/drivers/mfd/axp20x-i2c.c
> @@ -65,6 +65,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
>         { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
>         { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
>         { .compatible = "x-powers,axp313a", .data = (void *)AXP313A_ID },
> +       { .compatible = "x-powers,axp323", .data = (void *)AXP323_ID },
>         { .compatible = "x-powers,axp717", .data = (void *)AXP717_ID },
>         { .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
>         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index bc08ae433260..8d90962b56d9 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -42,6 +42,7 @@ static const char * const axp20x_model_names[] = {
>         [AXP223_ID] = "AXP223",
>         [AXP288_ID] = "AXP288",
>         [AXP313A_ID] = "AXP313a",
> +       [AXP323_ID] = "AXP323",
>         [AXP717_ID] = "AXP717",
>         [AXP803_ID] = "AXP803",
>         [AXP806_ID] = "AXP806",
> @@ -193,6 +194,10 @@ static const struct regmap_range axp313a_writeable_ranges[] = {
>         regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
>  };
>
> +static const struct regmap_range axp323_writeable_ranges[] = {
> +       regmap_reg_range(AXP313A_ON_INDICATE, AXP323_DCDC_MODE_CTRL2),
> +};
> +
>  static const struct regmap_range axp313a_volatile_ranges[] = {
>         regmap_reg_range(AXP313A_SHUTDOWN_CTRL, AXP313A_SHUTDOWN_CTRL),
>         regmap_reg_range(AXP313A_IRQ_STATE, AXP313A_IRQ_STATE),
> @@ -203,6 +208,11 @@ static const struct regmap_access_table axp313a_writeable_table = {
>         .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
>  };
>
> +static const struct regmap_access_table axp323_writeable_table = {
> +       .yes_ranges = axp323_writeable_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(axp323_writeable_ranges),
> +};
> +
>  static const struct regmap_access_table axp313a_volatile_table = {
>         .yes_ranges = axp313a_volatile_ranges,
>         .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> @@ -433,6 +443,15 @@ static const struct regmap_config axp313a_regmap_config = {
>         .cache_type = REGCACHE_MAPLE,
>  };
>
> +static const struct regmap_config axp323_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .wr_table = &axp323_writeable_table,
> +       .volatile_table = &axp313a_volatile_table,
> +       .max_register = AXP323_DCDC_MODE_CTRL2,
> +       .cache_type = REGCACHE_RBTREE,

Maple tree instead?

The rest looks fine, so once fixed,

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

> +};
> +
>  static const struct regmap_config axp717_regmap_config = {
>         .reg_bits = 8,
>         .val_bits = 8,
> @@ -1221,6 +1240,7 @@ static int axp20x_power_off(struct sys_off_data *data)
>         unsigned int shutdown_reg;
>
>         switch (axp20x->variant) {
> +       case AXP323_ID:
>         case AXP313A_ID:
>                 shutdown_reg = AXP313A_SHUTDOWN_CTRL;
>                 break;
> @@ -1289,6 +1309,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>                 axp20x->regmap_cfg = &axp313a_regmap_config;
>                 axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
>                 break;
> +       case AXP323_ID:
> +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> +               axp20x->cells = axp313a_cells;
> +               axp20x->regmap_cfg = &axp323_regmap_config;
> +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> +               break;
>         case AXP717_ID:
>                 axp20x->nr_cells = ARRAY_SIZE(axp717_cells);
>                 axp20x->cells = axp717_cells;
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 79ecaaaa2070..c3df0e615fbf 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -19,6 +19,7 @@ enum axp20x_variants {
>         AXP223_ID,
>         AXP288_ID,
>         AXP313A_ID,
> +       AXP323_ID,
>         AXP717_ID,
>         AXP803_ID,
>         AXP806_ID,
> @@ -113,6 +114,7 @@ enum axp20x_variants {
>  #define AXP313A_SHUTDOWN_CTRL          0x1a
>  #define AXP313A_IRQ_EN                 0x20
>  #define AXP313A_IRQ_STATE              0x21
> +#define AXP323_DCDC_MODE_CTRL2         0x22
>
>  #define AXP717_ON_INDICATE             0x00
>  #define AXP717_PMU_STATUS_2            0x01
> --
> 2.25.1
>

Re: [PATCH 5/5] regulator: axp20x: add support for the AXP323

[Chen-Yu Tsai]

On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> The X-Powers AXP323 is a very close sibling of the AXP313A. The only
> difference seems to be the ability to dual-phase the first two DC/DC
> converters.
>
> Place the new AXP323 ID next to the existing AXP313A checks, to let
> them share most code.
> The only difference is the poly-phase detection code, which gets
> extended to check the respective bit in a newly used register.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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


> ---
>  drivers/regulator/axp20x-regulator.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 3ba76dbd0fb9..e3cc59b82ea6 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -1341,6 +1341,7 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>                 step = 150;
>                 break;
>         case AXP313A_ID:
> +       case AXP323_ID:
>         case AXP717_ID:
>         case AXP15060_ID:
>                 /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> @@ -1527,6 +1528,15 @@ static bool axp20x_is_polyphase_slave(struct axp20x_dev *axp20x, int id)
>                 }
>                 break;
>
> +       case AXP323_ID:
> +               regmap_read(axp20x->regmap, AXP323_DCDC_MODE_CTRL2, &reg);
> +
> +               switch (id) {
> +               case AXP313A_DCDC2:
> +                       return !!(reg & BIT(1));
> +               }
> +               break;
> +
>         default:
>                 return false;
>         }
> @@ -1565,6 +1575,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>                                                   "x-powers,drive-vbus-en");
>                 break;
>         case AXP313A_ID:
> +       case AXP323_ID:
>                 regulators = axp313a_regulators;
>                 nregulators = AXP313A_REG_ID_MAX;
>                 break;
> --
> 2.25.1
>

Re: [PATCH 1/5] dt-bindings: mfd: x-powers,axp152: Document AXP323

[Rob Herring (Arm)]

On Thu, 03 Oct 2024 12:14:40 +0100, Andre Przywara wrote:
> The X-Powers AXP323 is a PMIC used on some newer Allwinner devices.
> It is almost the same as the AXP313, but supports dual-phasing the first
> two DC/DC converters. A pure AXP313 driver wouldn't know about this, and
> might turn the linked DCDC2 regulator off, as it does not seem to be
> used. This makes the AXP323 incompatible to the AXP313a.
> 
> Add the new compatible string, and treat it like the AXP313a.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 3/5] mfd: axp20x: Allow multiple regulators

[Andre Przywara]

On Thu, 3 Oct 2024 23:19:16 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi Chen-Yu,

thanks for having a look!

> On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > At the moment trying to register a second AXP chip makes the probe fail,
> > as some sysfs registration fails due to a duplicate name:
> >
> > ...
> > [    3.688215] axp20x-i2c 0-0035: AXP20X driver loaded
> > [    3.695610] axp20x-i2c 0-0036: AXP20x variant AXP323 found
> > [    3.706151] sysfs: cannot create duplicate filename '/bus/platform/devices/axp20x-regulator'
> > [    3.714718] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc1-00026-g50bf2e2c079d-dirty #192
> > [    3.724020] Hardware name: Avaota A1 (DT)
> > [    3.728029] Call trace:
> > [    3.730477]  dump_backtrace+0x94/0xec
> > [    3.734146]  show_stack+0x18/0x24
> > [    3.737462]  dump_stack_lvl+0x80/0xf4
> > [    3.741128]  dump_stack+0x18/0x24
> > [    3.744444]  sysfs_warn_dup+0x64/0x80
> > [    3.748109]  sysfs_do_create_link_sd+0xf0/0xf8
> > [    3.752553]  sysfs_create_link+0x20/0x40
> > [    3.756476]  bus_add_device+0x64/0x104
> > [    3.760229]  device_add+0x310/0x760
> > [    3.763717]  platform_device_add+0x10c/0x238
> > [    3.767990]  mfd_add_device+0x4ec/0x5c8
> > [    3.771829]  mfd_add_devices+0x88/0x11c
> > [    3.775666]  axp20x_device_probe+0x70/0x184
> > [    3.779851]  axp20x_i2c_probe+0x9c/0xd8
> > ...
> >
> > This is because we use PLATFORM_DEVID_NONE for the mfd_add_devices()
> > call, which would number the child devices in the same 0-based way, even
> > for the second (or any other) instance.
> >
> > Use PLATFORM_DEVID_AUTO instead, which automatically assigns
> > non-conflicting device numbers.  
> 
> That's weird... I don't remember running into this when working on the A80,
> which had two albeit different AXP chips. That was a long time ago though.

Yeah, I was wondering about this as well. And it's two different PMICs here
as well: most A523/T527 system seem to come with an AXP717/AXP323 combo.
Though there are not linked together in any way, like in this master/slave
mode of the AXP806.
I will test on (your old) A80 board, and will add a Fixes: tag in v2,
should it also fail there.

> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Thanks,
Andre

> 
> > ---
> >  drivers/mfd/axp20x.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index 5ceea359289f..bc08ae433260 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -1419,7 +1419,7 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
> >                 }
> >         }
> >
> > -       ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> > +       ret = mfd_add_devices(axp20x->dev, PLATFORM_DEVID_AUTO, axp20x->cells,
> >                               axp20x->nr_cells, NULL, 0, NULL);
> >
> >         if (ret) {
> > --
> > 2.25.1
> >  

Re: [PATCH 4/5] mfd: axp20x: Add support for AXP323

[Andre Przywara]

On Thu, 3 Oct 2024 23:20:58 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > The X-Powers AXP323 is a very close sibling of the AXP313A. The only
> > difference seems to be the ability to dual-phase the first two DC/DC
> > converter, which adds another register.
> >
> > Add the required boilerplate to introduce a new PMIC to the AXP MFD
> > driver. Where possible, this just maps into the existing structs defined
> > for the AXP313A, only deviating where needed.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/mfd/axp20x-i2c.c   |  1 +
> >  drivers/mfd/axp20x.c       | 26 ++++++++++++++++++++++++++
> >  include/linux/mfd/axp20x.h |  2 ++
> >  3 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> > index 791a0b4cb64b..5c93136f977e 100644
> > --- a/drivers/mfd/axp20x-i2c.c
> > +++ b/drivers/mfd/axp20x-i2c.c
> > @@ -65,6 +65,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
> >         { .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> >         { .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
> >         { .compatible = "x-powers,axp313a", .data = (void *)AXP313A_ID },
> > +       { .compatible = "x-powers,axp323", .data = (void *)AXP323_ID },
> >         { .compatible = "x-powers,axp717", .data = (void *)AXP717_ID },
> >         { .compatible = "x-powers,axp803", .data = (void *)AXP803_ID },
> >         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index bc08ae433260..8d90962b56d9 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -42,6 +42,7 @@ static const char * const axp20x_model_names[] = {
> >         [AXP223_ID] = "AXP223",
> >         [AXP288_ID] = "AXP288",
> >         [AXP313A_ID] = "AXP313a",
> > +       [AXP323_ID] = "AXP323",
> >         [AXP717_ID] = "AXP717",
> >         [AXP803_ID] = "AXP803",
> >         [AXP806_ID] = "AXP806",
> > @@ -193,6 +194,10 @@ static const struct regmap_range axp313a_writeable_ranges[] = {
> >         regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> >  };
> >
> > +static const struct regmap_range axp323_writeable_ranges[] = {
> > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP323_DCDC_MODE_CTRL2),
> > +};
> > +
> >  static const struct regmap_range axp313a_volatile_ranges[] = {
> >         regmap_reg_range(AXP313A_SHUTDOWN_CTRL, AXP313A_SHUTDOWN_CTRL),
> >         regmap_reg_range(AXP313A_IRQ_STATE, AXP313A_IRQ_STATE),
> > @@ -203,6 +208,11 @@ static const struct regmap_access_table axp313a_writeable_table = {
> >         .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
> >  };
> >
> > +static const struct regmap_access_table axp323_writeable_table = {
> > +       .yes_ranges = axp323_writeable_ranges,
> > +       .n_yes_ranges = ARRAY_SIZE(axp323_writeable_ranges),
> > +};
> > +
> >  static const struct regmap_access_table axp313a_volatile_table = {
> >         .yes_ranges = axp313a_volatile_ranges,
> >         .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> > @@ -433,6 +443,15 @@ static const struct regmap_config axp313a_regmap_config = {
> >         .cache_type = REGCACHE_MAPLE,
> >  };
> >
> > +static const struct regmap_config axp323_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .wr_table = &axp323_writeable_table,
> > +       .volatile_table = &axp313a_volatile_table,
> > +       .max_register = AXP323_DCDC_MODE_CTRL2,
> > +       .cache_type = REGCACHE_RBTREE,  
> 
> Maple tree instead?

Ah yes, you are right, this was lost over a rebase. Will fix in v2.

> The rest looks fine, so once fixed,
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Thanks!
Andre

> 
> > +};
> > +
> >  static const struct regmap_config axp717_regmap_config = {
> >         .reg_bits = 8,
> >         .val_bits = 8,
> > @@ -1221,6 +1240,7 @@ static int axp20x_power_off(struct sys_off_data *data)
> >         unsigned int shutdown_reg;
> >
> >         switch (axp20x->variant) {
> > +       case AXP323_ID:
> >         case AXP313A_ID:
> >                 shutdown_reg = AXP313A_SHUTDOWN_CTRL;
> >                 break;
> > @@ -1289,6 +1309,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >                 axp20x->regmap_cfg = &axp313a_regmap_config;
> >                 axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> >                 break;
> > +       case AXP323_ID:
> > +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > +               axp20x->cells = axp313a_cells;
> > +               axp20x->regmap_cfg = &axp323_regmap_config;
> > +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > +               break;
> >         case AXP717_ID:
> >                 axp20x->nr_cells = ARRAY_SIZE(axp717_cells);
> >                 axp20x->cells = axp717_cells;
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index 79ecaaaa2070..c3df0e615fbf 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -19,6 +19,7 @@ enum axp20x_variants {
> >         AXP223_ID,
> >         AXP288_ID,
> >         AXP313A_ID,
> > +       AXP323_ID,
> >         AXP717_ID,
> >         AXP803_ID,
> >         AXP806_ID,
> > @@ -113,6 +114,7 @@ enum axp20x_variants {
> >  #define AXP313A_SHUTDOWN_CTRL          0x1a
> >  #define AXP313A_IRQ_EN                 0x20
> >  #define AXP313A_IRQ_STATE              0x21
> > +#define AXP323_DCDC_MODE_CTRL2         0x22
> >
> >  #define AXP717_ON_INDICATE             0x00
> >  #define AXP717_PMU_STATUS_2            0x01
> > --
> > 2.25.1
> >  

Re: [PATCH 3/5] mfd: axp20x: Allow multiple regulators

[Andre Przywara]

On Thu, 3 Oct 2024 23:19:16 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

> On Thu, Oct 3, 2024 at 7:15 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > At the moment trying to register a second AXP chip makes the probe fail,
> > as some sysfs registration fails due to a duplicate name:
> >
> > ...
> > [    3.688215] axp20x-i2c 0-0035: AXP20X driver loaded
> > [    3.695610] axp20x-i2c 0-0036: AXP20x variant AXP323 found
> > [    3.706151] sysfs: cannot create duplicate filename '/bus/platform/devices/axp20x-regulator'
> > [    3.714718] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-rc1-00026-g50bf2e2c079d-dirty #192
> > [    3.724020] Hardware name: Avaota A1 (DT)
> > [    3.728029] Call trace:
> > [    3.730477]  dump_backtrace+0x94/0xec
> > [    3.734146]  show_stack+0x18/0x24
> > [    3.737462]  dump_stack_lvl+0x80/0xf4
> > [    3.741128]  dump_stack+0x18/0x24
> > [    3.744444]  sysfs_warn_dup+0x64/0x80
> > [    3.748109]  sysfs_do_create_link_sd+0xf0/0xf8
> > [    3.752553]  sysfs_create_link+0x20/0x40
> > [    3.756476]  bus_add_device+0x64/0x104
> > [    3.760229]  device_add+0x310/0x760
> > [    3.763717]  platform_device_add+0x10c/0x238
> > [    3.767990]  mfd_add_device+0x4ec/0x5c8
> > [    3.771829]  mfd_add_devices+0x88/0x11c
> > [    3.775666]  axp20x_device_probe+0x70/0x184
> > [    3.779851]  axp20x_i2c_probe+0x9c/0xd8
> > ...
> >
> > This is because we use PLATFORM_DEVID_NONE for the mfd_add_devices()
> > call, which would number the child devices in the same 0-based way, even
> > for the second (or any other) instance.
> >
> > Use PLATFORM_DEVID_AUTO instead, which automatically assigns
> > non-conflicting device numbers.  
> 
> That's weird... I don't remember running into this when working on the A80,
> which had two albeit different AXP chips. That was a long time ago though.

So I tested this on a Cubieboard 4, and it works there either way,
with or without this patch. That's RSB instead of I2C, which honestly
shouldn't make much of a difference, but maybe the call path differs?

But since it still works, I think this patch is correct regardless.

Thanks,
Andre


> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 
> > ---
> >  drivers/mfd/axp20x.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index 5ceea359289f..bc08ae433260 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -1419,7 +1419,7 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
> >                 }
> >         }
> >
> > -       ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> > +       ret = mfd_add_devices(axp20x->dev, PLATFORM_DEVID_AUTO, axp20x->cells,
> >                               axp20x->nr_cells, NULL, 0, NULL);
> >
> >         if (ret) {
> > --
> > 2.25.1
> >  
>