On Wed, 19 Feb 2020 12:40:08 -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.
>
> Fixes: e01e7eaf37d8 ("iio: light: introduce si1133")
>
> Reported-by: Simon Goyette <simon.goyette@xxxxxxxxx>
> Co-authored-by: Guillaume Champagne <champagne.guillaume.c@xxxxxxxxx>
> Signed-off-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@xxxxxxxxx>
> Signed-off-by: Guillaume Champagne <champagne.guillaume.c@xxxxxxxxx>
Given this will have significant potential userspace impact, I'm not
going to queue it up to go in as a fix but instead give it some time
until the next merge window.
Thanks,
Jonathan
> ---
> Changes in v2:
> * Add missing `Fixes` tag
> * Add Reported-by
> * Add missing signed-off-by from co-author.
>
> 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,
