[PATCH 0/4] regulator: Fix AXP717 PMIC support

[PATCH 0/4] regulator: Fix AXP717 PMIC support

[Andre Przywara]

Here are some fixes to the AXP717 PMIC support series. Lee put that in
an immutable branch already, so these here go on top.
Patch 1 contains fixes to the regulator descriptions: the LDOs had the
wrong supply source, and two numbers were wrong. The datasheet describes
the voltage ranges and register values differently from what our macros
expect, in a way that literally begs for off-by-ones, so here you go.
I don't know if that's still feasible, but it would be a good candidate
to squash into the patch that it fixes.

The other three patches add the "boost" regulator, which is meant to
provide the 5V USB VBUS power when operating from the battery. It's the
usual trinity of binding/mfd/regulator patches.
Again this could be squashed into the respective patches from the
original series, if people agree.

Please have a look and test on a device, since I could not do this.

Based on mfd/ib-mfd-regulator-6.10, as detailed below.

Cheers,
Andre

Andre Przywara (4):
  regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones
  dt-bindings: mfd: x-powers,axp152: add boost regulator
  mfd: axp20x: AXP717: Add support for boost regulator
  regulator: axp20x: AXP717: Add boost regulator

 .../bindings/mfd/x-powers,axp152.yaml         |  2 +-
 drivers/mfd/axp20x.c                          |  2 ++
 drivers/regulator/axp20x-regulator.c          | 32 +++++++++++--------
 include/linux/mfd/axp20x.h                    |  3 ++
 4 files changed, 24 insertions(+), 15 deletions(-)


base-commit: 4cece764965020c22cff7665b18a012006359095
prerequisite-patch-id: 2b5fb10f68e0994071fc4c7dce73db7047c23220
prerequisite-patch-id: 5d0735de888d155b2c1cdb814e852a5852a17ec7
prerequisite-patch-id: 29c30894b4bf0b9e1e71de065cabbd842505e248
prerequisite-patch-id: 0ab87cbf7362b6dc2d577d2264eb9574be47b5f6
-- 
2.35.8

[PATCH 1/4] regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones

[Andre Przywara]

The X-Powers AXP717 PMIC has separate input supply pins for each group
of LDOs, so they are not all using the same DCDC1 input, as described
currently.

Replace the "supply" member of each LDO description with the respective
group supply name, so that the supply dependencies can be correctly
described in the devicetree.
Also fix two off-by-ones in the regulator macros, after some double
checking the numbers against the datasheet.

Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/regulator/axp20x-regulator.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 34fcdd82b2eaa..3907606b091f6 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -140,7 +140,7 @@
 
 #define AXP717_DCDC1_NUM_VOLTAGES	88
 #define AXP717_DCDC2_NUM_VOLTAGES	107
-#define AXP717_DCDC3_NUM_VOLTAGES	104
+#define AXP717_DCDC3_NUM_VOLTAGES	103
 #define AXP717_DCDC_V_OUT_MASK		GENMASK(6, 0)
 #define AXP717_LDO_V_OUT_MASK		GENMASK(4, 0)
 
