Re: [PATCH v3 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 4/25/23 01:22, Mehdi Djait wrote:
> Since Kionix accelerometers use various numbers of bits to report data, a
> device-specific function is required.
> Implement the function as a callback in the device-specific chip_info structure
> 
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> ---
> v3:
> - fixed the warning of the kernel test robot in kx132_get_fifo_bytes

Do you mean kx022a_get_fifo_bytes? (I guess this won't go to commit logs 
so no need to respin for this).

> 	(invalid assignment: &=, left side has type restricted __le16
> 	right side has type unsigned short)
> 
> v2:
> - mentioned the kx132-1211 in the Kconfig
> - added a kx132-specific get_fifo_bytes function
> - changed the device name from "kx132" to "kx132-1211
> 
>   drivers/iio/accel/kionix-kx022a.c | 30 ++++++++++++++++++++----------
>   drivers/iio/accel/kionix-kx022a.h |  4 ++++
>   2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 66da3c8c3fd0..4a31d17c1f22 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -595,11 +595,28 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
>   	return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
>   }
>   
> +static int kx022a_get_fifo_bytes(struct kx022a_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret, fifo_bytes;
> +
> +	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> +	if (ret) {
> +		dev_err(dev, "Error reading buffer status\n");
> +		return ret;
> +	}
> +
> +	/* Let's not overflow if we for some reason get bogus value from i2c */

This comment would need fixing. I guess this is my bad - this comment 
goes back to first internal draft datasheets where the fifo lenght was 
said to be shorter than 255 bytes...

> +	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
> +		fifo_bytes = KX022A_FIFO_MAX_BYTES;

...Currently the fifo size (KX022A_FIFO_MAX_BYTES) is 258 bytes and 
KX022A_FIFO_FULL_VALUE is 255 (8-bit register) - meaning that no matter 
what noise there is in the I2C line, we won't overflow the buffer. This 
check is here to handle the 'quirk' where full 258 bytes of FIFO data is 
advertised by register max value (255).

It wouldn't be fair to require fixing this in context of this change - 
but I guess I can ask you to please update the comment if you happen to 
re-work this file ;)

> +
> +	return fifo_bytes;
> +}
> +

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~