[PATCH v8 0/3] regulator: Add X-Powers AXP313a PMIC support*

[PATCH v8 0/3] regulator: Add X-Powers AXP313a PMIC support*

[Martin Botka]

This patch series adds support for the X-Powers AXP313a PMIC, which is
often bundled with Allwinner H616 or H313 SoCs.
Andre Przywara took over the series for v6 but I now have a bit more time
so I took over the series again.

Up to v5 this was speaking of the AXP1530, which seems to be some internal
name. The chips we have seen in the wild are all labeled AXP313a, so we
go with this name here, from now on. This is supported by the fact that
there is an AXP313a datasheet, but none for the AXP1530.

Patch 1 is the binding documentation (just the new compatible string),
patch 2 adds the MFD bits (mostly describing the extent of the regmap),
while patch 3 describes the actual AXP313a register definitions.
Since this ties neatly into the existing AXP and generic regulator
framework, the patches are indeed only structure definitions, there is no
new code.

For now Andre papered over this "fixed customizable" RTC-LDO regulator in the
same way this was done before for other PMICs (AXP803, for instance), Andre
thinks we can fix this properly with a follow-up patch, for all instances.

Please have a look!

Cheers,
Martin

Changelog:
v7 .. v8:
- Add check for dcdcfreq being zero

v6 .. v7:
- Use alphabetical ordering

v5 .. v6:
- change name from AXP1530 to AXP313a
- extend commit messages
- drop AXP*_FREQUENCY register (not used anyway)
- better vertically align struct definitions
- rename IRQs to match names used for other PMICs
- add RTC_LDO regulator
- use decimal numbers for selector ranges
- use macro definitions to name some values
- force DC/DC switching frequency to be fixed at 3 MHz
- change LDO source supply to VIN1 (as per datasheet)

v4 .. v5:
- Use alphabetical ordering in mfd
- Correct { placement line
- Replace spaces with tabs in 1 struct

v3 .. v4:
- Fix indentation

v2 .. v3:
- Move AXP1530 dt-binding to alphabetical order

v1 .. v2:
- Remove RSB support.
- Drop .id = 0
- Add dt-binding for the AXP1530

Martin Botka (3):
  dt-bindings: mfd: x-powers,axp152: Document the AXP313a variant
  mfd: axp20x: Add support for AXP313a PMIC
  regulator: axp20x: Add support for AXP313a variant

 .../bindings/mfd/x-powers,axp152.yaml         |  1 +
 drivers/mfd/axp20x-i2c.c                      |  2 +
 drivers/mfd/axp20x.c                          | 61 +++++++++++++++++++
 drivers/regulator/axp20x-regulator.c          | 60 ++++++++++++++++++
 include/linux/mfd/axp20x.h                    | 32 ++++++++++
 5 files changed, 156 insertions(+)

-- 
2.39.0

[PATCH v8 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP313a variant

[Martin Botka]

The X-Powers AXP313a is a PMIC used on some devices with the Allwinner
H616 or H313 SoC.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
index b7a8747d5fa0..e2241cd28445 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -88,6 +88,7 @@ properties:
           - x-powers,axp209
           - x-powers,axp221
           - x-powers,axp223
+          - x-powers,axp313a
           - x-powers,axp803
           - x-powers,axp806
           - x-powers,axp809
-- 
2.39.0

Re: [PATCH v8 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP313a variant

[Martin Botka]

I forgot to add acked-by on this one from Krzystof.

On Fri, Jan 20 2023 at 07:44:57 PM +01:00:00, Martin Botka 
<martin.botka@somainline.org> wrote:
> The X-Powers AXP313a is a PMIC used on some devices with the Allwinner
> H616 or H313 SoC.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml 
> b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> index b7a8747d5fa0..e2241cd28445 100644
> --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> @@ -88,6 +88,7 @@ properties:
>            - x-powers,axp209
>            - x-powers,axp221
>            - x-powers,axp223
> +          - x-powers,axp313a
>            - x-powers,axp803
>            - x-powers,axp806
>            - x-powers,axp809
> --
> 2.39.0
> 

Re: [PATCH v8 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP313a variant

[Chen-Yu Tsai]

On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
<martin.botka@somainline.org> wrote:
>
> The X-Powers AXP313a is a PMIC used on some devices with the Allwinner
> H616 or H313 SoC.
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

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

[PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Martin Botka]

The AXP313a is a PMIC chip produced by X-Powers, it can be connected via
an I2C bus.
The name AXP1530 seems to appear as well, and this is what is used in
the BSP driver. From all we know it's the same chip, just a different
name. However we have only seen AXP313a chips in the wild, so go with
this name.

Compared to the other AXP PMICs it's a rather simple affair: just three
DCDC converters, three LDOs, and no battery charging support.

Describe the regmap and the MFD bits, along with the registers exposed
via I2C. Eventually advertise the device using the new compatible
string.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/mfd/axp20x-i2c.c   |  2 ++
 drivers/mfd/axp20x.c       | 61 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
 3 files changed, 95 insertions(+)

diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
index f49fbd307958..f061177cb18e 100644
--- a/drivers/mfd/axp20x-i2c.c
+++ b/drivers/mfd/axp20x-i2c.c
@@ -63,6 +63,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
 	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
 	{ .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,axp803", .data = (void *)AXP803_ID },
 	{ .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
 	{ },
@@ -76,6 +77,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
 	{ "axp209", 0 },
 	{ "axp221", 0 },
 	{ "axp223", 0 },
+	{ "axp313a", 0 },
 	{ "axp803", 0 },
 	{ "axp806", 0 },
 	{ },
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 01a6bbb6d266..ff15775f3c27 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
 	"AXP221",
 	"AXP223",
 	"AXP288",
+	"AXP313a",
 	"AXP803",
 	"AXP806",
 	"AXP809",
@@ -154,6 +155,24 @@ static const struct regmap_range axp806_writeable_ranges[] = {
 	regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
 };
 
+static const struct regmap_range axp313a_writeable_ranges[] = {
+	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
+};
+
+static const struct regmap_range axp313a_volatile_ranges[] = {
+	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
+};
+
+static const struct regmap_access_table axp313a_writeable_table = {
+	.yes_ranges = axp313a_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
+};
+
+static const struct regmap_access_table axp313a_volatile_table = {
+	.yes_ranges = axp313a_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
+};
+
 static const struct regmap_range axp806_volatile_ranges[] = {
 	regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
 };
@@ -272,6 +291,15 @@ static const struct regmap_config axp288_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config axp313a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.wr_table = &axp313a_writeable_table,
+	.volatile_table = &axp313a_volatile_table,
+	.max_register = AXP313A_IRQ_STATE,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_config axp806_regmap_config = {
 	.reg_bits	= 8,
 	.val_bits	= 8,
@@ -415,6 +443,16 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG,            5, 1),
 };
 
+static const struct regmap_irq axp313a_regmap_irqs[] = {
+	INIT_REGMAP_IRQ(AXP313A, PEK_RIS_EDGE,		0, 7),
+	INIT_REGMAP_IRQ(AXP313A, PEK_FAL_EDGE,		0, 6),
+	INIT_REGMAP_IRQ(AXP313A, PEK_SHORT,		0, 5),
+	INIT_REGMAP_IRQ(AXP313A, PEK_LONG,		0, 4),
+	INIT_REGMAP_IRQ(AXP313A, DCDC3_V_LOW,		0, 3),
+	INIT_REGMAP_IRQ(AXP313A, DCDC2_V_LOW,		0, 2),
+	INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH,		0, 0),
+};
+
 static const struct regmap_irq axp803_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP803, ACIN_OVER_V,		0, 7),
 	INIT_REGMAP_IRQ(AXP803, ACIN_PLUGIN,		0, 6),
@@ -548,6 +586,17 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
 
 };
 
+static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
+	.name			= "axp313a_irq_chip",
+	.status_base		= AXP313A_IRQ_STATE,
+	.ack_base		= AXP313A_IRQ_STATE,
+	.unmask_base		= AXP313A_IRQ_EN,
+	.init_ack_masked	= true,
+	.irqs			= axp313a_regmap_irqs,
+	.num_irqs		= ARRAY_SIZE(axp313a_regmap_irqs),
+	.num_regs		= 1,
+};
+
 static const struct regmap_irq_chip axp803_regmap_irq_chip = {
 	.name			= "axp803",
 	.status_base		= AXP20X_IRQ1_STATE,
@@ -676,6 +725,12 @@ static const struct mfd_cell axp152_cells[] = {
 	},
 };
 
+static struct mfd_cell axp313a_cells[] = {
+	{
+		.name = "axp20x-regulator",
+	},
+};
+
 static const struct resource axp288_adc_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
 };
@@ -892,6 +947,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
 		axp20x->irq_flags = IRQF_TRIGGER_LOW;
 		break;