@@ -766,7 +766,7 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
 static const struct linear_range axp717_dcdc2_ranges[] = {
 	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
 	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
-	REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
+	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
 };
 
 static const struct linear_range axp717_dcdc3_ranges[] = {
@@ -790,40 +790,40 @@ static const struct regulator_desc axp717_regulators[] = {
 	AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
 		 AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
 		 AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
-	AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
 		 AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
-	AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
 		 AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
-	AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
 		 AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
-	AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
 		 AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
-	AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
 		 AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
-	AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
 		 AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
-	AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
 		 AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
-	AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
 		 AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
-	AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
 		 AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
-	AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
 		 AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
-	AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
 		 AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
-	AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
+	AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
 		 AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
 	AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
-- 
2.35.8

[PATCH 2/4] dt-bindings: mfd: x-powers,axp152: add boost regulator

[Andre Przywara]

The X-Powers AXP717 contains a boost regulator, that it meant to provide
the 5V USB VBUS voltage when the devices operates on battery.

Add the name "boost" to the regexp describing the allowed node names,
to allow the regulator to be described in the devicetree.

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

diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
index b8e8db0d58e9c..14ab367fc8871 100644
--- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
+++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
@@ -274,7 +274,7 @@ properties:
           Defines the work frequency of DC-DC in kHz.
 
     patternProperties:
-      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
+      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":
         $ref: /schemas/regulator/regulator.yaml#
         type: object
         unevaluatedProperties: false
-- 
2.35.8

[PATCH 3/4] mfd: axp20x: AXP717: Add support for boost regulator

[Andre Przywara]

The AXP717 also contains a boost regulator, to provide the 5V USB VBUS
rail when running on battery.

Add the registers to the MFD description to be able to use them from the
regulator driver.

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

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 48ce6ea693cea..62f3dd254d57b 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -209,6 +209,8 @@ static const struct regmap_access_table axp313a_volatile_table = {
 };
 
 static const struct regmap_range axp717_writeable_ranges[] = {
+	regmap_reg_range(AXP717_MODULE_EN_CONTROL, AXP717_MODULE_EN_CONTROL),
+	regmap_reg_range(AXP717_BOOST_CONTROL, AXP717_BOOST_CONTROL),
 	regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
 	regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
 };
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 8c0a33a2e9ce2..4dad54fdf67ee 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -115,6 +115,8 @@ enum axp20x_variants {
 #define AXP313A_IRQ_STATE		0x21
 
 #define AXP717_ON_INDICATE		0x00
+#define AXP717_MODULE_EN_CONTROL	0x19
+#define AXP717_BOOST_CONTROL		0x1e
 #define AXP717_IRQ0_EN			0x40
 #define AXP717_IRQ1_EN			0x41
 #define AXP717_IRQ2_EN			0x42
-- 
2.35.8

[PATCH 4/4] regulator: axp20x: AXP717: Add boost regulator

[Andre Przywara]

The AXP717 also contains an adjustable boost regulator, to provide the
5V USB VBUS rail when running on battery.

Add the regulator description that states the voltage range this
regulator can cover.

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

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index 3907606b091f6..71407c5810941 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -143,6 +143,7 @@
 #define AXP717_DCDC3_NUM_VOLTAGES	103
 #define AXP717_DCDC_V_OUT_MASK		GENMASK(6, 0)
 #define AXP717_LDO_V_OUT_MASK		GENMASK(4, 0)
+#define AXP717_BOOST_V_OUT_MASK		GENMASK(7, 4)
 
 #define AXP803_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
 #define AXP803_PWR_OUT_DCDC2_MASK	BIT_MASK(1)
@@ -829,6 +830,9 @@ static const struct regulator_desc axp717_regulators[] = {
 	AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
 		 AXP717_CPUSLDO_CONTROL, AXP717_LDO_V_OUT_MASK,
 		 AXP717_LDO1_OUTPUT_CONTROL, BIT(4)),
+	AXP_DESC(AXP717, BOOST, "boost", "vin1", 4550, 5510, 64,
+		 AXP717_BOOST_CONTROL, AXP717_BOOST_V_OUT_MASK,
+		 AXP717_MODULE_EN_CONTROL, BIT(4)),
 };
 
 /* DCDC ranges shared with AXP813 */
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 4dad54fdf67ee..5e86b976c4caa 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -486,6 +486,7 @@ enum {
 	AXP717_CLDO3,
 	AXP717_CLDO4,
 	AXP717_CPUSLDO,
+	AXP717_BOOST,
 	AXP717_REG_ID_MAX,
 };
 
-- 
2.35.8

Re: [PATCH 2/4] dt-bindings: mfd: x-powers,axp152: add boost regulator

[Krzysztof Kozlowski]

On 30/03/2024 00:50, Andre Przywara wrote:
> The X-Powers AXP717 contains a boost regulator, that it meant to provide
> the 5V USB VBUS voltage when the devices operates on battery.
> 
> Add the name "boost" to the regexp describing the allowed node names,
> to allow the regulator to be described in the devicetree.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> index b8e8db0d58e9c..14ab367fc8871 100644
> --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> @@ -274,7 +274,7 @@ properties:
>            Defines the work frequency of DC-DC in kHz.
>  
>      patternProperties:
> -      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
> +      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":

That's not an easy to read regex...

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

If driver does not depend on _, please consider dropping (_|-).


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: mfd: x-powers,axp152: add boost regulator

[Andre Przywara]

On Sat, 30 Mar 2024 10:30:05 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi,

> On 30/03/2024 00:50, Andre Przywara wrote:
> > The X-Powers AXP717 contains a boost regulator, that it meant to provide
> > the 5V USB VBUS voltage when the devices operates on battery.
> > 
> > Add the name "boost" to the regexp describing the allowed node names,
> > to allow the regulator to be described in the devicetree.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > index b8e8db0d58e9c..14ab367fc8871 100644
> > --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > @@ -274,7 +274,7 @@ properties:
> >            Defines the work frequency of DC-DC in kHz.
> >  
> >      patternProperties:
> > -      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
> > +      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":  
> 
> That's not an easy to read regex...

TBH regexps are the least of my problems when reading bindings ;-)

> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks!

> If driver does not depend on _, please consider dropping (_|-).

The drivers (checked both Linux and FreeBSD) do look for a specific
string, but it's the real old regulators that used ldo_io[01] and
rtc_ldo, all the "newer" ones use a dash. Since this binding covers all
of them, we can't drop this from this regexp, but rest assured we only
go with dashes for new and upcoming devices.

Thanks,
Andre

Re: [PATCH 2/4] dt-bindings: mfd: x-powers,axp152: add boost regulator

[Andre Przywara]

On Sat, 30 Mar 2024 10:30:05 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

Hi,

> On 30/03/2024 00:50, Andre Przywara wrote:
> > The X-Powers AXP717 contains a boost regulator, that it meant to provide
> > the 5V USB VBUS voltage when the devices operates on battery.
> > 
> > Add the name "boost" to the regexp describing the allowed node names,
> > to allow the regulator to be described in the devicetree.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > index b8e8db0d58e9c..14ab367fc8871 100644
> > --- a/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/x-powers,axp152.yaml
> > @@ -274,7 +274,7 @@ properties:
> >            Defines the work frequency of DC-DC in kHz.
> >  
> >      patternProperties:
> > -      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo)$":
> > +      "^(([a-f])?ldo[0-9]|dcdc[0-7a-e]|ldo(_|-)io(0|1)|(dc1)?sw|rtc(_|-)ldo|cpusldo|drivevbus|dc5ldo|boost)$":  
> 
> That's not an easy to read regex...

