| Message ID | 20220420211105.14654-5-jagathjog1996@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: bma400: Add buffer, step and activity/inactivity | expand |
On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > Added trigger buffer support to read continuous acceleration > data from device with data ready interrupt which is mapped > to INT1 pin. LGTM, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > --- > drivers/iio/accel/Kconfig | 2 + > drivers/iio/accel/bma400.h | 10 +- > drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++- > drivers/iio/accel/bma400_i2c.c | 2 +- > drivers/iio/accel/bma400_spi.c | 2 +- > 5 files changed, 170 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index eac3f02662ae..958097814232 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -204,6 +204,8 @@ config BMA220 > config BMA400 > tristate "Bosch BMA400 3-Axis Accelerometer Driver" > select REGMAP > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > select BMA400_I2C if I2C > select BMA400_SPI if SPI > help > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h > index 1c8c47a9a317..907e1a6c0a38 100644 > --- a/drivers/iio/accel/bma400.h > +++ b/drivers/iio/accel/bma400.h > @@ -62,6 +62,13 @@ > #define BMA400_ACC_CONFIG2_REG 0x1b > #define BMA400_CMD_REG 0x7e > > +/* Interrupt registers */ > +#define BMA400_INT_CONFIG0_REG 0x1f > +#define BMA400_INT_CONFIG1_REG 0x20 > +#define BMA400_INT1_MAP_REG 0x21 > +#define BMA400_INT_IO_CTRL_REG 0x24 > +#define BMA400_INT_DRDY_MSK BIT(7) > + > /* Chip ID of BMA 400 devices found in the chip ID register. */ > #define BMA400_ID_REG_VAL 0x90 > > @@ -111,6 +118,7 @@ > > extern const struct regmap_config bma400_regmap_config; > > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name); > +int bma400_probe(struct device *dev, struct regmap *regmap, int irq, > + const char *name); > > #endif > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c > index 07674d89d978..57910ccf9180 100644 > --- a/drivers/iio/accel/bma400_core.c > +++ b/drivers/iio/accel/bma400_core.c > @@ -11,6 +11,7 @@ > * - Create channel for sensor time > */ > > +#include <linux/bitfield.h> > #include <linux/bitops.h> > #include <linux/device.h> > #include <linux/kernel.h> > @@ -20,6 +21,10 @@ > #include <linux/regulator/consumer.h> > > #include <linux/iio/iio.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > > #include "bma400.h" > > @@ -61,6 +66,14 @@ struct bma400_data { > struct bma400_sample_freq sample_freq; > int oversampling_ratio; > int scale; > + struct iio_trigger *trig; > + /* Correct time stamp alignment */ > + struct { > + __le16 buff[3]; > + u8 temperature; > + s64 ts __aligned(8); > + } buffer ____cacheline_aligned; > + __le16 status; > }; > > static bool bma400_is_writable_reg(struct device *dev, unsigned int reg) > @@ -152,7 +165,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = { > { } > }; > > -#define BMA400_ACC_CHANNEL(_axis) { \ > +#define BMA400_ACC_CHANNEL(_index, _axis) { \ > .type = IIO_ACCEL, \ > .modified = 1, \ > .channel2 = IIO_MOD_##_axis, \ > @@ -164,17 +177,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = { > BIT(IIO_CHAN_INFO_SCALE) | \ > BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > .ext_info = bma400_ext_info, \ > + .scan_index = _index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 12, \ > + .storagebits = 16, \ > + .endianness = IIO_LE, \ > + }, \ > } > > static const struct iio_chan_spec bma400_channels[] = { > - BMA400_ACC_CHANNEL(X), > - BMA400_ACC_CHANNEL(Y), > - BMA400_ACC_CHANNEL(Z), > + BMA400_ACC_CHANNEL(0, X), > + BMA400_ACC_CHANNEL(1, Y), > + BMA400_ACC_CHANNEL(2, Z), > { > .type = IIO_TEMP, > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), > + .scan_index = 3, > + .scan_type = { > + .sign = 's', > + .realbits = 8, > + .storagebits = 8, > + .endianness = IIO_LE, > + }, > }, > + IIO_CHAN_SOFT_TIMESTAMP(4), > }; > > static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2) > @@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data) > if (ret) > return ret; > > + /* Configure INT1 pin to open drain */ > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06); > + if (ret) > + return ret; > /* > * Once the interrupt engine is supported we might use the > * data_src_reg, but for now ensure this is set to the > @@ -807,6 +839,29 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev, > } > } > > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig, > + bool state) > +{ > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct bma400_data *data = iio_priv(indio_dev); > + int ret; > + > + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG, > + BMA400_INT_DRDY_MSK, > + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); > + if (ret) > + return ret; > + > + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, > + BMA400_INT_DRDY_MSK, > + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); > +} > + > +static const unsigned long bma400_avail_scan_masks[] = { > + GENMASK(3, 0), > + 0 > +}; > + > static const struct iio_info bma400_info = { > .read_raw = bma400_read_raw, > .read_avail = bma400_read_avail, > @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = { > .write_raw_get_fmt = bma400_write_raw_get_fmt, > }; > > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) > +static const struct iio_trigger_ops bma400_trigger_ops = { > + .set_trigger_state = &bma400_data_rdy_trigger_set_state, > + .validate_device = &iio_trigger_validate_own_device, > +}; > + > +static irqreturn_t bma400_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct bma400_data *data = iio_priv(indio_dev); > + int ret, temp; > + > + /* Lock to protect the data->buffer */ > + mutex_lock(&data->mutex); > + > + /* bulk read six registers, with the base being the LSB register */ > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > + &data->buffer.buff, sizeof(data->buffer.buff)); > + if (ret) > + goto unlock_err; > + > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > + if (ret) > + goto unlock_err; > + > + data->buffer.temperature = temp; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > + iio_get_time_ns(indio_dev)); > + > + mutex_unlock(&data->mutex); > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > + > +unlock_err: > + mutex_unlock(&data->mutex); > + return IRQ_NONE; > +} > + > +static irqreturn_t bma400_interrupt(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct bma400_data *data = iio_priv(indio_dev); > + int ret; > + > + /* Lock to protect the data->status */ > + mutex_lock(&data->mutex); > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, > + &data->status, > + sizeof(data->status)); > + if (ret) > + goto unlock_err; > + > + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(data->status))) { > + mutex_unlock(&data->mutex); > + iio_trigger_poll_chained(data->trig); > + return IRQ_HANDLED; > + } > + > +unlock_err: > + mutex_unlock(&data->mutex); > + return IRQ_NONE; > +} > + > +int bma400_probe(struct device *dev, struct regmap *regmap, int irq, > + const char *name) > { > struct iio_dev *indio_dev; > struct bma400_data *data; > @@ -841,8 +961,40 @@ int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) > indio_dev->info = &bma400_info; > indio_dev->channels = bma400_channels; > indio_dev->num_channels = ARRAY_SIZE(bma400_channels); > + indio_dev->available_scan_masks = bma400_avail_scan_masks; > indio_dev->modes = INDIO_DIRECT_MODE; > > + if (irq > 0) { > + data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > + indio_dev->name, > + iio_device_id(indio_dev)); > + if (!data->trig) > + return -ENOMEM; > + > + data->trig->ops = &bma400_trigger_ops; > + iio_trigger_set_drvdata(data->trig, indio_dev); > + > + ret = devm_iio_trigger_register(data->dev, data->trig); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "iio trigger register fail\n"); > + > + indio_dev->trig = iio_trigger_get(data->trig); > + ret = devm_request_threaded_irq(dev, irq, NULL, > + &bma400_interrupt, > + IRQF_TRIGGER_RISING | IRQF_ONESHOT, > + indio_dev->name, indio_dev); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "request irq %d failed\n", irq); > + } > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > + &bma400_trigger_handler, NULL); > + if (ret) > + return dev_err_probe(data->dev, ret, > + "iio triggered buffer setup failed\n"); > + > return devm_iio_device_register(dev, indio_dev); > } > EXPORT_SYMBOL_NS(bma400_probe, IIO_BMA400); > diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c > index 4f6e01a3b3a1..1ba2a982ea73 100644 > --- a/drivers/iio/accel/bma400_i2c.c > +++ b/drivers/iio/accel/bma400_i2c.c > @@ -24,7 +24,7 @@ static int bma400_i2c_probe(struct i2c_client *client, > return PTR_ERR(regmap); > } > > - return bma400_probe(&client->dev, regmap, id->name); > + return bma400_probe(&client->dev, regmap, client->irq, id->name); > } > > static const struct i2c_device_id bma400_i2c_ids[] = { > diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c > index 28e240400a3f..ec13c044b304 100644 > --- a/drivers/iio/accel/bma400_spi.c > +++ b/drivers/iio/accel/bma400_spi.c > @@ -84,7 +84,7 @@ static int bma400_spi_probe(struct spi_device *spi) > if (ret) > dev_err(&spi->dev, "Failed to read chip id register\n"); > > - return bma400_probe(&spi->dev, regmap, id->name); > + return bma400_probe(&spi->dev, regmap, spi->irq, id->name); > } > > static const struct spi_device_id bma400_spi_ids[] = { > -- > 2.17.1 >
On Wed, 27 Apr 2022 14:34:57 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > > Added trigger buffer support to read continuous acceleration > > data from device with data ready interrupt which is mapped > > to INT1 pin. > > LGTM, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Agreed. A couple of 'comments' inline but no actual need to change anything. One is contingent on a fix I've not sent out yet for the rest of IIO. The other is potentially a minor improvement for the future. Thanks, Jonathan > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > > --- > > drivers/iio/accel/Kconfig | 2 + > > drivers/iio/accel/bma400.h | 10 +- > > drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++- > > drivers/iio/accel/bma400_i2c.c | 2 +- > > drivers/iio/accel/bma400_spi.c | 2 +- > > 5 files changed, 170 insertions(+), 8 deletions(-) > > > > #include "bma400.h" > > > > @@ -61,6 +66,14 @@ struct bma400_data { > > struct bma400_sample_freq sample_freq; > > int oversampling_ratio; > > int scale; > > + struct iio_trigger *trig; > > + /* Correct time stamp alignment */ > > + struct { > > + __le16 buff[3]; > > + u8 temperature; > > + s64 ts __aligned(8); > > + } buffer ____cacheline_aligned; If you are rolling again, could you change this to __aligned(IIO_ALIGN); See https://lore.kernel.org/linux-iio/20220419121241.00002e42@Huawei.com/ for why. Note that I'll be sending a fix patch out for IIO_ALIGN to define it as ARCH_KMALLOC_ALIGN in next few days. If you'd pref not to get caught up in that, send it as it stands and I'll fix up once that fix is in place. What's one more driver on top of the 80+ I have to do anyway :) > > + __le16 status; > > }; > > > > + > > +static const unsigned long bma400_avail_scan_masks[] = { > > + GENMASK(3, 0), > > + 0 > > +}; > > + > > static const struct iio_info bma400_info = { > > .read_raw = bma400_read_raw, > > .read_avail = bma400_read_avail, > > @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = { > > .write_raw_get_fmt = bma400_write_raw_get_fmt, > > }; > > > > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) > > +static const struct iio_trigger_ops bma400_trigger_ops = { > > + .set_trigger_state = &bma400_data_rdy_trigger_set_state, > > + .validate_device = &iio_trigger_validate_own_device, > > +}; > > + > > +static irqreturn_t bma400_trigger_handler(int irq, void *p) > > +{ > > + struct iio_poll_func *pf = p; > > + struct iio_dev *indio_dev = pf->indio_dev; > > + struct bma400_data *data = iio_priv(indio_dev); > > + int ret, temp; > > + > > + /* Lock to protect the data->buffer */ > > + mutex_lock(&data->mutex); > > + > > + /* bulk read six registers, with the base being the LSB register */ > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > > + &data->buffer.buff, sizeof(data->buffer.buff)); > > + if (ret) > > + goto unlock_err; > > + > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); Given the temperature read is a separate action, it seems like you could sensible add another entry to bma400_avail_scan_masks() for just the accelerometer axis and then only perform this read if the temperature is requested. It would be a feature though, so no need to have it in this patch if you prefer not to. > > + if (ret) > > + goto unlock_err; > > + > > + data->buffer.temperature = temp; > > + > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > + iio_get_time_ns(indio_dev)); > > + > > + mutex_unlock(&data->mutex); > > + iio_trigger_notify_done(indio_dev->trig); > > + return IRQ_HANDLED; > > + > > +unlock_err: > > + mutex_unlock(&data->mutex); > > + return IRQ_NONE; > > +}
Hi Jonathan, On Sun, May 1, 2022 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 27 Apr 2022 14:34:57 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > > > > Added trigger buffer support to read continuous acceleration > > > data from device with data ready interrupt which is mapped > > > to INT1 pin. > > > > LGTM, > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Agreed. A couple of 'comments' inline but no actual need to change anything. > One is contingent on a fix I've not sent out yet for the rest of IIO. > The other is potentially a minor improvement for the future. > > Thanks, > > Jonathan > > > > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > > > --- > > > drivers/iio/accel/Kconfig | 2 + > > > drivers/iio/accel/bma400.h | 10 +- > > > drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++- > > > drivers/iio/accel/bma400_i2c.c | 2 +- > > > drivers/iio/accel/bma400_spi.c | 2 +- > > > 5 files changed, 170 insertions(+), 8 deletions(-) > > > > > > > #include "bma400.h" > > > > > > @@ -61,6 +66,14 @@ struct bma400_data { > > > struct bma400_sample_freq sample_freq; > > > int oversampling_ratio; > > > int scale; > > > + struct iio_trigger *trig; > > > + /* Correct time stamp alignment */ > > > + struct { > > > + __le16 buff[3]; > > > + u8 temperature; > > > + s64 ts __aligned(8); > > > + } buffer ____cacheline_aligned; > > If you are rolling again, could you change this to > __aligned(IIO_ALIGN); See > https://lore.kernel.org/linux-iio/20220419121241.00002e42@Huawei.com/ > for why. > Note that I'll be sending a fix patch out for IIO_ALIGN to define > it as ARCH_KMALLOC_ALIGN in next few days. > > If you'd pref not to get caught up in that, send it as it stands > and I'll fix up once that fix is in place. What's one more driver > on top of the 80+ I have to do anyway :) > > Sure, I will change that to __aligned(IIO_ALIGN); in the next series. > > > > + __le16 status; > > > }; > > > > > > > + > > > +static const unsigned long bma400_avail_scan_masks[] = { > > > + GENMASK(3, 0), > > > + 0 > > > +}; > > > + > > > static const struct iio_info bma400_info = { > > > .read_raw = bma400_read_raw, > > > .read_avail = bma400_read_avail, > > > @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = { > > > .write_raw_get_fmt = bma400_write_raw_get_fmt, > > > }; > > > > > > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) > > > +static const struct iio_trigger_ops bma400_trigger_ops = { > > > + .set_trigger_state = &bma400_data_rdy_trigger_set_state, > > > + .validate_device = &iio_trigger_validate_own_device, > > > +}; > > > + > > > +static irqreturn_t bma400_trigger_handler(int irq, void *p) > > > +{ > > > + struct iio_poll_func *pf = p; > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > + struct bma400_data *data = iio_priv(indio_dev); > > > + int ret, temp; > > > + > > > + /* Lock to protect the data->buffer */ > > > + mutex_lock(&data->mutex); > > > + > > > + /* bulk read six registers, with the base being the LSB register */ > > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > > > + &data->buffer.buff, sizeof(data->buffer.buff)); > > > + if (ret) > > > + goto unlock_err; > > > + > > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > > Given the temperature read is a separate action, it seems like you could sensible > add another entry to bma400_avail_scan_masks() for just the accelerometer axis > and then only perform this read if the temperature is requested. > > It would be a feature though, so no need to have it in this patch if you > prefer not to. Sure I will add another entry only for the accelerometer axis and I will make changes accordingly in the next series. Do I need to add 'Reviewed-by' tag if the patch gets modified again after getting the tag? > > > > + if (ret) > > > + goto unlock_err; > > > + > > > + data->buffer.temperature = temp; > > > + > > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > > + iio_get_time_ns(indio_dev)); > > > + > > > + mutex_unlock(&data->mutex); > > > + iio_trigger_notify_done(indio_dev->trig); > > > + return IRQ_HANDLED; > > > + > > > +unlock_err: > > > + mutex_unlock(&data->mutex); > > > + return IRQ_NONE; > > > +} >
On Mon, 2 May 2022 01:55:49 +0530 Jagath Jog J <jagathjog1996@gmail.com> wrote: > Hi Jonathan, > > On Sun, May 1, 2022 at 9:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 27 Apr 2022 14:34:57 +0200 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Wed, Apr 20, 2022 at 11:11 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > > > > > > Added trigger buffer support to read continuous acceleration > > > > data from device with data ready interrupt which is mapped > > > > to INT1 pin. > > > > > > LGTM, > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Agreed. A couple of 'comments' inline but no actual need to change anything. > > One is contingent on a fix I've not sent out yet for the rest of IIO. > > The other is potentially a minor improvement for the future. > > > > Thanks, > > > > Jonathan > > > > > > > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> > > > > --- > > > > drivers/iio/accel/Kconfig | 2 + > > > > drivers/iio/accel/bma400.h | 10 +- > > > > drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++- > > > > drivers/iio/accel/bma400_i2c.c | 2 +- > > > > drivers/iio/accel/bma400_spi.c | 2 +- > > > > 5 files changed, 170 insertions(+), 8 deletions(-) > > > > > > > > > > #include "bma400.h" > > > > > > > > @@ -61,6 +66,14 @@ struct bma400_data { > > > > struct bma400_sample_freq sample_freq; > > > > int oversampling_ratio; > > > > int scale; > > > > + struct iio_trigger *trig; > > > > + /* Correct time stamp alignment */ > > > > + struct { > > > > + __le16 buff[3]; > > > > + u8 temperature; > > > > + s64 ts __aligned(8); > > > > + } buffer ____cacheline_aligned; > > > > If you are rolling again, could you change this to > > __aligned(IIO_ALIGN); See > > https://lore.kernel.org/linux-iio/20220419121241.00002e42@Huawei.com/ > > for why. > > Note that I'll be sending a fix patch out for IIO_ALIGN to define > > it as ARCH_KMALLOC_ALIGN in next few days. > > > > If you'd pref not to get caught up in that, send it as it stands > > and I'll fix up once that fix is in place. What's one more driver > > on top of the 80+ I have to do anyway :) > > > > > > Sure, I will change that to __aligned(IIO_ALIGN); in the next series. > > > > > > > + __le16 status; > > > > }; > > > > > > > > > > + > > > > +static const unsigned long bma400_avail_scan_masks[] = { > > > > + GENMASK(3, 0), > > > > + 0 > > > > +}; > > > > + > > > > static const struct iio_info bma400_info = { > > > > .read_raw = bma400_read_raw, > > > > .read_avail = bma400_read_avail, > > > > @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = { > > > > .write_raw_get_fmt = bma400_write_raw_get_fmt, > > > > }; > > > > > > > > -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) > > > > +static const struct iio_trigger_ops bma400_trigger_ops = { > > > > + .set_trigger_state = &bma400_data_rdy_trigger_set_state, > > > > + .validate_device = &iio_trigger_validate_own_device, > > > > +}; > > > > + > > > > +static irqreturn_t bma400_trigger_handler(int irq, void *p) > > > > +{ > > > > + struct iio_poll_func *pf = p; > > > > + struct iio_dev *indio_dev = pf->indio_dev; > > > > + struct bma400_data *data = iio_priv(indio_dev); > > > > + int ret, temp; > > > > + > > > > + /* Lock to protect the data->buffer */ > > > > + mutex_lock(&data->mutex); > > > > + > > > > + /* bulk read six registers, with the base being the LSB register */ > > > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > > > > + &data->buffer.buff, sizeof(data->buffer.buff)); > > > > + if (ret) > > > > + goto unlock_err; > > > > + > > > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > > > > Given the temperature read is a separate action, it seems like you could sensible > > add another entry to bma400_avail_scan_masks() for just the accelerometer axis > > and then only perform this read if the temperature is requested. > > > > It would be a feature though, so no need to have it in this patch if you > > prefer not to. > > Sure I will add another entry only for the accelerometer axis and I > will make changes > accordingly in the next series. > > Do I need to add 'Reviewed-by' tag if the patch gets modified again > after getting the > tag? > It's something you have judge based on whether you think a change is significant enough to warrant dropping tags. If you do drop them you should always state why in the cover letter or change log. Thanks, Jonathan > > > > > > > + if (ret) > > > > + goto unlock_err; > > > > + > > > > + data->buffer.temperature = temp; > > > > + > > > > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > > > > + iio_get_time_ns(indio_dev)); > > > > + > > > > + mutex_unlock(&data->mutex); > > > > + iio_trigger_notify_done(indio_dev->trig); > > > > + return IRQ_HANDLED; > > > > + > > > > +unlock_err: > > > > + mutex_unlock(&data->mutex); > > > > + return IRQ_NONE; > > > > +} > >
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index eac3f02662ae..958097814232 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -204,6 +204,8 @@ config BMA220 config BMA400 tristate "Bosch BMA400 3-Axis Accelerometer Driver" select REGMAP + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER select BMA400_I2C if I2C select BMA400_SPI if SPI help diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h index 1c8c47a9a317..907e1a6c0a38 100644 --- a/drivers/iio/accel/bma400.h +++ b/drivers/iio/accel/bma400.h @@ -62,6 +62,13 @@ #define BMA400_ACC_CONFIG2_REG 0x1b #define BMA400_CMD_REG 0x7e +/* Interrupt registers */ +#define BMA400_INT_CONFIG0_REG 0x1f +#define BMA400_INT_CONFIG1_REG 0x20 +#define BMA400_INT1_MAP_REG 0x21 +#define BMA400_INT_IO_CTRL_REG 0x24 +#define BMA400_INT_DRDY_MSK BIT(7) + /* Chip ID of BMA 400 devices found in the chip ID register. */ #define BMA400_ID_REG_VAL 0x90 @@ -111,6 +118,7 @@ extern const struct regmap_config bma400_regmap_config; -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name); +int bma400_probe(struct device *dev, struct regmap *regmap, int irq, + const char *name); #endif diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c index 07674d89d978..57910ccf9180 100644 --- a/drivers/iio/accel/bma400_core.c +++ b/drivers/iio/accel/bma400_core.c @@ -11,6 +11,7 @@ * - Create channel for sensor time */ +#include <linux/bitfield.h> #include <linux/bitops.h> #include <linux/device.h> #include <linux/kernel.h> @@ -20,6 +21,10 @@ #include <linux/regulator/consumer.h> #include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> #include "bma400.h" @@ -61,6 +66,14 @@ struct bma400_data { struct bma400_sample_freq sample_freq; int oversampling_ratio; int scale; + struct iio_trigger *trig; + /* Correct time stamp alignment */ + struct { + __le16 buff[3]; + u8 temperature; + s64 ts __aligned(8); + } buffer ____cacheline_aligned; + __le16 status; }; static bool bma400_is_writable_reg(struct device *dev, unsigned int reg) @@ -152,7 +165,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = { { } }; -#define BMA400_ACC_CHANNEL(_axis) { \ +#define BMA400_ACC_CHANNEL(_index, _axis) { \ .type = IIO_ACCEL, \ .modified = 1, \ .channel2 = IIO_MOD_##_axis, \ @@ -164,17 +177,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = { BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ .ext_info = bma400_ext_info, \ + .scan_index = _index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 12, \ + .storagebits = 16, \ + .endianness = IIO_LE, \ + }, \ } static const struct iio_chan_spec bma400_channels[] = { - BMA400_ACC_CHANNEL(X), - BMA400_ACC_CHANNEL(Y), - BMA400_ACC_CHANNEL(Z), + BMA400_ACC_CHANNEL(0, X), + BMA400_ACC_CHANNEL(1, Y), + BMA400_ACC_CHANNEL(2, Z), { .type = IIO_TEMP, .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ), + .scan_index = 3, + .scan_type = { + .sign = 's', + .realbits = 8, + .storagebits = 8, + .endianness = IIO_LE, + }, }, + IIO_CHAN_SOFT_TIMESTAMP(4), }; static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2) @@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data) if (ret) return ret; + /* Configure INT1 pin to open drain */ + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06); + if (ret) + return ret; /* * Once the interrupt engine is supported we might use the * data_src_reg, but for now ensure this is set to the @@ -807,6 +839,29 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev, } } +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig, + bool state) +{ + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); + struct bma400_data *data = iio_priv(indio_dev); + int ret; + + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG, + BMA400_INT_DRDY_MSK, + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); + if (ret) + return ret; + + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, + BMA400_INT_DRDY_MSK, + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); +} + +static const unsigned long bma400_avail_scan_masks[] = { + GENMASK(3, 0), + 0 +}; + static const struct iio_info bma400_info = { .read_raw = bma400_read_raw, .read_avail = bma400_read_avail, @@ -814,7 +869,72 @@ static const struct iio_info bma400_info = { .write_raw_get_fmt = bma400_write_raw_get_fmt, }; -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) +static const struct iio_trigger_ops bma400_trigger_ops = { + .set_trigger_state = &bma400_data_rdy_trigger_set_state, + .validate_device = &iio_trigger_validate_own_device, +}; + +static irqreturn_t bma400_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct bma400_data *data = iio_priv(indio_dev); + int ret, temp; + + /* Lock to protect the data->buffer */ + mutex_lock(&data->mutex); + + /* bulk read six registers, with the base being the LSB register */ + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, + &data->buffer.buff, sizeof(data->buffer.buff)); + if (ret) + goto unlock_err; + + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); + if (ret) + goto unlock_err; + + data->buffer.temperature = temp; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, + iio_get_time_ns(indio_dev)); + + mutex_unlock(&data->mutex); + iio_trigger_notify_done(indio_dev->trig); + return IRQ_HANDLED; + +unlock_err: + mutex_unlock(&data->mutex); + return IRQ_NONE; +} + +static irqreturn_t bma400_interrupt(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct bma400_data *data = iio_priv(indio_dev); + int ret; + + /* Lock to protect the data->status */ + mutex_lock(&data->mutex); + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, + &data->status, + sizeof(data->status)); + if (ret) + goto unlock_err; + + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(data->status))) { + mutex_unlock(&data->mutex); + iio_trigger_poll_chained(data->trig); + return IRQ_HANDLED; + } + +unlock_err: + mutex_unlock(&data->mutex); + return IRQ_NONE; +} + +int bma400_probe(struct device *dev, struct regmap *regmap, int irq, + const char *name) { struct iio_dev *indio_dev; struct bma400_data *data; @@ -841,8 +961,40 @@ int bma400_probe(struct device *dev, struct regmap *regmap, const char *name) indio_dev->info = &bma400_info; indio_dev->channels = bma400_channels; indio_dev->num_channels = ARRAY_SIZE(bma400_channels); + indio_dev->available_scan_masks = bma400_avail_scan_masks; indio_dev->modes = INDIO_DIRECT_MODE; + if (irq > 0) { + data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", + indio_dev->name, + iio_device_id(indio_dev)); + if (!data->trig) + return -ENOMEM; + + data->trig->ops = &bma400_trigger_ops; + iio_trigger_set_drvdata(data->trig, indio_dev); + + ret = devm_iio_trigger_register(data->dev, data->trig); + if (ret) + return dev_err_probe(data->dev, ret, + "iio trigger register fail\n"); + + indio_dev->trig = iio_trigger_get(data->trig); + ret = devm_request_threaded_irq(dev, irq, NULL, + &bma400_interrupt, + IRQF_TRIGGER_RISING | IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (ret) + return dev_err_probe(data->dev, ret, + "request irq %d failed\n", irq); + } + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, + &bma400_trigger_handler, NULL); + if (ret) + return dev_err_probe(data->dev, ret, + "iio triggered buffer setup failed\n"); + return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL_NS(bma400_probe, IIO_BMA400); diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c index 4f6e01a3b3a1..1ba2a982ea73 100644 --- a/drivers/iio/accel/bma400_i2c.c +++ b/drivers/iio/accel/bma400_i2c.c @@ -24,7 +24,7 @@ static int bma400_i2c_probe(struct i2c_client *client, return PTR_ERR(regmap); } - return bma400_probe(&client->dev, regmap, id->name); + return bma400_probe(&client->dev, regmap, client->irq, id->name); } static const struct i2c_device_id bma400_i2c_ids[] = { diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c index 28e240400a3f..ec13c044b304 100644 --- a/drivers/iio/accel/bma400_spi.c +++ b/drivers/iio/accel/bma400_spi.c @@ -84,7 +84,7 @@ static int bma400_spi_probe(struct spi_device *spi) if (ret) dev_err(&spi->dev, "Failed to read chip id register\n"); - return bma400_probe(&spi->dev, regmap, id->name); + return bma400_probe(&spi->dev, regmap, spi->irq, id->name); } static const struct spi_device_id bma400_spi_ids[] = {
Added trigger buffer support to read continuous acceleration data from device with data ready interrupt which is mapped to INT1 pin. Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> --- drivers/iio/accel/Kconfig | 2 + drivers/iio/accel/bma400.h | 10 +- drivers/iio/accel/bma400_core.c | 162 +++++++++++++++++++++++++++++++- drivers/iio/accel/bma400_i2c.c | 2 +- drivers/iio/accel/bma400_spi.c | 2 +- 5 files changed, 170 insertions(+), 8 deletions(-)