[PATCH v5 0/3] AXP1530 PMIC

[PATCH v5 0/3] AXP1530 PMIC

[Martin Botka]

Hello,

This patch series adds support for the AXP1530 PMIC which is controlled via the
I2C bus.

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

Changes in v3:
Move AXP1530 dt-binding to alphabetical order

Changes in v4:
Fix indentation

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

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

 .../bindings/mfd/x-powers,axp152.yaml         |  1 +
 drivers/mfd/axp20x-i2c.c                      |  2 +
 drivers/mfd/axp20x.c                          | 62 +++++++++++++++++++
 drivers/regulator/axp20x-regulator.c          | 44 +++++++++++++
 include/linux/mfd/axp20x.h                    | 34 ++++++++++
 5 files changed, 143 insertions(+)

-- 
2.38.1

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

[Martin Botka]

AXP1530 is a PMIC used on board BIQU CB1 SoM

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 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..7559a13a071c 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -84,6 +84,7 @@ properties:
     oneOf:
       - enum:
           - x-powers,axp152
+          - x-powers,axp1530
           - x-powers,axp202
           - x-powers,axp209
           - x-powers,axp221
-- 
2.38.1

[PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Martin Botka]

AXP1530 is a PMIC chip produced by X-Powers and an be connected via
I2C bus.
Where AXP313A seems to be closely related so the same driver can be used and
seen it only paired with H616 SoC.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/mfd/axp20x-i2c.c   |  2 ++
 drivers/mfd/axp20x.c       | 62 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
index 8fd6727dc30a..6bfb931a580e 100644
--- a/drivers/mfd/axp20x-i2c.c
+++ b/drivers/mfd/axp20x-i2c.c
@@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
 #ifdef CONFIG_OF
 static const struct of_device_id axp20x_i2c_of_match[] = {
 	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
+	{ .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
 	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
 	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
 	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
@@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
 
 static const struct i2c_device_id axp20x_i2c_id[] = {
 	{ "axp152", 0 },
+	{ "axp1530", 0 },
 	{ "axp202", 0 },
 	{ "axp209", 0 },
 	{ "axp221", 0 },
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 880c41fa7021..6caa7e87ad80 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -34,6 +34,7 @@
 
 static const char * const axp20x_model_names[] = {
 	"AXP152",
+	"AXP1530",
 	"AXP202",
 	"AXP209",
 	"AXP221",
@@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
 };
 
+static const struct regmap_range axp1530_writeable_ranges[] = {
+	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
+};
+
+static const struct regmap_range axp1530_volatile_ranges[] = {
+	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
+};
+
+static const struct regmap_access_table axp1530_writeable_table = {
+	.yes_ranges = axp1530_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
+};
+
+static const struct regmap_access_table axp1530_volatile_table = {
+	.yes_ranges = axp1530_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
+};
+
 static const struct regmap_range axp20x_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
 	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
@@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config axp1530_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.wr_table = &axp1530_writeable_table,
+	.volatile_table = &axp1530_volatile_table,
+	.max_register = AXP1530_FREQUENCY,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static const struct regmap_config axp20x_regmap_config = {
 	.reg_bits	= 8,
 	.val_bits	= 8,
@@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
 };
 
+static const struct regmap_irq axp1530_regmap_irqs[] = {
+	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
+	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
+	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
+	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),
+	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
+	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
+	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
+};
+
 static const struct regmap_irq axp20x_regmap_irqs[] = {
 	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
 	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
@@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
 	.num_regs		= 3,
 };
 
+static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
+	.name = "axp1530_irq_chip",
+	.status_base = AXP1530_IRQ_STATUS1,
+	.ack_base = AXP1530_IRQ_STATUS1,
+	.mask_base = AXP1530_IRQ_ENABLE1,
+	.mask_invert = true,
+	.init_ack_masked = true,
+	.irqs = axp1530_regmap_irqs,
+	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
+	.num_regs = 1,
+};
+
 static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
 	.name			= "axp20x_irq_chip",
 	.status_base		= AXP20X_IRQ1_STATE,
@@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
 	},
 };
 
+static struct mfd_cell axp1530_cells[] = {
+	{
+		.name = "axp20x-regulator",
+	},
+};
+
 static const struct resource axp288_adc_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
 };
@@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_cfg = &axp152_regmap_config;
 		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
 		break;
+	case AXP1530_ID:
+		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
+		axp20x->cells = axp1530_cells;
+		axp20x->regmap_cfg = &axp1530_regmap_config;
+		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
+		break;
 	case AXP202_ID:
 	case AXP209_ID:
 		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 9ab0e2fca7ea..cad25754500f 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -12,6 +12,7 @@
 
 enum axp20x_variants {
 	AXP152_ID = 0,
+	AXP1530_ID,
 	AXP202_ID,
 	AXP209_ID,
 	AXP221_ID,
@@ -45,6 +46,18 @@ enum axp20x_variants {
 #define AXP152_DCDC_FREQ		0x37
 #define AXP152_DCDC_MODE		0x80
 
+#define AXP1530_ON_INDICATE		0x00
+#define AXP1530_OUTPUT_CONTROL	0x10
+#define AXP1530_DCDC1_CONRTOL	0x13
+#define AXP1530_DCDC2_CONRTOL	0x14
+#define AXP1530_DCDC3_CONRTOL	0x15
+#define AXP1530_ALDO1_CONRTOL	0x16
+#define AXP1530_DLDO1_CONRTOL	0x17
+#define AXP1530_OUTOUT_MONITOR	0x1D
+#define AXP1530_IRQ_ENABLE1		0x20
+#define AXP1530_IRQ_STATUS1		0x21
+#define AXP1530_FREQUENCY		0x87
+
 #define AXP20X_PWR_INPUT_STATUS		0x00
 #define AXP20X_PWR_OP_MODE		0x01
 #define AXP20X_USB_OTG_STATUS		0x02
@@ -287,6 +300,15 @@ enum axp20x_variants {
 #define AXP288_FG_TUNE5             0xed
 
 /* Regulators IDs */
+enum {
+	AXP1530_DCDC1 = 0,
+	AXP1530_DCDC2,
+	AXP1530_DCDC3,
+	AXP1530_LDO1,
+	AXP1530_LDO2,
+	AXP1530_REG_ID_MAX,
+};
+
 enum {
 	AXP20X_LDO1 = 0,
 	AXP20X_LDO2,
@@ -440,6 +462,16 @@ enum {
 	AXP152_IRQ_GPIO0_INPUT,
 };
 
+enum axp1530_irqs {
+	AXP1530_IRQ_TEMP_OVER,
+	AXP1530_IRQ_DCDC2_UNDER = 2,
+	AXP1530_IRQ_DCDC3_UNDER,
+	AXP1530_IRQ_POKLIRQ_EN,
+	AXP1530_IRQ_POKSIRQ_EN,
+	AXP1530_IRQ_KEY_L2H_EN,
+	AXP1530_IRQ_KEY_H2L_EN,
+};
+
 enum {
 	AXP20X_IRQ_ACIN_OVER_V = 1,
 	AXP20X_IRQ_ACIN_PLUGIN,
-- 
2.38.1

[PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant

[Martin Botka]

AXP1530 has a few regulators that are controlled via I2C Bus.

Add support for them.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index d260c442b788..9420839ff4f9 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = {
 		    AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK),
 };
 
+static const struct linear_range axp1530_dcdc1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
+	REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000),
+};
+
+static const struct linear_range axp1530_dcdc2_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
+};
+
+static const struct linear_range axp1530_dcdc3_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
+	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000),
+};
+
+static const struct regulator_desc axp1530_regulators[] = {
+	AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges,
+					0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
+					BIT(0)),
+	AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges,
+					0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
+					BIT(1)),
+	AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges,
+					0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
+					BIT(2)),
+	AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100,
+					AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
+					BIT(3)),
+	AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100,
+					AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
+					BIT(4)),
+};
+
 static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 {
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
@@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 		def = 3000;
 		step = 150;
 		break;
+	case AXP1530_ID:
+		/*
+		 * Do not set the DCDC frequency on AXP1530
+		 */
+		return 0;
+		break;
 	default:
 		dev_err(&pdev->dev,
 			"Setting DCDC frequency for unsupported AXP variant\n");
@@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 	bool drivevbus = false;
 
 	switch (axp20x->variant) {
+	case AXP1530_ID:
+		regulators = axp1530_regulators;
+		nregulators = AXP1530_REG_ID_MAX;
+		break;
 	case AXP202_ID:
 	case AXP209_ID:
 		regulators = axp20x_regulators;
-- 
2.38.1

Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant

[Andre Przywara]

On Wed, 14 Dec 2022 20:03:05 +0100
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> AXP1530 has a few regulators that are controlled via I2C Bus.
> 
> Add support for them.

thanks for putting this together!
After coming up with a very similar patch based on the AXP313A313
datasheet, I realised that those two must indeed be *somewhat*
compatible, so I am going to compare my patch with yours ;-)

> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index d260c442b788..9420839ff4f9 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = {
>  		    AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK),
>  };
>  
> +static const struct linear_range axp1530_dcdc1_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),

The AXP313A manual mentions "steps", in decimal
(0.5~1.2V，10mV/step，71steps), so I wonder if we should follow suit
here and describe the min_sel and max_sel members in decimal?

> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
> +	REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000),
> +};
> +
> +static const struct linear_range axp1530_dcdc2_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
> +};

The values up till here match exactly what I extracted from the AXP313A
manual.

> +
> +static const struct linear_range axp1530_dcdc3_ranges[] = {
> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000),
> +};

Can you double check that those are the values for DCDC3?
The AXP313A manual uses different ranges, essentially:
    REGULATOR_LINEAR_RANGE(800000, 0, 32, 10000),
    REGULATOR_LINEAR_RANGE(1140000, 33, 68, 20000),
So starting from 800mV, and using a slightly different split point.

I would just hope that's this doesn't turn out to be an incompatible register.

> +
> +static const struct regulator_desc axp1530_regulators[] = {
> +	AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges,
> +					0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,

Again I would code the steps in decimal. The other regulators use a
preprocessor constant, which helps the reader to get its meaning.
And please use at least GENMASK(6, 0) instead of 0x7f, or #define this
(can be shared for all DCDCs and the LDOs).

> +					BIT(0)),
> +	AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges,
> +					0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
> +					BIT(1)),
> +	AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges,
> +					0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
> +					BIT(2)),
> +	AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100,
> +					AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
> +					BIT(3)),
> +	AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100,
> +					AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
> +					BIT(4)),

Does this miss the fixed RTC-LDO? Or does the AXP1530 not have that?
        AXP_DESC_FIXED(AXP313, RTC_LDO, "rtc-ldo", "ips", 1800),
The AXP313A manual mentions that the voltage is customisable, either
1.8V or 3.3V. I don't know how to model that, exactly. Should this be a
DT property, then? Or do we fix it to one voltage, covering the value
that's used out there?

> +};
> +
>  static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>  {
>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> @@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>  		def = 3000;
>  		step = 150;
>  		break;
> +	case AXP1530_ID:
> +		/*
> +		 * Do not set the DCDC frequency on AXP1530

This should say that the frequency is fixed and cannot be programmed.
I also added a warning if the frequency is not 3 MHz.
Either this, or we make the "x-powers,dcdc-freq" DT property optional.

Cheers,
Andre

> +		 */
> +		return 0;
> +		break;
>  	default:
>  		dev_err(&pdev->dev,
>  			"Setting DCDC frequency for unsupported AXP variant\n");
> @@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>  	bool drivevbus = false;
>  
>  	switch (axp20x->variant) {
> +	case AXP1530_ID:
> +		regulators = axp1530_regulators;
> +		nregulators = AXP1530_REG_ID_MAX;
> +		break;
>  	case AXP202_ID:
>  	case AXP209_ID:
>  		regulators = axp20x_regulators;

Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant

[Martin Botka]

On December 16, 2022 12:16:15 AM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>On Wed, 14 Dec 2022 20:03:05 +0100
>Martin Botka <martin.botka@somainline.org> wrote:
>
>Hi Martin,
>
>> AXP1530 has a few regulators that are controlled via I2C Bus.
>> 
>> Add support for them.
>
>thanks for putting this together!
>After coming up with a very similar patch based on the AXP313A313
>datasheet, I realised that those two must indeed be *somewhat*
>compatible, so I am going to compare my patch with yours ;-)
>
Hello Andre,
Thanks so much for looking at this.
>> 
>> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>> ---
>>  drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>> 
>> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
>> index d260c442b788..9420839ff4f9 100644
>> --- a/drivers/regulator/axp20x-regulator.c
>> +++ b/drivers/regulator/axp20x-regulator.c
>> @@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = {
>>  		    AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK),
>>  };
>>  
>> +static const struct linear_range axp1530_dcdc1_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>
>The AXP313A manual mentions "steps", in decimal
>(0.5~1.2V，10mV/step，71steps), so I wonder if we should follow suit
>here and describe the min_sel and max_sel members in decimal?
>
Ack. We definitely can :)
>> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
>> +	REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000),
>> +};
>> +
>> +static const struct linear_range axp1530_dcdc2_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
>> +};
>
>The values up till here match exactly what I extracted from the AXP313A
>manual.
>
>> +
>> +static const struct linear_range axp1530_dcdc3_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000),
>> +};
>
>Can you double check that those are the values for DCDC3?
>The AXP313A manual uses different ranges, essentially:
>    REGULATOR_LINEAR_RANGE(800000, 0, 32, 10000),
>    REGULATOR_LINEAR_RANGE(1140000, 33, 68, 20000),
>So starting from 800mV, and using a slightly different split point.
>
>I would just hope that's this doesn't turn out to be an incompatible register.
>
Interesting. The unfortunate thing with 1530 is that i could not find any datasheet referencing it. The actual PMIC that is on the device i have here is 313A. Do i think it would be best if i rename this driver to 313A and follow the 313A datasheet which is public.

