| Message ID | fc12d40f7c2132c1a9a1b5f83b79231dc8ce539f.1682373451.git.mehdi.djait.k@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer | expand |
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>
Hi Matti, On Tue, Apr 25, 2023 at 10:07:54AM +0300, Matti Vaittinen wrote: > 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 ;) I will take care of the comment if there is a v4 -- Kind Regards Mehdi Djait
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 */ + if (fifo_bytes == KX022A_FIFO_FULL_VALUE) + fifo_bytes = KX022A_FIFO_MAX_BYTES; + + return fifo_bytes; +} + static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, bool irq) { struct kx022a_data *data = iio_priv(idev); - struct device *dev = regmap_get_device(data->regmap); __le16 *buffer; uint64_t sample_period; int count, fifo_bytes; @@ -611,15 +628,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, if (!buffer) return -ENOMEM; - 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 */ - if (fifo_bytes == KX022A_FIFO_FULL_VALUE) - fifo_bytes = KX022A_FIFO_MAX_BYTES; + fifo_bytes = data->chip_info->get_fifo_bytes(data); if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); @@ -1023,6 +1032,7 @@ const struct kx022a_chip_info kx022a_chip_info = { .inc5 = KX022A_REG_INC5, .inc6 = KX022A_REG_INC6, .xout_l = KX022A_REG_XOUT_L, + .get_fifo_bytes = kx022a_get_fifo_bytes, }; EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h index 3c31e7d88f78..f043767067b7 100644 --- a/drivers/iio/accel/kionix-kx022a.h +++ b/drivers/iio/accel/kionix-kx022a.h @@ -76,6 +76,8 @@ struct device; +struct kx022a_data; + /** * struct kx022a_chip_info - Kionix accelerometer chip specific information * @@ -100,6 +102,7 @@ struct device; * @inc5: interrupt control register 5 * @inc6: interrupt control register 6 * @xout_l: x-axis output least significant byte + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer */ struct kx022a_chip_info { const char *name; @@ -123,6 +126,7 @@ struct kx022a_chip_info { u8 inc5; u8 inc6; u8 xout_l; + int (*get_fifo_bytes)(struct kx022a_data *); }; int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
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 (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(-)