+	case AXP313A_ID:
+		axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
+		axp20x->cells = axp313a_cells;
+		axp20x->regmap_cfg = &axp313a_regmap_config;
+		axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
+		break;
 	case AXP803_ID:
 		axp20x->nr_cells = ARRAY_SIZE(axp803_cells);
 		axp20x->cells = axp803_cells;
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 2058194807bd..12e4fc3e8391 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -17,6 +17,7 @@ enum axp20x_variants {
 	AXP221_ID,
 	AXP223_ID,
 	AXP288_ID,
+	AXP313A_ID,
 	AXP803_ID,
 	AXP806_ID,
 	AXP809_ID,
@@ -91,6 +92,17 @@ enum axp20x_variants {
 #define AXP22X_ALDO3_V_OUT		0x2a
 #define AXP22X_CHRG_CTRL3		0x35
 
+#define AXP313A_ON_INDICATE		0x00
+#define AXP313A_OUTPUT_CONTROL		0x10
+#define AXP313A_DCDC1_CONRTOL		0x13
+#define AXP313A_DCDC2_CONRTOL		0x14
+#define AXP313A_DCDC3_CONRTOL		0x15
+#define AXP313A_ALDO1_CONRTOL		0x16
+#define AXP313A_DLDO1_CONRTOL		0x17
+#define AXP313A_OUTPUT_MONITOR		0x1d
+#define AXP313A_IRQ_EN			0x20
+#define AXP313A_IRQ_STATE		0x21
+
 #define AXP806_STARTUP_SRC		0x00
 #define AXP806_CHIP_ID			0x03
 #define AXP806_PWR_OUT_CTRL1		0x10
@@ -322,6 +334,16 @@ enum {
 	AXP22X_REG_ID_MAX,
 };
 
+enum {
+	AXP313A_DCDC1 = 0,
+	AXP313A_DCDC2,
+	AXP313A_DCDC3,
+	AXP313A_LDO1,
+	AXP313A_LDO2,
+	AXP313A_RTC_LDO,
+	AXP313A_REG_ID_MAX,
+};
+
 enum {
 	AXP806_DCDCA = 0,
 	AXP806_DCDCB,
@@ -548,6 +570,16 @@ enum axp288_irqs {
 	AXP288_IRQ_BC_USB_CHNG,
 };
 
+enum axp313a_irqs {
+	AXP313A_IRQ_DIE_TEMP_HIGH,
+	AXP313A_IRQ_DCDC2_V_LOW = 2,
+	AXP313A_IRQ_DCDC3_V_LOW,
+	AXP313A_IRQ_PEK_LONG,
+	AXP313A_IRQ_PEK_SHORT,
+	AXP313A_IRQ_PEK_FAL_EDGE,
+	AXP313A_IRQ_PEK_RIS_EDGE,
+};
+
 enum axp803_irqs {
 	AXP803_IRQ_ACIN_OVER_V = 1,
 	AXP803_IRQ_ACIN_PLUGIN,
-- 
2.39.0

Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Chen-Yu Tsai]

Hi,

On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
<martin.botka@somainline.org> wrote:
>
> The AXP313a is a PMIC chip produced by X-Powers, it can be connected via
> an I2C bus.
> The name AXP1530 seems to appear as well, and this is what is used in
> the BSP driver. From all we know it's the same chip, just a different
> name. However we have only seen AXP313a chips in the wild, so go with
> this name.
>
> Compared to the other AXP PMICs it's a rather simple affair: just three
> DCDC converters, three LDOs, and no battery charging support.
>
> Describe the regmap and the MFD bits, along with the registers exposed
> via I2C. Eventually advertise the device using the new compatible
> string.
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/mfd/axp20x-i2c.c   |  2 ++
>  drivers/mfd/axp20x.c       | 61 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
>  3 files changed, 95 insertions(+)
>
> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> index f49fbd307958..f061177cb18e 100644
> --- a/drivers/mfd/axp20x-i2c.c
> +++ b/drivers/mfd/axp20x-i2c.c
> @@ -63,6 +63,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
>         { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
>         { .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,axp803", .data = (void *)AXP803_ID },
>         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
>         { },
> @@ -76,6 +77,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
>         { "axp209", 0 },
>         { "axp221", 0 },
>         { "axp223", 0 },
> +       { "axp313a", 0 },
>         { "axp803", 0 },
>         { "axp806", 0 },
>         { },
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 01a6bbb6d266..ff15775f3c27 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
>         "AXP221",
>         "AXP223",
>         "AXP288",
> +       "AXP313a",
>         "AXP803",
>         "AXP806",
>         "AXP809",
> @@ -154,6 +155,24 @@ static const struct regmap_range axp806_writeable_ranges[] = {
>         regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
>  };
>
> +static const struct regmap_range axp313a_writeable_ranges[] = {
> +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> +};
> +
> +static const struct regmap_range axp313a_volatile_ranges[] = {
> +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),

Why set the whole range as volatile? Why bother with a cache then?

> +};
> +
> +static const struct regmap_access_table axp313a_writeable_table = {
> +       .yes_ranges = axp313a_writeable_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
> +};
> +
> +static const struct regmap_access_table axp313a_volatile_table = {
> +       .yes_ranges = axp313a_volatile_ranges,
> +       .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> +};
> +
>  static const struct regmap_range axp806_volatile_ranges[] = {
>         regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
>  };
> @@ -272,6 +291,15 @@ static const struct regmap_config axp288_regmap_config = {
>         .cache_type     = REGCACHE_RBTREE,
>  };
>
> +static const struct regmap_config axp313a_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .wr_table = &axp313a_writeable_table,
> +       .volatile_table = &axp313a_volatile_table,
> +       .max_register = AXP313A_IRQ_STATE,
> +       .cache_type = REGCACHE_RBTREE,
> +};
> +
>  static const struct regmap_config axp806_regmap_config = {
>         .reg_bits       = 8,
>         .val_bits       = 8,
> @@ -415,6 +443,16 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
>         INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG,            5, 1),
>  };
>
> +static const struct regmap_irq axp313a_regmap_irqs[] = {
> +       INIT_REGMAP_IRQ(AXP313A, PEK_RIS_EDGE,          0, 7),
> +       INIT_REGMAP_IRQ(AXP313A, PEK_FAL_EDGE,          0, 6),
> +       INIT_REGMAP_IRQ(AXP313A, PEK_SHORT,             0, 5),
> +       INIT_REGMAP_IRQ(AXP313A, PEK_LONG,              0, 4),
> +       INIT_REGMAP_IRQ(AXP313A, DCDC3_V_LOW,           0, 3),
> +       INIT_REGMAP_IRQ(AXP313A, DCDC2_V_LOW,           0, 2),
> +       INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH,         0, 0),
> +};
> +
>  static const struct regmap_irq axp803_regmap_irqs[] = {
>         INIT_REGMAP_IRQ(AXP803, ACIN_OVER_V,            0, 7),
>         INIT_REGMAP_IRQ(AXP803, ACIN_PLUGIN,            0, 6),
> @@ -548,6 +586,17 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
>
>  };
>
> +static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
> +       .name                   = "axp313a_irq_chip",
> +       .status_base            = AXP313A_IRQ_STATE,
> +       .ack_base               = AXP313A_IRQ_STATE,
> +       .unmask_base            = AXP313A_IRQ_EN,
> +       .init_ack_masked        = true,
> +       .irqs                   = axp313a_regmap_irqs,
> +       .num_irqs               = ARRAY_SIZE(axp313a_regmap_irqs),
> +       .num_regs               = 1,
> +};
> +
>  static const struct regmap_irq_chip axp803_regmap_irq_chip = {
>         .name                   = "axp803",
>         .status_base            = AXP20X_IRQ1_STATE,
> @@ -676,6 +725,12 @@ static const struct mfd_cell axp152_cells[] = {
>         },
>  };
>
> +static struct mfd_cell axp313a_cells[] = {
> +       {
> +               .name = "axp20x-regulator",

Lee asked for MFD_CELL_NAME() in v7 here.

Could you also add the power button cell? This would make it an actual
MFD, and also complete, since that is the only other function this
PMIC has. Or at least add a note mentioning it. Implementing it will
require a device that actually routes that pin out. AFAICT the MangoPi
doesn't.

> +       },
> +};
> +
>  static const struct resource axp288_adc_resources[] = {
>         DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
>  };
> @@ -892,6 +947,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>                 axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
>                 axp20x->irq_flags = IRQF_TRIGGER_LOW;
>                 break;
> +       case AXP313A_ID:
> +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> +               axp20x->cells = axp313a_cells;
> +               axp20x->regmap_cfg = &axp313a_regmap_config;
> +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> +               break;
>         case AXP803_ID:
>                 axp20x->nr_cells = ARRAY_SIZE(axp803_cells);
>                 axp20x->cells = axp803_cells;
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 2058194807bd..12e4fc3e8391 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -17,6 +17,7 @@ enum axp20x_variants {
>         AXP221_ID,
>         AXP223_ID,
>         AXP288_ID,
> +       AXP313A_ID,
>         AXP803_ID,
>         AXP806_ID,
>         AXP809_ID,
> @@ -91,6 +92,17 @@ enum axp20x_variants {
>  #define AXP22X_ALDO3_V_OUT             0x2a
>  #define AXP22X_CHRG_CTRL3              0x35
>
> +#define AXP313A_ON_INDICATE            0x00
> +#define AXP313A_OUTPUT_CONTROL         0x10
> +#define AXP313A_DCDC1_CONRTOL          0x13
> +#define AXP313A_DCDC2_CONRTOL          0x14
> +#define AXP313A_DCDC3_CONRTOL          0x15
> +#define AXP313A_ALDO1_CONRTOL          0x16
> +#define AXP313A_DLDO1_CONRTOL          0x17

Please also add register 0x1a (note, some bits of this are volatile)
and implement power off with bit 7. The current axp_power_off()
function will not work for this PMIC.

This PMIC also supports software-triggered reset with bit 6 in the same
register. This function would be nice to have, however there's no related
code in the mfd driver right now, since IIRC none of the other ones had
this.

> +#define AXP313A_OUTPUT_MONITOR         0x1d

Not sure why you need this?

> +#define AXP313A_IRQ_EN                 0x20
> +#define AXP313A_IRQ_STATE              0x21
> +
>  #define AXP806_STARTUP_SRC             0x00
>  #define AXP806_CHIP_ID                 0x03
>  #define AXP806_PWR_OUT_CTRL1           0x10
> @@ -322,6 +334,16 @@ enum {
>         AXP22X_REG_ID_MAX,
>  };
>
> +enum {
> +       AXP313A_DCDC1 = 0,
> +       AXP313A_DCDC2,
> +       AXP313A_DCDC3,
> +       AXP313A_LDO1,

This is called ALDO1 in the datasheet ...

> +       AXP313A_LDO2,

... and this one DLDO1.

You already have the registers named that way, so you might as well
fix the names here as well.


Thanks
ChenYu

> +       AXP313A_RTC_LDO,
> +       AXP313A_REG_ID_MAX,
> +};
> +
>  enum {
>         AXP806_DCDCA = 0,
>         AXP806_DCDCB,
> @@ -548,6 +570,16 @@ enum axp288_irqs {
>         AXP288_IRQ_BC_USB_CHNG,
>  };
>
> +enum axp313a_irqs {
> +       AXP313A_IRQ_DIE_TEMP_HIGH,
> +       AXP313A_IRQ_DCDC2_V_LOW = 2,
> +       AXP313A_IRQ_DCDC3_V_LOW,
> +       AXP313A_IRQ_PEK_LONG,
> +       AXP313A_IRQ_PEK_SHORT,
> +       AXP313A_IRQ_PEK_FAL_EDGE,
> +       AXP313A_IRQ_PEK_RIS_EDGE,
> +};
> +
>  enum axp803_irqs {
>         AXP803_IRQ_ACIN_OVER_V = 1,
>         AXP803_IRQ_ACIN_PLUGIN,
> --
> 2.39.0
>

Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Andre Przywara]

On Sat, 28 Jan 2023 01:40:34 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi Chen-Yu,

thanks for the review!

> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> <martin.botka@somainline.org> wrote:
> >
> > The AXP313a is a PMIC chip produced by X-Powers, it can be connected via
> > an I2C bus.
> > The name AXP1530 seems to appear as well, and this is what is used in
> > the BSP driver. From all we know it's the same chip, just a different
> > name. However we have only seen AXP313a chips in the wild, so go with
> > this name.
> >
> > Compared to the other AXP PMICs it's a rather simple affair: just three
> > DCDC converters, three LDOs, and no battery charging support.
> >
> > Describe the regmap and the MFD bits, along with the registers exposed
> > via I2C. Eventually advertise the device using the new compatible
> > string.
> >
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/mfd/axp20x-i2c.c   |  2 ++
> >  drivers/mfd/axp20x.c       | 61 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> >  3 files changed, 95 insertions(+)
> >
> > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> > index f49fbd307958..f061177cb18e 100644
> > --- a/drivers/mfd/axp20x-i2c.c
> > +++ b/drivers/mfd/axp20x-i2c.c
> > @@ -63,6 +63,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
> >         { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> >         { .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,axp803", .data = (void *)AXP803_ID },
> >         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
> >         { },
> > @@ -76,6 +77,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
> >         { "axp209", 0 },
> >         { "axp221", 0 },
> >         { "axp223", 0 },
> > +       { "axp313a", 0 },
> >         { "axp803", 0 },
> >         { "axp806", 0 },
> >         { },
> > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > index 01a6bbb6d266..ff15775f3c27 100644
> > --- a/drivers/mfd/axp20x.c
> > +++ b/drivers/mfd/axp20x.c
> > @@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
> >         "AXP221",
> >         "AXP223",
> >         "AXP288",
> > +       "AXP313a",
> >         "AXP803",
> >         "AXP806",
> >         "AXP809",
> > @@ -154,6 +155,24 @@ static const struct regmap_range axp806_writeable_ranges[] = {
> >         regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
> >  };
> >
> > +static const struct regmap_range axp313a_writeable_ranges[] = {
> > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> > +};
> > +
> > +static const struct regmap_range axp313a_volatile_ranges[] = {
> > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),  
> 
> Why set the whole range as volatile? Why bother with a cache then?

Fixed.

> 
> > +};
> > +
> > +static const struct regmap_access_table axp313a_writeable_table = {
> > +       .yes_ranges = axp313a_writeable_ranges,
> > +       .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
> > +};
> > +
> > +static const struct regmap_access_table axp313a_volatile_table = {
> > +       .yes_ranges = axp313a_volatile_ranges,
> > +       .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> > +};
> > +
> >  static const struct regmap_range axp806_volatile_ranges[] = {
> >         regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
> >  };
> > @@ -272,6 +291,15 @@ static const struct regmap_config axp288_regmap_config = {
> >         .cache_type     = REGCACHE_RBTREE,
> >  };
> >
> > +static const struct regmap_config axp313a_regmap_config = {
> > +       .reg_bits = 8,
> > +       .val_bits = 8,
> > +       .wr_table = &axp313a_writeable_table,
> > +       .volatile_table = &axp313a_volatile_table,
> > +       .max_register = AXP313A_IRQ_STATE,
> > +       .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> >  static const struct regmap_config axp806_regmap_config = {
> >         .reg_bits       = 8,
> >         .val_bits       = 8,
> > @@ -415,6 +443,16 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
> >         INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG,            5, 1),
> >  };
> >
> > +static const struct regmap_irq axp313a_regmap_irqs[] = {
> > +       INIT_REGMAP_IRQ(AXP313A, PEK_RIS_EDGE,          0, 7),
> > +       INIT_REGMAP_IRQ(AXP313A, PEK_FAL_EDGE,          0, 6),
> > +       INIT_REGMAP_IRQ(AXP313A, PEK_SHORT,             0, 5),
> > +       INIT_REGMAP_IRQ(AXP313A, PEK_LONG,              0, 4),
> > +       INIT_REGMAP_IRQ(AXP313A, DCDC3_V_LOW,           0, 3),
> > +       INIT_REGMAP_IRQ(AXP313A, DCDC2_V_LOW,           0, 2),
> > +       INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH,         0, 0),
> > +};
> > +
> >  static const struct regmap_irq axp803_regmap_irqs[] = {
> >         INIT_REGMAP_IRQ(AXP803, ACIN_OVER_V,            0, 7),
> >         INIT_REGMAP_IRQ(AXP803, ACIN_PLUGIN,            0, 6),
> > @@ -548,6 +586,17 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
> >
> >  };
> >
> > +static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
> > +       .name                   = "axp313a_irq_chip",
> > +       .status_base            = AXP313A_IRQ_STATE,
> > +       .ack_base               = AXP313A_IRQ_STATE,
> > +       .unmask_base            = AXP313A_IRQ_EN,
> > +       .init_ack_masked        = true,
> > +       .irqs                   = axp313a_regmap_irqs,
> > +       .num_irqs               = ARRAY_SIZE(axp313a_regmap_irqs),
> > +       .num_regs               = 1,
> > +};
> > +
> >  static const struct regmap_irq_chip axp803_regmap_irq_chip = {
> >         .name                   = "axp803",
> >         .status_base            = AXP20X_IRQ1_STATE,
> > @@ -676,6 +725,12 @@ static const struct mfd_cell axp152_cells[] = {
> >         },
> >  };
> >
> > +static struct mfd_cell axp313a_cells[] = {
> > +       {
> > +               .name = "axp20x-regulator",  
> 
> Lee asked for MFD_CELL_NAME() in v7 here.

Fixed.

> Could you also add the power button cell? This would make it an actual
> MFD, and also complete, since that is the only other function this
> PMIC has. Or at least add a note mentioning it. Implementing it will

Done.

> require a device that actually routes that pin out. AFAICT the MangoPi
> doesn't.
> 
> > +       },
> > +};
> > +
> >  static const struct resource axp288_adc_resources[] = {
> >         DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> >  };
> > @@ -892,6 +947,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >                 axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
> >                 axp20x->irq_flags = IRQF_TRIGGER_LOW;
> >                 break;
> > +       case AXP313A_ID:
> > +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > +               axp20x->cells = axp313a_cells;
> > +               axp20x->regmap_cfg = &axp313a_regmap_config;
> > +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > +               break;
> >         case AXP803_ID:
> >                 axp20x->nr_cells = ARRAY_SIZE(axp803_cells);
> >                 axp20x->cells = axp803_cells;
> > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > index 2058194807bd..12e4fc3e8391 100644
> > --- a/include/linux/mfd/axp20x.h
> > +++ b/include/linux/mfd/axp20x.h
> > @@ -17,6 +17,7 @@ enum axp20x_variants {
> >         AXP221_ID,
> >         AXP223_ID,
> >         AXP288_ID,
> > +       AXP313A_ID,
> >         AXP803_ID,
> >         AXP806_ID,
> >         AXP809_ID,
> > @@ -91,6 +92,17 @@ enum axp20x_variants {
> >  #define AXP22X_ALDO3_V_OUT             0x2a
> >  #define AXP22X_CHRG_CTRL3              0x35
> >
> > +#define AXP313A_ON_INDICATE            0x00
> > +#define AXP313A_OUTPUT_CONTROL         0x10
> > +#define AXP313A_DCDC1_CONRTOL          0x13
> > +#define AXP313A_DCDC2_CONRTOL          0x14
> > +#define AXP313A_DCDC3_CONRTOL          0x15
> > +#define AXP313A_ALDO1_CONRTOL          0x16
> > +#define AXP313A_DLDO1_CONRTOL          0x17  
> 
> Please also add register 0x1a (note, some bits of this are volatile)
> and implement power off with bit 7. The current axp_power_off()
> function will not work for this PMIC.

Ah, good catch, thanks. Though I guess this will be unused (on 64-bit
SoCs), since PSCI poweroff takes precedence due to its higher firmware
priority.
Fixed anyway, need to test this with a hacked priority value.

Though the volatility of this register is a bit questionable, isn't it? It
just seems to apply to the poweroff and reset bits, that are self-reset.
Nobody cares for the former, and we don't use the reset (yet). I added it
to the volatile range anyway.

> This PMIC also supports software-triggered reset with bit 6 in the same
> register. This function would be nice to have, however there's no related
> code in the mfd driver right now, since IIRC none of the other ones had
> this.

What would be the use case? Who would trigger that reset? I guess it might
be more useful for management firmware like crust?

> > +#define AXP313A_OUTPUT_MONITOR         0x1d  
> 
> Not sure why you need this?

Removed.

> > +#define AXP313A_IRQ_EN                 0x20
> > +#define AXP313A_IRQ_STATE              0x21
> > +
> >  #define AXP806_STARTUP_SRC             0x00
> >  #define AXP806_CHIP_ID                 0x03
> >  #define AXP806_PWR_OUT_CTRL1           0x10
> > @@ -322,6 +334,16 @@ enum {
> >         AXP22X_REG_ID_MAX,
> >  };
> >
> > +enum {
> > +       AXP313A_DCDC1 = 0,
> > +       AXP313A_DCDC2,
> > +       AXP313A_DCDC3,
> > +       AXP313A_LDO1,  
> 
> This is called ALDO1 in the datasheet ...
> 
> > +       AXP313A_LDO2,  
> 
> ... and this one DLDO1.
> 
> You already have the registers named that way, so you might as well
> fix the names here as well.

Fixed.

Thanks,
Andre

> 
> 
> Thanks
> ChenYu
> 
> > +       AXP313A_RTC_LDO,
> > +       AXP313A_REG_ID_MAX,
> > +};
> > +
> >  enum {
> >         AXP806_DCDCA = 0,
> >         AXP806_DCDCB,
> > @@ -548,6 +570,16 @@ enum axp288_irqs {
> >         AXP288_IRQ_BC_USB_CHNG,
> >  };
> >
> > +enum axp313a_irqs {
> > +       AXP313A_IRQ_DIE_TEMP_HIGH,
> > +       AXP313A_IRQ_DCDC2_V_LOW = 2,
> > +       AXP313A_IRQ_DCDC3_V_LOW,
> > +       AXP313A_IRQ_PEK_LONG,
> > +       AXP313A_IRQ_PEK_SHORT,
> > +       AXP313A_IRQ_PEK_FAL_EDGE,
> > +       AXP313A_IRQ_PEK_RIS_EDGE,
> > +};
> > +
> >  enum axp803_irqs {
> >         AXP803_IRQ_ACIN_OVER_V = 1,
> >         AXP803_IRQ_ACIN_PLUGIN,
> > --
> > 2.39.0
> >  

Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Chen-Yu Tsai]

On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sat, 28 Jan 2023 01:40:34 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
> Hi Chen-Yu,
>
> thanks for the review!
>
> > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > <martin.botka@somainline.org> wrote:
> > >
> > > The AXP313a is a PMIC chip produced by X-Powers, it can be connected via
> > > an I2C bus.
> > > The name AXP1530 seems to appear as well, and this is what is used in
> > > the BSP driver. From all we know it's the same chip, just a different
> > > name. However we have only seen AXP313a chips in the wild, so go with
> > > this name.
> > >
> > > Compared to the other AXP PMICs it's a rather simple affair: just three
> > > DCDC converters, three LDOs, and no battery charging support.
> > >
> > > Describe the regmap and the MFD bits, along with the registers exposed
> > > via I2C. Eventually advertise the device using the new compatible
> > > string.
> > >
> > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/mfd/axp20x-i2c.c   |  2 ++
> > >  drivers/mfd/axp20x.c       | 61 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> > >  3 files changed, 95 insertions(+)
> > >
> > > diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> > > index f49fbd307958..f061177cb18e 100644
> > > --- a/drivers/mfd/axp20x-i2c.c
> > > +++ b/drivers/mfd/axp20x-i2c.c
> > > @@ -63,6 +63,7 @@ static const struct of_device_id axp20x_i2c_of_match[] = {
> > >         { .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> > >         { .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,axp803", .data = (void *)AXP803_ID },
> > >         { .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
> > >         { },
> > > @@ -76,6 +77,7 @@ static const struct i2c_device_id axp20x_i2c_id[] = {
> > >         { "axp209", 0 },
> > >         { "axp221", 0 },
> > >         { "axp223", 0 },
> > > +       { "axp313a", 0 },
> > >         { "axp803", 0 },
> > >         { "axp806", 0 },
> > >         { },
> > > diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> > > index 01a6bbb6d266..ff15775f3c27 100644
> > > --- a/drivers/mfd/axp20x.c
> > > +++ b/drivers/mfd/axp20x.c
> > > @@ -39,6 +39,7 @@ static const char * const axp20x_model_names[] = {
> > >         "AXP221",
> > >         "AXP223",
> > >         "AXP288",
> > > +       "AXP313a",
> > >         "AXP803",
> > >         "AXP806",
> > >         "AXP809",
> > > @@ -154,6 +155,24 @@ static const struct regmap_range axp806_writeable_ranges[] = {
> > >         regmap_reg_range(AXP806_REG_ADDR_EXT, AXP806_REG_ADDR_EXT),
> > >  };
> > >
> > > +static const struct regmap_range axp313a_writeable_ranges[] = {
> > > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> > > +};
> > > +
> > > +static const struct regmap_range axp313a_volatile_ranges[] = {
> > > +       regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> >
> > Why set the whole range as volatile? Why bother with a cache then?
>
> Fixed.
>
> >
> > > +};
> > > +
> > > +static const struct regmap_access_table axp313a_writeable_table = {
> > > +       .yes_ranges = axp313a_writeable_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(axp313a_writeable_ranges),
> > > +};
> > > +
> > > +static const struct regmap_access_table axp313a_volatile_table = {
> > > +       .yes_ranges = axp313a_volatile_ranges,
> > > +       .n_yes_ranges = ARRAY_SIZE(axp313a_volatile_ranges),
> > > +};
> > > +
> > >  static const struct regmap_range axp806_volatile_ranges[] = {
> > >         regmap_reg_range(AXP20X_IRQ1_STATE, AXP20X_IRQ2_STATE),
> > >  };
> > > @@ -272,6 +291,15 @@ static const struct regmap_config axp288_regmap_config = {
> > >         .cache_type     = REGCACHE_RBTREE,
> > >  };
> > >
> > > +static const struct regmap_config axp313a_regmap_config = {
> > > +       .reg_bits = 8,
> > > +       .val_bits = 8,
> > > +       .wr_table = &axp313a_writeable_table,
> > > +       .volatile_table = &axp313a_volatile_table,
> > > +       .max_register = AXP313A_IRQ_STATE,
> > > +       .cache_type = REGCACHE_RBTREE,
> > > +};
> > > +
> > >  static const struct regmap_config axp806_regmap_config = {
> > >         .reg_bits       = 8,
> > >         .val_bits       = 8,
> > > @@ -415,6 +443,16 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
> > >         INIT_REGMAP_IRQ(AXP288, BC_USB_CHNG,            5, 1),
> > >  };
> > >
> > > +static const struct regmap_irq axp313a_regmap_irqs[] = {
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_RIS_EDGE,          0, 7),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_FAL_EDGE,          0, 6),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_SHORT,             0, 5),
> > > +       INIT_REGMAP_IRQ(AXP313A, PEK_LONG,              0, 4),
> > > +       INIT_REGMAP_IRQ(AXP313A, DCDC3_V_LOW,           0, 3),
> > > +       INIT_REGMAP_IRQ(AXP313A, DCDC2_V_LOW,           0, 2),
> > > +       INIT_REGMAP_IRQ(AXP313A, DIE_TEMP_HIGH,         0, 0),
> > > +};
> > > +
> > >  static const struct regmap_irq axp803_regmap_irqs[] = {
> > >         INIT_REGMAP_IRQ(AXP803, ACIN_OVER_V,            0, 7),
> > >         INIT_REGMAP_IRQ(AXP803, ACIN_PLUGIN,            0, 6),
> > > @@ -548,6 +586,17 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
> > >
> > >  };
> > >
> > > +static const struct regmap_irq_chip axp313a_regmap_irq_chip = {
> > > +       .name                   = "axp313a_irq_chip",
> > > +       .status_base            = AXP313A_IRQ_STATE,
> > > +       .ack_base               = AXP313A_IRQ_STATE,
> > > +       .unmask_base            = AXP313A_IRQ_EN,
> > > +       .init_ack_masked        = true,
> > > +       .irqs                   = axp313a_regmap_irqs,
> > > +       .num_irqs               = ARRAY_SIZE(axp313a_regmap_irqs),
> > > +       .num_regs               = 1,
> > > +};
> > > +
> > >  static const struct regmap_irq_chip axp803_regmap_irq_chip = {
> > >         .name                   = "axp803",
> > >         .status_base            = AXP20X_IRQ1_STATE,
> > > @@ -676,6 +725,12 @@ static const struct mfd_cell axp152_cells[] = {
> > >         },
> > >  };
> > >
> > > +static struct mfd_cell axp313a_cells[] = {
> > > +       {
> > > +               .name = "axp20x-regulator",
> >
> > Lee asked for MFD_CELL_NAME() in v7 here.
>
> Fixed.
>
> > Could you also add the power button cell? This would make it an actual
> > MFD, and also complete, since that is the only other function this
> > PMIC has. Or at least add a note mentioning it. Implementing it will
>
> Done.
>
> > require a device that actually routes that pin out. AFAICT the MangoPi
> > doesn't.
> >
> > > +       },
> > > +};
> > > +
> > >  static const struct resource axp288_adc_resources[] = {
> > >         DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> > >  };
> > > @@ -892,6 +947,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> > >                 axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
> > >                 axp20x->irq_flags = IRQF_TRIGGER_LOW;
> > >                 break;
> > > +       case AXP313A_ID:
> > > +               axp20x->nr_cells = ARRAY_SIZE(axp313a_cells);
> > > +               axp20x->cells = axp313a_cells;
> > > +               axp20x->regmap_cfg = &axp313a_regmap_config;
> > > +               axp20x->regmap_irq_chip = &axp313a_regmap_irq_chip;
> > > +               break;
> > >         case AXP803_ID:
> > >                 axp20x->nr_cells = ARRAY_SIZE(axp803_cells);
> > >                 axp20x->cells = axp803_cells;
> > > diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> > > index 2058194807bd..12e4fc3e8391 100644
> > > --- a/include/linux/mfd/axp20x.h
> > > +++ b/include/linux/mfd/axp20x.h
> > > @@ -17,6 +17,7 @@ enum axp20x_variants {
> > >         AXP221_ID,
> > >         AXP223_ID,
> > >         AXP288_ID,
> > > +       AXP313A_ID,
> > >         AXP803_ID,
> > >         AXP806_ID,
> > >         AXP809_ID,
> > > @@ -91,6 +92,17 @@ enum axp20x_variants {
> > >  #define AXP22X_ALDO3_V_OUT             0x2a
> > >  #define AXP22X_CHRG_CTRL3              0x35
> > >
> > > +#define AXP313A_ON_INDICATE            0x00
> > > +#define AXP313A_OUTPUT_CONTROL         0x10
> > > +#define AXP313A_DCDC1_CONRTOL          0x13
> > > +#define AXP313A_DCDC2_CONRTOL          0x14
> > > +#define AXP313A_DCDC3_CONRTOL          0x15
> > > +#define AXP313A_ALDO1_CONRTOL          0x16
> > > +#define AXP313A_DLDO1_CONRTOL          0x17
> >
> > Please also add register 0x1a (note, some bits of this are volatile)
> > and implement power off with bit 7. The current axp_power_off()
> > function will not work for this PMIC.
>
> Ah, good catch, thanks. Though I guess this will be unused (on 64-bit
> SoCs), since PSCI poweroff takes precedence due to its higher firmware
> priority.
> Fixed anyway, need to test this with a hacked priority value.
>
> Though the volatility of this register is a bit questionable, isn't it? It
> just seems to apply to the poweroff and reset bits, that are self-reset.
> Nobody cares for the former, and we don't use the reset (yet). I added it
> to the volatile range anyway.
>
> > This PMIC also supports software-triggered reset with bit 6 in the same
> > register. This function would be nice to have, however there's no related
> > code in the mfd driver right now, since IIRC none of the other ones had
> > this.
>
> What would be the use case? Who would trigger that reset? I guess it might
> be more useful for management firmware like crust?

That, or implementing system reset if PSCI/crust isn't available? PMIC reset
would be more thorough than the SoC watchdog reset, where you could have
regulators left in a state incorrect for BROM execution (such as storage
power disabled).


ChenYu

> > > +#define AXP313A_OUTPUT_MONITOR         0x1d
> >
> > Not sure why you need this?
>
> Removed.
>
> > > +#define AXP313A_IRQ_EN                 0x20
> > > +#define AXP313A_IRQ_STATE              0x21
> > > +
> > >  #define AXP806_STARTUP_SRC             0x00
> > >  #define AXP806_CHIP_ID                 0x03
> > >  #define AXP806_PWR_OUT_CTRL1           0x10
> > > @@ -322,6 +334,16 @@ enum {
> > >         AXP22X_REG_ID_MAX,
> > >  };
> > >
> > > +enum {
> > > +       AXP313A_DCDC1 = 0,
> > > +       AXP313A_DCDC2,
> > > +       AXP313A_DCDC3,
> > > +       AXP313A_LDO1,
> >
> > This is called ALDO1 in the datasheet ...
> >
> > > +       AXP313A_LDO2,
> >
> > ... and this one DLDO1.
> >
> > You already have the registers named that way, so you might as well
> > fix the names here as well.
>
> Fixed.
>
> Thanks,
> Andre
>
> >
> >
> > Thanks
> > ChenYu
> >
> > > +       AXP313A_RTC_LDO,
> > > +       AXP313A_REG_ID_MAX,
> > > +};
> > > +
> > >  enum {
> > >         AXP806_DCDCA = 0,
> > >         AXP806_DCDCB,
> > > @@ -548,6 +570,16 @@ enum axp288_irqs {
> > >         AXP288_IRQ_BC_USB_CHNG,
> > >  };
> > >
> > > +enum axp313a_irqs {
> > > +       AXP313A_IRQ_DIE_TEMP_HIGH,
> > > +       AXP313A_IRQ_DCDC2_V_LOW = 2,
> > > +       AXP313A_IRQ_DCDC3_V_LOW,
> > > +       AXP313A_IRQ_PEK_LONG,
> > > +       AXP313A_IRQ_PEK_SHORT,
> > > +       AXP313A_IRQ_PEK_FAL_EDGE,
> > > +       AXP313A_IRQ_PEK_RIS_EDGE,
> > > +};
> > > +
> > >  enum axp803_irqs {
> > >         AXP803_IRQ_ACIN_OVER_V = 1,
> > >         AXP803_IRQ_ACIN_PLUGIN,
> > > --
> > > 2.39.0
> > >
>

Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Shengyu Qu]

[-- Attachment #1.1.1: Type: text/plain, Size: 521 bytes --]

Hi Martin,
> +static const struct regmap_range axp313a_writeable_ranges[] = {
> +	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
No, according to datasheet, 0x00 reg is read-only.
> +};
> +
> +static const struct regmap_range axp313a_volatile_ranges[] = {
> +	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),

As Tsai said, no meaning to set all register to volatile.

Also, please check the datasheet I metioned in part3 reply, seems its 
newer than yours.

Best regards,

Shengyu


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6977 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 2/3] mfd: axp20x: Add support for AXP313a PMIC

[Martin Botka]

Hi Shengyu
On Sun, Feb 19 2023 at 01:52:50 AM +08:00:00, Shengyu Qu 
<wiagn233@outlook.com> wrote:
> Hi Martin,
>> +static const struct regmap_range axp313a_writeable_ranges[] = {
>> +	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> No, according to datasheet, 0x00 reg is read-only.
>> +};
>> +
>> +static const struct regmap_range axp313a_volatile_ranges[] = {
>> +	regmap_reg_range(AXP313A_ON_INDICATE, AXP313A_IRQ_STATE),
> 
> As Tsai said, no meaning to set all register to volatile.
ack
> 
> Also, please check the datasheet I metioned in part3 reply, seems its
> newer than yours.
ack
> 
> Best regards,
> 
> Shengyu
> 
Best regards,

Martin

[PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Martin Botka]

The AXP313a is your typical I2C controlled PMIC, although in a lighter
fashion compared to the other X-Powers PMICs: it has only three DCDC
rails, three LDOs, and no battery charging support.

The AXP313a datasheet does not describe a register to change the DCDC
switching frequency, and talks of it being fixed at 3 MHz. The BSP
driver hints at a register being able to change that, but we haven't
verified that, so leave that one out. It can be added later, if needed
and/or required.

The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
programmatically. On top of that, its voltage is customisable (either
1.8V or 3.3V), which we cannot describe easily using the existing
regulator wrapper functions. This should be fixed properly, using
regulator-{min,max}-microvolt in the DT, but this requires more changes
to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
same problem as well, we follow suit here and pretend it's a fixed 1.8V
regulator. A proper fix can follow later. The BSP code seems to ignore
this regulator altogether.

Describe the AXP313A's voltage settings and switch registers, how the
voltages are encoded, and connect this to the MFD device via its
regulator ID.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d260c442b788..3087bc98694f 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -134,6 +134,11 @@
 #define AXP22X_PWR_OUT_DLDO4_MASK	BIT_MASK(6)
 #define AXP22X_PWR_OUT_ALDO3_MASK	BIT_MASK(7)
 
+#define AXP313A_DCDC1_NUM_VOLTAGES	107
+#define AXP313A_DCDC23_NUM_VOLTAGES	88
+#define AXP313A_DCDC_V_OUT_MASK		GENMASK(6, 0)
+#define AXP313A_LDO_V_OUT_MASK		GENMASK(4, 0)
+
 #define AXP803_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
 #define AXP803_PWR_OUT_DCDC2_MASK	BIT_MASK(1)
 #define AXP803_PWR_OUT_DCDC3_MASK	BIT_MASK(2)
@@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
 	.ops		= &axp20x_ops_sw,
 };
 
+static const struct linear_range axp313a_dcdc1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
+	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
+};
+
+static const struct linear_range axp313a_dcdc2_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
+};
+
+/*
+ * This is deviating from the datasheet. The values here are taken from the
+ * BSP driver and have been confirmed by measurements.
+ */
+static const struct linear_range axp313a_dcdc3_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
+};
+
+static const struct regulator_desc axp313a_regulators[] = {
+	AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
+			axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
+			AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(0)),
+	AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
+			axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
+			AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(1)),
+	AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
+			axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
+			AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
+			AXP313A_OUTPUT_CONTROL, BIT(2)),
+	AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
+		 AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
+		 AXP313A_OUTPUT_CONTROL, BIT(3)),
+	AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
+		 AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
+		 AXP313A_OUTPUT_CONTROL, BIT(4)),
+	AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
+};
+
 /* DCDC ranges shared with AXP813 */
 static const struct linear_range axp803_dcdc234_ranges[] = {
 	REGULATOR_LINEAR_RANGE(500000,
@@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 		def = 3000;
 		step = 150;
 		break;
+	case AXP313A_ID:
+		/* The DCDC PWM frequency seems to be fixed to 3 MHz. */
+		if (dcdcfreq != 3000000 && dcdcfreq != 0) {
+			dev_err(&pdev->dev,
+				"DCDC frequency on AXP313a is fixed to 3 MHz.\n");
+			return -EINVAL;
+		}
+
+		return 0;
 	default:
 		dev_err(&pdev->dev,
 			"Setting DCDC frequency for unsupported AXP variant\n");
@@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 		drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
 						  "x-powers,drive-vbus-en");
 		break;
+	case AXP313A_ID:
+		regulators = axp313a_regulators;
+		nregulators = AXP313A_REG_ID_MAX;
+		break;
 	case AXP803_ID:
 		regulators = axp803_regulators;
 		nregulators = AXP803_REG_ID_MAX;
-- 
2.39.0

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Chen-Yu Tsai]

On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
<martin.botka@somainline.org> wrote:
>
> The AXP313a is your typical I2C controlled PMIC, although in a lighter
> fashion compared to the other X-Powers PMICs: it has only three DCDC
> rails, three LDOs, and no battery charging support.
>
> The AXP313a datasheet does not describe a register to change the DCDC
> switching frequency, and talks of it being fixed at 3 MHz. The BSP
> driver hints at a register being able to change that, but we haven't
> verified that, so leave that one out. It can be added later, if needed
> and/or required.

The datasheet released by MangoPi says this isn't configurable. The
thing that is configurable is spread-spectrum operation, and mode
switching between fixed PWM and hybrid PFM/PWM. So just drop the
DCDC frequency stuff and use the default code path.

> The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> programmatically. On top of that, its voltage is customisable (either
> 1.8V or 3.3V), which we cannot describe easily using the existing
> regulator wrapper functions. This should be fixed properly, using
> regulator-{min,max}-microvolt in the DT, but this requires more changes
> to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> same problem as well, we follow suit here and pretend it's a fixed 1.8V
> regulator. A proper fix can follow later. The BSP code seems to ignore
> this regulator altogether.
>
> Describe the AXP313A's voltage settings and switch registers, how the
> voltages are encoded, and connect this to the MFD device via its
> regulator ID.
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index d260c442b788..3087bc98694f 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -134,6 +134,11 @@
>  #define AXP22X_PWR_OUT_DLDO4_MASK      BIT_MASK(6)
>  #define AXP22X_PWR_OUT_ALDO3_MASK      BIT_MASK(7)
>
> +#define AXP313A_DCDC1_NUM_VOLTAGES     107
> +#define AXP313A_DCDC23_NUM_VOLTAGES    88
> +#define AXP313A_DCDC_V_OUT_MASK                GENMASK(6, 0)
> +#define AXP313A_LDO_V_OUT_MASK         GENMASK(4, 0)
> +
>  #define AXP803_PWR_OUT_DCDC1_MASK      BIT_MASK(0)
>  #define AXP803_PWR_OUT_DCDC2_MASK      BIT_MASK(1)
>  #define AXP803_PWR_OUT_DCDC3_MASK      BIT_MASK(2)
> @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
>         .ops            = &axp20x_ops_sw,
>  };
>
> +static const struct linear_range axp313a_dcdc1_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> +       REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> +       REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> +};
> +
> +static const struct linear_range axp313a_dcdc2_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> +       REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> +};
> +
> +/*
> + * This is deviating from the datasheet. The values here are taken from the
> + * BSP driver and have been confirmed by measurements.
> + */
> +static const struct linear_range axp313a_dcdc3_ranges[] = {
> +       REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> +       REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> +};
> +
> +static const struct regulator_desc axp313a_regulators[] = {
> +       AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> +                       axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> +                       AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +                       AXP313A_OUTPUT_CONTROL, BIT(0)),
> +       AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> +                       axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> +                       AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +                       AXP313A_OUTPUT_CONTROL, BIT(1)),
> +       AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> +                       axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> +                       AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> +                       AXP313A_OUTPUT_CONTROL, BIT(2)),
> +       AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
> +                AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> +                AXP313A_OUTPUT_CONTROL, BIT(3)),

