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@xxxxxxxxxx> 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