* [PATCH v2 1/5] iio: accel: bma400: Fix the scale min and max macro values
2022-03-26 19:41 [PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step Jagath Jog J
@ 2022-03-26 19:41 ` Jagath Jog J
2022-03-27 16:31 ` Jonathan Cameron
2022-03-26 19:41 ` [PATCH v2 2/5] iio: accel: bma400: conversion to device-managed function Jagath Jog J
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jagath Jog J @ 2022-03-26 19:41 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Changing the scale macro values to match the bma400 sensitivity
for 1 LSB of all the available ranges.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/bma400.h | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index c4c8d74155c2..190366debdb3 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -83,8 +83,26 @@
#define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
#define BMA400_ACC_ODR_MIN_HZ 12
-#define BMA400_SCALE_MIN 38357
-#define BMA400_SCALE_MAX 306864
+/* BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
+ * converting to micro values for +-2g range.
+ *
+ * For +-2g - 1 LSB = 0.976562 milli g = 0.009576 m/s^2
+ * For +-4g - 1 LSB = 1.953125 milli g = 0.019153 m/s^2
+ * For +-16g - 1 LSB = 7.8125 milli g = 0.076614 m/s^2
+ *
+ * The raw value which is used to select the different ranges is determined
+ * by the first bit set position from the scale value, so BMA400_SCALE_MIN
+ * should be odd.
+ *
+ * Scale values for +-2g, +-4g, +-8g and +-16g is populated into bma400_scales
+ * array by left shifting BMA400_SCALE_MIN.
+ * eg:
+ * To select +-2g = 9577 << 0 = raw value to write is 0.
+ * To select +-8g = 9577 << 2 = raw value to write is 2.
+ * To select +-16g = 9677 << 3 = raw value to write is 3.
+ */
+#define BMA400_SCALE_MIN 9577
+#define BMA400_SCALE_MAX 76617
#define BMA400_NUM_REGULATORS 2
#define BMA400_VDD_REGULATOR 0
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/5] iio: accel: bma400: Fix the scale min and max macro values
2022-03-26 19:41 ` [PATCH v2 1/5] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
@ 2022-03-27 16:31 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2022-03-27 16:31 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Sun, 27 Mar 2022 01:11:42 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Changing the scale macro values to match the bma400 sensitivity
> for 1 LSB of all the available ranges.
>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
Hi Jagath,
> ---
> drivers/iio/accel/bma400.h | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index c4c8d74155c2..190366debdb3 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -83,8 +83,26 @@
> #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> #define BMA400_ACC_ODR_MIN_HZ 12
>
> -#define BMA400_SCALE_MIN 38357
> -#define BMA400_SCALE_MAX 306864
> +/* BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
Multiline comment syntax in IIO is
/*
* BMA400....
> + * converting to micro values for +-2g range.
> + *
> + * For +-2g - 1 LSB = 0.976562 milli g = 0.009576 m/s^2
> + * For +-4g - 1 LSB = 1.953125 milli g = 0.019153 m/s^2
> + * For +-16g - 1 LSB = 7.8125 milli g = 0.076614 m/s^2
> + *
> + * The raw value which is used to select the different ranges is determined
> + * by the first bit set position from the scale value, so BMA400_SCALE_MIN
> + * should be odd.
> + *
> + * Scale values for +-2g, +-4g, +-8g and +-16g is populated into bma400_scales
> + * array by left shifting BMA400_SCALE_MIN.
> + * eg:
> + * To select +-2g = 9577 << 0 = raw value to write is 0.
> + * To select +-8g = 9577 << 2 = raw value to write is 2.
> + * To select +-16g = 9677 << 3 = raw value to write is 3.
9667?
> + */
> +#define BMA400_SCALE_MIN 9577
> +#define BMA400_SCALE_MAX 76617
>
> #define BMA400_NUM_REGULATORS 2
> #define BMA400_VDD_REGULATOR 0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/5] iio: accel: bma400: conversion to device-managed function
2022-03-26 19:41 [PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step Jagath Jog J
2022-03-26 19:41 ` [PATCH v2 1/5] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
@ 2022-03-26 19:41 ` Jagath Jog J
2022-03-27 16:35 ` Jonathan Cameron
2022-03-26 19:41 ` [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support Jagath Jog J
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: Jagath Jog J @ 2022-03-26 19:41 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
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 <jagathjog1996@gmail.com>
---
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 <dan@dlrobertson.com>");
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/5] iio: accel: bma400: conversion to device-managed function
2022-03-26 19:41 ` [PATCH v2 2/5] iio: accel: bma400: conversion to device-managed function Jagath Jog J
@ 2022-03-27 16:35 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2022-03-27 16:35 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Sun, 27 Mar 2022 01:11:43 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 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.
I would state here that previously the bma400 was not put into power down
mode in some error paths in probe where it now is, but that should cause no
harm.
>
> 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 <jagathjog1996@gmail.com>
one other minor thing inline.
Jonathan
> ---
> 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));
Drop the check on ret out of the locked region. No reason for it to be done
under the lock so generally nice not to do so. Also matches the previous
ordering so there shouldnt' be any questions about it.
> + 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);
> -
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
2022-03-26 19:41 [PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step Jagath Jog J
2022-03-26 19:41 ` [PATCH v2 1/5] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
2022-03-26 19:41 ` [PATCH v2 2/5] iio: accel: bma400: conversion to device-managed function Jagath Jog J
@ 2022-03-26 19:41 ` Jagath Jog J
2022-03-27 16:45 ` Jonathan Cameron
2022-03-27 19:45 ` Andy Shevchenko
2022-03-26 19:41 ` [PATCH v2 4/5] iio: accel: bma400: Add separate channel for step counter Jagath Jog J
2022-03-26 19:41 ` [PATCH v2 5/5] iio: accel: bma400: Add step change event Jagath Jog J
4 siblings, 2 replies; 18+ messages in thread
From: Jagath Jog J @ 2022-03-26 19:41 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Added trigger buffer support to read continuous acceleration
data from device with data ready interrupt which is mapped
to INT1 pin.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/Kconfig | 2 +
drivers/iio/accel/bma400.h | 10 +-
drivers/iio/accel/bma400_core.c | 162 ++++++++++++++++++++++++++++++--
drivers/iio/accel/bma400_i2c.c | 2 +-
drivers/iio/accel/bma400_spi.c | 2 +-
5 files changed, 168 insertions(+), 10 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 c1b3dbfbd98f..24d2b705343a 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
@@ -110,6 +117,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 dc273381a0a2..fa3f4b5f229f 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -11,16 +11,22 @@
* - Create channel for sensor time
*/
+#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/device.h>
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.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 +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 {
+ __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 +165,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
{ }
};
-#define BMA400_ACC_CHANNEL(_axis) { \
+#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
.channel2 = IIO_MOD_##_axis, \
@@ -164,17 +177,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.ext_info = bma400_ext_info, \
+ .scan_index = _index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
}
static const struct iio_chan_spec bma400_channels[] = {
- BMA400_ACC_CHANNEL(X),
- BMA400_ACC_CHANNEL(Y),
- BMA400_ACC_CHANNEL(Z),
+ BMA400_ACC_CHANNEL(0, X),
+ BMA400_ACC_CHANNEL(1, Y),
+ BMA400_ACC_CHANNEL(2, Z),
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_index = 3,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 8,
+ .storagebits = 8,
+ .endianness = IIO_LE,
+ },
},
+ IIO_CHAN_SOFT_TIMESTAMP(4),
};
static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
@@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data)
if (ret)
return ret;
+ /* Configure INT1 pin to open drain */
+ ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
+ if (ret)
+ return ret;
/*
* Once the interrupt engine is supported we might use the
* data_src_reg, but for now ensure this is set to the
@@ -807,6 +839,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,
@@ -814,7 +873,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);
+ irqreturn_t ret = IRQ_NONE;
+ __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);
+ ret = IRQ_HANDLED;
+ }
+
+ return ret;
+}
+
+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 +957,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(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[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
2022-03-26 19:41 ` [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support Jagath Jog J
@ 2022-03-27 16:45 ` Jonathan Cameron
2022-03-28 18:38 ` Jagath Jog J
2022-03-27 19:45 ` Andy Shevchenko
1 sibling, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2022-03-27 16:45 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Sun, 27 Mar 2022 01:11:44 +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>
Hi Jagath,
Just a few small things noticed on this read through.
Thanks,
Jonathan
> ---
> drivers/iio/accel/Kconfig | 2 +
> drivers/iio/accel/bma400.h | 10 +-
> drivers/iio/accel/bma400_core.c | 162 ++++++++++++++++++++++++++++++--
> drivers/iio/accel/bma400_i2c.c | 2 +-
> drivers/iio/accel/bma400_spi.c | 2 +-
> 5 files changed, 168 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index dc273381a0a2..fa3f4b5f229f 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -11,16 +11,22 @@
> * - Create channel for sensor time
> */
>
> +#include <linux/bitfield.h>
> #include <linux/bitops.h>
> #include <linux/device.h>
> -#include <linux/iio/iio.h>
> -#include <linux/iio/sysfs.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Is iio/sysfs.h actually used? It rarely is these days as it contains
the infrastructure for custom attributes and we try not to use any
of those anymore.
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
This reorganization of headers is good but shouldn't be in this patch.
Add an earlier patch in the series to move the existing pair down here
before this patch then adds the new ones.
...
>
> static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> @@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data)
> if (ret)
> return ret;
>
> + /* Configure INT1 pin to open drain */
> + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> + if (ret)
> + return ret;
> /*
> * Once the interrupt engine is supported we might use the
> * data_src_reg, but for now ensure this is set to the
> @@ -807,6 +839,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;
return regmap_update_bits()...
> +}
...
> +static irqreturn_t bma400_interrupt(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct bma400_data *data = iio_priv(indio_dev);
> + irqreturn_t ret = IRQ_NONE;
> + __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);
> + ret = IRQ_HANDLED;
Preference for this style
return IRQ_HANDLED;
> + }
> +
return IRQ_NONE;
and don't initialize above.
> + return ret;
> +}
> +
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
2022-03-27 16:45 ` Jonathan Cameron
@ 2022-03-28 18:38 ` Jagath Jog J
0 siblings, 0 replies; 18+ messages in thread
From: Jagath Jog J @ 2022-03-28 18:38 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
Hi Jonathan,
On Sun, Mar 27, 2022 at 05:45:45PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Mar 2022 01:11:44 +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>
> Hi Jagath,
>
> Just a few small things noticed on this read through.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/accel/Kconfig | 2 +
> > drivers/iio/accel/bma400.h | 10 +-
> > drivers/iio/accel/bma400_core.c | 162 ++++++++++++++++++++++++++++++--
> > drivers/iio/accel/bma400_i2c.c | 2 +-
> > drivers/iio/accel/bma400_spi.c | 2 +-
> > 5 files changed, 168 insertions(+), 10 deletions(-)
> >
>
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index dc273381a0a2..fa3f4b5f229f 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -11,16 +11,22 @@
> > * - Create channel for sensor time
> > */
> >
> > +#include <linux/bitfield.h>
> > #include <linux/bitops.h>
> > #include <linux/device.h>
> > -#include <linux/iio/iio.h>
> > -#include <linux/iio/sysfs.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> >
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> Is iio/sysfs.h actually used? It rarely is these days as it contains
> the infrastructure for custom attributes and we try not to use any
> of those anymore.
>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> This reorganization of headers is good but shouldn't be in this patch.
> Add an earlier patch in the series to move the existing pair down here
> before this patch then adds the new ones.
Sure I will do the reorganization of headers in the seperate patch.
>
>
> ...
>
> >
> > static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> > @@ -659,6 +687,10 @@ static int bma400_init(struct bma400_data *data)
> > if (ret)
> > return ret;
> >
> > + /* Configure INT1 pin to open drain */
> > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> > + if (ret)
> > + return ret;
> > /*
> > * Once the interrupt engine is supported we might use the
> > * data_src_reg, but for now ensure this is set to the
> > @@ -807,6 +839,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;
>
> return regmap_update_bits()...
>
> > +}
>
> ...
>
>
> > +static irqreturn_t bma400_interrupt(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + irqreturn_t ret = IRQ_NONE;
> > + __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);
> > + ret = IRQ_HANDLED;
> Preference for this style
> return IRQ_HANDLED;
> > + }
> > +
> return IRQ_NONE;
> and don't initialize above.
Sure I will make these changes and I will try to handle the events before
the data ready since step interrupt will occur less frequently compared to
data ready interrupts.
>
> > + return ret;
> > +}
> > +
>
>
Thank you,
Jagath
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
2022-03-26 19:41 ` [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support Jagath Jog J
2022-03-27 16:45 ` Jonathan Cameron
@ 2022-03-27 19:45 ` Andy Shevchenko
2022-03-28 17:02 ` Jonathan Cameron
1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2022-03-27 19:45 UTC (permalink / raw)
To: Jagath Jog J
Cc: Dan Robertson, Jonathan Cameron, linux-iio, Linux Kernel Mailing List
On Sat, Mar 26, 2022 at 9:41 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.
...
> + struct {
> + __le16 buff[3];
In my (mostly review) practice it's rare that sensors operate in LE mode.
Please, double check that.
> + u8 temperature;
> + s64 ts __aligned(8);
> + } buffer ____cacheline_aligned;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support
2022-03-27 19:45 ` Andy Shevchenko
@ 2022-03-28 17:02 ` Jonathan Cameron
0 siblings, 0 replies; 18+ messages in thread
From: Jonathan Cameron @ 2022-03-28 17:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jagath Jog J, Dan Robertson, linux-iio, Linux Kernel Mailing List
On Sun, 27 Mar 2022 22:45:16 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Sat, Mar 26, 2022 at 9:41 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.
>
> ...
>
> > + struct {
>
> > + __le16 buff[3];
>
> In my (mostly review) practice it's rare that sensors operate in LE mode.
> Please, double check that.
Just for giggles, we've had sensors that had most channels little endian
but with one that was big endian... Thankfully this isn't one of those.
I checked the datasheet and whilst indeed less common, these do appear to
be little endian.
https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bma400-ds000.pdf
page 50
ACC_X_LSB at address 0x04
ACC_X_MSB at address 0x05
>
> > + u8 temperature;
> > + s64 ts __aligned(8);
> > + } buffer ____cacheline_aligned;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/5] iio: accel: bma400: Add separate channel for step counter
2022-03-26 19:41 [PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step Jagath Jog J
` (2 preceding siblings ...)
2022-03-26 19:41 ` [PATCH v2 3/5] iio: accel: bma400: Add triggered buffer support Jagath Jog J
@ 2022-03-26 19:41 ` Jagath Jog J
2022-03-27 19:43 ` Andy Shevchenko
2022-03-26 19:41 ` [PATCH v2 5/5] iio: accel: bma400: Add step change event Jagath Jog J
4 siblings, 1 reply; 18+ messages in thread
From: Jagath Jog J @ 2022-03-26 19:41 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Added channel for step counter which can be enable or disable
through the sysfs interface.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/bma400.h | 1 +
drivers/iio/accel/bma400_core.c | 47 ++++++++++++++++++++++++++++++---
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 24d2b705343a..c9b856b37021 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -53,6 +53,7 @@
#define BMA400_STEP_CNT1_REG 0x16
#define BMA400_STEP_CNT3_REG 0x17
#define BMA400_STEP_STAT_REG 0x18
+#define BMA400_STEP_INT_MSK BIT(0)
/*
* Read-write configuration registers
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index fa3f4b5f229f..ec2f9c380bda 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -19,6 +19,7 @@
#include <linux/mutex.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <asm/unaligned.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
@@ -68,6 +69,7 @@ struct bma400_data {
int oversampling_ratio;
int scale;
struct iio_trigger *trig;
+ int steps_enabled;
/* Correct time stamp alignment */
struct {
__le16 buff[3];
@@ -202,6 +204,12 @@ static const struct iio_chan_spec bma400_channels[] = {
.endianness = IIO_LE,
},
},
+ {
+ .type = IIO_STEPS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_ENABLE),
+ .scan_index = -1, /* No buffer support */
+ },
IIO_CHAN_SOFT_TIMESTAMP(4),
};
@@ -706,13 +714,28 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
{
struct bma400_data *data = iio_priv(indio_dev);
int ret;
+ u8 steps_raw[3];
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
- mutex_lock(&data->mutex);
- ret = bma400_get_temp_reg(data, val, val2);
- mutex_unlock(&data->mutex);
- return ret;
+ switch (chan->type) {
+ case IIO_TEMP:
+ mutex_lock(&data->mutex);
+ ret = bma400_get_temp_reg(data, val, val2);
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_STEPS:
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_read(data->regmap, BMA400_STEP_CNT0_REG,
+ &steps_raw, sizeof(steps_raw));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ *val = get_unaligned_le24(steps_raw);
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_RAW:
mutex_lock(&data->mutex);
ret = bma400_get_accel_reg(data, chan, val);
@@ -753,6 +776,9 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
*val = data->oversampling_ratio;
return IIO_VAL_INT;
+ case IIO_CHAN_INFO_ENABLE:
+ *val = data->steps_enabled;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -818,6 +844,17 @@ static int bma400_write_raw(struct iio_dev *indio_dev,
ret = bma400_set_accel_oversampling_ratio(data, val);
mutex_unlock(&data->mutex);
return ret;
+ case IIO_CHAN_INFO_ENABLE:
+ if (data->steps_enabled == val)
+ return 0;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
+ BMA400_STEP_INT_MSK,
+ FIELD_PREP(BMA400_STEP_INT_MSK, !!val));
+ mutex_unlock(&data->mutex);
+ data->steps_enabled = val;
+ return ret;
default:
return -EINVAL;
}
@@ -834,6 +871,8 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
return IIO_VAL_INT;
+ case IIO_CHAN_INFO_ENABLE:
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v2 5/5] iio: accel: bma400: Add step change event
2022-03-26 19:41 [PATCH v2 0/5] iio: accel: bma400: Add support for buffer and step Jagath Jog J
` (3 preceding siblings ...)
2022-03-26 19:41 ` [PATCH v2 4/5] iio: accel: bma400: Add separate channel for step counter Jagath Jog J
@ 2022-03-26 19:41 ` Jagath Jog J
2022-03-27 16:50 ` Jonathan Cameron
4 siblings, 1 reply; 18+ messages in thread
From: Jagath Jog J @ 2022-03-26 19:41 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
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 <jagathjog1996@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event
2022-03-26 19:41 ` [PATCH v2 5/5] iio: accel: bma400: Add step change event Jagath Jog J
@ 2022-03-27 16:50 ` Jonathan Cameron
2022-03-28 20:37 ` Jagath Jog J
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2022-03-27 16:50 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Sun, 27 Mar 2022 01:11:46 +0530
Jagath Jog J <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com>
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;
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event
2022-03-27 16:50 ` Jonathan Cameron
@ 2022-03-28 20:37 ` Jagath Jog J
2022-04-02 16:37 ` Jonathan Cameron
0 siblings, 1 reply; 18+ messages in thread
From: Jagath Jog J @ 2022-03-28 20:37 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
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 <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com>
>
> 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;
> > }
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event
2022-03-28 20:37 ` Jagath Jog J
@ 2022-04-02 16:37 ` Jonathan Cameron
2022-04-03 7:48 ` Jagath Jog J
0 siblings, 1 reply; 18+ messages in thread
From: Jonathan Cameron @ 2022-04-02 16:37 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 29 Mar 2022 02:07:11 +0530
Jagath Jog J <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com>
> >
> > 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;
> > > }
> > >
> >
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 5/5] iio: accel: bma400: Add step change event
2022-04-02 16:37 ` Jonathan Cameron
@ 2022-04-03 7:48 ` Jagath Jog J
0 siblings, 0 replies; 18+ messages in thread
From: Jagath Jog J @ 2022-04-03 7:48 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
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 <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com> 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 <jagathjog1996@gmail.com>
> > >
> > > 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;
> > > > }
> > > >
> > >
>
^ permalink raw reply [flat|nested] 18+ messages in thread