TBH regexps are the least of my problems when reading bindings ;-)

> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks!

> If driver does not depend on _, please consider dropping (_|-).

The drivers (checked both Linux and FreeBSD) do look for a specific
string, but it's the real old regulators that used ldo_io[01] and
rtc_ldo, all the "newer" ones use a dash. Since this binding covers all
of them, we can't drop this from this regexp, but rest assured we only
go with dashes for new and upcoming devices.

Thanks,
Andre

Re: [PATCH 0/4] regulator: Fix AXP717 PMIC support

[Lee Jones]

On Fri, 29 Mar 2024, Andre Przywara wrote:

> Here are some fixes to the AXP717 PMIC support series. Lee put that in
> an immutable branch already, so these here go on top.
> Patch 1 contains fixes to the regulator descriptions: the LDOs had the
> wrong supply source, and two numbers were wrong. The datasheet describes
> the voltage ranges and register values differently from what our macros
> expect, in a way that literally begs for off-by-ones, so here you go.
> I don't know if that's still feasible, but it would be a good candidate
> to squash into the patch that it fixes.
> 
> The other three patches add the "boost" regulator, which is meant to
> provide the 5V USB VBUS power when operating from the battery. It's the
> usual trinity of binding/mfd/regulator patches.
> Again this could be squashed into the respective patches from the
> original series, if people agree.
> 
> Please have a look and test on a device, since I could not do this.
> 
> Based on mfd/ib-mfd-regulator-6.10, as detailed below.
> 
> Cheers,
> Andre
> 
> Andre Przywara (4):
>   regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones
>   dt-bindings: mfd: x-powers,axp152: add boost regulator
>   mfd: axp20x: AXP717: Add support for boost regulator
>   regulator: axp20x: AXP717: Add boost regulator
> 
>  .../bindings/mfd/x-powers,axp152.yaml         |  2 +-
>  drivers/mfd/axp20x.c                          |  2 ++
>  drivers/regulator/axp20x-regulator.c          | 32 +++++++++++--------
>  include/linux/mfd/axp20x.h                    |  3 ++
>  4 files changed, 24 insertions(+), 15 deletions(-)

