| Message ID | 20220319181023.8090-4-jagathjog1996@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: bma400: Add support for buffer and step | expand |
Hi Jagath,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.17-rc8]
[cannot apply to jic23-iio/togreg next-20220318]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-support-for-buffer-and-step/20220320-021114
base: 09688c0166e76ce2fb85e86b9d99be8b0084cdf9
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220320/202203201124.OLMstZaW-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/8dc9a46d5af9a53917108ce2b103b3d9a50986a5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-support-for-buffer-and-step/20220320-021114
git checkout 8dc9a46d5af9a53917108ce2b103b3d9a50986a5
# save the config file to linux build tree
mkdir build_dir
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/iio/accel/bma400_core.c:906:13: sparse: sparse: restricted __le16 degrades to integer
vim +906 drivers/iio/accel/bma400_core.c
891
892 static irqreturn_t bma400_interrupt(int irq, void *private)
893 {
894 struct iio_dev *indio_dev = private;
895 struct bma400_data *data = iio_priv(indio_dev);
896 int ret;
897 __le16 status;
898
899 mutex_lock(&data->mutex);
900 ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
901 sizeof(status));
902 mutex_unlock(&data->mutex);
903 if (ret)
904 goto out;
905
> 906 if (status & BMA400_INT_DRDY_MSK)
907 iio_trigger_poll_chained(data->trig);
908
909 out:
910 return IRQ_HANDLED;
911 }
912
On Sat, 19 Mar 2022 23:40:21 +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> > --- > drivers/iio/accel/Kconfig | 2 + > drivers/iio/accel/bma400.h | 10 +- > drivers/iio/accel/bma400_core.c | 161 +++++++++++++++++++++++++++++++- > drivers/iio/accel/bma400_i2c.c | 2 +- > drivers/iio/accel/bma400_spi.c | 2 +- > 5 files changed, 169 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index 49587c992a6d..0eb379578e00 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -177,6 +177,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 cfc2c9bacec8..b306a5ad513a 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 > > @@ -92,6 +99,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 dcc7549c7a0e..797403c7dd85 100644 > --- a/drivers/iio/accel/bma400_core.c > +++ b/drivers/iio/accel/bma400_core.c > @@ -20,6 +20,12 @@ > #include <linux/mutex.h> > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/iio/trigger_consumer.h> > > #include "bma400.h" > > @@ -61,6 +67,13 @@ struct bma400_data { > struct bma400_sample_freq sample_freq; > int oversampling_ratio; > int scale; > + struct iio_trigger *trig; > + /* Correct time stamp alignment */ > + struct { > + __be16 buff[3]; > + u8 temperature; > + s64 ts __aligned(8); > + } buffer; you are doing bulk reads from an spi device into here. There is a long running discussion about what we can assume about need for DMA safety when regmap is involved. Current state is we can't assume we don't need to be DMA safe. As such this should be in a separate cacheline from anything that might be touched concurrently with DMA. Mark buffer ___cacheline_aligned; and we should be fine. If curious, Wolfram Sang did a good talk on this at ELCE a few years back that google should find for you. It's an interesting little corner of horribleness :) > }; > > 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) > @@ -632,6 +660,11 @@ static int bma400_init(struct bma400_data *data) > if (ret) > goto err_reg_disable; > > + /* Configure INT-1 pin to push pull */ Hmm. We should be getting the requirements for using this pin from DT, though I can't immediately think how to do it. If the interrupt controller is happy with open drain, then we should do that as well. Ultimately I think this code will be happy with shared interrupts so lets not make it harder than we need to. > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x02); > + if (ret) > + goto err_reg_disable; > + > /* > * Once the interrupt engine is supported we might use the > * data_src_reg, but for now ensure this is set to the > @@ -786,6 +819,33 @@ 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; > + > + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, > + BMA400_INT_DRDY_MSK, > + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); > + if (ret) > + return ret; > + > + return 0; > +} > + > +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, > @@ -793,6 +853,63 @@ static const struct iio_info bma400_info = { > .write_raw_get_fmt = bma400_write_raw_get_fmt, > }; > > +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, 3 * sizeof(__be16)); > + mutex_unlock(&data->mutex); > + if (ret) > + goto out; > + > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); > + if (ret) > + goto out; > + > + data->buffer.temperature = temp; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, > + iio_get_time_ns(indio_dev)); > + > +out: > + 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) > + goto out; > + > + if (status & BMA400_INT_DRDY_MSK) 0-day pointed this out. You need an le16_to_cpu() > + iio_trigger_poll_chained(data->trig); > + > +out: > + return IRQ_HANDLED; > +} > + > static void bma400_disable(void *data_ptr) > { > struct bma400_data *data = data_ptr; > @@ -806,7 +923,8 @@ static void bma400_disable(void *data_ptr) > regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > } > > -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) > { > struct iio_dev *indio_dev; > struct bma400_data *data; > @@ -833,12 +951,45 @@ 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; > > ret = devm_add_action_or_reset(dev, bma400_disable, data); > if (ret) > return ret; > > + 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) { > + dev_err(dev, "iio trigger register failed\n"); > + return ret; > + } > + 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) { > + dev_err(dev, "request irq %d failed\n", irq); > + return ret; > + } > + } > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > + &bma400_trigger_handler, NULL); > + if (ret) { > + dev_err(dev, "iio triggered buffer setup failed\n"); > + return ret; > + } > return devm_iio_device_register(dev, indio_dev); > }
On Sat, Mar 19, 2022 at 8:10 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. ... > #include <linux/mutex.h> > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > +#include <linux/bits.h> > +#include <linux/bitfield.h> It would be nice to keep the above in order. > +#include <linux/iio/buffer.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/iio/trigger_consumer.h> These ones, possibly including iio headers from the above piece, are good to be grouped together here with a blank line in between the above part and iio/*. ... > +static const unsigned long bma400_avail_scan_masks[] = { > + GENMASK(3, 0), > + 0, No need to have a comma in terminator entry. > +}; ... > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > + &data->buffer.buff, 3 * sizeof(__be16)); sizeof(buff) ... > +out: A useless label. Moreover this raises a question: why is it okay to always mark IRQ as handled? > + return IRQ_HANDLED; ... > + dev_err(dev, "iio trigger register failed\n"); > + return ret; return dev_err_probe(); ... > + dev_err(dev, "request irq %d failed\n", irq); > + return ret; Ditto. ... > + dev_err(dev, "iio triggered buffer setup failed\n"); > + return ret; Ditto.
Hello Andy, On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote: > On Sat, Mar 19, 2022 at 8:10 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. > > ... > > > #include <linux/mutex.h> > > #include <linux/regmap.h> > > #include <linux/regulator/consumer.h> > > +#include <linux/bits.h> > > +#include <linux/bitfield.h> > > It would be nice to keep the above in order. > > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/trigger.h> > > +#include <linux/iio/triggered_buffer.h> > > +#include <linux/iio/trigger_consumer.h> > > These ones, possibly including iio headers from the above piece, are > good to be grouped together here with a blank line in between the > above part and iio/*. > > ... > > > +static const unsigned long bma400_avail_scan_masks[] = { > > + GENMASK(3, 0), > > > + 0, > > No need to have a comma in terminator entry. > > > +}; > > ... > > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG, > > + &data->buffer.buff, 3 * sizeof(__be16)); > > sizeof(buff) > > ... > > > +out: Just to skip the below "if()" if error occurs in previous regmap read, I used this label. if (status & BMA400_INT_DRDY_MSK) iio_trigger_poll_chained(data->trig); I will remove the label in next patch > > A useless label. Moreover this raises a question: why is it okay to > always mark IRQ as handled? > > > + return IRQ_HANDLED; Since I was not using top-half of the interrupt so I marked IRQ as handled even for error case in the handler. > > ... > > > + dev_err(dev, "iio trigger register failed\n"); > > + return ret; > > return dev_err_probe(); > > ... > > > + dev_err(dev, "request irq %d failed\n", irq); > > + return ret; > > Ditto. > > ... > > > + dev_err(dev, "iio triggered buffer setup failed\n"); > > + return ret; > > Ditto. I will change this in the next patch version. > > -- > With Best Regards, > Andy Shevchenko
On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote: > > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: First of all, you left many uncommented comments. I assume you agree with my comments and are going to address them. If it's not the case, please elaborate. ... > > > +out: > > Just to skip the below "if()" if error occurs in previous regmap read, > I used this label. > if (status & BMA400_INT_DRDY_MSK) > iio_trigger_poll_chained(data->trig); > > I will remove the label in next patch Just return directly. ... > > A useless label. Moreover this raises a question: why is it okay to > > always mark IRQ as handled? > > > > > + return IRQ_HANDLED; > > Since I was not using top-half of the interrupt so I marked IRQ as handled > even for error case in the handler. Yes, but why? Isn't it an erroneous state? Does it mean spurious interrupt? Does it mean interrupt is unserviced?
Hello Andy, On Tue, Mar 22, 2022 at 10:54:53AM +0200, Andy Shevchenko wrote: > On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote: > > > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > First of all, you left many uncommented comments. I assume you agree > with my comments and are going to address them. If it's not the case, > please elaborate. Yes Andy, I agree with your comments and I will address them in the next v2 series. > > ... > > > > > +out: > > > > Just to skip the below "if()" if error occurs in previous regmap read, > > I used this label. > > if (status & BMA400_INT_DRDY_MSK) > > iio_trigger_poll_chained(data->trig); > > > > I will remove the label in next patch > > Just return directly. > > ... > > > > A useless label. Moreover this raises a question: why is it okay to > > > always mark IRQ as handled? > > > > > > > + return IRQ_HANDLED; > > > > Since I was not using top-half of the interrupt so I marked IRQ as handled > > even for error case in the handler. > > Yes, but why? Isn't it an erroneous state? Does it mean spurious > interrupt? Does it mean interrupt is unserviced? Sorry, even for erroneous state I was returning IRQ_HANDLED. As shown below, now for erroneous state and spurious interrupt I will return IRQ_NONE and for valid interrupt IRQ_HANDLED will be returned. Is below method is correct? 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 (le16_to_cpu(status) & BMA400_INT_DRDY_MSK) { iio_trigger_poll_chained(data->trig); return IRQ_HANDLED; } return IRQ_NONE; } > > -- > With Best Regards, > Andy Shevchenko
On Tue, Mar 22, 2022 at 5:40 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: > On Tue, Mar 22, 2022 at 10:54:53AM +0200, Andy Shevchenko wrote: > > On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote: > > > On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote: > > > > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote: ... > > > > A useless label. Moreover this raises a question: why is it okay to > > > > always mark IRQ as handled? > > > > > > > > > + return IRQ_HANDLED; > > > > > > Since I was not using top-half of the interrupt so I marked IRQ as handled > > > even for error case in the handler. > > > > Yes, but why? Isn't it an erroneous state? Does it mean spurious > > interrupt? Does it mean interrupt is unserviced? > > Sorry, even for erroneous state I was returning IRQ_HANDLED. > As shown below, now for erroneous state and spurious interrupt I will return > IRQ_NONE and for valid interrupt IRQ_HANDLED will be returned. > > Is below method is correct? The thing is that I don't know. I am not familiar with this hardware. So, you have to investigate and decide. > 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 (le16_to_cpu(status) & BMA400_INT_DRDY_MSK) { > iio_trigger_poll_chained(data->trig); > return IRQ_HANDLED; > } > > return IRQ_NONE; If you are going with this approach, try to handle errors first, i.e. if (...) return IRQ_NONE; ... return IRQ_HANDLED; > }
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 49587c992a6d..0eb379578e00 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -177,6 +177,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 cfc2c9bacec8..b306a5ad513a 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 @@ -92,6 +99,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 dcc7549c7a0e..797403c7dd85 100644 --- a/drivers/iio/accel/bma400_core.c +++ b/drivers/iio/accel/bma400_core.c @@ -20,6 +20,12 @@ #include <linux/mutex.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <linux/bits.h> +#include <linux/bitfield.h> +#include <linux/iio/buffer.h> +#include <linux/iio/trigger.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/trigger_consumer.h> #include "bma400.h" @@ -61,6 +67,13 @@ struct bma400_data { struct bma400_sample_freq sample_freq; int oversampling_ratio; int scale; + struct iio_trigger *trig; + /* Correct time stamp alignment */ + struct { + __be16 buff[3]; + u8 temperature; + s64 ts __aligned(8); + } buffer; }; 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) @@ -632,6 +660,11 @@ static int bma400_init(struct bma400_data *data) if (ret) goto err_reg_disable; + /* Configure INT-1 pin to push pull */ + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x02); + if (ret) + goto err_reg_disable; + /* * Once the interrupt engine is supported we might use the * data_src_reg, but for now ensure this is set to the @@ -786,6 +819,33 @@ 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; + + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG, + BMA400_INT_DRDY_MSK, + FIELD_PREP(BMA400_INT_DRDY_MSK, state)); + if (ret) + return ret; + + return 0; +} + +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, @@ -793,6 +853,63 @@ static const struct iio_info bma400_info = { .write_raw_get_fmt = bma400_write_raw_get_fmt, }; +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, 3 * sizeof(__be16)); + mutex_unlock(&data->mutex); + if (ret) + goto out; + + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp); + if (ret) + goto out; + + data->buffer.temperature = temp; + + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer, + iio_get_time_ns(indio_dev)); + +out: + 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) + goto out; + + if (status & BMA400_INT_DRDY_MSK) + iio_trigger_poll_chained(data->trig); + +out: + return IRQ_HANDLED; +} + static void bma400_disable(void *data_ptr) { struct bma400_data *data = data_ptr; @@ -806,7 +923,8 @@ static void bma400_disable(void *data_ptr) regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); } -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) { struct iio_dev *indio_dev; struct bma400_data *data; @@ -833,12 +951,45 @@ 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; ret = devm_add_action_or_reset(dev, bma400_disable, data); if (ret) return ret; + 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) { + dev_err(dev, "iio trigger register failed\n"); + return ret; + } + 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) { + dev_err(dev, "request irq %d failed\n", irq); + return ret; + } + } + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, + &bma400_trigger_handler, NULL); + if (ret) { + dev_err(dev, "iio triggered buffer setup failed\n"); + return ret; + } return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL(bma400_probe); diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c index 56da06537562..49b0ae13fdc8 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 96dc9c215401..2957ebc51543 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 | 161 +++++++++++++++++++++++++++++++- drivers/iio/accel/bma400_i2c.c | 2 +- drivers/iio/accel/bma400_spi.c | 2 +- 5 files changed, 169 insertions(+), 8 deletions(-)