lkml.org 
[lkml]   [2023]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: EXT: Re: [PATCH] drivers: iio: adc: ltc2497: fix LSB shift
    On Sat, Jan 28, 2023 at 06:00:32PM +0000, Jonathan Cameron wrote:
    >
    > On Fri, 27 Jan 2023 17:47:56 +0000
    > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
    >
    > > On Fri, 27 Jan 2023 14:57:14 +0200
    > > Ian Ray <ian.ray@ge.com> wrote:
    > >
    > > > Correct the "sub_lsb" shift for both ltc2497 and ltc2499.
    >
    > I dropped the ltc2499 mention here as that one seems to be unaffected
    > by this patch (beyond as a simplification)

    Makes sense.


    >
    > >
    > > Hi Iain, Thanks for the patch.
    > >
    > > >
    > > > An earlier version of the code shifted by 14 but this was a consequence
    > > > of reading three bytes into a __be32 buffer and using be32_to_cpu(), so
    > > > eight extra bits needed to be skipped. Now we use get_unaligned_be24()
    > > > and thus the additional skip is wrong.
    > > >
    > > > Fixes 2187cfe ("drivers: iio: adc: ltc2497: LTC2499 support")
    > >
    > > This isn't in standard Fixes tag format. If nothing else comes
    > > up I'll fix up whilst applying.
    > >
    > > > Signed-off-by: Ian Ray <ian.ray@ge.com>
    >
    > Fix looks good to me. (I noted my reasoning below on basis someone might
    > find it useful)
    >
    > Applied to the fixes-togreg branch of iio.git with fixes tag fixed and
    > marked for stable.
    >
    > Please take a look at:
    > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=fixes-togreg&id=6327a930ab7bfa1ab33bcdffd5f5f4b1e7131504
    > to check I didn't mess anything up.

    lgtm

    Many thanks!


    >
    > Thanks,
    >
    > Jonathan
    >
    >
    > > > ---
    > > > drivers/iio/adc/ltc2497.c | 6 ++----
    > > > 1 file changed, 2 insertions(+), 4 deletions(-)
    > > >
    > > > diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
    > > > index 17370c5..ec198c6 100644
    > > > --- a/drivers/iio/adc/ltc2497.c
    > > > +++ b/drivers/iio/adc/ltc2497.c
    > > > @@ -28,7 +28,6 @@ struct ltc2497_driverdata {
    > > > struct ltc2497core_driverdata common_ddata;
    > > > struct i2c_client *client;
    > > > u32 recv_size;
    > > > - u32 sub_lsb;
    > > > /*
    > > > * DMA (thus cache coherency maintenance) may require the
    > > > * transfer buffers to live in their own cache lines.
    > > > @@ -65,10 +64,10 @@ static int ltc2497_result_and_measure(struct ltc2497core_driverdata *ddata,
    > > > * equivalent to a sign extension.
    > > > */
    > > > if (st->recv_size == 3) {
    > > > - *val = (get_unaligned_be24(st->data.d8) >> st->sub_lsb)
    > > > + *val = (get_unaligned_be24(st->data.d8) >> 6)
    > Prior to the buggy patch.
    >
    > 1) Read into top 3 bytes of BE32 which corresponds to 8 bits high.
    > 2) Shift by 14.
    >
    > Before this patch
    > 1) Read only 3 bytes
    > 2) shift by (31 - (resolution + 1) where resolution is 16 so shift by 14.
    > Indeed bug.
    >
    > After this patch.
    > 1) Read 3 bytes
    > 2) shift by 6
    >
    > Matches original behaviour - fix is correct.
    >
    >
    > > > - BIT(ddata->chip_info->resolution + 1);
    > > > } else {
    > > > - *val = (be32_to_cpu(st->data.d32) >> st->sub_lsb)
    > > > + *val = (be32_to_cpu(st->data.d32) >> 6)
    >
    > Prior to this patch
    > sub_lsb = 31 - (resolution + 1) where in this case resolution is 24
    > 31 - 25 = 6 so this change is a noop change, so this is good.
    >
    > > > - BIT(ddata->chip_info->resolution + 1);
    > > > }
    > > >
    > > > @@ -122,7 +121,6 @@ static int ltc2497_probe(struct i2c_client *client)
    > > > st->common_ddata.chip_info = chip_info;
    > > >
    > > > resolution = chip_info->resolution;
    > > > - st->sub_lsb = 31 - (resolution + 1);
    > > > st->recv_size = BITS_TO_BYTES(resolution) + 1;
    > > >
    > > > return ltc2497core_probe(dev, indio_dev);
    > >
    >
    >

    \
     
     \ /
      Last update: 2023-03-27 00:01    [W:2.551 / U:0.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site