| Message ID | 2c5d71e37fc7e000091189b3c5d66ede1a6015d2.1682019544.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 |
Hi Mehdi, Thanks for the v2! On 4/20/23 23:22, Mehdi Djait wrote: > Since Kionix accelerometers use various numbers of bits to report data, a > device-specific function is required. I think this is the right approach. Thanks for adding this device-specific function. > Move the driver's private data to the header file to support the new function. Hmm. Why this move is necessary? I didn't immediately spot this struct being used outside this C-file. I'd rather saw the struct in C-file if possible. > Make the allocation of the "buffer" array in the fifo_flush function dynamic > and more generic. > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > --- > v2: > - separated this change from the chip_info introduction and made it a patch in v2 I am unsure if this separation was needed. I'd only separate the "naming changes" which bring no changes to code flow, and then patches which are fixes and need to be backported (to minimize backporting effort and impact to stable branches). Well, I am fine with this separation though, seems like I am just making a noise here :). > - 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, > }; Yours, -- Matti
On Fri, 21 Apr 2023 09:14:10 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hi Mehdi, > > Thanks for the v2! > > On 4/20/23 23:22, Mehdi Djait wrote: > > Since Kionix accelerometers use various numbers of bits to report data, a > > device-specific function is required. > > I think this is the right approach. Thanks for adding this > device-specific function. > > > Move the driver's private data to the header file to support the new function. > > Hmm. Why this move is necessary? I didn't immediately spot this struct > being used outside this C-file. I'd rather saw the struct in C-file if > possible. > > > Make the allocation of the "buffer" array in the fifo_flush function dynamic > > and more generic. > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > --- > > v2: > > - separated this change from the chip_info introduction and made it a patch in v2 > > I am unsure if this separation was needed. I'd only separate the "naming > changes" which bring no changes to code flow, and then patches which are > fixes and need to be backported (to minimize backporting effort and > impact to stable branches). Well, I am fine with this separation though, > seems like I am just making a noise here :). I prefer them split. I want to see a noop patch for refactors. It's easier to quickly check it really makes no functional change. Adding new structures for a new device just adds noise to checking that. > > > - 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, > > }; > > Yours, > -- Matti >
On Sat, 22 Apr 2023 18:36:26 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 21 Apr 2023 09:14:10 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > Hi Mehdi, > > > > Thanks for the v2! > > > > On 4/20/23 23:22, Mehdi Djait wrote: > > > Since Kionix accelerometers use various numbers of bits to report data, a > > > device-specific function is required. > > > > I think this is the right approach. Thanks for adding this > > device-specific function. > > > > > Move the driver's private data to the header file to support the new function. > > > > Hmm. Why this move is necessary? I didn't immediately spot this struct > > being used outside this C-file. I'd rather saw the struct in C-file if > > possible. > > > > > Make the allocation of the "buffer" array in the fifo_flush function dynamic > > > and more generic. > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > > --- > > > v2: > > > - separated this change from the chip_info introduction and made it a patch in v2 > > > > I am unsure if this separation was needed. I'd only separate the "naming > > changes" which bring no changes to code flow, and then patches which are > > fixes and need to be backported (to minimize backporting effort and > > impact to stable branches). Well, I am fine with this separation though, > > seems like I am just making a noise here :). > > I prefer them split. I want to see a noop patch for refactors. It's easier > to quickly check it really makes no functional change. Adding new > structures for a new device just adds noise to checking that. Oops. I got the patches confused. Ignore that comment. > > > > > > > - 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, > > > }; > > > > Yours, > > -- Matti > > >
On Thu, 20 Apr 2023 22:22:04 +0200 Mehdi Djait <mehdi.djait.k@gmail.com> 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@gmail.com> 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);
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@gmail.com> 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@gmail.com> > > 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
Hi Matti, On Fri, Apr 21, 2023 at 09:14:10AM +0300, Matti Vaittinen wrote: > Hi Mehdi, > > Thanks for the v2! > > On 4/20/23 23:22, Mehdi Djait wrote: > > Since Kionix accelerometers use various numbers of bits to report data, a > > device-specific function is required. > > I think this is the right approach. Thanks for adding this device-specific > function. > > > Move the driver's private data to the header file to support the new function. > > Hmm. Why this move is necessary? I didn't immediately spot this struct being > used outside this C-file. I'd rather saw the struct in C-file if possible. My bad, I will move it back -- Kind Regards Mehdi Djait
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; - } 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_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[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); + 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); 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"); - 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);
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@gmail.com> --- 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(-)