This was already proposed in one of my device tree series.
What do you think of this idea Andre ?
>> +
>> +static const struct regulator_desc axp1530_regulators[] = {
>> +	AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges,
>> +					0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
>
>Again I would code the steps in decimal. The other regulators use a
>preprocessor constant, which helps the reader to get its meaning.
>And please use at least GENMASK(6, 0) instead of 0x7f, or #define this
>(can be shared for all DCDCs and the LDOs).
>
Ack. Will use GENMASK.
>> +					BIT(0)),
>> +	AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges,
>> +					0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
>> +					BIT(1)),
>> +	AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges,
>> +					0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
>> +					BIT(2)),
>> +	AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100,
>> +					AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
>> +					BIT(3)),
>> +	AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100,
>> +					AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
>> +					BIT(4)),
>
>Does this miss the fixed RTC-LDO? Or does the AXP1530 not have that?
>        AXP_DESC_FIXED(AXP313, RTC_LDO, "rtc-ldo", "ips", 1800),
>The AXP313A manual mentions that the voltage is customisable, either
>1.8V or 3.3V. I don't know how to model that, exactly. Should this be a
>DT property, then? Or do we fix it to one voltage, covering the value
>that's used out there?
>
This is what always struck me as weird. This driver is based upon downstream version it indeed does miss the rtc-ldo.

Afaik this may be the only difference between 1530 and 313 (other then what you pointed out the dcdc3 registers)
>> +};
>> +
>>  static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>>  {
>>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> @@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>>  		def = 3000;
>>  		step = 150;
>>  		break;
>> +	case AXP1530_ID:
>> +		/*
>> +		 * Do not set the DCDC frequency on AXP1530
>
>This should say that the frequency is fixed and cannot be programmed.
>I also added a warning if the frequency is not 3 MHz.
>Either this, or we make the "x-powers,dcdc-freq" DT property optional.
>
Ack. Will reword and add warning.
>Cheers,
>Andre
>
Cheers,
Martin
>> +		 */
>> +		return 0;
>> +		break;
>>  	default:
>>  		dev_err(&pdev->dev,
>>  			"Setting DCDC frequency for unsupported AXP variant\n");
>> @@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>>  	bool drivevbus = false;
>>  
>>  	switch (axp20x->variant) {
>> +	case AXP1530_ID:
>> +		regulators = axp1530_regulators;
>> +		nregulators = AXP1530_REG_ID_MAX;
>> +		break;
>>  	case AXP202_ID:
>>  	case AXP209_ID:
>>  		regulators = axp20x_regulators;
>

Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant

[Andre Przywara]

On Fri, 16 Dec 2022 06:26:38 +0100
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> On December 16, 2022 12:16:15 AM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
> >On Wed, 14 Dec 2022 20:03:05 +0100
> >Martin Botka <martin.botka@somainline.org> wrote:
> >
> >Hi Martin,
> >  
> >> AXP1530 has a few regulators that are controlled via I2C Bus.
> >> 
> >> Add support for them.  
> >
> >thanks for putting this together!
> >After coming up with a very similar patch based on the AXP313A313
> >datasheet, I realised that those two must indeed be *somewhat*
> >compatible, so I am going to compare my patch with yours ;-)
> >  
> Hello Andre,
> Thanks so much for looking at this.
> >> 
> >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> >> ---
> >>  drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++
> >>  1 file changed, 44 insertions(+)
> >> 
> >> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> >> index d260c442b788..9420839ff4f9 100644
> >> --- a/drivers/regulator/axp20x-regulator.c
> >> +++ b/drivers/regulator/axp20x-regulator.c
> >> @@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = {
> >>  		    AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK),
> >>  };
> >>  
> >> +static const struct linear_range axp1530_dcdc1_ranges[] = {
> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),  
> >
> >The AXP313A manual mentions "steps", in decimal
> >(0.5~1.2V，10mV/step，71steps), so I wonder if we should follow suit
> >here and describe the min_sel and max_sel members in decimal?
> >  
> Ack. We definitely can :)
> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
> >> +	REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000),
> >> +};
> >> +
> >> +static const struct linear_range axp1530_dcdc2_ranges[] = {
> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
> >> +};  
> >
> >The values up till here match exactly what I extracted from the AXP313A
> >manual.
> >  
> >> +
> >> +static const struct linear_range axp1530_dcdc3_ranges[] = {
> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000),
> >> +};  
> >
> >Can you double check that those are the values for DCDC3?
> >The AXP313A manual uses different ranges, essentially:
> >    REGULATOR_LINEAR_RANGE(800000, 0, 32, 10000),
> >    REGULATOR_LINEAR_RANGE(1140000, 33, 68, 20000),
> >So starting from 800mV, and using a slightly different split point.
> >
> >I would just hope that's this doesn't turn out to be an incompatible register.
> >  
> Interesting. The unfortunate thing with 1530 is that i could not find any datasheet referencing it. The actual PMIC that is on the device i have here is 313A. Do i think it would be best if i rename this driver to 313A and follow the 313A datasheet which is public.

So where does the 1530 name and the other bits come from? Was there some
statement somewhere that AXP1530 is the canonical chip, and the 313A is
just a variant? Or are there devices using this chip, and you just happen
to not have them, but a "compatible enough" 313A instead?

> This was already proposed in one of my device tree series.
> What do you think of this idea Andre ?

In general, I think it's too dangerous to develop against something you
cannot test against. So I would always call this driver after the chip I
have access to, especially if there is also a datasheet for it.
If someone later finds the AXP1530 to be compatible, they can always use:
	compatible = "x-powers,axp1530", "x-powers,axp313a";
Or later on add explicit support for that chip if it turns out to be not
fully compatible.

The only reason to not do it this way around would be if one was a strict
subset of the other. So if the 313A is the 1530 plus RTCLDO, for instance,
it would make sense to reflect that in the compatible strings (by swapping
them in the above example).
But then again there is really no legacy here, so we could just upstream
support for both variants in one go, using separate compatible strings,
and let them just share some data structures internally. That is probably
cleaner, and if both are going it at the same time, there is no downside,
really.

> >> +static const struct regulator_desc axp1530_regulators[] = {
> >> +	AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges,
> >> +					0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,  
> >
> >Again I would code the steps in decimal. The other regulators use a
> >preprocessor constant, which helps the reader to get its meaning.
> >And please use at least GENMASK(6, 0) instead of 0x7f, or #define this
> >(can be shared for all DCDCs and the LDOs).
> >  
> Ack. Will use GENMASK.
> >> +					BIT(0)),
> >> +	AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges,
> >> +					0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
> >> +					BIT(1)),
> >> +	AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges,
> >> +					0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
> >> +					BIT(2)),
> >> +	AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100,
> >> +					AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
> >> +					BIT(3)),
> >> +	AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100,
> >> +					AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
> >> +					BIT(4)),  
> >
> >Does this miss the fixed RTC-LDO? Or does the AXP1530 not have that?
> >        AXP_DESC_FIXED(AXP313, RTC_LDO, "rtc-ldo", "ips", 1800),
> >The AXP313A manual mentions that the voltage is customisable, either
> >1.8V or 3.3V. I don't know how to model that, exactly. Should this be a
> >DT property, then? Or do we fix it to one voltage, covering the value
> >that's used out there?
> >  
> This is what always struck me as weird. This driver is based upon downstream version it indeed does miss the rtc-ldo.

Well, in my experience downstream (BSP) drivers cannot be really trusted
in this regard.
So they could just papered over it by modelling this as a separate
"regulator-fixed"? Or maybe this was their solution for the "customise"
problem? So instead of having this as part of the AXP, they just put a
per-board fixed regulator in and set the voltage there?

Cheers,
Andre

> Afaik this may be the only difference between 1530 and 313 (other then what you pointed out the dcdc3 registers)
> >> +};
> >> +
> >>  static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> >>  {
> >>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> >> @@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
> >>  		def = 3000;
> >>  		step = 150;
> >>  		break;
> >> +	case AXP1530_ID:
> >> +		/*
> >> +		 * Do not set the DCDC frequency on AXP1530  
> >
> >This should say that the frequency is fixed and cannot be programmed.
> >I also added a warning if the frequency is not 3 MHz.
> >Either this, or we make the "x-powers,dcdc-freq" DT property optional.
> >  
> Ack. Will reword and add warning.
> >Cheers,
> >Andre
> >  
> Cheers,
> Martin
> >> +		 */
> >> +		return 0;
> >> +		break;
> >>  	default:
> >>  		dev_err(&pdev->dev,
> >>  			"Setting DCDC frequency for unsupported AXP variant\n");
> >> @@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
> >>  	bool drivevbus = false;
> >>  
> >>  	switch (axp20x->variant) {
> >> +	case AXP1530_ID:
> >> +		regulators = axp1530_regulators;
> >> +		nregulators = AXP1530_REG_ID_MAX;
> >> +		break;
> >>  	case AXP202_ID:
> >>  	case AXP209_ID:
> >>  		regulators = axp20x_regulators;  
> >  

Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant

[Martin Botka]

On December 16, 2022 12:52:14 PM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>On Fri, 16 Dec 2022 06:26:38 +0100
>Martin Botka <martin.botka@somainline.org> wrote:
>
>Hi Martin,
>
>> On December 16, 2022 12:16:15 AM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>> >On Wed, 14 Dec 2022 20:03:05 +0100
>> >Martin Botka <martin.botka@somainline.org> wrote:
>> >
>> >Hi Martin,
>> >  
>> >> AXP1530 has a few regulators that are controlled via I2C Bus.
>> >> 
>> >> Add support for them.  
>> >
>> >thanks for putting this together!
>> >After coming up with a very similar patch based on the AXP313A313
>> >datasheet, I realised that those two must indeed be *somewhat*
>> >compatible, so I am going to compare my patch with yours ;-)
>> >  
>> Hello Andre,
>> Thanks so much for looking at this.
>> >> 
>> >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>> >> ---
>> >>  drivers/regulator/axp20x-regulator.c | 44 ++++++++++++++++++++++++++++
>> >>  1 file changed, 44 insertions(+)
>> >> 
>> >> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
>> >> index d260c442b788..9420839ff4f9 100644
>> >> --- a/drivers/regulator/axp20x-regulator.c
>> >> +++ b/drivers/regulator/axp20x-regulator.c
>> >> @@ -1001,6 +1001,40 @@ static const struct regulator_desc axp813_regulators[] = {
>> >>  		    AXP22X_PWR_OUT_CTRL2, AXP22X_PWR_OUT_DC1SW_MASK),
>> >>  };
>> >>  
>> >> +static const struct linear_range axp1530_dcdc1_ranges[] = {
>> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),  
>> >
>> >The AXP313A manual mentions "steps", in decimal
>> >(0.5~1.2V，10mV/step，71steps), so I wonder if we should follow suit
>> >here and describe the min_sel and max_sel members in decimal?
>> >  
>> Ack. We definitely can :)
>> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
>> >> +	REGULATOR_LINEAR_RANGE(1600000, 0x58, 0x6A, 100000),
>> >> +};
>> >> +
>> >> +static const struct linear_range axp1530_dcdc2_ranges[] = {
>> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x57, 20000),
>> >> +};  
>> >
>> >The values up till here match exactly what I extracted from the AXP313A
>> >manual.
>> >  
>> >> +
>> >> +static const struct linear_range axp1530_dcdc3_ranges[] = {
>> >> +	REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>> >> +	REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x66, 20000),
>> >> +};  
>> >
>> >Can you double check that those are the values for DCDC3?
>> >The AXP313A manual uses different ranges, essentially:
>> >    REGULATOR_LINEAR_RANGE(800000, 0, 32, 10000),
>> >    REGULATOR_LINEAR_RANGE(1140000, 33, 68, 20000),
>> >So starting from 800mV, and using a slightly different split point.
>> >
>> >I would just hope that's this doesn't turn out to be an incompatible register.
>> >  
>> Interesting. The unfortunate thing with 1530 is that i could not find any datasheet referencing it. The actual PMIC that is on the device i have here is 313A. Do i think it would be best if i rename this driver to 313A and follow the 313A datasheet which is public.
>
>So where does the 1530 name and the other bits come from? Was there some
>statement somewhere that AXP1530 is the canonical chip, and the 313A is
>just a variant? Or are there devices using this chip, and you just happen
>to not have them, but a "compatible enough" 313A instead?
>
I have yet to see actual device using this chip. If they have the driver its AXP313A. I have Biqu CB1 which uses the AXP313A but in code and in some places in datasheet for this device (not yet public. Hopefully that will change) its referenced as AXP1530. The driver was also named like this. So this is where the name comes from.

