Hello everyone,
On Sat, Apr 22, 2023 at 06:46:53PM +0100, Jonathan Cameron wrote:
> 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
I will come up with a better solution in the v3
>
>
>
> > ---
> > 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.
I will make a separate patch.
>
> > }
> >
> > 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.
I will add this change to the 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.
I will
--
Kind Regards
Mehdi Djait