The datasheet says this one is called ALDO1 ...

> +       AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> +                AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> +                AXP313A_OUTPUT_CONTROL, BIT(4)),

... and this one DLDO1.

> +       AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> +};
> +
>  /* DCDC ranges shared with AXP813 */
>  static const struct linear_range axp803_dcdc234_ranges[] = {
>         REGULATOR_LINEAR_RANGE(500000,
> @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>                 def = 3000;
>                 step = 150;
>                 break;
> +       case AXP313A_ID:
> +               /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> +               if (dcdcfreq != 3000000 && dcdcfreq != 0) {
> +                       dev_err(&pdev->dev,
> +                               "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> +                       return -EINVAL;
> +               }
> +
> +               return 0;

As mentioned above, please drop this.

Besides the bits mentioned above, this looks OK.

>         default:
>                 dev_err(&pdev->dev,
>                         "Setting DCDC frequency for unsupported AXP variant\n");
> @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>                 drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
>                                                   "x-powers,drive-vbus-en");
>                 break;
> +       case AXP313A_ID:
> +               regulators = axp313a_regulators;
> +               nregulators = AXP313A_REG_ID_MAX;
> +               break;
>         case AXP803_ID:
>                 regulators = axp803_regulators;
>                 nregulators = AXP803_REG_ID_MAX;
> --
> 2.39.0
>

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Shengyu Qu]

[-- Attachment #1.1.1: Type: text/plain, Size: 1135 bytes --]

Hi Martin,

> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> <martin.botka@somainline.org> wrote:
>> The AXP313a is your typical I2C controlled PMIC, although in a lighter
>> fashion compared to the other X-Powers PMICs: it has only three DCDC
>> rails, three LDOs, and no battery charging support.
>>
>> The AXP313a datasheet does not describe a register to change the DCDC
>> switching frequency, and talks of it being fixed at 3 MHz. The BSP
>> driver hints at a register being able to change that, but we haven't
>> verified that, so leave that one out. It can be added later, if needed
>> and/or required.
> The datasheet released by MangoPi says this isn't configurable. The
> thing that is configurable is spread-spectrum operation, and mode
> switching between fixed PWM and hybrid PFM/PWM. So just drop the
> DCDC frequency stuff and use the default code path.

You could get full datasheet of AXP313A here:

https://github.com/YuzukiHD/YuzukiChameleon/blob/master/Datasheet/AXP313A_Datasheet_V1.0_cn.pdf

Btw I'm working on AXP15060 support mostly based on your series.


Best regards,

Shengyu


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6977 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Martin Botka]

Hi Shengyu,
On Sat, Feb 18 2023 at 06:08:06 PM +08:00:00, Shengyu Qu 
<wiagn233@outlook.com> wrote:
> Hi Martin,
> 
>> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
>> <martin.botka@somainline.org> wrote:
>>> The AXP313a is your typical I2C controlled PMIC, although in a 
>>> lighter
>>> fashion compared to the other X-Powers PMICs: it has only three DCDC
>>> rails, three LDOs, and no battery charging support.
>>> 
>>> The AXP313a datasheet does not describe a register to change the 
>>> DCDC
>>> switching frequency, and talks of it being fixed at 3 MHz. The BSP
>>> driver hints at a register being able to change that, but we haven't
>>> verified that, so leave that one out. It can be added later, if 
>>> needed
>>> and/or required.
>> The datasheet released by MangoPi says this isn't configurable. The
>> thing that is configurable is spread-spectrum operation, and mode
>> switching between fixed PWM and hybrid PFM/PWM. So just drop the
>> DCDC frequency stuff and use the default code path.
> 
> You could get full datasheet of AXP313A here:
> 
> https://github.com/YuzukiHD/YuzukiChameleon/blob/master/Datasheet/AXP313A_Datasheet_V1.0_cn.pdf

I do have the datasheet but maybe this one is more up to date somehow. 
Will have to check.
> 
> Btw I'm working on AXP15060 support mostly based on your series.
Lovely to hear. So sorry for the very very late reply. New semester 
began 3 weeks ago and been quite the ride.
Would love to get the series more up to date in the upcoming weeks :) I 
will see what time allows :)
> 
> 
> Best regards,
> 
> Shengyu
Best regards,

