* [PATCH v5 0/3] AXP1530 PMIC @ 2022-12-14 19:03 Martin Botka 2022-12-14 19:03 ` [PATCH v5 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP1530 variant Martin Botka ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Martin Botka @ 2022-12-14 19:03 UTC (permalink / raw) To: martin.botka1 Cc: Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Martin Botka, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP1530 variant 2022-12-14 19:03 [PATCH v5 0/3] AXP1530 PMIC Martin Botka @ 2022-12-14 19:03 ` Martin Botka 2022-12-14 19:03 ` [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC Martin Botka 2022-12-14 19:03 ` [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant Martin Botka 2 siblings, 0 replies; 14+ messages in thread From: Martin Botka @ 2022-12-14 19:03 UTC (permalink / raw) To: martin.botka1 Cc: Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Martin Botka, Krzysztof Kozlowski, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2022-12-14 19:03 [PATCH v5 0/3] AXP1530 PMIC Martin Botka 2022-12-14 19:03 ` [PATCH v5 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP1530 variant Martin Botka @ 2022-12-14 19:03 ` Martin Botka 2022-12-16 18:17 ` Andre Przywara 2022-12-14 19:03 ` [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant Martin Botka 2 siblings, 1 reply; 14+ messages in thread From: Martin Botka @ 2022-12-14 19:03 UTC (permalink / raw) To: martin.botka1 Cc: Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Martin Botka, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2022-12-14 19:03 ` [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC Martin Botka @ 2022-12-16 18:17 ` Andre Przywara 2022-12-17 0:13 ` Martin Botka 0 siblings, 1 reply; 14+ messages in thread From: Andre Przywara @ 2022-12-16 18:17 UTC (permalink / raw) To: Martin Botka Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2022-12-16 18:17 ` Andre Przywara @ 2022-12-17 0:13 ` Martin Botka 2023-01-10 0:00 ` Andre Przywara 0 siblings, 1 reply; 14+ messages in thread From: Martin Botka @ 2022-12-17 0:13 UTC (permalink / raw) To: Andre Przywara Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2022-12-17 0:13 ` Martin Botka @ 2023-01-10 0:00 ` Andre Przywara 2023-01-10 0:32 ` Martin Botka 0 siblings, 1 reply; 14+ messages in thread From: Andre Przywara @ 2023-01-10 0:00 UTC (permalink / raw) To: Martin Botka Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2023-01-10 0:00 ` Andre Przywara @ 2023-01-10 0:32 ` Martin Botka 2023-01-13 0:35 ` Andre Przywara 0 siblings, 1 reply; 14+ messages in thread From: Martin Botka @ 2023-01-10 0:32 UTC (permalink / raw) To: Andre Przywara Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, >> > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2023-01-10 0:32 ` Martin Botka @ 2023-01-13 0:35 ` Andre Przywara 2023-01-13 11:26 ` Martin Botka 0 siblings, 1 reply; 14+ messages in thread From: Andre Przywara @ 2023-01-13 0:35 UTC (permalink / raw) To: Martin Botka Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, > >> > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC 2023-01-13 0:35 ` Andre Przywara @ 2023-01-13 11:26 ` Martin Botka 0 siblings, 0 replies; 14+ messages in thread From: Martin Botka @ 2023-01-13 11:26 UTC (permalink / raw) To: Andre Przywara Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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, >> >> > >> > >> >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant 2022-12-14 19:03 [PATCH v5 0/3] AXP1530 PMIC Martin Botka 2022-12-14 19:03 ` [PATCH v5 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP1530 variant Martin Botka 2022-12-14 19:03 ` [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC Martin Botka @ 2022-12-14 19:03 ` Martin Botka 2022-12-15 23:16 ` Andre Przywara 2 siblings, 1 reply; 14+ messages in thread From: Martin Botka @ 2022-12-14 19:03 UTC (permalink / raw) To: martin.botka1 Cc: Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Martin Botka, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant 2022-12-14 19:03 ` [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant Martin Botka @ 2022-12-15 23:16 ` Andre Przywara 2022-12-16 5:26 ` Martin Botka 0 siblings, 1 reply; 14+ messages in thread From: Andre Przywara @ 2022-12-15 23:16 UTC (permalink / raw) To: Martin Botka Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Samuel Holland, Jernej Škrabec, linux-sunxi 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; ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant 2022-12-15 23:16 ` Andre Przywara @ 2022-12-16 5:26 ` Martin Botka 2022-12-16 11:52 ` Andre Przywara 0 siblings, 1 reply; 14+ messages in thread From: Martin Botka @ 2022-12-16 5:26 UTC (permalink / raw) To: Andre Przywara Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Samuel Holland, Jernej Škrabec, linux-sunxi 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; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant 2022-12-16 5:26 ` Martin Botka @ 2022-12-16 11:52 ` Andre Przywara 2022-12-16 12:20 ` Martin Botka 0 siblings, 1 reply; 14+ messages in thread From: Andre Przywara @ 2022-12-16 11:52 UTC (permalink / raw) To: Martin Botka Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Samuel Holland, Jernej Škrabec, linux-sunxi 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; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant 2022-12-16 11:52 ` Andre Przywara @ 2022-12-16 12:20 ` Martin Botka 0 siblings, 0 replies; 14+ messages in thread From: Martin Botka @ 2022-12-16 12:20 UTC (permalink / raw) To: Andre Przywara Cc: martin.botka1, Konrad Dybcio, AngeloGioacchino Del Regno, Marijn Suijten, Jami Kettunen, Paul Bouchara, Jan Trmal, Lee Jones, Rob Herring, Krzysztof Kozlowski, Chen-Yu Tsai, Liam Girdwood, Mark Brown, devicetree, linux-kernel, Samuel Holland, Jernej Škrabec, linux-sunxi 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; >> > > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-01-13 11:38 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-14 19:03 [PATCH v5 0/3] AXP1530 PMIC Martin Botka 2022-12-14 19:03 ` [PATCH v5 1/3] dt-bindings: mfd: x-powers,axp152: Document the AXP1530 variant Martin Botka 2022-12-14 19:03 ` [PATCH v5 2/3] mfd: ax20x: Add suppport for AXP1530 PMIC Martin Botka 2022-12-16 18:17 ` Andre Przywara 2022-12-17 0:13 ` Martin Botka 2023-01-10 0:00 ` Andre Przywara 2023-01-10 0:32 ` Martin Botka 2023-01-13 0:35 ` Andre Przywara 2023-01-13 11:26 ` Martin Botka 2022-12-14 19:03 ` [PATCH v5 3/3] regulator: axp20x: Add support for AXP1530 variant Martin Botka 2022-12-15 23:16 ` Andre Przywara 2022-12-16 5:26 ` Martin Botka 2022-12-16 11:52 ` Andre Przywara 2022-12-16 12:20 ` Martin Botka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.