| Message ID | 20220411203133.19929-5-jagathjog1996@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: bma400: Add buffer, step and activity/inactivity | expand |
On Mon, Apr 11, 2022 at 11:31 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. Can you explain the locking schema in this driver? > + /* Configure INT1 pin to open drain */ > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06); > + if (ret) > + return ret; No locking (or regmap only locking). ... > +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)); > +} Ditto. ... > + 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)); > + mutex_unlock(&data->mutex); > + if (ret) > + return IRQ_NONE; But here only above with locking... > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > + if (ret) > + return IRQ_NONE; ...followed by no locking. ... > + mutex_lock(&data->mutex); > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status, > + sizeof(status)); > + mutex_unlock(&data->mutex); > + if (ret) > + return IRQ_NONE; And again with locking. ... So, 1) Does regmap is configured with locking? What for? 2) What's the role of data->mutex?
Hello Andy, On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote: > On Mon, Apr 11, 2022 at 11:31 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. > > Can you explain the locking schema in this driver? > > > + /* Configure INT1 pin to open drain */ > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06); > > + if (ret) > > + return ret; > > No locking (or regmap only locking). This is bma400_init() function which will run when probe runs so there is no locking in this bma400_init(). > > ... > > > +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)); > > +} > > Ditto. Sorry, I missed this. I will add lock and unlocking in the next patch. > > ... > > > + 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)); > > + mutex_unlock(&data->mutex); > > + if (ret) > > + return IRQ_NONE; > > But here only above with locking... > > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > > + if (ret) > > + return IRQ_NONE; > > ...followed by no locking. Okay I will add lock in the next patch. > > ... > > > + mutex_lock(&data->mutex); > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status, > > + sizeof(status)); > > + mutex_unlock(&data->mutex); > > + if (ret) > > + return IRQ_NONE; > > And again with locking. > > ... > > So, > 1) Does regmap is configured with locking? What for? > 2) What's the role of data->mutex? 1. NO, regmap is not configured with locking. In the regmap_config structure variable below these members are not defined in the driver. struct regmap_config { regmap_lock lock; regmap_unlock unlock; void *lock_arg; 2. data->mutex is used to protect the register read, write access from the device. Is the regmap functions handle locking and unlocking internally if these below struct members are not defined? regmap_lock lock; regmap_unlock unlock; void *lock_arg; > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote: > On Wednesday, April 13, 2022, Andy Shevchenko <andy.shevchenko@gmail.com> > wrote: > > > > > > > On Tuesday, April 12, 2022, Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > >> Hello Andy, > >> > >> On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote: > >> > On Mon, Apr 11, 2022 at 11:31 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. > >> > > >> > Can you explain the locking schema in this driver? > >> > > >> > > + /* Configure INT1 pin to open drain */ > >> > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, > >> 0x06); > >> > > + if (ret) > >> > > + return ret; > >> > > >> > No locking (or regmap only locking). > >> > >> This is bma400_init() function which will run when probe runs so there is > >> no > >> locking in this bma400_init(). > >> > >> > > >> > ... > >> > > >> > > +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)); > >> > > +} > >> > > >> > Ditto. > >> > >> Sorry, I missed this. > >> I will add lock and unlocking in the next patch. > >> > >> > > >> > ... > >> > > >> > > + 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)); > >> > > + mutex_unlock(&data->mutex); > >> > > + if (ret) > >> > > + return IRQ_NONE; > >> > > >> > But here only above with locking... > >> > > >> > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > >> > > + if (ret) > >> > > + return IRQ_NONE; > >> > > >> > ...followed by no locking. > >> > >> Okay I will add lock in the next patch. > >> > >> > > >> > ... > >> > > >> > > + mutex_lock(&data->mutex); > >> > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, > >> &status, > >> > > + sizeof(status)); > >> > > + mutex_unlock(&data->mutex); > >> > > + if (ret) > >> > > + return IRQ_NONE; > >> > > >> > And again with locking. > >> > > >> > ... > >> > > >> > So, > >> > 1) Does regmap is configured with locking? What for? > >> > 2) What's the role of data->mutex? > >> > >> 1. > >> NO, > > > > > > Are you sure? > > > > > >> regmap is not configured with locking. > >> In the remap_config structure variable below these members are not defined > >> in the driver. > >> > >> struct regmap_config { > >> regmap_lock lock; > >> regmap_unlock unlock; > >> void *lock_arg; > >> > >> > > It means that default may be used. > > > > > >> 2. > >> data->mutex is used to protect the register read, write access from the > >> device. > >> > >> Is the regmap functions handle locking and unlocking internally if these > >> below > >> struct members are not defined? > > > > > > Yes. Look at this: https://elixir.bootlin.com/linux/latest/C/ident/ > > disable_locking Please your advise will be very helpful for this. I'm going through the same documentation. In this driver, disable_locking is not initialized. The functions which are called in the bma400_init() are not protected by mutex for regmap since bma400_init() will run when the probe runs. The functions which are called by read_raw() and write_raw() are protected by mutex for regmap access. There are some members in the device's private data structure and they are being accessed in multiple places in the driver. struct bma400_data { enum bma400_power_mode power_mode; struct bma400_sample_freq sample_freq; int oversampling_ratio; int scale; ..... I think mutex is used to protect these above struct members since they are critical resource, but in the struct bma400_data comment for mutex is "data register lock". > > > > > >> > >> regmap_lock lock; > >> regmap_unlock unlock; > >> void *lock_arg; > >> > >> > >> > > >> > -- > >> > With Best Regards, > >> > Andy Shevchenko > >> > > > >> > > You may read the kernel documentation what those fields mean: > https://elixir.bootlin.com/linux/latest/source/include/linux/regmap.h#L278 > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > > > > > > -- > With Best Regards, > Andy Shevchenko Thank you, Jagath
Hi Andy, On Wed, Apr 13, 2022 at 07:53:46PM +0530, Jagath Jog J wrote: > On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote: > > On Wednesday, April 13, 2022, Andy Shevchenko <andy.shevchenko@gmail.com> > > wrote: > > > > > > > > > > > On Tuesday, April 12, 2022, Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > > > >> Hello Andy, > > >> > > >> On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote: > > >> > On Mon, Apr 11, 2022 at 11:31 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. > > >> > > > >> > Can you explain the locking schema in this driver? > > >> > > > >> > > + /* Configure INT1 pin to open drain */ > > >> > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, > > >> 0x06); > > >> > > + if (ret) > > >> > > + return ret; > > >> > > > >> > No locking (or regmap only locking). > > >> > > >> This is bma400_init() function which will run when probe runs so there is > > >> no > > >> locking in this bma400_init(). > > >> > > >> > > > >> > ... > > >> > > > >> > > +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)); > > >> > > +} > > >> > > > >> > Ditto. > > >> > > >> Sorry, I missed this. > > >> I will add lock and unlocking in the next patch. > > >> > > >> > > > >> > ... > > >> > > > >> > > + 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)); > > >> > > + mutex_unlock(&data->mutex); > > >> > > + if (ret) > > >> > > + return IRQ_NONE; > > >> > > > >> > But here only above with locking... > > >> > > > >> > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > > >> > > + if (ret) > > >> > > + return IRQ_NONE; > > >> > > > >> > ...followed by no locking. > > >> > > >> Okay I will add lock in the next patch. > > >> > > >> > > > >> > ... > > >> > > > >> > > + mutex_lock(&data->mutex); > > >> > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, > > >> &status, > > >> > > + sizeof(status)); > > >> > > + mutex_unlock(&data->mutex); > > >> > > + if (ret) > > >> > > + return IRQ_NONE; > > >> > > > >> > And again with locking. > > >> > > > >> > ... > > >> > > > >> > So, > > >> > 1) Does regmap is configured with locking? What for? > > >> > 2) What's the role of data->mutex? > > >> > > >> 1. > > >> NO, > > > > > > > > > Are you sure? > > > > > > > > >> regmap is not configured with locking. > > >> In the remap_config structure variable below these members are not defined > > >> in the driver. > > >> > > >> struct regmap_config { > > >> regmap_lock lock; > > >> regmap_unlock unlock; > > >> void *lock_arg; > > >> > > >> > > > It means that default may be used. > > > > > > > > >> 2. > > >> data->mutex is used to protect the register read, write access from the > > >> device. > > >> > > >> Is the regmap functions handle locking and unlocking internally if these > > >> below > > >> struct members are not defined? > > > > > > > > > Yes. Look at this: https://elixir.bootlin.com/linux/latest/C/ident/ > > > disable_locking > > Please your advise will be very helpful for this. > > I'm going through the same documentation. In this driver, disable_locking is > not initialized. > > The functions which are called in the bma400_init() are not protected by mutex > for regmap since bma400_init() will run when the probe runs. > > The functions which are called by read_raw() and write_raw() are protected by > mutex for regmap access. > > There are some members in the device's private data structure and they are being > accessed in multiple places in the driver. > > struct bma400_data { > enum bma400_power_mode power_mode; > struct bma400_sample_freq sample_freq; > int oversampling_ratio; > int scale; > ..... > > I think mutex is used to protect these above struct members since they are > critical resource, but in the struct bma400_data comment for mutex > is "data register lock". Sorry, I got my mistake about mutex lock and unlocking and I will correct those in the next patch series. Thank you Jagath > > > > > > > > > > >> > > >> regmap_lock lock; > > >> regmap_unlock unlock; > > >> void *lock_arg; > > >> > > >> > > >> > > > >> > -- > > >> > With Best Regards, > > >> > Andy Shevchenko > > >> > > > > > >> > > > > You may read the kernel documentation what those fields mean: > > https://elixir.bootlin.com/linux/latest/source/include/linux/regmap.h#L278 > > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko > > > > > > > > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > Thank you, > Jagath
On Tue, 12 Apr 2022 02:01:28 +0530 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. > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com> A few comments inline. Jonathan > --- > drivers/iio/accel/Kconfig | 2 + > drivers/iio/accel/bma400.h | 10 ++- > drivers/iio/accel/bma400_core.c | 153 ++++++++++++++++++++++++++++++-- > drivers/iio/accel/bma400_i2c.c | 2 +- > drivers/iio/accel/bma400_spi.c | 2 +- > 5 files changed, 161 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 8dbf85eeb005..a7482a66b36b 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..b7b2b67aef31 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,13 @@ 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; See below, but I'd suggest adding __le16 status; here to be in the same cacheline as the buffer and hence also DMA safe (as it's not in the same line as anything else which could be modified concurrently.) > }; > > static bool bma400_is_writable_reg(struct device *dev, unsigned int reg) > @@ -152,7 +164,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 +176,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 +686,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 +838,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 +868,64 @@ 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; > + > + 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)); > + mutex_unlock(&data->mutex); > + if (ret) > + return IRQ_NONE; > + > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > + if (ret) > + return IRQ_NONE; > + > + data->buffer.temperature = temp; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > + iio_get_time_ns(indio_dev)); > + > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > +} > + > +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; > + __le16 status; > + > + mutex_lock(&data->mutex); > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status, > + sizeof(status)); regmap_bulk_read() (may) need a DMA safe buffer. Which means you can't use a variable on the stack. Look at using the carefully aligned and padded data->buffer if you can as that is DMA safe. Note that then you will need that lock as it protects that buffer... You could also just add a suitable buffer after that instead of reusing that particular structure. > + mutex_unlock(&data->mutex); > + if (ret) > + return IRQ_NONE; > + > + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) { > + iio_trigger_poll_chained(data->trig); > + return IRQ_HANDLED; > + } > + > + 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 8dbf85eeb005..a7482a66b36b 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..b7b2b67aef31 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,13 @@ 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; }; static bool bma400_is_writable_reg(struct device *dev, unsigned int reg) @@ -152,7 +164,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 +176,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 +686,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 +838,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 +868,64 @@ 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; + + 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)); + mutex_unlock(&data->mutex); + if (ret) + return IRQ_NONE; + + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); + if (ret) + return IRQ_NONE; + + data->buffer.temperature = temp; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, + iio_get_time_ns(indio_dev)); + + iio_trigger_notify_done(indio_dev->trig); + return IRQ_HANDLED; +} + +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; + __le16 status; + + mutex_lock(&data->mutex); + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status, + sizeof(status)); + mutex_unlock(&data->mutex); + if (ret) + return IRQ_NONE; + + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) { + iio_trigger_poll_chained(data->trig); + return IRQ_HANDLED; + } + + 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 +952,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 | 153 ++++++++++++++++++++++++++++++-- drivers/iio/accel/bma400_i2c.c | 2 +- drivers/iio/accel/bma400_spi.c | 2 +- 5 files changed, 161 insertions(+), 8 deletions(-)