This patch series adds trigger buffer support with data ready interrupt,
separate channel for step counter and an event for step change interrupt.
changes since v1
1. Added comment section that describes the math for scale calculation.
2. Added separate devm_add_action_or_reset() calls to disable regulator
and to put the sensor in power down mode.
3. Remove the err_reg_disable and out, goto labels and returning directly
if error occurs.
4. Added mutex calls while putting sensor in power down.
5. Added ___cacheline_aligned for device data.
6. Ordering the header includes.
7. Handling erroneous and spurious interrupts in the interrupt handler
by returning IRQ_NONE.
8. Using dev_err_probe() instead of dev_err().
9. Configured the interrupt to open drain.
10. Using le16_to_cpu() to fix the sparse warning.
11. Checking the step change event is enabled or not.
12. Enabling the step change event will also enable the step channel.
13. Using FIELD_GET() instead of bitwise operation.
14. Removal of dead code in the _event_config().
Jagath Jog J (5):
iio: accel: bma400: Fix the scale min and max macro values
iio: accel: bma400: conversion to device-managed function
iio: accel: bma400: Add triggered buffer support
iio: accel: bma400: Add separate channel for step counter
iio: accel: bma400: Add step change event
drivers/iio/accel/Kconfig | 2 +
drivers/iio/accel/bma400.h | 37 +++-
drivers/iio/accel/bma400_core.c | 351 +++++++++++++++++++++++++++-----
drivers/iio/accel/bma400_i2c.c | 10 +-
drivers/iio/accel/bma400_spi.c | 10 +-
5 files changed, 341 insertions(+), 69 deletions(-)
--
2.17.1
Added support for event when there is a detection of step change.
INT1 pin is used to interrupt and event is pushed to userspace.
Signed-off-by: Jagath Jog J <[email protected]>
---
drivers/iio/accel/bma400.h | 2 +
drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
2 files changed, 75 insertions(+)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index c9b856b37021..c4ec0cf6dc00 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -39,6 +39,7 @@
#define BMA400_INT_STAT0_REG 0x0e
#define BMA400_INT_STAT1_REG 0x0f
#define BMA400_INT_STAT2_REG 0x10
+#define BMA400_INT12_MAP_REG 0x23
/* Temperature register */
#define BMA400_TEMP_DATA_REG 0x11
@@ -54,6 +55,7 @@
#define BMA400_STEP_CNT3_REG 0x17
#define BMA400_STEP_STAT_REG 0x18
#define BMA400_STEP_INT_MSK BIT(0)
+#define BMA400_STEP_STAT_MASK GENMASK(9, 8)
/*
* Read-write configuration registers
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index ec2f9c380bda..aaa104a2698b 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -24,6 +24,7 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
#include <linux/iio/trigger.h>
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
@@ -70,6 +71,7 @@ struct bma400_data {
int scale;
struct iio_trigger *trig;
int steps_enabled;
+ bool step_event_en;
/* Correct time stamp alignment */
struct {
__le16 buff[3];
@@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
{ }
};
+static const struct iio_event_spec bma400_step_detect_event = {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+};
+
#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
BIT(IIO_CHAN_INFO_ENABLE),
.scan_index = -1, /* No buffer support */
+ .event_spec = &bma400_step_detect_event,
+ .num_event_specs = 1,
},
IIO_CHAN_SOFT_TIMESTAMP(4),
};
@@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
}
}
+static int bma400_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_CHANGE:
+ return data->step_event_en;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bma400_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int state)
+{
+ int ret;
+ struct bma400_data *data = iio_priv(indio_dev);
+
+ switch (type) {
+ case IIO_EV_TYPE_CHANGE:
+ mutex_lock(&data->mutex);
+ if (!data->steps_enabled) {
+ ret = regmap_update_bits(data->regmap,
+ BMA400_INT_CONFIG1_REG,
+ BMA400_STEP_INT_MSK,
+ FIELD_PREP(BMA400_STEP_INT_MSK,
+ 1));
+ if (ret)
+ return ret;
+ data->steps_enabled = 1;
+ }
+
+ ret = regmap_update_bits(data->regmap,
+ BMA400_INT12_MAP_REG,
+ BMA400_STEP_INT_MSK,
+ FIELD_PREP(BMA400_STEP_INT_MSK,
+ state));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ data->step_event_en = state;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
bool state)
{
@@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
.read_avail = bma400_read_avail,
.write_raw = bma400_write_raw,
.write_raw_get_fmt = bma400_write_raw_get_fmt,
+ .read_event_config = bma400_read_event_config,
+ .write_event_config = bma400_write_event_config,
};
static const struct iio_trigger_ops bma400_trigger_ops = {
@@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
ret = IRQ_HANDLED;
}
+ if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
+ iio_push_event(indio_dev,
+ IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
+ IIO_EV_DIR_NONE,
+ IIO_EV_TYPE_CHANGE, 0, 0, 0),
+ iio_get_time_ns(indio_dev));
+ ret = IRQ_HANDLED;
+ }
+
return ret;
}
--
2.17.1
This is a conversion to device-managed by using devm_iio_device_register
inside probe function, now disabling the regulator and putting bma400 to
power down via a devm_add_action_or_reset() hook.
The dev_set_drvdata() call, bma400_remove() function and hooks in the I2C
and SPI driver struct is removed as devm_iio_device_register function is
used to automatically unregister on driver detach.
Signed-off-by: Jagath Jog J <[email protected]>
---
drivers/iio/accel/bma400.h | 2 -
drivers/iio/accel/bma400_core.c | 77 ++++++++++++++++-----------------
drivers/iio/accel/bma400_i2c.c | 8 ----
drivers/iio/accel/bma400_spi.c | 8 ----
4 files changed, 38 insertions(+), 57 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 190366debdb3..c1b3dbfbd98f 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -112,6 +112,4 @@ extern const struct regmap_config bma400_regmap_config;
int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
-void bma400_remove(struct device *dev);
-
#endif
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index fd2647b728d3..dc273381a0a2 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -560,6 +560,26 @@ static void bma400_init_tables(void)
}
}
+static void bma400_regulators_disable(void *data_ptr)
+{
+ struct bma400_data *data = data_ptr;
+
+ regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
+static void bma400_power_disable(void *data_ptr)
+{
+ struct bma400_data *data = data_ptr;
+ int ret;
+
+ mutex_lock(&data->mutex);
+ ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
+ if (ret)
+ dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
+ ERR_PTR(ret));
+ mutex_unlock(&data->mutex);
+}
+
static int bma400_init(struct bma400_data *data)
{
unsigned int val;
@@ -569,13 +589,12 @@ static int bma400_init(struct bma400_data *data)
ret = regmap_read(data->regmap, BMA400_CHIP_ID_REG, &val);
if (ret) {
dev_err(data->dev, "Failed to read chip id register\n");
- goto out;
+ return ret;
}
if (val != BMA400_ID_REG_VAL) {
dev_err(data->dev, "Chip ID mismatch\n");
- ret = -ENODEV;
- goto out;
+ return -ENODEV;
}
data->regulators[BMA400_VDD_REGULATOR].supply = "vdd";
@@ -589,27 +608,31 @@ static int bma400_init(struct bma400_data *data)
"Failed to get regulators: %d\n",
ret);
- goto out;
+ return ret;
}
ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
data->regulators);
if (ret) {
dev_err(data->dev, "Failed to enable regulators: %d\n",
ret);
- goto out;
+ return ret;
}
+ ret = devm_add_action_or_reset(data->dev, bma400_regulators_disable, data);
+ if (ret)
+ return ret;
+
ret = bma400_get_power_mode(data);
if (ret) {
dev_err(data->dev, "Failed to get the initial power-mode\n");
- goto err_reg_disable;
+ return ret;
}
if (data->power_mode != POWER_MODE_NORMAL) {
ret = bma400_set_power_mode(data, POWER_MODE_NORMAL);
if (ret) {
dev_err(data->dev, "Failed to wake up the device\n");
- goto err_reg_disable;
+ return ret;
}
/*
* TODO: The datasheet waits 1500us here in the example, but
@@ -618,19 +641,23 @@ static int bma400_init(struct bma400_data *data)
usleep_range(1500, 2000);
}
+ ret = devm_add_action_or_reset(data->dev, bma400_power_disable, data);
+ if (ret)
+ return ret;
+
bma400_init_tables();
ret = bma400_get_accel_output_data_rate(data);
if (ret)
- goto err_reg_disable;
+ return ret;
ret = bma400_get_accel_oversampling_ratio(data);
if (ret)
- goto err_reg_disable;
+ return ret;
ret = bma400_get_accel_scale(data);
if (ret)
- goto err_reg_disable;
+ return ret;
/*
* Once the interrupt engine is supported we might use the
@@ -639,12 +666,6 @@ static int bma400_init(struct bma400_data *data)
* channel.
*/
return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
-
-err_reg_disable:
- regulator_bulk_disable(ARRAY_SIZE(data->regulators),
- data->regulators);
-out:
- return ret;
}
static int bma400_read_raw(struct iio_dev *indio_dev,
@@ -822,32 +843,10 @@ int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
indio_dev->modes = INDIO_DIRECT_MODE;
- dev_set_drvdata(dev, indio_dev);
-
- return iio_device_register(indio_dev);
+ return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL(bma400_probe);
-void bma400_remove(struct device *dev)
-{
- struct iio_dev *indio_dev = dev_get_drvdata(dev);
- struct bma400_data *data = iio_priv(indio_dev);
- int ret;
-
- mutex_lock(&data->mutex);
- ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
- mutex_unlock(&data->mutex);
-
- if (ret)
- dev_warn(dev, "Failed to put device into sleep mode (%pe)\n", ERR_PTR(ret));
-
- regulator_bulk_disable(ARRAY_SIZE(data->regulators),
- data->regulators);
-
- iio_device_unregister(indio_dev);
-}
-EXPORT_SYMBOL(bma400_remove);
-
MODULE_AUTHOR("Dan Robertson <[email protected]>");
MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index f50df5310beb..56da06537562 100644
--- a/drivers/iio/accel/bma400_i2c.c
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -27,13 +27,6 @@ static int bma400_i2c_probe(struct i2c_client *client,
return bma400_probe(&client->dev, regmap, id->name);
}
-static int bma400_i2c_remove(struct i2c_client *client)
-{
- bma400_remove(&client->dev);
-
- return 0;
-}
-
static const struct i2c_device_id bma400_i2c_ids[] = {
{ "bma400", 0 },
{ }
@@ -52,7 +45,6 @@ static struct i2c_driver bma400_i2c_driver = {
.of_match_table = bma400_of_i2c_match,
},
.probe = bma400_i2c_probe,
- .remove = bma400_i2c_remove,
.id_table = bma400_i2c_ids,
};
diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
index 9f622e37477b..96dc9c215401 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -87,13 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
return bma400_probe(&spi->dev, regmap, id->name);
}
-static int bma400_spi_remove(struct spi_device *spi)
-{
- bma400_remove(&spi->dev);
-
- return 0;
-}
-
static const struct spi_device_id bma400_spi_ids[] = {
{ "bma400", 0 },
{ }
@@ -112,7 +105,6 @@ static struct spi_driver bma400_spi_driver = {
.of_match_table = bma400_of_spi_match,
},
.probe = bma400_spi_probe,
- .remove = bma400_spi_remove,
.id_table = bma400_spi_ids,
};
--
2.17.1
On Sun, 27 Mar 2022 01:11:46 +0530
Jagath Jog J <[email protected]> wrote:
> Added support for event when there is a detection of step change.
> INT1 pin is used to interrupt and event is pushed to userspace.
>
> Signed-off-by: Jagath Jog J <[email protected]>
These last two patches look fine to me. Simply having the
event enable the channel makes things simpler.
I briefly wondered if we need to care about sequences like
1) Enable event
2) Enable channel (already enabled, but perhaps this indicates separate intent)
3) Disable event.
4) Is the channel still enabled?
or the simpler case of whether we should disable the channel if the event is
disabled and it wasn't otherwise turned on.
However, I can't see a sensible way to do so. Hence I think what you have
gone with is the best we can do.
Thanks,
Jonathan
> ---
> drivers/iio/accel/bma400.h | 2 +
> drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index c9b856b37021..c4ec0cf6dc00 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -39,6 +39,7 @@
> #define BMA400_INT_STAT0_REG 0x0e
> #define BMA400_INT_STAT1_REG 0x0f
> #define BMA400_INT_STAT2_REG 0x10
> +#define BMA400_INT12_MAP_REG 0x23
>
> /* Temperature register */
> #define BMA400_TEMP_DATA_REG 0x11
> @@ -54,6 +55,7 @@
> #define BMA400_STEP_CNT3_REG 0x17
> #define BMA400_STEP_STAT_REG 0x18
> #define BMA400_STEP_INT_MSK BIT(0)
> +#define BMA400_STEP_STAT_MASK GENMASK(9, 8)
>
> /*
> * Read-write configuration registers
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index ec2f9c380bda..aaa104a2698b 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -24,6 +24,7 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/trigger.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
> @@ -70,6 +71,7 @@ struct bma400_data {
> int scale;
> struct iio_trigger *trig;
> int steps_enabled;
> + bool step_event_en;
> /* Correct time stamp alignment */
> struct {
> __le16 buff[3];
> @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> { }
> };
>
> +static const struct iio_event_spec bma400_step_detect_event = {
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_NONE,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
> #define BMA400_ACC_CHANNEL(_index, _axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> BIT(IIO_CHAN_INFO_ENABLE),
> .scan_index = -1, /* No buffer support */
> + .event_spec = &bma400_step_detect_event,
> + .num_event_specs = 1,
> },
> IIO_CHAN_SOFT_TIMESTAMP(4),
> };
> @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> }
> }
>
> +static int bma400_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> +
> + switch (type) {
> + case IIO_EV_TYPE_CHANGE:
> + return data->step_event_en;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bma400_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + int ret;
> + struct bma400_data *data = iio_priv(indio_dev);
> +
> + switch (type) {
> + case IIO_EV_TYPE_CHANGE:
> + mutex_lock(&data->mutex);
> + if (!data->steps_enabled) {
> + ret = regmap_update_bits(data->regmap,
> + BMA400_INT_CONFIG1_REG,
> + BMA400_STEP_INT_MSK,
> + FIELD_PREP(BMA400_STEP_INT_MSK,
> + 1));
> + if (ret)
> + return ret;
> + data->steps_enabled = 1;
> + }
> +
> + ret = regmap_update_bits(data->regmap,
> + BMA400_INT12_MAP_REG,
> + BMA400_STEP_INT_MSK,
> + FIELD_PREP(BMA400_STEP_INT_MSK,
> + state));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + data->step_event_en = state;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> bool state)
> {
> @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> .read_avail = bma400_read_avail,
> .write_raw = bma400_write_raw,
> .write_raw_get_fmt = bma400_write_raw_get_fmt,
> + .read_event_config = bma400_read_event_config,
> + .write_event_config = bma400_write_event_config,
> };
>
> static const struct iio_trigger_ops bma400_trigger_ops = {
> @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> ret = IRQ_HANDLED;
> }
>
> + if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> + IIO_EV_DIR_NONE,
> + IIO_EV_TYPE_CHANGE, 0, 0, 0),
> + iio_get_time_ns(indio_dev));
> + ret = IRQ_HANDLED;
> + }
> +
> return ret;
> }
>
Hi Jonathan,
On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Mar 2022 01:11:46 +0530
> Jagath Jog J <[email protected]> wrote:
>
> > Added support for event when there is a detection of step change.
> > INT1 pin is used to interrupt and event is pushed to userspace.
> >
> > Signed-off-by: Jagath Jog J <[email protected]>
>
> These last two patches look fine to me. Simply having the
> event enable the channel makes things simpler.
Means do I need to drop the step _INFO_ENABLE and handle the
enabling and disabling of step channel through the event enable and
disable?
> I briefly wondered if we need to care about sequences like
>
> 1) Enable event
> 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> 3) Disable event.
> 4) Is the channel still enabled?
>
> or the simpler case of whether we should disable the channel if the event is
> disabled and it wasn't otherwise turned on.
>
> However, I can't see a sensible way to do so. Hence I think what you have
> gone with is the best we can do.
>
> Thanks,
>
> Jonathan
Thanks for reviewing the patch series. I will also address all the comments
from Andy in the next patch v3.
Thank you
Jagath
>
> > ---
> > drivers/iio/accel/bma400.h | 2 +
> > drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> > 2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index c9b856b37021..c4ec0cf6dc00 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -39,6 +39,7 @@
> > #define BMA400_INT_STAT0_REG 0x0e
> > #define BMA400_INT_STAT1_REG 0x0f
> > #define BMA400_INT_STAT2_REG 0x10
> > +#define BMA400_INT12_MAP_REG 0x23
> >
> > /* Temperature register */
> > #define BMA400_TEMP_DATA_REG 0x11
> > @@ -54,6 +55,7 @@
> > #define BMA400_STEP_CNT3_REG 0x17
> > #define BMA400_STEP_STAT_REG 0x18
> > #define BMA400_STEP_INT_MSK BIT(0)
> > +#define BMA400_STEP_STAT_MASK GENMASK(9, 8)
> >
> > /*
> > * Read-write configuration registers
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index ec2f9c380bda..aaa104a2698b 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -24,6 +24,7 @@
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > #include <linux/iio/trigger.h>
> > #include <linux/iio/trigger_consumer.h>
> > #include <linux/iio/triggered_buffer.h>
> > @@ -70,6 +71,7 @@ struct bma400_data {
> > int scale;
> > struct iio_trigger *trig;
> > int steps_enabled;
> > + bool step_event_en;
> > /* Correct time stamp alignment */
> > struct {
> > __le16 buff[3];
> > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > { }
> > };
> >
> > +static const struct iio_event_spec bma400_step_detect_event = {
> > + .type = IIO_EV_TYPE_CHANGE,
> > + .dir = IIO_EV_DIR_NONE,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +};
> > +
> > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > .type = IIO_ACCEL, \
> > .modified = 1, \
> > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > BIT(IIO_CHAN_INFO_ENABLE),
> > .scan_index = -1, /* No buffer support */
> > + .event_spec = &bma400_step_detect_event,
> > + .num_event_specs = 1,
> > },
> > IIO_CHAN_SOFT_TIMESTAMP(4),
> > };
> > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > }
> > }
> >
> > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_CHANGE:
> > + return data->step_event_en;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir, int state)
> > +{
> > + int ret;
> > + struct bma400_data *data = iio_priv(indio_dev);
> > +
> > + switch (type) {
> > + case IIO_EV_TYPE_CHANGE:
> > + mutex_lock(&data->mutex);
> > + if (!data->steps_enabled) {
> > + ret = regmap_update_bits(data->regmap,
> > + BMA400_INT_CONFIG1_REG,
> > + BMA400_STEP_INT_MSK,
> > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > + 1));
> > + if (ret)
> > + return ret;
> > + data->steps_enabled = 1;
> > + }
> > +
> > + ret = regmap_update_bits(data->regmap,
> > + BMA400_INT12_MAP_REG,
> > + BMA400_STEP_INT_MSK,
> > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > + state));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + data->step_event_en = state;
> > + return 0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > bool state)
> > {
> > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> > .read_avail = bma400_read_avail,
> > .write_raw = bma400_write_raw,
> > .write_raw_get_fmt = bma400_write_raw_get_fmt,
> > + .read_event_config = bma400_read_event_config,
> > + .write_event_config = bma400_write_event_config,
> > };
> >
> > static const struct iio_trigger_ops bma400_trigger_ops = {
> > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > ret = IRQ_HANDLED;
> > }
> >
> > + if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > + IIO_EV_DIR_NONE,
> > + IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > + iio_get_time_ns(indio_dev));
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > return ret;
> > }
> >
>
On Tue, 29 Mar 2022 02:07:11 +0530
Jagath Jog J <[email protected]> wrote:
> Hi Jonathan,
>
> On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> > On Sun, 27 Mar 2022 01:11:46 +0530
> > Jagath Jog J <[email protected]> wrote:
> >
> > > Added support for event when there is a detection of step change.
> > > INT1 pin is used to interrupt and event is pushed to userspace.
> > >
> > > Signed-off-by: Jagath Jog J <[email protected]>
> >
> > These last two patches look fine to me. Simply having the
> > event enable the channel makes things simpler.
>
> Means do I need to drop the step _INFO_ENABLE and handle the
> enabling and disabling of step channel through the event enable and
> disable?
No. I was trying to say I like the solution you have now.
>
> > I briefly wondered if we need to care about sequences like
> >
> > 1) Enable event
> > 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> > 3) Disable event.
> > 4) Is the channel still enabled?
> >
> > or the simpler case of whether we should disable the channel if the event is
> > disabled and it wasn't otherwise turned on.
> >
> > However, I can't see a sensible way to do so. Hence I think what you have
> > gone with is the best we can do.
> >
> > Thanks,
> >
> > Jonathan
>
> Thanks for reviewing the patch series. I will also address all the comments
> from Andy in the next patch v3.
>
> Thank you
> Jagath
> >
> > > ---
> > > drivers/iio/accel/bma400.h | 2 +
> > > drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> > > 2 files changed, 75 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index c9b856b37021..c4ec0cf6dc00 100644
> > > --- a/drivers/iio/accel/bma400.h
> > > +++ b/drivers/iio/accel/bma400.h
> > > @@ -39,6 +39,7 @@
> > > #define BMA400_INT_STAT0_REG 0x0e
> > > #define BMA400_INT_STAT1_REG 0x0f
> > > #define BMA400_INT_STAT2_REG 0x10
> > > +#define BMA400_INT12_MAP_REG 0x23
> > >
> > > /* Temperature register */
> > > #define BMA400_TEMP_DATA_REG 0x11
> > > @@ -54,6 +55,7 @@
> > > #define BMA400_STEP_CNT3_REG 0x17
> > > #define BMA400_STEP_STAT_REG 0x18
> > > #define BMA400_STEP_INT_MSK BIT(0)
> > > +#define BMA400_STEP_STAT_MASK GENMASK(9, 8)
> > >
> > > /*
> > > * Read-write configuration registers
> > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > index ec2f9c380bda..aaa104a2698b 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -24,6 +24,7 @@
> > > #include <linux/iio/iio.h>
> > > #include <linux/iio/sysfs.h>
> > > #include <linux/iio/buffer.h>
> > > +#include <linux/iio/events.h>
> > > #include <linux/iio/trigger.h>
> > > #include <linux/iio/trigger_consumer.h>
> > > #include <linux/iio/triggered_buffer.h>
> > > @@ -70,6 +71,7 @@ struct bma400_data {
> > > int scale;
> > > struct iio_trigger *trig;
> > > int steps_enabled;
> > > + bool step_event_en;
> > > /* Correct time stamp alignment */
> > > struct {
> > > __le16 buff[3];
> > > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > > { }
> > > };
> > >
> > > +static const struct iio_event_spec bma400_step_detect_event = {
> > > + .type = IIO_EV_TYPE_CHANGE,
> > > + .dir = IIO_EV_DIR_NONE,
> > > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > +};
> > > +
> > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > .type = IIO_ACCEL, \
> > > .modified = 1, \
> > > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > BIT(IIO_CHAN_INFO_ENABLE),
> > > .scan_index = -1, /* No buffer support */
> > > + .event_spec = &bma400_step_detect_event,
> > > + .num_event_specs = 1,
> > > },
> > > IIO_CHAN_SOFT_TIMESTAMP(4),
> > > };
> > > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > }
> > > }
> > >
> > > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir)
> > > +{
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_CHANGE:
> > > + return data->step_event_en;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir, int state)
> > > +{
> > > + int ret;
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > +
> > > + switch (type) {
> > > + case IIO_EV_TYPE_CHANGE:
> > > + mutex_lock(&data->mutex);
> > > + if (!data->steps_enabled) {
> > > + ret = regmap_update_bits(data->regmap,
> > > + BMA400_INT_CONFIG1_REG,
> > > + BMA400_STEP_INT_MSK,
> > > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > > + 1));
> > > + if (ret)
> > > + return ret;
> > > + data->steps_enabled = 1;
> > > + }
> > > +
> > > + ret = regmap_update_bits(data->regmap,
> > > + BMA400_INT12_MAP_REG,
> > > + BMA400_STEP_INT_MSK,
> > > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > > + state));
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + data->step_event_en = state;
> > > + return 0;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > > bool state)
> > > {
> > > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> > > .read_avail = bma400_read_avail,
> > > .write_raw = bma400_write_raw,
> > > .write_raw_get_fmt = bma400_write_raw_get_fmt,
> > > + .read_event_config = bma400_read_event_config,
> > > + .write_event_config = bma400_write_event_config,
> > > };
> > >
> > > static const struct iio_trigger_ops bma400_trigger_ops = {
> > > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > > ret = IRQ_HANDLED;
> > > }
> > >
> > > + if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > > + iio_push_event(indio_dev,
> > > + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > > + IIO_EV_DIR_NONE,
> > > + IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > > + iio_get_time_ns(indio_dev));
> > > + ret = IRQ_HANDLED;
> > > + }
> > > +
> > > return ret;
> > > }
> > >
> >
On Sat, Apr 02, 2022 at 05:37:07PM +0100, Jonathan Cameron wrote:
> On Tue, 29 Mar 2022 02:07:11 +0530
> Jagath Jog J <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > On Sun, Mar 27, 2022 at 05:50:36PM +0100, Jonathan Cameron wrote:
> > > On Sun, 27 Mar 2022 01:11:46 +0530
> > > Jagath Jog J <[email protected]> wrote:
> > >
> > > > Added support for event when there is a detection of step change.
> > > > INT1 pin is used to interrupt and event is pushed to userspace.
> > > >
> > > > Signed-off-by: Jagath Jog J <[email protected]>
> > >
> > > These last two patches look fine to me. Simply having the
> > > event enable the channel makes things simpler.
> >
> > Means do I need to drop the step _INFO_ENABLE and handle the
> > enabling and disabling of step channel through the event enable and
> > disable?
>
> No. I was trying to say I like the solution you have now.
Thanks, I will keep the solution same.
Currently I am testing the BMA400 activity events like STILL, WALKING,
RUNNING and also BMA400 acceleration threshold events, soon I will send the
next v3 patch series by including these events.
>
> >
> > > I briefly wondered if we need to care about sequences like
> > >
> > > 1) Enable event
> > > 2) Enable channel (already enabled, but perhaps this indicates separate intent)
> > > 3) Disable event.
> > > 4) Is the channel still enabled?
> > >
> > > or the simpler case of whether we should disable the channel if the event is
> > > disabled and it wasn't otherwise turned on.
> > >
> > > However, I can't see a sensible way to do so. Hence I think what you have
> > > gone with is the best we can do.
> > >
> > > Thanks,
> > >
> > > Jonathan
> >
> > Thanks for reviewing the patch series. I will also address all the comments
> > from Andy in the next patch v3.
> >
> > Thank you
> > Jagath
> > >
> > > > ---
> > > > drivers/iio/accel/bma400.h | 2 +
> > > > drivers/iio/accel/bma400_core.c | 73 +++++++++++++++++++++++++++++++++
> > > > 2 files changed, 75 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > index c9b856b37021..c4ec0cf6dc00 100644
> > > > --- a/drivers/iio/accel/bma400.h
> > > > +++ b/drivers/iio/accel/bma400.h
> > > > @@ -39,6 +39,7 @@
> > > > #define BMA400_INT_STAT0_REG 0x0e
> > > > #define BMA400_INT_STAT1_REG 0x0f
> > > > #define BMA400_INT_STAT2_REG 0x10
> > > > +#define BMA400_INT12_MAP_REG 0x23
> > > >
> > > > /* Temperature register */
> > > > #define BMA400_TEMP_DATA_REG 0x11
> > > > @@ -54,6 +55,7 @@
> > > > #define BMA400_STEP_CNT3_REG 0x17
> > > > #define BMA400_STEP_STAT_REG 0x18
> > > > #define BMA400_STEP_INT_MSK BIT(0)
> > > > +#define BMA400_STEP_STAT_MASK GENMASK(9, 8)
> > > >
> > > > /*
> > > > * Read-write configuration registers
> > > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > > index ec2f9c380bda..aaa104a2698b 100644
> > > > --- a/drivers/iio/accel/bma400_core.c
> > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > @@ -24,6 +24,7 @@
> > > > #include <linux/iio/iio.h>
> > > > #include <linux/iio/sysfs.h>
> > > > #include <linux/iio/buffer.h>
> > > > +#include <linux/iio/events.h>
> > > > #include <linux/iio/trigger.h>
> > > > #include <linux/iio/trigger_consumer.h>
> > > > #include <linux/iio/triggered_buffer.h>
> > > > @@ -70,6 +71,7 @@ struct bma400_data {
> > > > int scale;
> > > > struct iio_trigger *trig;
> > > > int steps_enabled;
> > > > + bool step_event_en;
> > > > /* Correct time stamp alignment */
> > > > struct {
> > > > __le16 buff[3];
> > > > @@ -167,6 +169,12 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> > > > { }
> > > > };
> > > >
> > > > +static const struct iio_event_spec bma400_step_detect_event = {
> > > > + .type = IIO_EV_TYPE_CHANGE,
> > > > + .dir = IIO_EV_DIR_NONE,
> > > > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > > +};
> > > > +
> > > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > > .type = IIO_ACCEL, \
> > > > .modified = 1, \
> > > > @@ -209,6 +217,8 @@ static const struct iio_chan_spec bma400_channels[] = {
> > > > .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> > > > BIT(IIO_CHAN_INFO_ENABLE),
> > > > .scan_index = -1, /* No buffer support */
> > > > + .event_spec = &bma400_step_detect_event,
> > > > + .num_event_specs = 1,
> > > > },
> > > > IIO_CHAN_SOFT_TIMESTAMP(4),
> > > > };
> > > > @@ -878,6 +888,58 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> > > > }
> > > > }
> > > >
> > > > +static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir)
> > > > +{
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > +
> > > > + switch (type) {
> > > > + case IIO_EV_TYPE_CHANGE:
> > > > + return data->step_event_en;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > +static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir, int state)
> > > > +{
> > > > + int ret;
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > +
> > > > + switch (type) {
> > > > + case IIO_EV_TYPE_CHANGE:
> > > > + mutex_lock(&data->mutex);
> > > > + if (!data->steps_enabled) {
> > > > + ret = regmap_update_bits(data->regmap,
> > > > + BMA400_INT_CONFIG1_REG,
> > > > + BMA400_STEP_INT_MSK,
> > > > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > > > + 1));
> > > > + if (ret)
> > > > + return ret;
> > > > + data->steps_enabled = 1;
> > > > + }
> > > > +
> > > > + ret = regmap_update_bits(data->regmap,
> > > > + BMA400_INT12_MAP_REG,
> > > > + BMA400_STEP_INT_MSK,
> > > > + FIELD_PREP(BMA400_STEP_INT_MSK,
> > > > + state));
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + data->step_event_en = state;
> > > > + return 0;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > > > bool state)
> > > > {
> > > > @@ -910,6 +972,8 @@ static const struct iio_info bma400_info = {
> > > > .read_avail = bma400_read_avail,
> > > > .write_raw = bma400_write_raw,
> > > > .write_raw_get_fmt = bma400_write_raw_get_fmt,
> > > > + .read_event_config = bma400_read_event_config,
> > > > + .write_event_config = bma400_write_event_config,
> > > > };
> > > >
> > > > static const struct iio_trigger_ops bma400_trigger_ops = {
> > > > @@ -965,6 +1029,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > > > ret = IRQ_HANDLED;
> > > > }
> > > >
> > > > + if (FIELD_GET(BMA400_STEP_STAT_MASK, le16_to_cpu(status))) {
> > > > + iio_push_event(indio_dev,
> > > > + IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > > > + IIO_EV_DIR_NONE,
> > > > + IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > > > + iio_get_time_ns(indio_dev));
> > > > + ret = IRQ_HANDLED;
> > > > + }
> > > > +
> > > > return ret;
> > > > }
> > > >
> > >
>