Martin

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Andre Przywara]

On Sat, 28 Jan 2023 01:24:18 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi,

> On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> <martin.botka@somainline.org> wrote:
> >
> > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > rails, three LDOs, and no battery charging support.
> >
> > The AXP313a datasheet does not describe a register to change the DCDC
> > switching frequency, and talks of it being fixed at 3 MHz. The BSP
> > driver hints at a register being able to change that, but we haven't
> > verified that, so leave that one out. It can be added later, if needed
> > and/or required.  
> 
> The datasheet released by MangoPi says this isn't configurable. The
> thing that is configurable is spread-spectrum operation, and mode
> switching between fixed PWM and hybrid PFM/PWM. So just drop the
> DCDC frequency stuff and use the default code path.

The default code path is fatal to the driver, so we can't really do this.
axp20x_set_dcdc_freq is *always* called, even when the property is missing,
in this case the frequency will just be 0.
If we don't specify the variant ID in the switch/case, we get an error and
the driver bails out with -EINVAL.
So the minimal implementation would be:
	case AXP313A_ID:
		return 0;
To be a bit more robust and catch cases where people try to specify some
DCDC frequency, I added this extra check for 3MHz or 0 (no property).

> > The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> > programmatically. On top of that, its voltage is customisable (either
> > 1.8V or 3.3V), which we cannot describe easily using the existing
> > regulator wrapper functions. This should be fixed properly, using
> > regulator-{min,max}-microvolt in the DT, but this requires more changes
> > to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> > same problem as well, we follow suit here and pretend it's a fixed 1.8V
> > regulator. A proper fix can follow later. The BSP code seems to ignore
> > this regulator altogether.
> >
> > Describe the AXP313A's voltage settings and switch registers, how the
> > voltages are encoded, and connect this to the MFD device via its
> > regulator ID.
> >
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > index d260c442b788..3087bc98694f 100644
> > --- a/drivers/regulator/axp20x-regulator.c
> > +++ b/drivers/regulator/axp20x-regulator.c
> > @@ -134,6 +134,11 @@
> >  #define AXP22X_PWR_OUT_DLDO4_MASK      BIT_MASK(6)
> >  #define AXP22X_PWR_OUT_ALDO3_MASK      BIT_MASK(7)
> >
> > +#define AXP313A_DCDC1_NUM_VOLTAGES     107
> > +#define AXP313A_DCDC23_NUM_VOLTAGES    88
> > +#define AXP313A_DCDC_V_OUT_MASK                GENMASK(6, 0)
> > +#define AXP313A_LDO_V_OUT_MASK         GENMASK(4, 0)
> > +
> >  #define AXP803_PWR_OUT_DCDC1_MASK      BIT_MASK(0)
> >  #define AXP803_PWR_OUT_DCDC2_MASK      BIT_MASK(1)
> >  #define AXP803_PWR_OUT_DCDC3_MASK      BIT_MASK(2)
> > @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
> >         .ops            = &axp20x_ops_sw,
> >  };
> >
> > +static const struct linear_range axp313a_dcdc1_ranges[] = {
> > +       REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> > +       REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> > +       REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> > +};
> > +
> > +static const struct linear_range axp313a_dcdc2_ranges[] = {
> > +       REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> > +       REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> > +};
> > +
> > +/*
> > + * This is deviating from the datasheet. The values here are taken from the
> > + * BSP driver and have been confirmed by measurements.
> > + */
> > +static const struct linear_range axp313a_dcdc3_ranges[] = {
> > +       REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> > +       REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> > +};
> > +
> > +static const struct regulator_desc axp313a_regulators[] = {
> > +       AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> > +                       axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> > +                       AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > +                       AXP313A_OUTPUT_CONTROL, BIT(0)),
> > +       AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> > +                       axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > +                       AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > +                       AXP313A_OUTPUT_CONTROL, BIT(1)),
> > +       AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> > +                       axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > +                       AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > +                       AXP313A_OUTPUT_CONTROL, BIT(2)),
> > +       AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > +                AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > +                AXP313A_OUTPUT_CONTROL, BIT(3)),  
> 
> The datasheet says this one is called ALDO1 ...
> 
> > +       AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > +                AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > +                AXP313A_OUTPUT_CONTROL, BIT(4)),  
> 
> ... and this one DLDO1.