I do have contact to the developers so what i will do is contact them and ask why this is done.
>> This was already proposed in one of my device tree series.
>> What do you think of this idea Andre ?
>
>In general, I think it's too dangerous to develop against something you
>cannot test against. So I would always call this driver after the chip I
>have access to, especially if there is also a datasheet for it.
>If someone later finds the AXP1530 to be compatible, they can always use:
>	compatible = "x-powers,axp1530", "x-powers,axp313a";
>Or later on add explicit support for that chip if it turns out to be not
>fully compatible.
>
>The only reason to not do it this way around would be if one was a strict
>subset of the other. So if the 313A is the 1530 plus RTCLDO, for instance,
>it would make sense to reflect that in the compatible strings (by swapping
>them in the above example).
>But then again there is really no legacy here, so we could just upstream
>support for both variants in one go, using separate compatible strings,
>and let them just share some data structures internally. That is probably
>cleaner, and if both are going it at the same time, there is no downside,
>really.
>
Yea sounds good. So rename to 313A change bits and bobs
And if someone has 1530 they can add it later.
>> >> +static const struct regulator_desc axp1530_regulators[] = {
>> >> +	AXP_DESC_RANGES(AXP1530, DCDC1, "dcdc1", "vin1", axp1530_dcdc1_ranges,
>> >> +					0x6B, AXP1530_DCDC1_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,  
>> >
>> >Again I would code the steps in decimal. The other regulators use a
>> >preprocessor constant, which helps the reader to get its meaning.
>> >And please use at least GENMASK(6, 0) instead of 0x7f, or #define this
>> >(can be shared for all DCDCs and the LDOs).
>> >  
>> Ack. Will use GENMASK.
>> >> +					BIT(0)),
>> >> +	AXP_DESC_RANGES(AXP1530, DCDC2, "dcdc2", "vin2", axp1530_dcdc2_ranges,
>> >> +					0x58, AXP1530_DCDC2_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
>> >> +					BIT(1)),
>> >> +	AXP_DESC_RANGES(AXP1530, DCDC3, "dcdc3", "vin3", axp1530_dcdc3_ranges,
>> >> +					0x58, AXP1530_DCDC3_CONRTOL, 0x7f, AXP1530_OUTPUT_CONTROL,
>> >> +					BIT(2)),
>> >> +	AXP_DESC(AXP1530, LDO1, "ldo1", "ldo1in", 500, 3500, 100,
>> >> +					AXP1530_ALDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
>> >> +					BIT(3)),
>> >> +	AXP_DESC(AXP1530, LDO2, "ldo2", "ldo2in", 500, 3500, 100,
>> >> +					AXP1530_DLDO1_CONRTOL, 0x1f, AXP1530_OUTPUT_CONTROL,
>> >> +					BIT(4)),  
>> >
>> >Does this miss the fixed RTC-LDO? Or does the AXP1530 not have that?
>> >        AXP_DESC_FIXED(AXP313, RTC_LDO, "rtc-ldo", "ips", 1800),
>> >The AXP313A manual mentions that the voltage is customisable, either
>> >1.8V or 3.3V. I don't know how to model that, exactly. Should this be a
>> >DT property, then? Or do we fix it to one voltage, covering the value
>> >that's used out there?
>> >  
>> This is what always struck me as weird. This driver is based upon downstream version it indeed does miss the rtc-ldo.
>
>Well, in my experience downstream (BSP) drivers cannot be really trusted
>in this regard.
>So they could just papered over it by modelling this as a separate
>"regulator-fixed"? Or maybe this was their solution for the "customise"
>problem? So instead of having this as part of the AXP, they just put a
>per-board fixed regulator in and set the voltage there?
>
Very fair point.
I will add it in the 313A :)