I need an Ack from Mark before I can process these.

-- 
Lee Jones [李琼斯]

Re: [PATCH 3/4] mfd: axp20x: AXP717: Add support for boost regulator

[John Watts]

On Fri, Mar 29, 2024 at 11:50:32PM +0000, Andre Przywara wrote:
> The AXP717 also contains a boost regulator, to provide the 5V USB VBUS
> rail when running on battery.
> 
> Add the registers to the MFD description to be able to use them from the
> regulator driver.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Hi,

This patch looks correct to me- checked against the AXP717 datasheet.

John.

Reviewed-by: John Watts <contact@jookia.org>

> ---
>  drivers/mfd/axp20x.c       | 2 ++
>  include/linux/mfd/axp20x.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 48ce6ea693cea..62f3dd254d57b 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -209,6 +209,8 @@ static const struct regmap_access_table axp313a_volatile_table = {
>  };
>  
>  static const struct regmap_range axp717_writeable_ranges[] = {
> +	regmap_reg_range(AXP717_MODULE_EN_CONTROL, AXP717_MODULE_EN_CONTROL),
> +	regmap_reg_range(AXP717_BOOST_CONTROL, AXP717_BOOST_CONTROL),
>  	regmap_reg_range(AXP717_IRQ0_EN, AXP717_IRQ4_EN),
>  	regmap_reg_range(AXP717_DCDC_OUTPUT_CONTROL, AXP717_CPUSLDO_CONTROL),
>  };
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 8c0a33a2e9ce2..4dad54fdf67ee 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -115,6 +115,8 @@ enum axp20x_variants {
>  #define AXP313A_IRQ_STATE		0x21
>  
>  #define AXP717_ON_INDICATE		0x00
> +#define AXP717_MODULE_EN_CONTROL	0x19
> +#define AXP717_BOOST_CONTROL		0x1e
>  #define AXP717_IRQ0_EN			0x40
>  #define AXP717_IRQ1_EN			0x41
>  #define AXP717_IRQ2_EN			0x42
> -- 
> 2.35.8
> 

Re: [PATCH 4/4] regulator: axp20x: AXP717: Add boost regulator

[John Watts]

On Fri, Mar 29, 2024 at 11:50:33PM +0000, Andre Przywara wrote:
> The AXP717 also contains an adjustable boost regulator, to provide the
> 5V USB VBUS rail when running on battery.
> 
> Add the regulator description that states the voltage range this
> regulator can cover.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Hi,

I checked this against the AXP717 datasheet and it looks correct.

John.

Reviewed-by: John Watts <contact@jookia.org>

> ---
>  drivers/regulator/axp20x-regulator.c | 4 ++++
>  include/linux/mfd/axp20x.h           | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 3907606b091f6..71407c5810941 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -143,6 +143,7 @@
>  #define AXP717_DCDC3_NUM_VOLTAGES	103
>  #define AXP717_DCDC_V_OUT_MASK		GENMASK(6, 0)
>  #define AXP717_LDO_V_OUT_MASK		GENMASK(4, 0)
> +#define AXP717_BOOST_V_OUT_MASK		GENMASK(7, 4)
>  
>  #define AXP803_PWR_OUT_DCDC1_MASK	BIT_MASK(0)
>  #define AXP803_PWR_OUT_DCDC2_MASK	BIT_MASK(1)
> @@ -829,6 +830,9 @@ static const struct regulator_desc axp717_regulators[] = {
>  	AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
>  		 AXP717_CPUSLDO_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(4)),
> +	AXP_DESC(AXP717, BOOST, "boost", "vin1", 4550, 5510, 64,
> +		 AXP717_BOOST_CONTROL, AXP717_BOOST_V_OUT_MASK,
> +		 AXP717_MODULE_EN_CONTROL, BIT(4)),
>  };
>  
>  /* DCDC ranges shared with AXP813 */
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index 4dad54fdf67ee..5e86b976c4caa 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -486,6 +486,7 @@ enum {
>  	AXP717_CLDO3,
>  	AXP717_CLDO4,
>  	AXP717_CPUSLDO,
> +	AXP717_BOOST,
>  	AXP717_REG_ID_MAX,
>  };
>  
> -- 
> 2.35.8
> 

