On Thu, 20 Apr 2023 22:22:04 +0200
Mehdi Djait <mehdi.djait.k@xxxxxxxxx> wrote:
> Since Kionix accelerometers use various numbers of bits to report data, a
> device-specific function is required.
> Move the driver's private data to the header file to support the new function.
> Make the allocation of the "buffer" array in the fifo_flush function dynamic
> and more generic.
>
> Signed-off-by: Mehdi Djait <mehdi.djait.k@xxxxxxxxx>
This results in some fifo_length changes in here and some in the previous patch.
Either keep it fixed for first patch, then make those changes and the ones you have
here in a single patch, or if that's hard maybe a single patch is cleaner.
I'd only expect the stuff about bytes in buffer to be in this patch.
Jonathan
> ---
> v2:
> - separated this change from the chip_info introduction and made it a patch in v2
> - changed the function from generic implementation for to device-specific one
> - removed blank lines pointed out by checkpatch
> - changed the allocation of the "buffer" array in __kx022a_fifo_flush
>
> drivers/iio/accel/kionix-kx022a.c | 72 +++++++++++++------------------
> drivers/iio/accel/kionix-kx022a.h | 37 ++++++++++++++++
> 2 files changed, 66 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
> index 7f9a2c29790b..1c81ea1657b9 100644
> --- a/drivers/iio/accel/kionix-kx022a.c
> +++ b/drivers/iio/accel/kionix-kx022a.c
> @@ -150,36 +150,6 @@ static const struct regmap_config kx022a_regmap_config = {
> .cache_type = REGCACHE_RBTREE,
> };
>
> -struct kx022a_data {
> - struct regmap *regmap;
> - struct iio_trigger *trig;
> - struct device *dev;
> - struct iio_mount_matrix orientation;
> - int64_t timestamp, old_timestamp;
> -
> - int irq;
> - int inc_reg;
> - int ien_reg;
> -
> - unsigned int state;
> - unsigned int odr_ns;
> -
> - bool trigger_enabled;
> - /*
> - * Prevent toggling the sensor stby/active state (PC1 bit) in the
> - * middle of a configuration, or when the fifo is enabled. Also,
> - * protect the data stored/retrieved from this structure from
> - * concurrent accesses.
> - */
> - struct mutex mutex;
> - u8 watermark;
> -
> - /* 3 x 16bit accel data + timestamp */
> - __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
> - struct {
> - __le16 channels[3];
> - s64 ts __aligned(8);
> - } scan;
> };
>
> static const struct iio_mount_matrix *
> @@ -340,7 +310,6 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
>
> return ret;
> -
Grumpy maintainer hat on: I don't want to see white space changes in unrelated code
in a patch doing anything other than white space cleanup.
> }
>
> static int kx022a_turn_off_lock(struct kx022a_data *data)
> @@ -595,34 +564,50 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
> return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
> }
>
> 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[KX022A_FIFO_LENGTH * 3];
> + __le16 *buffer;
> uint64_t sample_period;
> int count, fifo_bytes;
> bool renable = false;
> int64_t tstamp;
> int ret, i;
>
> - ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
> - if (ret) {
> - dev_err(dev, "Error reading buffer status\n");
> - return ret;
> - }
> + /* 3 axis, 2 bytes of data for each of the axis */
> + buffer = kmalloc(data->chip_info->fifo_length * 6, GFP_KERNEL);
Split that 6 up into sizeof(*buffer) * 3
Then just comment on 3 axis. Or use a define for number of axis and drop the
comment entirely.
Also, this feels like it fitst better in previous patch.
> + if (!buffer)
> + return -ENOMEM;
>
> - /* 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);
Only this small part is what this patch claims to do.. (+ the callback of course).
>
> if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
> dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
>
> count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
> - if (!count)
> + if (!count) {
> + kfree(buffer);
> return 0;
> + }
>
> /*
> * If we are being called from IRQ handler we know the stored timestamp
> @@ -704,6 +689,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> if (renable)
> enable_irq(data->irq);
>
> + kfree(buffer);
> return ret;
> }
>
> @@ -1016,6 +1002,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);
>
> @@ -1143,7 +1130,6 @@ int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chi
> if (ret)
> return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>
> -
Another one. If those predate your series, feel free to clean them up either
at the start or end of this series.
> ret = devm_iio_trigger_register(dev, indio_trig);
> if (ret)
> return dev_err_probe(data->dev, ret,
> diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> index 3c31e7d88f78..43e32e688258 100644
> --- a/drivers/iio/accel/kionix-kx022a.h
> +++ b/drivers/iio/accel/kionix-kx022a.h
> @@ -11,6 +11,8 @@
> #include <linux/bits.h>
> #include <linux/regmap.h>
>
> +#include <linux/iio/iio.h>
> +
> #define KX022A_REG_WHO 0x0f
> #define KX022A_ID 0xc8
>
> @@ -76,6 +78,39 @@
>
> struct device;
>
> +struct kx022a_data {
> + const struct kx022a_chip_info *chip_info;
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + struct device *dev;
> + struct iio_mount_matrix orientation;
> + int64_t timestamp, old_timestamp;
> +
> + int irq;
> + int inc_reg;
> + int ien_reg;
> +
> + unsigned int state;
> + unsigned int odr_ns;
> +
> + bool trigger_enabled;
> + /*
> + * Prevent toggling the sensor stby/active state (PC1 bit) in the
> + * middle of a configuration, or when the fifo is enabled. Also,
> + * protect the data stored/retrieved from this structure from
> + * concurrent accesses.
> + */
> + struct mutex mutex;
> + u8 watermark;
> +
> + /* 3 x 16bit accel data + timestamp */
> + __le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
> + struct {
> + __le16 channels[3];
> + s64 ts __aligned(8);
> + } scan;
> +};
> +
> /**
> * struct kx022a_chip_info - Kionix accelerometer chip specific information
> *
> @@ -100,6 +135,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 +159,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);
