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

Matti Vaittinen
2023-04-25T07:08:06Z

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! ~~