Re: [PATCH] iio: si1133: read 24 signed integer for measurement

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 14 Feb 2020 10:27:45 -0500
Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com> wrote:

> On Fri, Feb 14, 2020 at 9:22 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri,  7 Feb 2020 11:07:40 -0500
> > Maxime Roussin-Bélanger         <maxime.roussinbelanger@gmail.com> wrote:
> >  
> > > The chip is configured in 24 bit mode. The values read from it must
> > > always be treated as is. This fixes the issue by replacing the previous
> > > 16 bits value by a 24 bits buffer.
> > >
> > > This changes affects the value output by previous version of the driver,
> > > since the least significant byte was missing. The upper half of 16
> > > bit values previously output are now the upper half of a 24 bit value.
> > >
> > > Co-authored-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> > > Signed-off-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com  
> >
> > Patch looks fine, so question is whether we treat this as an enhancement,
> > or a fix? If it's a fix please provide a suitable fixes tag.  
> 
> I'm not 100% of what fixes tag mean, but I assume it's something like
> 
> Tested-By: SomeoneTestedIt <TheDudeEmail@gmail.com>
Nope it identifies the point at which the issue was originally introduced.

See  Documentation/process/submitting-patches.rst

Jonathan

> 
> Am I correct?
> 
> Thanks,
> Max.
> 
> >
> > Thanks,
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/light/si1133.c | 37 ++++++++++++++++++++++++-------------
> > >  1 file changed, 24 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
> > > index 777b1a0848c9..509af982e185 100644
> > > --- a/drivers/iio/light/si1133.c
> > > +++ b/drivers/iio/light/si1133.c
> > > @@ -102,6 +102,9 @@
> > >  #define SI1133_INPUT_FRACTION_LOW    15
> > >  #define SI1133_LUX_OUTPUT_FRACTION   12
> > >  #define SI1133_LUX_BUFFER_SIZE               9
> > > +#define SI1133_MEASURE_BUFFER_SIZE   3
> > > +
> > > +#define SI1133_SIGN_BIT_INDEX 23
> > >
> > >  static const int si1133_scale_available[] = {
> > >       1, 2, 4, 8, 16, 32, 64, 128};
> > > @@ -234,13 +237,13 @@ static const struct si1133_lux_coeff lux_coeff = {
> > >       }
> > >  };
> > >
> > > -static int si1133_calculate_polynomial_inner(u32 input, u8 fraction,  
> u16 mag,
> > > +static int si1133_calculate_polynomial_inner(s32 input, u8 fraction,  
> u16 mag,
> > >                                            s8 shift)
> > >  {
> > >       return ((input << fraction) / mag) << shift;
> > >  }
> > >
> > > -static int si1133_calculate_output(u32 x, u32 y, u8 x_order, u8  
> y_order,
> > > +static int si1133_calculate_output(s32 x, s32 y, u8 x_order, u8  
> y_order,
> > >                                  u8 input_fraction, s8 sign,
> > >                                  const struct si1133_coeff *coeffs)
> > >  {
> > > @@ -276,7 +279,7 @@ static int si1133_calculate_output(u32 x, u32 y, u8  
> x_order, u8 y_order,
> > >   * The algorithm is from:
> > >   *  
> https://siliconlabs.github.io/Gecko_SDK_Doc/efm32zg/html/si1133_8c_source.html#l00716
> > >   */
> > > -static int si1133_calc_polynomial(u32 x, u32 y, u8 input_fraction, u8  
> num_coeff,
> > > +static int si1133_calc_polynomial(s32 x, s32 y, u8 input_fraction, u8  
> num_coeff,
> > >                                 const struct si1133_coeff *coeffs)
> > >  {
> > >       u8 x_order, y_order;
> > > @@ -614,23 +617,24 @@ static int si1133_measure(struct si1133_data  
> *data,
> > >  {
> > >       int err;
> > >
> > > -     __be16 resp;
> > > +     u8 buffer[SI1133_MEASURE_BUFFER_SIZE];
> > >
> > >       err = si1133_set_adcmux(data, 0, chan->channel);
> > >       if (err)
> > >               return err;
> > >
> > >       /* Deactivate lux measurements if they were active */
> > >       err = si1133_set_chlist(data, BIT(0));
> > >       if (err)
> > >               return err;
> > >
> > > -     err = si1133_bulk_read(data, SI1133_REG_HOSTOUT(0), sizeof(resp),
> > > -                            (u8 *)&resp);
> > > +     err = si1133_bulk_read(data, SI1133_REG_HOSTOUT(0),  
> sizeof(buffer),
> > > +                            buffer);
> > >       if (err)
> > >               return err;
> > >
> > > -     *val = be16_to_cpu(resp);
> > > +     *val = sign_extend32((buffer[0] << 16) | (buffer[1] << 8) |  
> buffer[2],
> > > +                          SI1133_SIGN_BIT_INDEX);
> > >
> > >       return err;
> > >  }
> > > @@ -704,9 +708,9 @@ static int si1133_get_lux(struct si1133_data *data,  
> int *val)
> > >  {
> > >       int err;
> > >       int lux;
> > > -     u32 high_vis;
> > > -     u32 low_vis;
> > > -     u32 ir;
> > > +     s32 high_vis;
> > > +     s32 low_vis;
> > > +     s32 ir;
> > >       u8 buffer[SI1133_LUX_BUFFER_SIZE];
> > >
> > >       /* Activate lux channels */
> > > @@ -719,9 +723,16 @@ static int si1133_get_lux(struct si1133_data  
> *data, int *val)
> > >       if (err)
> > >               return err;
> > >
> > > -     high_vis = (buffer[0] << 16) | (buffer[1] << 8) | buffer[2];
> > > -     low_vis = (buffer[3] << 16) | (buffer[4] << 8) | buffer[5];
> > > -     ir = (buffer[6] << 16) | (buffer[7] << 8) | buffer[8];
> > > +     high_vis =
> > > +             sign_extend32((buffer[0] << 16) | (buffer[1] << 8) |  
> buffer[2],
> > > +                           SI1133_SIGN_BIT_INDEX);
> > > +
> > > +     low_vis =
> > > +             sign_extend32((buffer[3] << 16) | (buffer[4] << 8) |  
> buffer[5],
> > > +                           SI1133_SIGN_BIT_INDEX);
> > > +
> > > +     ir = sign_extend32((buffer[6] << 16) | (buffer[7] << 8) |  
> buffer[8],
> > > +                        SI1133_SIGN_BIT_INDEX);
> > >
> > >       if (high_vis > SI1133_ADC_THRESHOLD || ir > SI1133_ADC_THRESHOLD)
> > >               lux = si1133_calc_polynomial(high_vis, ir,