Re: [PATCH 1/4] regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones

[John Watts]

On Fri, Mar 29, 2024 at 11:50:30PM +0000, Andre Przywara wrote:
> The X-Powers AXP717 PMIC has separate input supply pins for each group
> of LDOs, so they are not all using the same DCDC1 input, as described
> currently.
> 
> Replace the "supply" member of each LDO description with the respective
> group supply name, so that the supply dependencies can be correctly
> described in the devicetree.
> Also fix two off-by-ones in the regulator macros, after some double
> checking the numbers against the datasheet.
> 
> Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Hi,

> ---
>  drivers/regulator/axp20x-regulator.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 34fcdd82b2eaa..3907606b091f6 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -140,7 +140,7 @@
>  
>  #define AXP717_DCDC1_NUM_VOLTAGES	88
>  #define AXP717_DCDC2_NUM_VOLTAGES	107
> -#define AXP717_DCDC3_NUM_VOLTAGES	104
> +#define AXP717_DCDC3_NUM_VOLTAGES	103
>  #define AXP717_DCDC_V_OUT_MASK		GENMASK(6, 0)
>  #define AXP717_LDO_V_OUT_MASK		GENMASK(4, 0)
>  
> @@ -766,7 +766,7 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
>  static const struct linear_range axp717_dcdc2_ranges[] = {
>  	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
>  	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> -	REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
> +	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
>  };

I'm not entirely sure these are correct after reading the datasheet.

From what I can tell REGULATOR_LINEAR_RANGE is inclusive, so for DCDC1
we have these ranges (we agree DCDC1 is correct, it is not affected by
this patch):

#define AXP717_DCDC1_NUM_VOLTAGES	88
static const struct linear_range axp717_dcdc1_ranges[] = {
	REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
	REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
};

The datasheet says this for the register:

0.5~1.2V,10mV/step,71steps
1.22~1.54V,20mV/step,17steps
0000000: 0.50V
0000001: 0.51V
...
1000110: 1.20V
1000111: 1.22V
1001000: 1.24V
...
1010111: 1.54V
1011000~1111111: Reserve

Converting to decimal:

0: 0.50V (range 1 start)
1: 0.51V
...
70: 1.20V (range 1 end)
71: 1.22V (range 2 start)
72: 1.24V
...
87: 1.54V (range 2 end)
88 onwards: reserved

The maximum voltages are the last value plus one (to include voltage zero).

For DCDC2 after applying this patch we get:

#define AXP717_DCDC2_NUM_VOLTAGES	107
static const struct linear_range axp717_dcdc2_ranges[] = {
	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
};

The datasheet marks the maximum value as 1101011: 3.40V, so 106 (the old
value for range) seems correct, but the NUM_VOLTAGES seems like it
should be bumped up to 108. I think you fixed the wrong thing here.

#define AXP717_DCDC3_NUM_VOLTAGES	103
static const struct linear_range axp717_dcdc2_ranges[] = {
	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
};

The datasheet marks the maximum value as 1101011: 3.40V, so 106 (the old
value for range) seems correct, but the NUM_VOLTAGES seems like it
should be bumped up to 108. I think you fixed the wrong thing here.
For DCDC3 after applying this patch we get:

#define AXP717_DCDC3_NUM_VOLTAGES	103
static const struct linear_range axp717_dcdc3_ranges[] = {
	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
};

The datasheet marks the maximum value as 1100110: 1.84V, which is 102.
So this patch to correct the AXP717_DCDC3_NUM_VOLTAGES is correct here.

John.