Fixed.


> > +       AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> > +};
> > +
> >  /* DCDC ranges shared with AXP813 */
> >  static const struct linear_range axp803_dcdc234_ranges[] = {
> >         REGULATOR_LINEAR_RANGE(500000,
> > @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> >                 def = 3000;
> >                 step = 150;
> >                 break;
> > +       case AXP313A_ID:
> > +               /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> > +               if (dcdcfreq != 3000000 && dcdcfreq != 0) {
> > +                       dev_err(&pdev->dev,
> > +                               "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +               return 0;  
> 
> As mentioned above, please drop this.

As mentioned above, we need at least the variant ID and a "return 0;". Do
you want me to drop the extra checks as well? Doesn't really hurt, and
provides extra info in case people try something stupid.

> Besides the bits mentioned above, this looks OK.

Thanks!
Andre

> 
> >         default:
> >                 dev_err(&pdev->dev,
> >                         "Setting DCDC frequency for unsupported AXP variant\n");
> > @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> >                 drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
> >                                                   "x-powers,drive-vbus-en");
> >                 break;
> > +       case AXP313A_ID:
> > +               regulators = axp313a_regulators;
> > +               nregulators = AXP313A_REG_ID_MAX;
> > +               break;
> >         case AXP803_ID:
> >                 regulators = axp803_regulators;
> >                 nregulators = AXP803_REG_ID_MAX;
> > --
> > 2.39.0
> >  

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Chen-Yu Tsai]

On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Sat, 28 Jan 2023 01:24:18 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
> Hi,
>
> > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > <martin.botka@somainline.org> wrote:
> > >
> > > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > > rails, three LDOs, and no battery charging support.
> > >
> > > The AXP313a datasheet does not describe a register to change the DCDC
> > > switching frequency, and talks of it being fixed at 3 MHz. The BSP
> > > driver hints at a register being able to change that, but we haven't
> > > verified that, so leave that one out. It can be added later, if needed
> > > and/or required.
> >
> > The datasheet released by MangoPi says this isn't configurable. The
> > thing that is configurable is spread-spectrum operation, and mode
> > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > DCDC frequency stuff and use the default code path.
>
> The default code path is fatal to the driver, so we can't really do this.
> axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> in this case the frequency will just be 0.
> If we don't specify the variant ID in the switch/case, we get an error and
> the driver bails out with -EINVAL.
> So the minimal implementation would be:
>         case AXP313A_ID:
>                 return 0;
> To be a bit more robust and catch cases where people try to specify some
> DCDC frequency, I added this extra check for 3MHz or 0 (no property).

Given that it's fixed, it really shouldn't be specified as a property.

> > > The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> > > programmatically. On top of that, its voltage is customisable (either
> > > 1.8V or 3.3V), which we cannot describe easily using the existing
> > > regulator wrapper functions. This should be fixed properly, using
> > > regulator-{min,max}-microvolt in the DT, but this requires more changes
> > > to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> > > same problem as well, we follow suit here and pretend it's a fixed 1.8V
> > > regulator. A proper fix can follow later. The BSP code seems to ignore
> > > this regulator altogether.
> > >
> > > Describe the AXP313A's voltage settings and switch registers, how the
> > > voltages are encoded, and connect this to the MFD device via its
> > > regulator ID.
> > >
> > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > >  1 file changed, 60 insertions(+)
> > >
> > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > index d260c442b788..3087bc98694f 100644
> > > --- a/drivers/regulator/axp20x-regulator.c
> > > +++ b/drivers/regulator/axp20x-regulator.c
> > > @@ -134,6 +134,11 @@
> > >  #define AXP22X_PWR_OUT_DLDO4_MASK      BIT_MASK(6)
> > >  #define AXP22X_PWR_OUT_ALDO3_MASK      BIT_MASK(7)
> > >
> > > +#define AXP313A_DCDC1_NUM_VOLTAGES     107
> > > +#define AXP313A_DCDC23_NUM_VOLTAGES    88
> > > +#define AXP313A_DCDC_V_OUT_MASK                GENMASK(6, 0)
> > > +#define AXP313A_LDO_V_OUT_MASK         GENMASK(4, 0)
> > > +
> > >  #define AXP803_PWR_OUT_DCDC1_MASK      BIT_MASK(0)
> > >  #define AXP803_PWR_OUT_DCDC2_MASK      BIT_MASK(1)
> > >  #define AXP803_PWR_OUT_DCDC3_MASK      BIT_MASK(2)
> > > @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
> > >         .ops            = &axp20x_ops_sw,
> > >  };
> > >
> > > +static const struct linear_range axp313a_dcdc1_ranges[] = {
> > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> > > +       REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> > > +       REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> > > +};
> > > +
> > > +static const struct linear_range axp313a_dcdc2_ranges[] = {
> > > +       REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> > > +};
> > > +
> > > +/*
> > > + * This is deviating from the datasheet. The values here are taken from the
> > > + * BSP driver and have been confirmed by measurements.
> > > + */
> > > +static const struct linear_range axp313a_dcdc3_ranges[] = {
> > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> > > +};
> > > +
> > > +static const struct regulator_desc axp313a_regulators[] = {
> > > +       AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> > > +                       axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> > > +                       AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > +                       AXP313A_OUTPUT_CONTROL, BIT(0)),
> > > +       AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> > > +                       axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > +                       AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > +                       AXP313A_OUTPUT_CONTROL, BIT(1)),
> > > +       AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> > > +                       axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > +                       AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > +                       AXP313A_OUTPUT_CONTROL, BIT(2)),
> > > +       AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > +                AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > +                AXP313A_OUTPUT_CONTROL, BIT(3)),
> >
> > The datasheet says this one is called ALDO1 ...
> >
> > > +       AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > +                AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > +                AXP313A_OUTPUT_CONTROL, BIT(4)),
> >
> > ... and this one DLDO1.
>
> Fixed.
>
>
> > > +       AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> > > +};
> > > +
> > >  /* DCDC ranges shared with AXP813 */
> > >  static const struct linear_range axp803_dcdc234_ranges[] = {
> > >         REGULATOR_LINEAR_RANGE(500000,
> > > @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> > >                 def = 3000;
> > >                 step = 150;
> > >                 break;
> > > +       case AXP313A_ID:
> > > +               /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> > > +               if (dcdcfreq != 3000000 && dcdcfreq != 0) {
> > > +                       dev_err(&pdev->dev,
> > > +                               "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > +                       return -EINVAL;
> > > +               }
> > > +
> > > +               return 0;
> >
> > As mentioned above, please drop this.
>
> As mentioned above, we need at least the variant ID and a "return 0;". Do
> you want me to drop the extra checks as well? Doesn't really hurt, and
> provides extra info in case people try something stupid.

Ah, OK. In that case let's allow the "no property" case, and error out
for all any other value set.

ChenYu

> > Besides the bits mentioned above, this looks OK.
>
> Thanks!
> Andre
>
> >
> > >         default:
> > >                 dev_err(&pdev->dev,
> > >                         "Setting DCDC frequency for unsupported AXP variant\n");
> > > @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> > >                 drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
> > >                                                   "x-powers,drive-vbus-en");
> > >                 break;
> > > +       case AXP313A_ID:
> > > +               regulators = axp313a_regulators;
> > > +               nregulators = AXP313A_REG_ID_MAX;
> > > +               break;
> > >         case AXP803_ID:
> > >                 regulators = axp803_regulators;
> > >                 nregulators = AXP803_REG_ID_MAX;
> > > --
> > > 2.39.0
> > >
>

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Andre Przywara]

On Thu, 23 Mar 2023 22:21:44 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi,

> On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Sat, 28 Jan 2023 01:24:18 +0800
> > Chen-Yu Tsai <wens@csie.org> wrote:
> >
> > Hi,
> >  
> > > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > > <martin.botka@somainline.org> wrote:  
> > > >
> > > > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > > > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > > > rails, three LDOs, and no battery charging support.
> > > >
> > > > The AXP313a datasheet does not describe a register to change the DCDC
> > > > switching frequency, and talks of it being fixed at 3 MHz. The BSP
> > > > driver hints at a register being able to change that, but we haven't
> > > > verified that, so leave that one out. It can be added later, if needed
> > > > and/or required.  
> > >
> > > The datasheet released by MangoPi says this isn't configurable. The
> > > thing that is configurable is spread-spectrum operation, and mode
> > > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > > DCDC frequency stuff and use the default code path.  
> >
> > The default code path is fatal to the driver, so we can't really do this.
> > axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> > in this case the frequency will just be 0.
> > If we don't specify the variant ID in the switch/case, we get an error and
> > the driver bails out with -EINVAL.
> > So the minimal implementation would be:
> >         case AXP313A_ID:
> >                 return 0;
> > To be a bit more robust and catch cases where people try to specify some
> > DCDC frequency, I added this extra check for 3MHz or 0 (no property).  
> 
> Given that it's fixed, it really shouldn't be specified as a property.

I agree, I guess this means we need to disallow it in the DT binding then?

> 
> > > > The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> > > > programmatically. On top of that, its voltage is customisable (either
> > > > 1.8V or 3.3V), which we cannot describe easily using the existing
> > > > regulator wrapper functions. This should be fixed properly, using
> > > > regulator-{min,max}-microvolt in the DT, but this requires more changes
> > > > to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> > > > same problem as well, we follow suit here and pretend it's a fixed 1.8V
> > > > regulator. A proper fix can follow later. The BSP code seems to ignore
> > > > this regulator altogether.
> > > >
> > > > Describe the AXP313A's voltage settings and switch registers, how the
> > > > voltages are encoded, and connect this to the MFD device via its
> > > > regulator ID.
> > > >
> > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > ---
> > > >  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > > >  1 file changed, 60 insertions(+)
> > > >
> > > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > > index d260c442b788..3087bc98694f 100644
> > > > --- a/drivers/regulator/axp20x-regulator.c
> > > > +++ b/drivers/regulator/axp20x-regulator.c
> > > > @@ -134,6 +134,11 @@
> > > >  #define AXP22X_PWR_OUT_DLDO4_MASK      BIT_MASK(6)
> > > >  #define AXP22X_PWR_OUT_ALDO3_MASK      BIT_MASK(7)
> > > >
> > > > +#define AXP313A_DCDC1_NUM_VOLTAGES     107
> > > > +#define AXP313A_DCDC23_NUM_VOLTAGES    88
> > > > +#define AXP313A_DCDC_V_OUT_MASK                GENMASK(6, 0)
> > > > +#define AXP313A_LDO_V_OUT_MASK         GENMASK(4, 0)
> > > > +
> > > >  #define AXP803_PWR_OUT_DCDC1_MASK      BIT_MASK(0)
> > > >  #define AXP803_PWR_OUT_DCDC2_MASK      BIT_MASK(1)
> > > >  #define AXP803_PWR_OUT_DCDC3_MASK      BIT_MASK(2)
> > > > @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
> > > >         .ops            = &axp20x_ops_sw,
> > > >  };
> > > >
> > > > +static const struct linear_range axp313a_dcdc1_ranges[] = {
> > > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> > > > +       REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> > > > +       REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> > > > +};
> > > > +
> > > > +static const struct linear_range axp313a_dcdc2_ranges[] = {
> > > > +       REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> > > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> > > > +};
> > > > +
> > > > +/*
> > > > + * This is deviating from the datasheet. The values here are taken from the
> > > > + * BSP driver and have been confirmed by measurements.
> > > > + */
> > > > +static const struct linear_range axp313a_dcdc3_ranges[] = {
> > > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> > > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> > > > +};
> > > > +
> > > > +static const struct regulator_desc axp313a_regulators[] = {
> > > > +       AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> > > > +                       axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> > > > +                       AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > +                       AXP313A_OUTPUT_CONTROL, BIT(0)),
> > > > +       AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> > > > +                       axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > > +                       AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > +                       AXP313A_OUTPUT_CONTROL, BIT(1)),
> > > > +       AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> > > > +                       axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > > +                       AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > +                       AXP313A_OUTPUT_CONTROL, BIT(2)),
> > > > +       AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > > +                AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > +                AXP313A_OUTPUT_CONTROL, BIT(3)),  
> > >
> > > The datasheet says this one is called ALDO1 ...
> > >  
> > > > +       AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > > +                AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > +                AXP313A_OUTPUT_CONTROL, BIT(4)),  
> > >
> > > ... and this one DLDO1.  
> >
> > Fixed.
> >
> >  
> > > > +       AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> > > > +};
> > > > +
> > > >  /* DCDC ranges shared with AXP813 */
> > > >  static const struct linear_range axp803_dcdc234_ranges[] = {
> > > >         REGULATOR_LINEAR_RANGE(500000,
> > > > @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> > > >                 def = 3000;
> > > >                 step = 150;
> > > >                 break;
> > > > +       case AXP313A_ID:
> > > > +               /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> > > > +               if (dcdcfreq != 3000000 && dcdcfreq != 0) {
> > > > +                       dev_err(&pdev->dev,
> > > > +                               "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > > +                       return -EINVAL;
> > > > +               }
> > > > +
> > > > +               return 0;  
> > >
> > > As mentioned above, please drop this.  
> >
> > As mentioned above, we need at least the variant ID and a "return 0;". Do
> > you want me to drop the extra checks as well? Doesn't really hurt, and
> > provides extra info in case people try something stupid.  
> 
> Ah, OK. In that case let's allow the "no property" case, and error out
> for all any other value set.

OK, will do.

Thanks,
Andre

> 
> ChenYu
> 
> > > Besides the bits mentioned above, this looks OK.  
> >
> > Thanks!
> > Andre
> >  
> > >  
> > > >         default:
> > > >                 dev_err(&pdev->dev,
> > > >                         "Setting DCDC frequency for unsupported AXP variant\n");
> > > > @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> > > >                 drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
> > > >                                                   "x-powers,drive-vbus-en");
> > > >                 break;
> > > > +       case AXP313A_ID:
> > > > +               regulators = axp313a_regulators;
> > > > +               nregulators = AXP313A_REG_ID_MAX;
> > > > +               break;
> > > >         case AXP803_ID:
> > > >                 regulators = axp803_regulators;
> > > >                 nregulators = AXP803_REG_ID_MAX;
> > > > --
> > > > 2.39.0
> > > >  
> >  

Re: [PATCH v8 3/3] regulator: axp20x: Add support for AXP313a variant

[Chen-Yu Tsai]

On Fri, Mar 24, 2023 at 12:07 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 23 Mar 2023 22:21:44 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
> Hi,
>
> > On Thu, Mar 23, 2023 at 9:59 PM Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > On Sat, 28 Jan 2023 01:24:18 +0800
> > > Chen-Yu Tsai <wens@csie.org> wrote:
> > >
> > > Hi,
> > >
> > > > On Sat, Jan 21, 2023 at 2:45 AM Martin Botka
> > > > <martin.botka@somainline.org> wrote:
> > > > >
> > > > > The AXP313a is your typical I2C controlled PMIC, although in a lighter
> > > > > fashion compared to the other X-Powers PMICs: it has only three DCDC
> > > > > rails, three LDOs, and no battery charging support.
> > > > >
> > > > > The AXP313a datasheet does not describe a register to change the DCDC
> > > > > switching frequency, and talks of it being fixed at 3 MHz. The BSP
> > > > > driver hints at a register being able to change that, but we haven't
> > > > > verified that, so leave that one out. It can be added later, if needed
> > > > > and/or required.
> > > >
> > > > The datasheet released by MangoPi says this isn't configurable. The
> > > > thing that is configurable is spread-spectrum operation, and mode
> > > > switching between fixed PWM and hybrid PFM/PWM. So just drop the
> > > > DCDC frequency stuff and use the default code path.
> > >
> > > The default code path is fatal to the driver, so we can't really do this.
> > > axp20x_set_dcdc_freq is *always* called, even when the property is missing,
> > > in this case the frequency will just be 0.
> > > If we don't specify the variant ID in the switch/case, we get an error and
> > > the driver bails out with -EINVAL.
> > > So the minimal implementation would be:
> > >         case AXP313A_ID:
> > >                 return 0;
> > > To be a bit more robust and catch cases where people try to specify some
> > > DCDC frequency, I added this extra check for 3MHz or 0 (no property).
> >
> > Given that it's fixed, it really shouldn't be specified as a property.
>
> I agree, I guess this means we need to disallow it in the DT binding then?

That's right.

> >
> > > > > The third LDO, RTCLDO, is fixed, and cannot even be turned on or off,
> > > > > programmatically. On top of that, its voltage is customisable (either
> > > > > 1.8V or 3.3V), which we cannot describe easily using the existing
> > > > > regulator wrapper functions. This should be fixed properly, using
> > > > > regulator-{min,max}-microvolt in the DT, but this requires more changes
> > > > > to the code. As some other PMICs (AXP2xx, AXP803) seem to paper over the
> > > > > same problem as well, we follow suit here and pretend it's a fixed 1.8V
> > > > > regulator. A proper fix can follow later. The BSP code seems to ignore
> > > > > this regulator altogether.
> > > > >
> > > > > Describe the AXP313A's voltage settings and switch registers, how the
> > > > > voltages are encoded, and connect this to the MFD device via its
> > > > > regulator ID.
> > > > >
> > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > > ---
> > > > >  drivers/regulator/axp20x-regulator.c | 60 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 60 insertions(+)
> > > > >
> > > > > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > > > > index d260c442b788..3087bc98694f 100644
> > > > > --- a/drivers/regulator/axp20x-regulator.c
> > > > > +++ b/drivers/regulator/axp20x-regulator.c
> > > > > @@ -134,6 +134,11 @@
> > > > >  #define AXP22X_PWR_OUT_DLDO4_MASK      BIT_MASK(6)
> > > > >  #define AXP22X_PWR_OUT_ALDO3_MASK      BIT_MASK(7)
> > > > >
> > > > > +#define AXP313A_DCDC1_NUM_VOLTAGES     107
> > > > > +#define AXP313A_DCDC23_NUM_VOLTAGES    88
> > > > > +#define AXP313A_DCDC_V_OUT_MASK                GENMASK(6, 0)
> > > > > +#define AXP313A_LDO_V_OUT_MASK         GENMASK(4, 0)
> > > > > +
> > > > >  #define AXP803_PWR_OUT_DCDC1_MASK      BIT_MASK(0)
> > > > >  #define AXP803_PWR_OUT_DCDC2_MASK      BIT_MASK(1)
> > > > >  #define AXP803_PWR_OUT_DCDC3_MASK      BIT_MASK(2)
> > > > > @@ -638,6 +643,48 @@ static const struct regulator_desc axp22x_drivevbus_regulator = {
> > > > >         .ops            = &axp20x_ops_sw,
> > > > >  };
> > > > >
> > > > > +static const struct linear_range axp313a_dcdc1_ranges[] = {
> > > > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> > > > > +       REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> > > > > +       REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> > > > > +};
> > > > > +
> > > > > +static const struct linear_range axp313a_dcdc2_ranges[] = {
> > > > > +       REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> > > > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * This is deviating from the datasheet. The values here are taken from the
> > > > > + * BSP driver and have been confirmed by measurements.
> > > > > + */
> > > > > +static const struct linear_range axp313a_dcdc3_ranges[] = {
> > > > > +       REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> > > > > +       REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> > > > > +};
> > > > > +
> > > > > +static const struct regulator_desc axp313a_regulators[] = {
> > > > > +       AXP_DESC_RANGES(AXP313A, DCDC1, "dcdc1", "vin1",
> > > > > +                       axp313a_dcdc1_ranges, AXP313A_DCDC1_NUM_VOLTAGES,
> > > > > +                       AXP313A_DCDC1_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > > +                       AXP313A_OUTPUT_CONTROL, BIT(0)),
> > > > > +       AXP_DESC_RANGES(AXP313A, DCDC2, "dcdc2", "vin2",
> > > > > +                       axp313a_dcdc2_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > > > +                       AXP313A_DCDC2_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > > +                       AXP313A_OUTPUT_CONTROL, BIT(1)),
> > > > > +       AXP_DESC_RANGES(AXP313A, DCDC3, "dcdc3", "vin3",
> > > > > +                       axp313a_dcdc3_ranges, AXP313A_DCDC23_NUM_VOLTAGES,
> > > > > +                       AXP313A_DCDC3_CONRTOL, AXP313A_DCDC_V_OUT_MASK,
> > > > > +                       AXP313A_OUTPUT_CONTROL, BIT(2)),
> > > > > +       AXP_DESC(AXP313A, LDO1, "ldo1", "vin1", 500, 3500, 100,
> > > > > +                AXP313A_ALDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > > +                AXP313A_OUTPUT_CONTROL, BIT(3)),
> > > >
> > > > The datasheet says this one is called ALDO1 ...
> > > >
> > > > > +       AXP_DESC(AXP313A, LDO2, "ldo2", "vin1", 500, 3500, 100,
> > > > > +                AXP313A_DLDO1_CONRTOL, AXP313A_LDO_V_OUT_MASK,
> > > > > +                AXP313A_OUTPUT_CONTROL, BIT(4)),
> > > >
> > > > ... and this one DLDO1.
> > >
> > > Fixed.
> > >
> > >
> > > > > +       AXP_DESC_FIXED(AXP313A, RTC_LDO, "rtc-ldo", "vin1", 1800),
> > > > > +};
> > > > > +
> > > > >  /* DCDC ranges shared with AXP813 */
> > > > >  static const struct linear_range axp803_dcdc234_ranges[] = {
> > > > >         REGULATOR_LINEAR_RANGE(500000,
> > > > > @@ -1040,6 +1087,15 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> > > > >                 def = 3000;
> > > > >                 step = 150;
> > > > >                 break;
> > > > > +       case AXP313A_ID:
> > > > > +               /* The DCDC PWM frequency seems to be fixed to 3 MHz. */
> > > > > +               if (dcdcfreq != 3000000 && dcdcfreq != 0) {
> > > > > +                       dev_err(&pdev->dev,
> > > > > +                               "DCDC frequency on AXP313a is fixed to 3 MHz.\n");
> > > > > +                       return -EINVAL;
> > > > > +               }
> > > > > +
> > > > > +               return 0;
> > > >
> > > > As mentioned above, please drop this.
> > >
> > > As mentioned above, we need at least the variant ID and a "return 0;". Do
> > > you want me to drop the extra checks as well? Doesn't really hurt, and
> > > provides extra info in case people try something stupid.
> >
> > Ah, OK. In that case let's allow the "no property" case, and error out
> > for all any other value set.
>
> OK, will do.
>
> Thanks,
> Andre
>
> >
> > ChenYu
> >
> > > > Besides the bits mentioned above, this looks OK.
> > >
> > > Thanks!
> > > Andre
> > >
> > > >
> > > > >         default:
> > > > >                 dev_err(&pdev->dev,
> > > > >                         "Setting DCDC frequency for unsupported AXP variant\n");
> > > > > @@ -1232,6 +1288,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> > > > >                 drivevbus = of_property_read_bool(pdev->dev.parent->of_node,
> > > > >                                                   "x-powers,drive-vbus-en");
> > > > >                 break;
> > > > > +       case AXP313A_ID:
> > > > > +               regulators = axp313a_regulators;
> > > > > +               nregulators = AXP313A_REG_ID_MAX;
> > > > > +               break;
> > > > >         case AXP803_ID:
> > > > >                 regulators = axp803_regulators;
> > > > >                 nregulators = AXP803_REG_ID_MAX;
> > > > > --
> > > > > 2.39.0
> > > > >
> > >
>