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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 19 Feb 2020 12:40:08 -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.
> 
> Fixes: e01e7eaf37d8 ("iio: light: introduce si1133")
> 
> Reported-by: Simon Goyette <simon.goyette@gmail.com>
> Co-authored-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>
> Signed-off-by: Maxime Roussin-Bélanger <maxime.roussinbelanger@gmail.com>
> Signed-off-by: Guillaume Champagne <champagne.guillaume.c@gmail.com>

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,