Cheers,
Martin
>Cheers,
>Andre
>
>> Afaik this may be the only difference between 1530 and 313 (other then what you pointed out the dcdc3 registers)
>> >> +};
>> >> +
>> >>  static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>> >>  {
>> >>  	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
>> >> @@ -1040,6 +1074,12 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
>> >>  		def = 3000;
>> >>  		step = 150;
>> >>  		break;
>> >> +	case AXP1530_ID:
>> >> +		/*
>> >> +		 * Do not set the DCDC frequency on AXP1530  
>> >
>> >This should say that the frequency is fixed and cannot be programmed.
>> >I also added a warning if the frequency is not 3 MHz.
>> >Either this, or we make the "x-powers,dcdc-freq" DT property optional.
>> >  
>> Ack. Will reword and add warning.
>> >Cheers,
>> >Andre
>> >  
>> Cheers,
>> Martin
>> >> +		 */
>> >> +		return 0;
>> >> +		break;
>> >>  	default:
>> >>  		dev_err(&pdev->dev,
>> >>  			"Setting DCDC frequency for unsupported AXP variant\n");
>> >> @@ -1220,6 +1260,10 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
>> >>  	bool drivevbus = false;
>> >>  
>> >>  	switch (axp20x->variant) {
>> >> +	case AXP1530_ID:
>> >> +		regulators = axp1530_regulators;
>> >> +		nregulators = AXP1530_REG_ID_MAX;
>> >> +		break;
>> >>  	case AXP202_ID:
>> >>  	case AXP209_ID:
>> >>  		regulators = axp20x_regulators;  
>> >  
>

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Andre Przywara]

On Wed, 14 Dec 2022 20:03:04 +0100
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> AXP1530 is a PMIC chip produced by X-Powers and an be connected via
> I2C bus.
> Where AXP313A seems to be closely related so the same driver can be used and
> seen it only paired with H616 SoC.

So as mentioned, I am pretending this is for the AXP313A now, looking at
its datasheet.
Of course the elephant in the room is s/AXP1530/AXP313A/, but other than
that:
 
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  drivers/mfd/axp20x-i2c.c   |  2 ++
>  drivers/mfd/axp20x.c       | 62 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> index 8fd6727dc30a..6bfb931a580e 100644
> --- a/drivers/mfd/axp20x-i2c.c
> +++ b/drivers/mfd/axp20x-i2c.c
> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
>  #ifdef CONFIG_OF
>  static const struct of_device_id axp20x_i2c_of_match[] = {
>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
> +	{ .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>  
>  static const struct i2c_device_id axp20x_i2c_id[] = {
>  	{ "axp152", 0 },
> +	{ "axp1530", 0 },
>  	{ "axp202", 0 },
>  	{ "axp209", 0 },
>  	{ "axp221", 0 },
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 880c41fa7021..6caa7e87ad80 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -34,6 +34,7 @@
>  
>  static const char * const axp20x_model_names[] = {
>  	"AXP152",
> +	"AXP1530",
>  	"AXP202",
>  	"AXP209",
>  	"AXP221",
> @@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
>  };
>  
> +static const struct regmap_range axp1530_writeable_ranges[] = {
> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),

Where does this FREQUENCY register come from? BSP source? Is that the
lost register to set the PWM frequency?
The 313 datasheet doesn't mention it, and since we deny programming the
frequency, I would just leave it out.
If people find it existing (and useful!) later on, we should be able to
add it without breaking anything.

> +};
> +
> +static const struct regmap_range axp1530_volatile_ranges[] = {
> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> +};
> +
> +static const struct regmap_access_table axp1530_writeable_table = {
> +	.yes_ranges = axp1530_writeable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
> +};
> +
> +static const struct regmap_access_table axp1530_volatile_table = {
> +	.yes_ranges = axp1530_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
> +};
> +
>  static const struct regmap_range axp20x_writeable_ranges[] = {
>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
> @@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
>  	.cache_type	= REGCACHE_RBTREE,
>  };
>  
> +static const struct regmap_config axp1530_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.wr_table = &axp1530_writeable_table,
> +	.volatile_table = &axp1530_volatile_table,
> +	.max_register = AXP1530_FREQUENCY,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
>  static const struct regmap_config axp20x_regmap_config = {
>  	.reg_bits	= 8,
>  	.val_bits	= 8,
> @@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
>  };
>  
> +static const struct regmap_irq axp1530_regmap_irqs[] = {
> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),

Are those identifiers from the BSP source? The (translated) manual gives
some explanation, namely:
	PWRON key rising edge
	PWRON key falling edge
	Short press the PWRON button
	Long press the PWRON button

So I'd suggest we follow the existing naming:
	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)

Or come up with names that people could actually decipher ;-)


> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
> +};
> +
>  static const struct regmap_irq axp20x_regmap_irqs[] = {
>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
>  	.num_regs		= 3,
>  };
>  
> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
> +	.name = "axp1530_irq_chip",
> +	.status_base = AXP1530_IRQ_STATUS1,
> +	.ack_base = AXP1530_IRQ_STATUS1,
> +	.mask_base = AXP1530_IRQ_ENABLE1,
> +	.mask_invert = true,
> +	.init_ack_masked = true,
> +	.irqs = axp1530_regmap_irqs,
> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
> +	.num_regs = 1,
> +};
> +
>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
>  	.name			= "axp20x_irq_chip",
>  	.status_base		= AXP20X_IRQ1_STATE,
> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
>  	},
>  };
>  
> +static struct mfd_cell axp1530_cells[] = {
> +	{
> +		.name = "axp20x-regulator",
> +	},
> +};
> +
>  static const struct resource axp288_adc_resources[] = {
>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
>  };
> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>  		axp20x->regmap_cfg = &axp152_regmap_config;
>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
>  		break;
> +	case AXP1530_ID:
> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
> +		axp20x->cells = axp1530_cells;
> +		axp20x->regmap_cfg = &axp1530_regmap_config;
> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
> +		break;
>  	case AXP202_ID:
>  	case AXP209_ID:
>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 9ab0e2fca7ea..cad25754500f 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -12,6 +12,7 @@
>  
>  enum axp20x_variants {
>  	AXP152_ID = 0,
> +	AXP1530_ID,
>  	AXP202_ID,
>  	AXP209_ID,
>  	AXP221_ID,
> @@ -45,6 +46,18 @@ enum axp20x_variants {
>  #define AXP152_DCDC_FREQ		0x37
>  #define AXP152_DCDC_MODE		0x80
>  
> +#define AXP1530_ON_INDICATE		0x00
> +#define AXP1530_OUTPUT_CONTROL	0x10
> +#define AXP1530_DCDC1_CONRTOL	0x13
> +#define AXP1530_DCDC2_CONRTOL	0x14
> +#define AXP1530_DCDC3_CONRTOL	0x15
> +#define AXP1530_ALDO1_CONRTOL	0x16
> +#define AXP1530_DLDO1_CONRTOL	0x17
> +#define AXP1530_OUTOUT_MONITOR	0x1D

Shall this read AXP1530_OUTPUT_MONITOR?

> +#define AXP1530_IRQ_ENABLE1		0x20
> +#define AXP1530_IRQ_STATUS1		0x21

There is only one interrupt register, so can we drop the trailing number?

> +#define AXP1530_FREQUENCY		0x87

As mentioned, the manual does not mention it, and we don't use it anyway.

> +
>  #define AXP20X_PWR_INPUT_STATUS		0x00
>  #define AXP20X_PWR_OP_MODE		0x01
>  #define AXP20X_USB_OTG_STATUS		0x02
> @@ -287,6 +300,15 @@ enum axp20x_variants {
>  #define AXP288_FG_TUNE5             0xed
>  
>  /* Regulators IDs */
> +enum {
> +	AXP1530_DCDC1 = 0,
> +	AXP1530_DCDC2,
> +	AXP1530_DCDC3,
> +	AXP1530_LDO1,
> +	AXP1530_LDO2,

I guess we should add the RTC LDO as LDO3 here.

The rest of the numbers match with the datasheet.

Cheers,
Andre

> +	AXP1530_REG_ID_MAX,
> +};
> +
>  enum {
>  	AXP20X_LDO1 = 0,
>  	AXP20X_LDO2,
> @@ -440,6 +462,16 @@ enum {
>  	AXP152_IRQ_GPIO0_INPUT,
>  };
>  
> +enum axp1530_irqs {
> +	AXP1530_IRQ_TEMP_OVER,
> +	AXP1530_IRQ_DCDC2_UNDER = 2,
> +	AXP1530_IRQ_DCDC3_UNDER,
> +	AXP1530_IRQ_POKLIRQ_EN,
> +	AXP1530_IRQ_POKSIRQ_EN,
> +	AXP1530_IRQ_KEY_L2H_EN,
> +	AXP1530_IRQ_KEY_H2L_EN,
> +};
> +
>  enum {
>  	AXP20X_IRQ_ACIN_OVER_V = 1,
>  	AXP20X_IRQ_ACIN_PLUGIN,

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Martin Botka]

On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>On Wed, 14 Dec 2022 20:03:04 +0100
>Martin Botka <martin.botka@somainline.org> wrote:
>
>Hi Martin,
>
>> AXP1530 is a PMIC chip produced by X-Powers and an be connected via
>> I2C bus.
>> Where AXP313A seems to be closely related so the same driver can be used and
>> seen it only paired with H616 SoC.
>
>So as mentioned, I am pretending this is for the AXP313A now, looking at
>its datasheet.
>Of course the elephant in the room is s/AXP1530/AXP313A/, but other than
>that:
> 
>> 
>> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>> ---
>>  drivers/mfd/axp20x-i2c.c   |  2 ++
>>  drivers/mfd/axp20x.c       | 62 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
>>  3 files changed, 96 insertions(+)
>> 
>> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>> index 8fd6727dc30a..6bfb931a580e 100644
>> --- a/drivers/mfd/axp20x-i2c.c
>> +++ b/drivers/mfd/axp20x-i2c.c
>> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
>>  #ifdef CONFIG_OF
>>  static const struct of_device_id axp20x_i2c_of_match[] = {
>>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
>> +	{ .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
>>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
>>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
>>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
>> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>>  
>>  static const struct i2c_device_id axp20x_i2c_id[] = {
>>  	{ "axp152", 0 },
>> +	{ "axp1530", 0 },
>>  	{ "axp202", 0 },
>>  	{ "axp209", 0 },
>>  	{ "axp221", 0 },
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 880c41fa7021..6caa7e87ad80 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -34,6 +34,7 @@
>>  
>>  static const char * const axp20x_model_names[] = {
>>  	"AXP152",
>> +	"AXP1530",
>>  	"AXP202",
>>  	"AXP209",
>>  	"AXP221",
>> @@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
>>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
>>  };
>>  
>> +static const struct regmap_range axp1530_writeable_ranges[] = {
>> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
>
>Where does this FREQUENCY register come from? BSP source? Is that the
>lost register to set the PWM frequency?
>The 313 datasheet doesn't mention it, and since we deny programming the
>frequency, I would just leave it out.
>If people find it existing (and useful!) later on, we should be able to
>add it without breaking anything.

BSP. Ack.
>
>> +};
>> +
>> +static const struct regmap_range axp1530_volatile_ranges[] = {
>> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
>> +};
>> +
>> +static const struct regmap_access_table axp1530_writeable_table = {
>> +	.yes_ranges = axp1530_writeable_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
>> +};
>> +
>> +static const struct regmap_access_table axp1530_volatile_table = {
>> +	.yes_ranges = axp1530_volatile_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
>> +};
>> +
>>  static const struct regmap_range axp20x_writeable_ranges[] = {
>>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
>> @@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
>>  	.cache_type	= REGCACHE_RBTREE,
>>  };
>>  
>> +static const struct regmap_config axp1530_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.wr_table = &axp1530_writeable_table,
>> +	.volatile_table = &axp1530_volatile_table,
>> +	.max_register = AXP1530_FREQUENCY,
>> +	.cache_type = REGCACHE_RBTREE,
>> +};
>> +
>>  static const struct regmap_config axp20x_regmap_config = {
>>  	.reg_bits	= 8,
>>  	.val_bits	= 8,
>> @@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
>>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
>>  };
>>  
>> +static const struct regmap_irq axp1530_regmap_irqs[] = {
>> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
>> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
>> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
>> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),
>
>Are those identifiers from the BSP source? The (translated) manual gives
>some explanation, namely:
>	PWRON key rising edge
>	PWRON key falling edge
>	Short press the PWRON button
>	Long press the PWRON button
>
>So I'd suggest we follow the existing naming:
>	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
>
>Or come up with names that people could actually decipher ;-)
>
>
Ack.
>> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
>> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
>> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
>> +};
>> +
>>  static const struct regmap_irq axp20x_regmap_irqs[] = {
>>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
>>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
>> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
>>  	.num_regs		= 3,
>>  };
>>  
>> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
>> +	.name = "axp1530_irq_chip",
>> +	.status_base = AXP1530_IRQ_STATUS1,
>> +	.ack_base = AXP1530_IRQ_STATUS1,
>> +	.mask_base = AXP1530_IRQ_ENABLE1,
>> +	.mask_invert = true,
>> +	.init_ack_masked = true,
>> +	.irqs = axp1530_regmap_irqs,
>> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
>> +	.num_regs = 1,
>> +};
>> +
>>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
>>  	.name			= "axp20x_irq_chip",
>>  	.status_base		= AXP20X_IRQ1_STATE,
>> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
>>  	},
>>  };
>>  
>> +static struct mfd_cell axp1530_cells[] = {
>> +	{
>> +		.name = "axp20x-regulator",
>> +	},
>> +};
>> +
>>  static const struct resource axp288_adc_resources[] = {
>>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
>>  };
>> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
>>  		axp20x->regmap_cfg = &axp152_regmap_config;
>>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
>>  		break;
>> +	case AXP1530_ID:
>> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
>> +		axp20x->cells = axp1530_cells;
>> +		axp20x->regmap_cfg = &axp1530_regmap_config;
>> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
>> +		break;
>>  	case AXP202_ID:
>>  	case AXP209_ID:
>>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
>> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
>> index 9ab0e2fca7ea..cad25754500f 100644
>> --- a/include/linux/mfd/axp20x.h
>> +++ b/include/linux/mfd/axp20x.h
>> @@ -12,6 +12,7 @@
>>  
>>  enum axp20x_variants {
>>  	AXP152_ID = 0,
>> +	AXP1530_ID,
>>  	AXP202_ID,
>>  	AXP209_ID,
>>  	AXP221_ID,
>> @@ -45,6 +46,18 @@ enum axp20x_variants {
>>  #define AXP152_DCDC_FREQ		0x37
>>  #define AXP152_DCDC_MODE		0x80
>>  
>> +#define AXP1530_ON_INDICATE		0x00
>> +#define AXP1530_OUTPUT_CONTROL	0x10
>> +#define AXP1530_DCDC1_CONRTOL	0x13
>> +#define AXP1530_DCDC2_CONRTOL	0x14
>> +#define AXP1530_DCDC3_CONRTOL	0x15
>> +#define AXP1530_ALDO1_CONRTOL	0x16
>> +#define AXP1530_DLDO1_CONRTOL	0x17
>> +#define AXP1530_OUTOUT_MONITOR	0x1D
>
>Shall this read AXP1530_OUTPUT_MONITOR?
>
>> +#define AXP1530_IRQ_ENABLE1		0x20
>> +#define AXP1530_IRQ_STATUS1		0x21
>
>There is only one interrupt register, so can we drop the trailing number?
>
Yep.
>> +#define AXP1530_FREQUENCY		0x87
>
>As mentioned, the manual does not mention it, and we don't use it anyway.
>
Ack.
>> +
>>  #define AXP20X_PWR_INPUT_STATUS		0x00
>>  #define AXP20X_PWR_OP_MODE		0x01
>>  #define AXP20X_USB_OTG_STATUS		0x02
>> @@ -287,6 +300,15 @@ enum axp20x_variants {
>>  #define AXP288_FG_TUNE5             0xed
>>  
>>  /* Regulators IDs */
>> +enum {
>> +	AXP1530_DCDC1 = 0,
>> +	AXP1530_DCDC2,
>> +	AXP1530_DCDC3,
>> +	AXP1530_LDO1,
>> +	AXP1530_LDO2,
>
>I guess we should add the RTC LDO as LDO3 here.
>
>The rest of the numbers match with the datasheet.
>
Ack.

I will take some time off due to Uni so v6 will be delayed peobably last holidays.

Best regards and happy holidays,
Martin
>Cheers,
>Andre
>
>> +	AXP1530_REG_ID_MAX,
>> +};
>> +
>>  enum {
>>  	AXP20X_LDO1 = 0,
>>  	AXP20X_LDO2,
>> @@ -440,6 +462,16 @@ enum {
>>  	AXP152_IRQ_GPIO0_INPUT,
>>  };
>>  
>> +enum axp1530_irqs {
>> +	AXP1530_IRQ_TEMP_OVER,
>> +	AXP1530_IRQ_DCDC2_UNDER = 2,
>> +	AXP1530_IRQ_DCDC3_UNDER,
>> +	AXP1530_IRQ_POKLIRQ_EN,
>> +	AXP1530_IRQ_POKSIRQ_EN,
>> +	AXP1530_IRQ_KEY_L2H_EN,
>> +	AXP1530_IRQ_KEY_H2L_EN,
>> +};
>> +
>>  enum {
>>  	AXP20X_IRQ_ACIN_OVER_V = 1,
>>  	AXP20X_IRQ_ACIN_PLUGIN,
>

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Andre Przywara]

On Sat, 17 Dec 2022 01:13:01 +0100
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

hope you had a good break! Did you have any chance to come back to this
again? Now would be a good time to send a new version, otherwise it's
getting pretty tight for v6.3 already.

On Friday, "junari" in the #linux-sunxi IRC channel, made some
interesting discovery: he is playing around with an AXP313a on some
H616 device and figured that DCDC3 is not behaving like the datasheet: 
https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528;
He later confirmed the voltage:
https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373;

Basically it looks like the DCDC3 parameters you harvested from the BSP
code seem to be correct after all. Do you have any chance to measure
the voltage?
If not, can we try to deduce what the right settings are? The voltage
difference seems to be significant (860mV vs 1200mV), I wonder if any
device connected there (DRAM?) would work with the wrong setting?

Cheers,
Andre

> On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
> >On Wed, 14 Dec 2022 20:03:04 +0100
> >Martin Botka <martin.botka@somainline.org> wrote:
> >
> >Hi Martin,
> >  
> >> AXP1530 is a PMIC chip produced by X-Powers and an be connected via
> >> I2C bus.
> >> Where AXP313A seems to be closely related so the same driver can be used and
> >> seen it only paired with H616 SoC.  
> >
> >So as mentioned, I am pretending this is for the AXP313A now, looking at
> >its datasheet.
> >Of course the elephant in the room is s/AXP1530/AXP313A/, but other than
> >that:
> >   
> >> 
> >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> >> ---
> >>  drivers/mfd/axp20x-i2c.c   |  2 ++
> >>  drivers/mfd/axp20x.c       | 62 ++++++++++++++++++++++++++++++++++++++
> >>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> >>  3 files changed, 96 insertions(+)
> >> 
> >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> >> index 8fd6727dc30a..6bfb931a580e 100644
> >> --- a/drivers/mfd/axp20x-i2c.c
> >> +++ b/drivers/mfd/axp20x-i2c.c
> >> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct i2c_client *i2c)
> >>  #ifdef CONFIG_OF
> >>  static const struct of_device_id axp20x_i2c_of_match[] = {
> >>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
> >> +	{ .compatible = "x-powers,axp1530", .data = (void *)AXP1530_ID},
> >>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
> >>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> >>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> >> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
> >>  
> >>  static const struct i2c_device_id axp20x_i2c_id[] = {
> >>  	{ "axp152", 0 },
> >> +	{ "axp1530", 0 },
> >>  	{ "axp202", 0 },
> >>  	{ "axp209", 0 },
> >>  	{ "axp221", 0 },
> >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> >> index 880c41fa7021..6caa7e87ad80 100644
> >> --- a/drivers/mfd/axp20x.c
> >> +++ b/drivers/mfd/axp20x.c
> >> @@ -34,6 +34,7 @@
> >>  
> >>  static const char * const axp20x_model_names[] = {
> >>  	"AXP152",
> >> +	"AXP1530",
> >>  	"AXP202",
> >>  	"AXP209",
> >>  	"AXP221",
> >> @@ -66,6 +67,24 @@ static const struct regmap_access_table axp152_volatile_table = {
> >>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
> >>  };
> >>  
> >> +static const struct regmap_range axp1530_writeable_ranges[] = {
> >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),  
> >
> >Where does this FREQUENCY register come from? BSP source? Is that the
> >lost register to set the PWM frequency?
> >The 313 datasheet doesn't mention it, and since we deny programming the
> >frequency, I would just leave it out.
> >If people find it existing (and useful!) later on, we should be able to
> >add it without breaking anything.  
> 
> BSP. Ack.
> >  
> >> +};
> >> +
> >> +static const struct regmap_range axp1530_volatile_ranges[] = {
> >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> >> +};
> >> +
> >> +static const struct regmap_access_table axp1530_writeable_table = {
> >> +	.yes_ranges = axp1530_writeable_ranges,
> >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
> >> +};
> >> +
> >> +static const struct regmap_access_table axp1530_volatile_table = {
> >> +	.yes_ranges = axp1530_volatile_ranges,
> >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
> >> +};
> >> +
> >>  static const struct regmap_range axp20x_writeable_ranges[] = {
> >>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> >>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
> >> @@ -245,6 +264,15 @@ static const struct regmap_config axp152_regmap_config = {
> >>  	.cache_type	= REGCACHE_RBTREE,
> >>  };
> >>  
> >> +static const struct regmap_config axp1530_regmap_config = {
> >> +	.reg_bits = 8,
> >> +	.val_bits = 8,
> >> +	.wr_table = &axp1530_writeable_table,
> >> +	.volatile_table = &axp1530_volatile_table,
> >> +	.max_register = AXP1530_FREQUENCY,
> >> +	.cache_type = REGCACHE_RBTREE,
> >> +};
> >> +
> >>  static const struct regmap_config axp20x_regmap_config = {
> >>  	.reg_bits	= 8,
> >>  	.val_bits	= 8,
> >> @@ -304,6 +332,16 @@ static const struct regmap_irq axp152_regmap_irqs[] = {
> >>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
> >>  };
> >>  
> >> +static const struct regmap_irq axp1530_regmap_irqs[] = {
> >> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
> >> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
> >> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
> >> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),  
> >
> >Are those identifiers from the BSP source? The (translated) manual gives
> >some explanation, namely:
> >	PWRON key rising edge
> >	PWRON key falling edge
> >	Short press the PWRON button
> >	Long press the PWRON button
> >
> >So I'd suggest we follow the existing naming:
> >	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
> >
> >Or come up with names that people could actually decipher ;-)
> >
> >  
> Ack.
> >> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
> >> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
> >> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
> >> +};
> >> +
> >>  static const struct regmap_irq axp20x_regmap_irqs[] = {
> >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
> >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
> >> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip axp152_regmap_irq_chip = {
> >>  	.num_regs		= 3,
> >>  };
> >>  
> >> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
> >> +	.name = "axp1530_irq_chip",
> >> +	.status_base = AXP1530_IRQ_STATUS1,
> >> +	.ack_base = AXP1530_IRQ_STATUS1,
> >> +	.mask_base = AXP1530_IRQ_ENABLE1,
> >> +	.mask_invert = true,
> >> +	.init_ack_masked = true,
> >> +	.irqs = axp1530_regmap_irqs,
> >> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
> >> +	.num_regs = 1,
> >> +};
> >> +
> >>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> >>  	.name			= "axp20x_irq_chip",
> >>  	.status_base		= AXP20X_IRQ1_STATE,
> >> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] = {
> >>  	},
> >>  };
> >>  
> >> +static struct mfd_cell axp1530_cells[] = {
> >> +	{
> >> +		.name = "axp20x-regulator",
> >> +	},
> >> +};
> >> +
> >>  static const struct resource axp288_adc_resources[] = {
> >>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> >>  };
> >> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
> >>  		axp20x->regmap_cfg = &axp152_regmap_config;
> >>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
> >>  		break;
> >> +	case AXP1530_ID:
> >> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
> >> +		axp20x->cells = axp1530_cells;
> >> +		axp20x->regmap_cfg = &axp1530_regmap_config;
> >> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
> >> +		break;
> >>  	case AXP202_ID:
> >>  	case AXP209_ID:
> >>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> >> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> >> index 9ab0e2fca7ea..cad25754500f 100644
> >> --- a/include/linux/mfd/axp20x.h
> >> +++ b/include/linux/mfd/axp20x.h
> >> @@ -12,6 +12,7 @@
> >>  
> >>  enum axp20x_variants {
> >>  	AXP152_ID = 0,
> >> +	AXP1530_ID,
> >>  	AXP202_ID,
> >>  	AXP209_ID,
> >>  	AXP221_ID,
> >> @@ -45,6 +46,18 @@ enum axp20x_variants {
> >>  #define AXP152_DCDC_FREQ		0x37
> >>  #define AXP152_DCDC_MODE		0x80
> >>  
> >> +#define AXP1530_ON_INDICATE		0x00
> >> +#define AXP1530_OUTPUT_CONTROL	0x10
> >> +#define AXP1530_DCDC1_CONRTOL	0x13
> >> +#define AXP1530_DCDC2_CONRTOL	0x14
> >> +#define AXP1530_DCDC3_CONRTOL	0x15
> >> +#define AXP1530_ALDO1_CONRTOL	0x16
> >> +#define AXP1530_DLDO1_CONRTOL	0x17
> >> +#define AXP1530_OUTOUT_MONITOR	0x1D  
> >
> >Shall this read AXP1530_OUTPUT_MONITOR?
> >  
> >> +#define AXP1530_IRQ_ENABLE1		0x20
> >> +#define AXP1530_IRQ_STATUS1		0x21  
> >
> >There is only one interrupt register, so can we drop the trailing number?
> >  
> Yep.
> >> +#define AXP1530_FREQUENCY		0x87  
> >
> >As mentioned, the manual does not mention it, and we don't use it anyway.
> >  
> Ack.
> >> +
> >>  #define AXP20X_PWR_INPUT_STATUS		0x00
> >>  #define AXP20X_PWR_OP_MODE		0x01
> >>  #define AXP20X_USB_OTG_STATUS		0x02
> >> @@ -287,6 +300,15 @@ enum axp20x_variants {
> >>  #define AXP288_FG_TUNE5             0xed
> >>  
> >>  /* Regulators IDs */
> >> +enum {
> >> +	AXP1530_DCDC1 = 0,
> >> +	AXP1530_DCDC2,
> >> +	AXP1530_DCDC3,
> >> +	AXP1530_LDO1,
> >> +	AXP1530_LDO2,  
> >
> >I guess we should add the RTC LDO as LDO3 here.
> >
> >The rest of the numbers match with the datasheet.
> >  
> Ack.
> 
> I will take some time off due to Uni so v6 will be delayed peobably last holidays.
> 
> Best regards and happy holidays,
> Martin
> >Cheers,
> >Andre
> >  
> >> +	AXP1530_REG_ID_MAX,
> >> +};
> >> +
> >>  enum {
> >>  	AXP20X_LDO1 = 0,
> >>  	AXP20X_LDO2,
> >> @@ -440,6 +462,16 @@ enum {
> >>  	AXP152_IRQ_GPIO0_INPUT,
> >>  };
> >>  
> >> +enum axp1530_irqs {
> >> +	AXP1530_IRQ_TEMP_OVER,
> >> +	AXP1530_IRQ_DCDC2_UNDER = 2,
> >> +	AXP1530_IRQ_DCDC3_UNDER,
> >> +	AXP1530_IRQ_POKLIRQ_EN,
> >> +	AXP1530_IRQ_POKSIRQ_EN,
> >> +	AXP1530_IRQ_KEY_L2H_EN,
> >> +	AXP1530_IRQ_KEY_H2L_EN,
> >> +};
> >> +
> >>  enum {
> >>  	AXP20X_IRQ_ACIN_OVER_V = 1,
> >>  	AXP20X_IRQ_ACIN_PLUGIN,  
> >  

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Martin Botka]

On Tue, Jan 10 2023 at 12:00:25 AM +00:00:00, Andre Przywara 
<andre.przywara@arm.com> wrote:
> On Sat, 17 Dec 2022 01:13:01 +0100
> Martin Botka <martin.botka@somainline.org> wrote:
> 
> Hi Martin,
Hello Andre,
> 
> hope you had a good break! Did you have any chance to come back to 
> this
> again? Now would be a good time to send a new version, otherwise it's
> getting pretty tight for v6.3 already.
> 
I did have a good break :)
I unfortunately did not. Was bit busy with issues to one of my projects.
Will see if I can sort this out tomorrow but you said you already have 
AXP313A driver
ready. So it may be better if you send your driver.
> On Friday, "junari" in the #linux-sunxi IRC channel, made some
> interesting discovery: he is playing around with an AXP313a on some
> H616 device and figured that DCDC3 is not behaving like the datasheet:
> https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528;
> He later confirmed the voltage:
> https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373;
> 
Ah interesting. What i also found out right after signing off is from a 
friend at BIQU
that the AXP1530 is the AXP313A. It is the same chip. The only 
difference being that AXP1530
is the internal naming AXP uses. I mean we kinda figured that one out 
but its nice to have
confirmation on this :)
> Basically it looks like the DCDC3 parameters you harvested from the 
> BSP
> code seem to be correct after all. Do you have any chance to measure
> the voltage?
I wish i had. Dont think my multimeter would be able to be so precise.
Will try to source some oscilloscope but that wont be in time 
unfortunately.
> If not, can we try to deduce what the right settings are? The voltage
> difference seems to be significant (860mV vs 1200mV), I wonder if any
> device connected there (DRAM?) would work with the wrong setting?
I will try to be around in IRC sunxi channel "today" from around 
13:00UTC+1.
Would be prob best to discuss this there and then share our findings 
here on LKML :)

Cheers,
Martin
> 
> Cheers,
> Andre
> 
>>  On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara 
>> <andre.przywara@arm.com> wrote:
>>  >On Wed, 14 Dec 2022 20:03:04 +0100
>>  >Martin Botka <martin.botka@somainline.org> wrote:
>>  >
>>  >Hi Martin,
>>  >
>>  >> AXP1530 is a PMIC chip produced by X-Powers and an be connected 
>> via
>>  >> I2C bus.
>>  >> Where AXP313A seems to be closely related so the same driver can 
>> be used and
>>  >> seen it only paired with H616 SoC.
>>  >
>>  >So as mentioned, I am pretending this is for the AXP313A now, 
>> looking at
>>  >its datasheet.
>>  >Of course the elephant in the room is s/AXP1530/AXP313A/, but 
>> other than
>>  >that:
>>  >
>>  >>
>>  >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>  >> ---
>>  >>  drivers/mfd/axp20x-i2c.c   |  2 ++
>>  >>  drivers/mfd/axp20x.c       | 62 
>> ++++++++++++++++++++++++++++++++++++++
>>  >>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
>>  >>  3 files changed, 96 insertions(+)
>>  >>
>>  >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>>  >> index 8fd6727dc30a..6bfb931a580e 100644
>>  >> --- a/drivers/mfd/axp20x-i2c.c
>>  >> +++ b/drivers/mfd/axp20x-i2c.c
>>  >> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct 
>> i2c_client *i2c)
>>  >>  #ifdef CONFIG_OF
>>  >>  static const struct of_device_id axp20x_i2c_of_match[] = {
>>  >>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
>>  >> +	{ .compatible = "x-powers,axp1530", .data = (void 
>> *)AXP1530_ID},
>>  >>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
>>  >>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
>>  >>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
>>  >> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>>  >>
>>  >>  static const struct i2c_device_id axp20x_i2c_id[] = {
>>  >>  	{ "axp152", 0 },
>>  >> +	{ "axp1530", 0 },
>>  >>  	{ "axp202", 0 },
>>  >>  	{ "axp209", 0 },
>>  >>  	{ "axp221", 0 },
>>  >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>  >> index 880c41fa7021..6caa7e87ad80 100644
>>  >> --- a/drivers/mfd/axp20x.c
>>  >> +++ b/drivers/mfd/axp20x.c
>>  >> @@ -34,6 +34,7 @@
>>  >>
>>  >>  static const char * const axp20x_model_names[] = {
>>  >>  	"AXP152",
>>  >> +	"AXP1530",
>>  >>  	"AXP202",
>>  >>  	"AXP209",
>>  >>  	"AXP221",
>>  >> @@ -66,6 +67,24 @@ static const struct regmap_access_table 
>> axp152_volatile_table = {
>>  >>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
>>  >>  };
>>  >>
>>  >> +static const struct regmap_range axp1530_writeable_ranges[] = {
>>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
>>  >
>>  >Where does this FREQUENCY register come from? BSP source? Is that 
>> the
>>  >lost register to set the PWM frequency?
>>  >The 313 datasheet doesn't mention it, and since we deny 
>> programming the
>>  >frequency, I would just leave it out.
>>  >If people find it existing (and useful!) later on, we should be 
>> able to
>>  >add it without breaking anything.
>> 
>>  BSP. Ack.
>>  >
>>  >> +};
>>  >> +
>>  >> +static const struct regmap_range axp1530_volatile_ranges[] = {
>>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
>>  >> +};
>>  >> +
>>  >> +static const struct regmap_access_table axp1530_writeable_table 
>> = {
>>  >> +	.yes_ranges = axp1530_writeable_ranges,
>>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
>>  >> +};
>>  >> +
>>  >> +static const struct regmap_access_table axp1530_volatile_table 
>> = {
>>  >> +	.yes_ranges = axp1530_volatile_ranges,
>>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
>>  >> +};
>>  >> +
>>  >>  static const struct regmap_range axp20x_writeable_ranges[] = {
>>  >>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>>  >>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
>>  >> @@ -245,6 +264,15 @@ static const struct regmap_config 
>> axp152_regmap_config = {
>>  >>  	.cache_type	= REGCACHE_RBTREE,
>>  >>  };
>>  >>
>>  >> +static const struct regmap_config axp1530_regmap_config = {
>>  >> +	.reg_bits = 8,
>>  >> +	.val_bits = 8,
>>  >> +	.wr_table = &axp1530_writeable_table,
>>  >> +	.volatile_table = &axp1530_volatile_table,
>>  >> +	.max_register = AXP1530_FREQUENCY,
>>  >> +	.cache_type = REGCACHE_RBTREE,
>>  >> +};
>>  >> +
>>  >>  static const struct regmap_config axp20x_regmap_config = {
>>  >>  	.reg_bits	= 8,
>>  >>  	.val_bits	= 8,
>>  >> @@ -304,6 +332,16 @@ static const struct regmap_irq 
>> axp152_regmap_irqs[] = {
>>  >>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
>>  >>  };
>>  >>
>>  >> +static const struct regmap_irq axp1530_regmap_irqs[] = {
>>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
>>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
>>  >> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
>>  >> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),
>>  >
>>  >Are those identifiers from the BSP source? The (translated) manual 
>> gives
>>  >some explanation, namely:
>>  >	PWRON key rising edge
>>  >	PWRON key falling edge
>>  >	Short press the PWRON button
>>  >	Long press the PWRON button
>>  >
>>  >So I'd suggest we follow the existing naming:
>>  >	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
>>  >
>>  >Or come up with names that people could actually decipher ;-)
>>  >
>>  >
>>  Ack.
>>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
>>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
>>  >> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
>>  >> +};
>>  >> +
>>  >>  static const struct regmap_irq axp20x_regmap_irqs[] = {
>>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
>>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
>>  >> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip 
>> axp152_regmap_irq_chip = {
>>  >>  	.num_regs		= 3,
>>  >>  };
>>  >>
>>  >> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
>>  >> +	.name = "axp1530_irq_chip",
>>  >> +	.status_base = AXP1530_IRQ_STATUS1,
>>  >> +	.ack_base = AXP1530_IRQ_STATUS1,
>>  >> +	.mask_base = AXP1530_IRQ_ENABLE1,
>>  >> +	.mask_invert = true,
>>  >> +	.init_ack_masked = true,
>>  >> +	.irqs = axp1530_regmap_irqs,
>>  >> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
>>  >> +	.num_regs = 1,
>>  >> +};
>>  >> +
>>  >>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
>>  >>  	.name			= "axp20x_irq_chip",
>>  >>  	.status_base		= AXP20X_IRQ1_STATE,
>>  >> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[] 
>> = {
>>  >>  	},
>>  >>  };
>>  >>
>>  >> +static struct mfd_cell axp1530_cells[] = {
>>  >> +	{
>>  >> +		.name = "axp20x-regulator",
>>  >> +	},
>>  >> +};
>>  >> +
>>  >>  static const struct resource axp288_adc_resources[] = {
>>  >>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
>>  >>  };
>>  >> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev 
>> *axp20x)
>>  >>  		axp20x->regmap_cfg = &axp152_regmap_config;
>>  >>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
>>  >>  		break;
>>  >> +	case AXP1530_ID:
>>  >> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
>>  >> +		axp20x->cells = axp1530_cells;
>>  >> +		axp20x->regmap_cfg = &axp1530_regmap_config;
>>  >> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
>>  >> +		break;
>>  >>  	case AXP202_ID:
>>  >>  	case AXP209_ID:
>>  >>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
>>  >> diff --git a/include/linux/mfd/axp20x.h 
>> b/include/linux/mfd/axp20x.h
>>  >> index 9ab0e2fca7ea..cad25754500f 100644
>>  >> --- a/include/linux/mfd/axp20x.h
>>  >> +++ b/include/linux/mfd/axp20x.h
>>  >> @@ -12,6 +12,7 @@
>>  >>
>>  >>  enum axp20x_variants {
>>  >>  	AXP152_ID = 0,
>>  >> +	AXP1530_ID,
>>  >>  	AXP202_ID,
>>  >>  	AXP209_ID,
>>  >>  	AXP221_ID,
>>  >> @@ -45,6 +46,18 @@ enum axp20x_variants {
>>  >>  #define AXP152_DCDC_FREQ		0x37
>>  >>  #define AXP152_DCDC_MODE		0x80
>>  >>
>>  >> +#define AXP1530_ON_INDICATE		0x00
>>  >> +#define AXP1530_OUTPUT_CONTROL	0x10
>>  >> +#define AXP1530_DCDC1_CONRTOL	0x13
>>  >> +#define AXP1530_DCDC2_CONRTOL	0x14
>>  >> +#define AXP1530_DCDC3_CONRTOL	0x15
>>  >> +#define AXP1530_ALDO1_CONRTOL	0x16
>>  >> +#define AXP1530_DLDO1_CONRTOL	0x17
>>  >> +#define AXP1530_OUTOUT_MONITOR	0x1D
>>  >
>>  >Shall this read AXP1530_OUTPUT_MONITOR?
>>  >
>>  >> +#define AXP1530_IRQ_ENABLE1		0x20
>>  >> +#define AXP1530_IRQ_STATUS1		0x21
>>  >
>>  >There is only one interrupt register, so can we drop the trailing 
>> number?
>>  >
>>  Yep.
>>  >> +#define AXP1530_FREQUENCY		0x87
>>  >
>>  >As mentioned, the manual does not mention it, and we don't use it 
>> anyway.
>>  >
>>  Ack.
>>  >> +
>>  >>  #define AXP20X_PWR_INPUT_STATUS		0x00
>>  >>  #define AXP20X_PWR_OP_MODE		0x01
>>  >>  #define AXP20X_USB_OTG_STATUS		0x02
>>  >> @@ -287,6 +300,15 @@ enum axp20x_variants {
>>  >>  #define AXP288_FG_TUNE5             0xed
>>  >>
>>  >>  /* Regulators IDs */
>>  >> +enum {
>>  >> +	AXP1530_DCDC1 = 0,
>>  >> +	AXP1530_DCDC2,
>>  >> +	AXP1530_DCDC3,
>>  >> +	AXP1530_LDO1,
>>  >> +	AXP1530_LDO2,
>>  >
>>  >I guess we should add the RTC LDO as LDO3 here.
>>  >
>>  >The rest of the numbers match with the datasheet.
>>  >
>>  Ack.
>> 
>>  I will take some time off due to Uni so v6 will be delayed peobably 
>> last holidays.
>> 
>>  Best regards and happy holidays,
>>  Martin
>>  >Cheers,
>>  >Andre
>>  >
>>  >> +	AXP1530_REG_ID_MAX,
>>  >> +};
>>  >> +
>>  >>  enum {
>>  >>  	AXP20X_LDO1 = 0,
>>  >>  	AXP20X_LDO2,
>>  >> @@ -440,6 +462,16 @@ enum {
>>  >>  	AXP152_IRQ_GPIO0_INPUT,
>>  >>  };
>>  >>
>>  >> +enum axp1530_irqs {
>>  >> +	AXP1530_IRQ_TEMP_OVER,
>>  >> +	AXP1530_IRQ_DCDC2_UNDER = 2,
>>  >> +	AXP1530_IRQ_DCDC3_UNDER,
>>  >> +	AXP1530_IRQ_POKLIRQ_EN,
>>  >> +	AXP1530_IRQ_POKSIRQ_EN,
>>  >> +	AXP1530_IRQ_KEY_L2H_EN,
>>  >> +	AXP1530_IRQ_KEY_H2L_EN,
>>  >> +};
>>  >> +
>>  >>  enum {
>>  >>  	AXP20X_IRQ_ACIN_OVER_V = 1,
>>  >>  	AXP20X_IRQ_ACIN_PLUGIN,
>>  >
> 

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Andre Przywara]

On Tue, 10 Jan 2023 01:32:58 +0100
Martin Botka <martin.botka@somainline.org> wrote:

Hi Martin,

> On Tue, Jan 10 2023 at 12:00:25 AM +00:00:00, Andre Przywara 
> <andre.przywara@arm.com> wrote:
> > On Sat, 17 Dec 2022 01:13:01 +0100
> > Martin Botka <martin.botka@somainline.org> wrote:
> > 
> > Hi Martin,  
> Hello Andre,
> > 
> > hope you had a good break! Did you have any chance to come back to 
> > this
> > again? Now would be a good time to send a new version, otherwise it's
> > getting pretty tight for v6.3 already.
> >   
> I did have a good break :)
> I unfortunately did not. Was bit busy with issues to one of my projects.
> Will see if I can sort this out tomorrow but you said you already have 
> AXP313A driver
> ready. So it may be better if you send your driver.

Well, so my patch just compiled, and it wasn't as complete as yours.
Also I have no means of testing it. Also you were first with the patch,
so deserve the credits.

Since you seem to be busy in the next days, I could offer to address
the comments myself (since you acked them), and send a v6 on
your behalf. I would need to rely on you for testing, maybe Junari
(from #linux-sunxi) could also help out here.

> > On Friday, "junari" in the #linux-sunxi IRC channel, made some
> > interesting discovery: he is playing around with an AXP313a on some
> > H616 device and figured that DCDC3 is not behaving like the datasheet:
> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528;
> > He later confirmed the voltage:
> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373;
> >   
> Ah interesting. What i also found out right after signing off is from a 
> friend at BIQU
> that the AXP1530 is the AXP313A. It is the same chip. The only 
> difference being that AXP1530
> is the internal naming AXP uses. I mean we kinda figured that one out 
> but its nice to have
> confirmation on this :)

Indeed, thanks for that. Aside from the BSP driver, there doesn't seem
to be many records of the AXP1530, so I would go with the AXP313a name,
and leave the AXP1530 for the trivia section.

Cheers,
Andre


> > Basically it looks like the DCDC3 parameters you harvested from the 
> > BSP
> > code seem to be correct after all. Do you have any chance to measure
> > the voltage?  
> I wish i had. Dont think my multimeter would be able to be so precise.
> Will try to source some oscilloscope but that wont be in time 
> unfortunately.
> > If not, can we try to deduce what the right settings are? The voltage
> > difference seems to be significant (860mV vs 1200mV), I wonder if any
> > device connected there (DRAM?) would work with the wrong setting?  
> I will try to be around in IRC sunxi channel "today" from around 
> 13:00UTC+1.
> Would be prob best to discuss this there and then share our findings 
> here on LKML :)
> 
> Cheers,
> Martin
> > 
> > Cheers,
> > Andre
> >   
> >>  On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara 
> >> <andre.przywara@arm.com> wrote:  
> >>  >On Wed, 14 Dec 2022 20:03:04 +0100
> >>  >Martin Botka <martin.botka@somainline.org> wrote:
> >>  >
> >>  >Hi Martin,
> >>  >  
> >>  >> AXP1530 is a PMIC chip produced by X-Powers and an be connected   
> >> via  
> >>  >> I2C bus.
> >>  >> Where AXP313A seems to be closely related so the same driver can   
> >> be used and  
> >>  >> seen it only paired with H616 SoC.  
> >>  >
> >>  >So as mentioned, I am pretending this is for the AXP313A now,   
> >> looking at  
> >>  >its datasheet.
> >>  >Of course the elephant in the room is s/AXP1530/AXP313A/, but   
> >> other than  
> >>  >that:
> >>  >  
> >>  >>
> >>  >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> >>  >> ---
> >>  >>  drivers/mfd/axp20x-i2c.c   |  2 ++
> >>  >>  drivers/mfd/axp20x.c       | 62   
> >> ++++++++++++++++++++++++++++++++++++++  
> >>  >>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
> >>  >>  3 files changed, 96 insertions(+)
> >>  >>
> >>  >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
> >>  >> index 8fd6727dc30a..6bfb931a580e 100644
> >>  >> --- a/drivers/mfd/axp20x-i2c.c
> >>  >> +++ b/drivers/mfd/axp20x-i2c.c
> >>  >> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct   
> >> i2c_client *i2c)  
> >>  >>  #ifdef CONFIG_OF
> >>  >>  static const struct of_device_id axp20x_i2c_of_match[] = {
> >>  >>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
> >>  >> +	{ .compatible = "x-powers,axp1530", .data = (void   
> >> *)AXP1530_ID},  
> >>  >>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
> >>  >>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
> >>  >>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
> >>  >> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
> >>  >>
> >>  >>  static const struct i2c_device_id axp20x_i2c_id[] = {
> >>  >>  	{ "axp152", 0 },
> >>  >> +	{ "axp1530", 0 },
> >>  >>  	{ "axp202", 0 },
> >>  >>  	{ "axp209", 0 },
> >>  >>  	{ "axp221", 0 },
> >>  >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> >>  >> index 880c41fa7021..6caa7e87ad80 100644
> >>  >> --- a/drivers/mfd/axp20x.c
> >>  >> +++ b/drivers/mfd/axp20x.c
> >>  >> @@ -34,6 +34,7 @@
> >>  >>
> >>  >>  static const char * const axp20x_model_names[] = {
> >>  >>  	"AXP152",
> >>  >> +	"AXP1530",
> >>  >>  	"AXP202",
> >>  >>  	"AXP209",
> >>  >>  	"AXP221",
> >>  >> @@ -66,6 +67,24 @@ static const struct regmap_access_table   
> >> axp152_volatile_table = {  
> >>  >>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
> >>  >>  };
> >>  >>
> >>  >> +static const struct regmap_range axp1530_writeable_ranges[] = {
> >>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),  
> >>  >
> >>  >Where does this FREQUENCY register come from? BSP source? Is that   
> >> the  
> >>  >lost register to set the PWM frequency?
> >>  >The 313 datasheet doesn't mention it, and since we deny   
> >> programming the  
> >>  >frequency, I would just leave it out.
> >>  >If people find it existing (and useful!) later on, we should be   
> >> able to  
> >>  >add it without breaking anything.  
> >> 
> >>  BSP. Ack.  
> >>  >  
> >>  >> +};
> >>  >> +
> >>  >> +static const struct regmap_range axp1530_volatile_ranges[] = {
> >>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
> >>  >> +};
> >>  >> +
> >>  >> +static const struct regmap_access_table axp1530_writeable_table   
> >> = {  
> >>  >> +	.yes_ranges = axp1530_writeable_ranges,
> >>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
> >>  >> +};
> >>  >> +
> >>  >> +static const struct regmap_access_table axp1530_volatile_table   
> >> = {  
> >>  >> +	.yes_ranges = axp1530_volatile_ranges,
> >>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
> >>  >> +};
> >>  >> +
> >>  >>  static const struct regmap_range axp20x_writeable_ranges[] = {
> >>  >>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
> >>  >>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
> >>  >> @@ -245,6 +264,15 @@ static const struct regmap_config   
> >> axp152_regmap_config = {  
> >>  >>  	.cache_type	= REGCACHE_RBTREE,
> >>  >>  };
> >>  >>
> >>  >> +static const struct regmap_config axp1530_regmap_config = {
> >>  >> +	.reg_bits = 8,
> >>  >> +	.val_bits = 8,
> >>  >> +	.wr_table = &axp1530_writeable_table,
> >>  >> +	.volatile_table = &axp1530_volatile_table,
> >>  >> +	.max_register = AXP1530_FREQUENCY,
> >>  >> +	.cache_type = REGCACHE_RBTREE,
> >>  >> +};
> >>  >> +
> >>  >>  static const struct regmap_config axp20x_regmap_config = {
> >>  >>  	.reg_bits	= 8,
> >>  >>  	.val_bits	= 8,
> >>  >> @@ -304,6 +332,16 @@ static const struct regmap_irq   
> >> axp152_regmap_irqs[] = {  
> >>  >>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
> >>  >>  };
> >>  >>
> >>  >> +static const struct regmap_irq axp1530_regmap_irqs[] = {
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),  
> >>  >
> >>  >Are those identifiers from the BSP source? The (translated) manual   
> >> gives  
> >>  >some explanation, namely:
> >>  >	PWRON key rising edge
> >>  >	PWRON key falling edge
> >>  >	Short press the PWRON button
> >>  >	Long press the PWRON button
> >>  >
> >>  >So I'd suggest we follow the existing naming:
> >>  >	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
> >>  >
> >>  >Or come up with names that people could actually decipher ;-)
> >>  >
> >>  >  
> >>  Ack.  
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
> >>  >> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
> >>  >> +};
> >>  >> +
> >>  >>  static const struct regmap_irq axp20x_regmap_irqs[] = {
> >>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
> >>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
> >>  >> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip   
> >> axp152_regmap_irq_chip = {  
> >>  >>  	.num_regs		= 3,
> >>  >>  };
> >>  >>
> >>  >> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
> >>  >> +	.name = "axp1530_irq_chip",
> >>  >> +	.status_base = AXP1530_IRQ_STATUS1,
> >>  >> +	.ack_base = AXP1530_IRQ_STATUS1,
> >>  >> +	.mask_base = AXP1530_IRQ_ENABLE1,
> >>  >> +	.mask_invert = true,
> >>  >> +	.init_ack_masked = true,
> >>  >> +	.irqs = axp1530_regmap_irqs,
> >>  >> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
> >>  >> +	.num_regs = 1,
> >>  >> +};
> >>  >> +
> >>  >>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
> >>  >>  	.name			= "axp20x_irq_chip",
> >>  >>  	.status_base		= AXP20X_IRQ1_STATE,
> >>  >> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[]   
> >> = {  
> >>  >>  	},
> >>  >>  };
> >>  >>
> >>  >> +static struct mfd_cell axp1530_cells[] = {
> >>  >> +	{
> >>  >> +		.name = "axp20x-regulator",
> >>  >> +	},
> >>  >> +};
> >>  >> +
> >>  >>  static const struct resource axp288_adc_resources[] = {
> >>  >>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
> >>  >>  };
> >>  >> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev   
> >> *axp20x)  
> >>  >>  		axp20x->regmap_cfg = &axp152_regmap_config;
> >>  >>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
> >>  >>  		break;
> >>  >> +	case AXP1530_ID:
> >>  >> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
> >>  >> +		axp20x->cells = axp1530_cells;
> >>  >> +		axp20x->regmap_cfg = &axp1530_regmap_config;
> >>  >> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
> >>  >> +		break;
> >>  >>  	case AXP202_ID:
> >>  >>  	case AXP209_ID:
> >>  >>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
> >>  >> diff --git a/include/linux/mfd/axp20x.h   
> >> b/include/linux/mfd/axp20x.h  
> >>  >> index 9ab0e2fca7ea..cad25754500f 100644
> >>  >> --- a/include/linux/mfd/axp20x.h
> >>  >> +++ b/include/linux/mfd/axp20x.h
> >>  >> @@ -12,6 +12,7 @@
> >>  >>
> >>  >>  enum axp20x_variants {
> >>  >>  	AXP152_ID = 0,
> >>  >> +	AXP1530_ID,
> >>  >>  	AXP202_ID,
> >>  >>  	AXP209_ID,
> >>  >>  	AXP221_ID,
> >>  >> @@ -45,6 +46,18 @@ enum axp20x_variants {
> >>  >>  #define AXP152_DCDC_FREQ		0x37
> >>  >>  #define AXP152_DCDC_MODE		0x80
> >>  >>
> >>  >> +#define AXP1530_ON_INDICATE		0x00
> >>  >> +#define AXP1530_OUTPUT_CONTROL	0x10
> >>  >> +#define AXP1530_DCDC1_CONRTOL	0x13
> >>  >> +#define AXP1530_DCDC2_CONRTOL	0x14
> >>  >> +#define AXP1530_DCDC3_CONRTOL	0x15
> >>  >> +#define AXP1530_ALDO1_CONRTOL	0x16
> >>  >> +#define AXP1530_DLDO1_CONRTOL	0x17
> >>  >> +#define AXP1530_OUTOUT_MONITOR	0x1D  
> >>  >
> >>  >Shall this read AXP1530_OUTPUT_MONITOR?
> >>  >  
> >>  >> +#define AXP1530_IRQ_ENABLE1		0x20
> >>  >> +#define AXP1530_IRQ_STATUS1		0x21  
> >>  >
> >>  >There is only one interrupt register, so can we drop the trailing   
> >> number?  
> >>  >  
> >>  Yep.  
> >>  >> +#define AXP1530_FREQUENCY		0x87  
> >>  >
> >>  >As mentioned, the manual does not mention it, and we don't use it   
> >> anyway.  
> >>  >  
> >>  Ack.  
> >>  >> +
> >>  >>  #define AXP20X_PWR_INPUT_STATUS		0x00
> >>  >>  #define AXP20X_PWR_OP_MODE		0x01
> >>  >>  #define AXP20X_USB_OTG_STATUS		0x02
> >>  >> @@ -287,6 +300,15 @@ enum axp20x_variants {
> >>  >>  #define AXP288_FG_TUNE5             0xed
> >>  >>
> >>  >>  /* Regulators IDs */
> >>  >> +enum {
> >>  >> +	AXP1530_DCDC1 = 0,
> >>  >> +	AXP1530_DCDC2,
> >>  >> +	AXP1530_DCDC3,
> >>  >> +	AXP1530_LDO1,
> >>  >> +	AXP1530_LDO2,  
> >>  >
> >>  >I guess we should add the RTC LDO as LDO3 here.
> >>  >
> >>  >The rest of the numbers match with the datasheet.
> >>  >  
> >>  Ack.
> >> 
> >>  I will take some time off due to Uni so v6 will be delayed peobably 
> >> last holidays.
> >> 
> >>  Best regards and happy holidays,
> >>  Martin  
> >>  >Cheers,
> >>  >Andre
> >>  >  
> >>  >> +	AXP1530_REG_ID_MAX,
> >>  >> +};
> >>  >> +
> >>  >>  enum {
> >>  >>  	AXP20X_LDO1 = 0,
> >>  >>  	AXP20X_LDO2,
> >>  >> @@ -440,6 +462,16 @@ enum {
> >>  >>  	AXP152_IRQ_GPIO0_INPUT,
> >>  >>  };
> >>  >>
> >>  >> +enum axp1530_irqs {
> >>  >> +	AXP1530_IRQ_TEMP_OVER,
> >>  >> +	AXP1530_IRQ_DCDC2_UNDER = 2,
> >>  >> +	AXP1530_IRQ_DCDC3_UNDER,
> >>  >> +	AXP1530_IRQ_POKLIRQ_EN,
> >>  >> +	AXP1530_IRQ_POKSIRQ_EN,
> >>  >> +	AXP1530_IRQ_KEY_L2H_EN,
> >>  >> +	AXP1530_IRQ_KEY_H2L_EN,
> >>  >> +};
> >>  >> +
> >>  >>  enum {
> >>  >>  	AXP20X_IRQ_ACIN_OVER_V = 1,
> >>  >>  	AXP20X_IRQ_ACIN_PLUGIN,  
> >>  >  
> >   
> 
> 

Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC

[Martin Botka]

On January 13, 2023 1:35:55 AM GMT+01:00, Andre Przywara <andre.przywara@arm.com> wrote:
>On Tue, 10 Jan 2023 01:32:58 +0100
>Martin Botka <martin.botka@somainline.org> wrote:
>
>Hi Martin,
>
>> On Tue, Jan 10 2023 at 12:00:25 AM +00:00:00, Andre Przywara 
>> <andre.przywara@arm.com> wrote:
>> > On Sat, 17 Dec 2022 01:13:01 +0100
>> > Martin Botka <martin.botka@somainline.org> wrote:
>> > 
>> > Hi Martin,  
>> Hello Andre,
>> > 
>> > hope you had a good break! Did you have any chance to come back to 
>> > this
>> > again? Now would be a good time to send a new version, otherwise it's
>> > getting pretty tight for v6.3 already.
>> >   
>> I did have a good break :)
>> I unfortunately did not. Was bit busy with issues to one of my projects.
>> Will see if I can sort this out tomorrow but you said you already have 
>> AXP313A driver
>> ready. So it may be better if you send your driver.
>
>Well, so my patch just compiled, and it wasn't as complete as yours.
>Also I have no means of testing it. Also you were first with the patch,
>so deserve the credits.
>
>Since you seem to be busy in the next days, I could offer to address
>the comments myself (since you acked them), and send a v6 on
>your behalf. I would need to rely on you for testing, maybe Junari
>(from #linux-sunxi) could also help out here.
That is very kind of you. But please add yourself as co-author or something like that so your contribution isn't without credit.

As for testing I can after 21:30UTC+1. I should be on IRC after that hopefully.

Cheers,
Martin
>
>> > On Friday, "junari" in the #linux-sunxi IRC channel, made some
>> > interesting discovery: he is playing around with an AXP313a on some
>> > H616 device and figured that DCDC3 is not behaving like the datasheet:
>> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-06#31784528;
>> > He later confirmed the voltage:
>> > https://oftc.irclog.whitequark.org/linux-sunxi/2023-01-08#31788373;
>> >   
>> Ah interesting. What i also found out right after signing off is from a 
>> friend at BIQU
>> that the AXP1530 is the AXP313A. It is the same chip. The only 
>> difference being that AXP1530
>> is the internal naming AXP uses. I mean we kinda figured that one out 
>> but its nice to have
>> confirmation on this :)
>
>Indeed, thanks for that. Aside from the BSP driver, there doesn't seem
>to be many records of the AXP1530, so I would go with the AXP313a name,
>and leave the AXP1530 for the trivia section.
>
>Cheers,
>Andre
>
>
>> > Basically it looks like the DCDC3 parameters you harvested from the 
>> > BSP
>> > code seem to be correct after all. Do you have any chance to measure
>> > the voltage?  
>> I wish i had. Dont think my multimeter would be able to be so precise.
>> Will try to source some oscilloscope but that wont be in time 
>> unfortunately.
>> > If not, can we try to deduce what the right settings are? The voltage
>> > difference seems to be significant (860mV vs 1200mV), I wonder if any
>> > device connected there (DRAM?) would work with the wrong setting?  
>> I will try to be around in IRC sunxi channel "today" from around 
>> 13:00UTC+1.
>> Would be prob best to discuss this there and then share our findings 
>> here on LKML :)
>> 
>> Cheers,
>> Martin
>> > 
>> > Cheers,
>> > Andre
>> >   
>> >>  On December 16, 2022 7:17:52 PM GMT+01:00, Andre Przywara 
>> >> <andre.przywara@arm.com> wrote:  
>> >>  >On Wed, 14 Dec 2022 20:03:04 +0100
>> >>  >Martin Botka <martin.botka@somainline.org> wrote:
>> >>  >
>> >>  >Hi Martin,
>> >>  >  
>> >>  >> AXP1530 is a PMIC chip produced by X-Powers and an be connected   
>> >> via  
>> >>  >> I2C bus.
>> >>  >> Where AXP313A seems to be closely related so the same driver can   
>> >> be used and  
>> >>  >> seen it only paired with H616 SoC.  
>> >>  >
>> >>  >So as mentioned, I am pretending this is for the AXP313A now,   
>> >> looking at  
>> >>  >its datasheet.
>> >>  >Of course the elephant in the room is s/AXP1530/AXP313A/, but   
>> >> other than  
>> >>  >that:
>> >>  >  
>> >>  >>
>> >>  >> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>> >>  >> ---
>> >>  >>  drivers/mfd/axp20x-i2c.c   |  2 ++
>> >>  >>  drivers/mfd/axp20x.c       | 62   
>> >> ++++++++++++++++++++++++++++++++++++++  
>> >>  >>  include/linux/mfd/axp20x.h | 32 ++++++++++++++++++++
>> >>  >>  3 files changed, 96 insertions(+)
>> >>  >>
>> >>  >> diff --git a/drivers/mfd/axp20x-i2c.c b/drivers/mfd/axp20x-i2c.c
>> >>  >> index 8fd6727dc30a..6bfb931a580e 100644
>> >>  >> --- a/drivers/mfd/axp20x-i2c.c
>> >>  >> +++ b/drivers/mfd/axp20x-i2c.c
>> >>  >> @@ -60,6 +60,7 @@ static void axp20x_i2c_remove(struct   
>> >> i2c_client *i2c)  
>> >>  >>  #ifdef CONFIG_OF
>> >>  >>  static const struct of_device_id axp20x_i2c_of_match[] = {
>> >>  >>  	{ .compatible = "x-powers,axp152", .data = (void *)AXP152_ID },
>> >>  >> +	{ .compatible = "x-powers,axp1530", .data = (void   
>> >> *)AXP1530_ID},  
>> >>  >>  	{ .compatible = "x-powers,axp202", .data = (void *)AXP202_ID },
>> >>  >>  	{ .compatible = "x-powers,axp209", .data = (void *)AXP209_ID },
>> >>  >>  	{ .compatible = "x-powers,axp221", .data = (void *)AXP221_ID },
>> >>  >> @@ -73,6 +74,7 @@ MODULE_DEVICE_TABLE(of, axp20x_i2c_of_match);
>> >>  >>
>> >>  >>  static const struct i2c_device_id axp20x_i2c_id[] = {
>> >>  >>  	{ "axp152", 0 },
>> >>  >> +	{ "axp1530", 0 },
>> >>  >>  	{ "axp202", 0 },
>> >>  >>  	{ "axp209", 0 },
>> >>  >>  	{ "axp221", 0 },
>> >>  >> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> >>  >> index 880c41fa7021..6caa7e87ad80 100644
>> >>  >> --- a/drivers/mfd/axp20x.c
>> >>  >> +++ b/drivers/mfd/axp20x.c
>> >>  >> @@ -34,6 +34,7 @@
>> >>  >>
>> >>  >>  static const char * const axp20x_model_names[] = {
>> >>  >>  	"AXP152",
>> >>  >> +	"AXP1530",
>> >>  >>  	"AXP202",
>> >>  >>  	"AXP209",
>> >>  >>  	"AXP221",
>> >>  >> @@ -66,6 +67,24 @@ static const struct regmap_access_table   
>> >> axp152_volatile_table = {  
>> >>  >>  	.n_yes_ranges	= ARRAY_SIZE(axp152_volatile_ranges),
>> >>  >>  };
>> >>  >>
>> >>  >> +static const struct regmap_range axp1530_writeable_ranges[] = {
>> >>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),  
>> >>  >
>> >>  >Where does this FREQUENCY register come from? BSP source? Is that   
>> >> the  
>> >>  >lost register to set the PWM frequency?
>> >>  >The 313 datasheet doesn't mention it, and since we deny   
>> >> programming the  
>> >>  >frequency, I would just leave it out.
>> >>  >If people find it existing (and useful!) later on, we should be   
>> >> able to  
>> >>  >add it without breaking anything.  
>> >> 
>> >>  BSP. Ack.  
>> >>  >  
>> >>  >> +};
>> >>  >> +
>> >>  >> +static const struct regmap_range axp1530_volatile_ranges[] = {
>> >>  >> +	regmap_reg_range(AXP1530_ON_INDICATE, AXP1530_FREQUENCY),
>> >>  >> +};
>> >>  >> +
>> >>  >> +static const struct regmap_access_table axp1530_writeable_table   
>> >> = {  
>> >>  >> +	.yes_ranges = axp1530_writeable_ranges,
>> >>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_writeable_ranges),
>> >>  >> +};
>> >>  >> +
>> >>  >> +static const struct regmap_access_table axp1530_volatile_table   
>> >> = {  
>> >>  >> +	.yes_ranges = axp1530_volatile_ranges,
>> >>  >> +	.n_yes_ranges = ARRAY_SIZE(axp1530_volatile_ranges),
>> >>  >> +};
>> >>  >> +
>> >>  >>  static const struct regmap_range axp20x_writeable_ranges[] = {
>> >>  >>  	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_IRQ5_STATE),
>> >>  >>  	regmap_reg_range(AXP20X_CHRG_CTRL1, AXP20X_CHRG_CTRL2),
>> >>  >> @@ -245,6 +264,15 @@ static const struct regmap_config   
>> >> axp152_regmap_config = {  
>> >>  >>  	.cache_type	= REGCACHE_RBTREE,
>> >>  >>  };
>> >>  >>
>> >>  >> +static const struct regmap_config axp1530_regmap_config = {
>> >>  >> +	.reg_bits = 8,
>> >>  >> +	.val_bits = 8,
>> >>  >> +	.wr_table = &axp1530_writeable_table,
>> >>  >> +	.volatile_table = &axp1530_volatile_table,
>> >>  >> +	.max_register = AXP1530_FREQUENCY,
>> >>  >> +	.cache_type = REGCACHE_RBTREE,
>> >>  >> +};
>> >>  >> +
>> >>  >>  static const struct regmap_config axp20x_regmap_config = {
>> >>  >>  	.reg_bits	= 8,
>> >>  >>  	.val_bits	= 8,
>> >>  >> @@ -304,6 +332,16 @@ static const struct regmap_irq   
>> >> axp152_regmap_irqs[] = {  
>> >>  >>  	INIT_REGMAP_IRQ(AXP152, GPIO0_INPUT,		2, 0),
>> >>  >>  };
>> >>  >>
>> >>  >> +static const struct regmap_irq axp1530_regmap_irqs[] = {
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_L2H_EN, 0, 7),
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, KEY_H2L_EN, 0, 6),
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, POKSIRQ_EN, 0, 5),
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, POKLIRQ_EN, 0, 4),  
>> >>  >
>> >>  >Are those identifiers from the BSP source? The (translated) manual   
>> >> gives  
>> >>  >some explanation, namely:
>> >>  >	PWRON key rising edge
>> >>  >	PWRON key falling edge
>> >>  >	Short press the PWRON button
>> >>  >	Long press the PWRON button
>> >>  >
>> >>  >So I'd suggest we follow the existing naming:
>> >>  >	PEK_RIS_EDGE, PEK_FAL_EDGE, PEK_SHORT, PEK_LONG (respectively)
>> >>  >
>> >>  >Or come up with names that people could actually decipher ;-)
>> >>  >
>> >>  >  
>> >>  Ack.  
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC3_UNDER, 0, 3),
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, DCDC2_UNDER, 0, 2),
>> >>  >> +	INIT_REGMAP_IRQ(AXP1530, TEMP_OVER, 0, 0),
>> >>  >> +};
>> >>  >> +
>> >>  >>  static const struct regmap_irq axp20x_regmap_irqs[] = {
>> >>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_OVER_V,		0, 7),
>> >>  >>  	INIT_REGMAP_IRQ(AXP20X, ACIN_PLUGIN,		0, 6),
>> >>  >> @@ -514,6 +552,18 @@ static const struct regmap_irq_chip   
>> >> axp152_regmap_irq_chip = {  
>> >>  >>  	.num_regs		= 3,
>> >>  >>  };
>> >>  >>
>> >>  >> +static const struct regmap_irq_chip axp1530_regmap_irq_chip = {
>> >>  >> +	.name = "axp1530_irq_chip",
>> >>  >> +	.status_base = AXP1530_IRQ_STATUS1,
>> >>  >> +	.ack_base = AXP1530_IRQ_STATUS1,
>> >>  >> +	.mask_base = AXP1530_IRQ_ENABLE1,
>> >>  >> +	.mask_invert = true,
>> >>  >> +	.init_ack_masked = true,
>> >>  >> +	.irqs = axp1530_regmap_irqs,
>> >>  >> +	.num_irqs = ARRAY_SIZE(axp1530_regmap_irqs),
>> >>  >> +	.num_regs = 1,
>> >>  >> +};
>> >>  >> +
>> >>  >>  static const struct regmap_irq_chip axp20x_regmap_irq_chip = {
>> >>  >>  	.name			= "axp20x_irq_chip",
>> >>  >>  	.status_base		= AXP20X_IRQ1_STATE,
>> >>  >> @@ -683,6 +733,12 @@ static const struct mfd_cell axp152_cells[]   
>> >> = {  
>> >>  >>  	},
>> >>  >>  };
>> >>  >>
>> >>  >> +static struct mfd_cell axp1530_cells[] = {
>> >>  >> +	{
>> >>  >> +		.name = "axp20x-regulator",
>> >>  >> +	},
>> >>  >> +};
>> >>  >> +
>> >>  >>  static const struct resource axp288_adc_resources[] = {
>> >>  >>  	DEFINE_RES_IRQ_NAMED(AXP288_IRQ_GPADC, "GPADC"),
>> >>  >>  };
>> >>  >> @@ -874,6 +930,12 @@ int axp20x_match_device(struct axp20x_dev   
>> >> *axp20x)  
>> >>  >>  		axp20x->regmap_cfg = &axp152_regmap_config;
>> >>  >>  		axp20x->regmap_irq_chip = &axp152_regmap_irq_chip;
>> >>  >>  		break;
>> >>  >> +	case AXP1530_ID:
>> >>  >> +		axp20x->nr_cells = ARRAY_SIZE(axp1530_cells);
>> >>  >> +		axp20x->cells = axp1530_cells;
>> >>  >> +		axp20x->regmap_cfg = &axp1530_regmap_config;
>> >>  >> +		axp20x->regmap_irq_chip = &axp1530_regmap_irq_chip;
>> >>  >> +		break;
>> >>  >>  	case AXP202_ID:
>> >>  >>  	case AXP209_ID:
>> >>  >>  		axp20x->nr_cells = ARRAY_SIZE(axp20x_cells);
>> >>  >> diff --git a/include/linux/mfd/axp20x.h   
>> >> b/include/linux/mfd/axp20x.h  
>> >>  >> index 9ab0e2fca7ea..cad25754500f 100644
>> >>  >> --- a/include/linux/mfd/axp20x.h
>> >>  >> +++ b/include/linux/mfd/axp20x.h
>> >>  >> @@ -12,6 +12,7 @@
>> >>  >>
>> >>  >>  enum axp20x_variants {
>> >>  >>  	AXP152_ID = 0,
>> >>  >> +	AXP1530_ID,
>> >>  >>  	AXP202_ID,
>> >>  >>  	AXP209_ID,
>> >>  >>  	AXP221_ID,
>> >>  >> @@ -45,6 +46,18 @@ enum axp20x_variants {
>> >>  >>  #define AXP152_DCDC_FREQ		0x37
>> >>  >>  #define AXP152_DCDC_MODE		0x80
>> >>  >>
>> >>  >> +#define AXP1530_ON_INDICATE		0x00
>> >>  >> +#define AXP1530_OUTPUT_CONTROL	0x10
>> >>  >> +#define AXP1530_DCDC1_CONRTOL	0x13
>> >>  >> +#define AXP1530_DCDC2_CONRTOL	0x14
>> >>  >> +#define AXP1530_DCDC3_CONRTOL	0x15
>> >>  >> +#define AXP1530_ALDO1_CONRTOL	0x16
>> >>  >> +#define AXP1530_DLDO1_CONRTOL	0x17
>> >>  >> +#define AXP1530_OUTOUT_MONITOR	0x1D  
>> >>  >
>> >>  >Shall this read AXP1530_OUTPUT_MONITOR?
>> >>  >  
>> >>  >> +#define AXP1530_IRQ_ENABLE1		0x20
>> >>  >> +#define AXP1530_IRQ_STATUS1		0x21  
>> >>  >
>> >>  >There is only one interrupt register, so can we drop the trailing   
>> >> number?  
>> >>  >  
>> >>  Yep.  
>> >>  >> +#define AXP1530_FREQUENCY		0x87  
>> >>  >
>> >>  >As mentioned, the manual does not mention it, and we don't use it   
>> >> anyway.  
>> >>  >  
>> >>  Ack.  
>> >>  >> +
>> >>  >>  #define AXP20X_PWR_INPUT_STATUS		0x00
>> >>  >>  #define AXP20X_PWR_OP_MODE		0x01
>> >>  >>  #define AXP20X_USB_OTG_STATUS		0x02
>> >>  >> @@ -287,6 +300,15 @@ enum axp20x_variants {
>> >>  >>  #define AXP288_FG_TUNE5             0xed
>> >>  >>
>> >>  >>  /* Regulators IDs */
>> >>  >> +enum {
>> >>  >> +	AXP1530_DCDC1 = 0,
>> >>  >> +	AXP1530_DCDC2,
>> >>  >> +	AXP1530_DCDC3,
>> >>  >> +	AXP1530_LDO1,
>> >>  >> +	AXP1530_LDO2,  
>> >>  >
>> >>  >I guess we should add the RTC LDO as LDO3 here.
>> >>  >
>> >>  >The rest of the numbers match with the datasheet.
>> >>  >  
>> >>  Ack.
>> >> 
>> >>  I will take some time off due to Uni so v6 will be delayed peobably 
>> >> last holidays.
>> >> 
>> >>  Best regards and happy holidays,
>> >>  Martin  
>> >>  >Cheers,
>> >>  >Andre
>> >>  >  
>> >>  >> +	AXP1530_REG_ID_MAX,
>> >>  >> +};
>> >>  >> +
>> >>  >>  enum {
>> >>  >>  	AXP20X_LDO1 = 0,
>> >>  >>  	AXP20X_LDO2,
>> >>  >> @@ -440,6 +462,16 @@ enum {
>> >>  >>  	AXP152_IRQ_GPIO0_INPUT,
>> >>  >>  };
>> >>  >>
>> >>  >> +enum axp1530_irqs {
>> >>  >> +	AXP1530_IRQ_TEMP_OVER,
>> >>  >> +	AXP1530_IRQ_DCDC2_UNDER = 2,
>> >>  >> +	AXP1530_IRQ_DCDC3_UNDER,
>> >>  >> +	AXP1530_IRQ_POKLIRQ_EN,
>> >>  >> +	AXP1530_IRQ_POKSIRQ_EN,
>> >>  >> +	AXP1530_IRQ_KEY_L2H_EN,
>> >>  >> +	AXP1530_IRQ_KEY_H2L_EN,
>> >>  >> +};
>> >>  >> +
>> >>  >>  enum {
>> >>  >>  	AXP20X_IRQ_ACIN_OVER_V = 1,
>> >>  >>  	AXP20X_IRQ_ACIN_PLUGIN,  
>> >>  >  
>> >   
>> 
>> 
>