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.8On 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
>
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
> >
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
© 2016 - 2026 Red Hat, Inc.