| Message ID | 20211116213746.264378-1-boger@wirenboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: adc: axp20x_adc: fix charging current reporting on AXP22x | expand |
On Wed, 17 Nov 2021 00:37:46 +0300 Evgeny Boger <boger@wirenboard.com> wrote: > Both the charging and discharging currents on AXP22x are stored as > 12-bit integers, in accordance with the datasheet. > It's also confirmed by vendor BSP (axp20x_adc.c:axp22_icharge_to_mA). > > The scale factor of 0.5 is never mentioned in datasheet, nor in the > vendor source code. I think it was here to compensate for > erroneous addition bit in register width. > > Tested on custom A40i+AXP221s board with external ammeter as > a reference. > > Signed-off-by: Evgeny Boger <boger@wirenboard.com> I know Quentin has moved on from Bootlin, so looking for input from Chen-Yu Tsai for these as I have no idea :) I have pinged Quentin as well on off chance he still wants to take a look. Jonathan > --- > > Notes: > Changes from v1: > - return scale factor of 1 as IIO_VAL_INT, not IIO_VAL_INT_PLUS_MICRO > - get rid of unused variable > > drivers/iio/adc/axp20x_adc.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c > index 3e0c0233b431..df99f1365c39 100644 > --- a/drivers/iio/adc/axp20x_adc.c > +++ b/drivers/iio/adc/axp20x_adc.c > @@ -251,19 +251,8 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, int *val) > { > struct axp20x_adc_iio *info = iio_priv(indio_dev); > - int size; > > - /* > - * N.B.: Unlike the Chinese datasheets tell, the charging current is > - * stored on 12 bits, not 13 bits. Only discharging current is on 13 > - * bits. > - */ > - if (chan->type == IIO_CURRENT && chan->channel == AXP22X_BATT_DISCHRG_I) > - size = 13; > - else > - size = 12; > - > - *val = axp20x_read_variable_width(info->regmap, chan->address, size); > + *val = axp20x_read_variable_width(info->regmap, chan->address, 12); > if (*val < 0) > return *val; > > @@ -386,9 +375,8 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val, > return IIO_VAL_INT_PLUS_MICRO; > > case IIO_CURRENT: > - *val = 0; > - *val2 = 500000; > - return IIO_VAL_INT_PLUS_MICRO; > + *val = 1; > + return IIO_VAL_INT; > > case IIO_TEMP: > *val = 100;
Hi, On Sun, Nov 21, 2021 at 1:44 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 17 Nov 2021 00:37:46 +0300 > Evgeny Boger <boger@wirenboard.com> wrote: > > > Both the charging and discharging currents on AXP22x are stored as > > 12-bit integers, in accordance with the datasheet. > > It's also confirmed by vendor BSP (axp20x_adc.c:axp22_icharge_to_mA). > > > > The scale factor of 0.5 is never mentioned in datasheet, nor in the > > vendor source code. I think it was here to compensate for > > erroneous addition bit in register width. > > > > Tested on custom A40i+AXP221s board with external ammeter as > > a reference. > > > > Signed-off-by: Evgeny Boger <boger@wirenboard.com> > > I know Quentin has moved on from Bootlin, so looking for input from Chen-Yu Tsai > for these as I have no idea :) The datasheet only lists the registers for reading the value, but nothing is said about how to interpret the data read. And the datasheet lists 13 bits split between two registers. Evgeny mentioned that the original code is wrong, and the BSP code is likely right, and has test results that match. That's good enough for me. Unfortunately I don't have any way to double check it right now. So Acked-by: Chen-Yu Tsai <wens@csie.org>
On Sun, 21 Nov 2021 01:58:08 +0800 Chen-Yu Tsai <wens@csie.org> wrote: > Hi, > > On Sun, Nov 21, 2021 at 1:44 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 17 Nov 2021 00:37:46 +0300 > > Evgeny Boger <boger@wirenboard.com> wrote: > > > > > Both the charging and discharging currents on AXP22x are stored as > > > 12-bit integers, in accordance with the datasheet. > > > It's also confirmed by vendor BSP (axp20x_adc.c:axp22_icharge_to_mA). > > > > > > The scale factor of 0.5 is never mentioned in datasheet, nor in the > > > vendor source code. I think it was here to compensate for > > > erroneous addition bit in register width. > > > > > > Tested on custom A40i+AXP221s board with external ammeter as > > > a reference. > > > > > > Signed-off-by: Evgeny Boger <boger@wirenboard.com> > > > > I know Quentin has moved on from Bootlin, so looking for input from Chen-Yu Tsai > > for these as I have no idea :) > > The datasheet only lists the registers for reading the value, but nothing > is said about how to interpret the data read. And the datasheet lists 13 > bits split between two registers. > > Evgeny mentioned that the original code is wrong, and the BSP code is > likely right, and has test results that match. That's good enough for > me. Unfortunately I don't have any way to double check it right now. So > > Acked-by: Chen-Yu Tsai <wens@csie.org> Applied to the fixes-togreg branch of iio.git with a fixes tag for the original driver introduction (as it seems this dates back that far) and marked for stable. Thanks, Jonathan
diff --git a/drivers/iio/adc/axp20x_adc.c b/drivers/iio/adc/axp20x_adc.c index 3e0c0233b431..df99f1365c39 100644 --- a/drivers/iio/adc/axp20x_adc.c +++ b/drivers/iio/adc/axp20x_adc.c @@ -251,19 +251,8 @@ static int axp22x_adc_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val) { struct axp20x_adc_iio *info = iio_priv(indio_dev); - int size; - /* - * N.B.: Unlike the Chinese datasheets tell, the charging current is - * stored on 12 bits, not 13 bits. Only discharging current is on 13 - * bits. - */ - if (chan->type == IIO_CURRENT && chan->channel == AXP22X_BATT_DISCHRG_I) - size = 13; - else - size = 12; - - *val = axp20x_read_variable_width(info->regmap, chan->address, size); + *val = axp20x_read_variable_width(info->regmap, chan->address, 12); if (*val < 0) return *val; @@ -386,9 +375,8 @@ static int axp22x_adc_scale(struct iio_chan_spec const *chan, int *val, return IIO_VAL_INT_PLUS_MICRO; case IIO_CURRENT: - *val = 0; - *val2 = 500000; - return IIO_VAL_INT_PLUS_MICRO; + *val = 1; + return IIO_VAL_INT; case IIO_TEMP: *val = 100;
Both the charging and discharging currents on AXP22x are stored as 12-bit integers, in accordance with the datasheet. It's also confirmed by vendor BSP (axp20x_adc.c:axp22_icharge_to_mA). The scale factor of 0.5 is never mentioned in datasheet, nor in the vendor source code. I think it was here to compensate for erroneous addition bit in register width. Tested on custom A40i+AXP221s board with external ammeter as a reference. Signed-off-by: Evgeny Boger <boger@wirenboard.com> --- Notes: Changes from v1: - return scale factor of 1 as IIO_VAL_INT, not IIO_VAL_INT_PLUS_MICRO - get rid of unused variable drivers/iio/adc/axp20x_adc.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)