>  
>  static const struct linear_range axp717_dcdc3_ranges[] = {
> @@ -790,40 +790,40 @@ static const struct regulator_desc axp717_regulators[] = {
>  	AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
>  		 AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
>  		 AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
> -	AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
>  		 AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
> -	AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
>  		 AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
> -	AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
>  		 AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
> -	AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
>  		 AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
> -	AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
>  		 AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
> -	AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
>  		 AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
> -	AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
>  		 AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
> -	AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
>  		 AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
> -	AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
>  		 AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
> -	AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
>  		 AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
> -	AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
>  		 AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
> -	AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
> +	AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
>  		 AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
>  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
>  	AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
> -- 
> 2.35.8
> 

Re: [PATCH 1/4] regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones

[Andre Przywara]

On Sun, 14 Apr 2024 18:00:09 +1000
John Watts <contact@jookia.org> wrote:

Hi John,

many thanks for the detailed review (also on the other two patches), much
appreciated!

> On Fri, Mar 29, 2024 at 11:50:30PM +0000, Andre Przywara wrote:
> > The X-Powers AXP717 PMIC has separate input supply pins for each group
> > of LDOs, so they are not all using the same DCDC1 input, as described
> > currently.
> > 
> > Replace the "supply" member of each LDO description with the respective
> > group supply name, so that the supply dependencies can be correctly
> > described in the devicetree.
> > Also fix two off-by-ones in the regulator macros, after some double
> > checking the numbers against the datasheet.
> > 
> > Fixes: d2ac3df75c3a ("regulator: axp20x: add support for the AXP717")
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Hi,
> 
> > ---
> >  drivers/regulator/axp20x-regulator.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> > index 34fcdd82b2eaa..3907606b091f6 100644
> > --- a/drivers/regulator/axp20x-regulator.c
> > +++ b/drivers/regulator/axp20x-regulator.c
> > @@ -140,7 +140,7 @@
> >  
> >  #define AXP717_DCDC1_NUM_VOLTAGES	88
> >  #define AXP717_DCDC2_NUM_VOLTAGES	107
> > -#define AXP717_DCDC3_NUM_VOLTAGES	104
> > +#define AXP717_DCDC3_NUM_VOLTAGES	103
> >  #define AXP717_DCDC_V_OUT_MASK		GENMASK(6, 0)
> >  #define AXP717_LDO_V_OUT_MASK		GENMASK(4, 0)
> >  
> > @@ -766,7 +766,7 @@ static const struct linear_range axp717_dcdc1_ranges[] = {
> >  static const struct linear_range axp717_dcdc2_ranges[] = {
> >  	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> >  	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> > -	REGULATOR_LINEAR_RANGE(1600000, 88, 107, 100000),
> > +	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> >  };  
> 
> I'm not entirely sure these are correct after reading the datasheet.
> 
> From what I can tell REGULATOR_LINEAR_RANGE is inclusive, so for DCDC1
> we have these ranges (we agree DCDC1 is correct, it is not affected by
> this patch):
> 
> #define AXP717_DCDC1_NUM_VOLTAGES	88
> static const struct linear_range axp717_dcdc1_ranges[] = {
> 	REGULATOR_LINEAR_RANGE(500000,   0, 70, 10000),
> 	REGULATOR_LINEAR_RANGE(1220000, 71, 87, 20000),
> };
> 
> The datasheet says this for the register:
> 
> 0.5~1.2V,10mV/step,71steps
> 1.22~1.54V,20mV/step,17steps
> 0000000: 0.50V
> 0000001: 0.51V
> ...
> 1000110: 1.20V
> 1000111: 1.22V
> 1001000: 1.24V
> ...
> 1010111: 1.54V
> 1011000~1111111: Reserve
> 
> Converting to decimal:
> 
> 0: 0.50V (range 1 start)
> 1: 0.51V
> ...
> 70: 1.20V (range 1 end)
> 71: 1.22V (range 2 start)
> 72: 1.24V
> ...
> 87: 1.54V (range 2 end)
> 88 onwards: reserved
> 
> The maximum voltages are the last value plus one (to include voltage zero).
> 
> For DCDC2 after applying this patch we get:
> 
> #define AXP717_DCDC2_NUM_VOLTAGES	107
> static const struct linear_range axp717_dcdc2_ranges[] = {
> 	REGULATOR_LINEAR_RANGE(500000,   0,  70,  10000),
> 	REGULATOR_LINEAR_RANGE(1220000, 71,  87,  20000),
> 	REGULATOR_LINEAR_RANGE(1600000, 88, 106, 100000),
> };
> 
> The datasheet marks the maximum value as 1101011: 3.40V, so 106 (the old
> value for range) seems correct, but the NUM_VOLTAGES seems like it
> should be bumped up to 108. I think you fixed the wrong thing here.

I see what you mean, though I actually looked at the number of steps
mentioned in the first part of the register description. Now
triple-checking this I came up with this table (generated by a spreadsheet
to minimise human error):
voltage	decimal	binary	
1600	88	1011000
1700	89	1011001
1800	90	1011010
1900	91	1011011
2000	92	1011100
2100	93	1011101
2200	94	1011110
2300	95	1011111
2400	96	1100000
2500	97	1100001
2600	98	1100010
2700	99	1100011
2800	100	1100100
2900	101	1100101
3000	102	1100110
3100	103	1100111
3200	104	1101000
3300	105	1101001
3400	106	1101010

Which means the final binary value in the datasheet is wrong, as 1101011
would mean 3.5V.
Also  1101010 = 106
     -1011000 = 88
=============
      0010010 = 18
and 18 * 100 + 1600 = 3400, right?

This *is* admittedly quite bonkers, especially since the representations
between the manual and the code are so different, but can you check that
this makes sense?

I discovered some other issue in the original patch (missed declaring the
range of IRQ acknowledge registers in the MFD part), so I will send a v2 of
this series soonish.

> For DCDC3 after applying this patch we get:
> 
> #define AXP717_DCDC3_NUM_VOLTAGES	103
> static const struct linear_range axp717_dcdc3_ranges[] = {
> 	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> 	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> };
> 
> The datasheet marks the maximum value as 1100110: 1.84V, which is 102.
> So this patch to correct the AXP717_DCDC3_NUM_VOLTAGES is correct here.

I agree ;-) thanks for checking!

Cheers,
Andre

> >  static const struct linear_range axp717_dcdc3_ranges[] = {
> > @@ -790,40 +790,40 @@ static const struct regulator_desc axp717_regulators[] = {
> >  	AXP_DESC(AXP717, DCDC4, "dcdc4", "vin4", 1000, 3700, 100,
> >  		 AXP717_DCDC4_CONTROL, AXP717_DCDC_V_OUT_MASK,
> >  		 AXP717_DCDC_OUTPUT_CONTROL, BIT(3)),
> > -	AXP_DESC(AXP717, ALDO1, "aldo1", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, ALDO1, "aldo1", "aldoin", 500, 3500, 100,
> >  		 AXP717_ALDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(0)),
> > -	AXP_DESC(AXP717, ALDO2, "aldo2", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, ALDO2, "aldo2", "aldoin", 500, 3500, 100,
> >  		 AXP717_ALDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(1)),
> > -	AXP_DESC(AXP717, ALDO3, "aldo3", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, ALDO3, "aldo3", "aldoin", 500, 3500, 100,
> >  		 AXP717_ALDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(2)),
> > -	AXP_DESC(AXP717, ALDO4, "aldo4", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, ALDO4, "aldo4", "aldoin", 500, 3500, 100,
> >  		 AXP717_ALDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(3)),
> > -	AXP_DESC(AXP717, BLDO1, "bldo1", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, BLDO1, "bldo1", "bldoin", 500, 3500, 100,
> >  		 AXP717_BLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(4)),
> > -	AXP_DESC(AXP717, BLDO2, "bldo2", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, BLDO2, "bldo2", "bldoin", 500, 3500, 100,
> >  		 AXP717_BLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(5)),
> > -	AXP_DESC(AXP717, BLDO3, "bldo3", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, BLDO3, "bldo3", "bldoin", 500, 3500, 100,
> >  		 AXP717_BLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(6)),
> > -	AXP_DESC(AXP717, BLDO4, "bldo4", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, BLDO4, "bldo4", "bldoin", 500, 3500, 100,
> >  		 AXP717_BLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO0_OUTPUT_CONTROL, BIT(7)),
> > -	AXP_DESC(AXP717, CLDO1, "cldo1", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, CLDO1, "cldo1", "cldoin", 500, 3500, 100,
> >  		 AXP717_CLDO1_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(0)),
> > -	AXP_DESC(AXP717, CLDO2, "cldo2", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, CLDO2, "cldo2", "cldoin", 500, 3500, 100,
> >  		 AXP717_CLDO2_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(1)),
> > -	AXP_DESC(AXP717, CLDO3, "cldo3", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, CLDO3, "cldo3", "cldoin", 500, 3500, 100,
> >  		 AXP717_CLDO3_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(2)),
> > -	AXP_DESC(AXP717, CLDO4, "cldo4", "vin1", 500, 3500, 100,
> > +	AXP_DESC(AXP717, CLDO4, "cldo4", "cldoin", 500, 3500, 100,
> >  		 AXP717_CLDO4_CONTROL, AXP717_LDO_V_OUT_MASK,
> >  		 AXP717_LDO1_OUTPUT_CONTROL, BIT(3)),
> >  	AXP_DESC(AXP717, CPUSLDO, "cpusldo", "vin1", 500, 1400, 50,
> > -- 
> > 2.35.8
> >   

Re: [PATCH 1/4] regulator: axp20x: AXP717: fix LDO supply rails and off-by-ones

[John Watts]

On Tue, Apr 16, 2024 at 12:23:05PM +0100, Andre Przywara wrote:
> On Sun, 14 Apr 2024 18:00:09 +1000
> John Watts <contact@jookia.org> wrote:
> 
> Hi John,
> 
> many thanks for the detailed review (also on the other two patches), much
> appreciated!

No problem!

> I see what you mean, though I actually looked at the number of steps
> mentioned in the first part of the register description. Now
> triple-checking this I came up with this table (generated by a spreadsheet
> to minimise human error):
> voltage	decimal	binary	
> 1600	88	1011000
> 1700	89	1011001
> 1800	90	1011010
> 1900	91	1011011
> 2000	92	1011100
> 2100	93	1011101
> 2200	94	1011110
> 2300	95	1011111
> 2400	96	1100000
> 2500	97	1100001
> 2600	98	1100010
> 2700	99	1100011
> 2800	100	1100100
> 2900	101	1100101
> 3000	102	1100110
> 3100	103	1100111
> 3200	104	1101000
> 3300	105	1101001
> 3400	106	1101010
> 
> Which means the final binary value in the datasheet is wrong, as 1101011
> would mean 3.5V.
> Also  1101010 = 106
>      -1011000 = 88
> =============
>       0010010 = 18
> and 18 * 100 + 1600 = 3400, right?
> 
> This *is* admittedly quite bonkers, especially since the representations
> between the manual and the code are so different, but can you check that
> this makes sense?

I wrote a program in Python that steps through each range and prints its
value, and according to it value 106 is 3.4V. I dumped it at the end of
this email for anyone curious. Your math checks out too.

So the datasheet must be wrong. Maybe it originally supported up to 3.5V
and someone who doesn't know binary updated the sheet.

I think you should add a note saying that the datasheet is wrong, to
show people this isn't a bug and also save time of others trying to
write their own drivers and check their logic. Something like this:

Warning, the datasheet specifies that 3.40V is 107, which is incorrect:
- There are only 107 steps in total, making the highest step value 106
- 1.60V is listed as 1011000 (88 in decimal), with 18 steps after that 
- Adding 100mV for each of the 18 steps past 1.60V gives 3.4V

I think this logic convinces me at least. :)

John.

> I discovered some other issue in the original patch (missed declaring the
> range of IRQ acknowledge registers in the MFD part), so I will send a v2 of
> this series soonish.
> 
> > For DCDC3 after applying this patch we get:
> > 
> > #define AXP717_DCDC3_NUM_VOLTAGES	103
> > static const struct linear_range axp717_dcdc3_ranges[] = {
> > 	REGULATOR_LINEAR_RANGE(500000,   0,  70, 10000),
> > 	REGULATOR_LINEAR_RANGE(1220000, 71, 102, 20000),
> > };
> > 
> > The datasheet marks the maximum value as 1100110: 1.84V, which is 102.
> > So this patch to correct the AXP717_DCDC3_NUM_VOLTAGES is correct here.
> 
> I agree ;-) thanks for checking!
> 
> Cheers,
> Andre

---

Python program:

reg = 0
value = 500
for x in range(71):
	print("%i: %imV" % (reg, value))
	value += 10
	reg += 1
value = 1220
for x in range(17):
	print("%i: %imV" % (reg, value))
	value += 20
	reg += 1
value = 1600
for x in range(19):
	print("%i: %imV" % (reg, value))
	value += 100
	reg += 1