On Fri, 14 Feb 2020 10:27:45 -0500
Maxime Roussin-Bélanger <maxime.roussinbelanger@xxxxxxxxx> wrote:
> On Fri, Feb 14, 2020 at 9:22 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Fri, 7 Feb 2020 11:07:40 -0500
> > Maxime Roussin-Bélanger <maxime.roussinbelanger@xxxxxxxxx> 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@xxxxxxxxx>
> > > Signed-off-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@xxxxxxxxx
> >
> > 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@xxxxxxxxx>
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,
