* [PATCH v3 1/9] iio: accel: bma400: Fix the scale min and max macro values
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-12 8:59 ` Andy Shevchenko
2022-04-11 20:31 ` [PATCH v3 2/9] iio: accel: bma400: Reordering of header files Jagath Jog J
` (7 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 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 | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index c4c8d74155c2..5d6a1976503f 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -83,8 +83,27 @@
#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 = 9577 << 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] 28+ messages in thread* Re: [PATCH v3 1/9] iio: accel: bma400: Fix the scale min and max macro values
2022-04-11 20:31 ` [PATCH v3 1/9] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
@ 2022-04-12 8:59 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2022-04-12 8:59 UTC (permalink / raw)
To: Jagath Jog J
Cc: Dan Robertson, Jonathan Cameron, linux-iio,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 11:31 PM 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.
Shouldn't this carry a Fixes tag?
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
> drivers/iio/accel/bma400.h | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index c4c8d74155c2..5d6a1976503f 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -83,8 +83,27 @@
> #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
are populated
> + * array by left shifting BMA400_SCALE_MIN.
> + * eg:
e.g.:
> + * 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 = 9577 << 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
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 2/9] iio: accel: bma400: Reordering of header files
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
2022-04-11 20:31 ` [PATCH v3 1/9] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-12 9:00 ` Andy Shevchenko
2022-04-11 20:31 ` [PATCH v3 3/9] iio: accel: bma400: conversion to device-managed function Jagath Jog J
` (6 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Reordering of header files and removing the iio/sysfs.h since
custom attributes are not being used in the driver.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/bma400_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 043002fe6f63..25ad1f7339bc 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -13,14 +13,14 @@
#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 "bma400.h"
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/9] iio: accel: bma400: Reordering of header files
2022-04-11 20:31 ` [PATCH v3 2/9] iio: accel: bma400: Reordering of header files Jagath Jog J
@ 2022-04-12 9:00 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2022-04-12 9:00 UTC (permalink / raw)
To: Jagath Jog J
Cc: Dan Robertson, Jonathan Cameron, linux-iio,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Reordering of header files and removing the iio/sysfs.h since
> custom attributes are not being used in the driver.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
> drivers/iio/accel/bma400_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 043002fe6f63..25ad1f7339bc 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -13,14 +13,14 @@
>
> #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 "bma400.h"
>
> /*
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/9] iio: accel: bma400: conversion to device-managed function
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
2022-04-11 20:31 ` [PATCH v3 1/9] iio: accel: bma400: Fix the scale min and max macro values Jagath Jog J
2022-04-11 20:31 ` [PATCH v3 2/9] iio: accel: bma400: Reordering of header files Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-12 9:04 ` Andy Shevchenko
2022-04-11 20:31 ` [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support Jagath Jog J
` (5 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 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. 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>
---
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 | 6 ---
4 files changed, 38 insertions(+), 55 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 5d6a1976503f..8dbf85eeb005 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -113,6 +113,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 25ad1f7339bc..07674d89d978 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);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
+ ERR_PTR(ret));
+}
+
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_NS(bma400_probe, IIO_BMA400);
-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_NS(bma400_remove, IIO_BMA400);
-
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 da104ffd3fe0..4f6e01a3b3a1 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 51f23bdc0ea5..28e240400a3f 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -87,11 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
return bma400_probe(&spi->dev, regmap, id->name);
}
-static void bma400_spi_remove(struct spi_device *spi)
-{
- bma400_remove(&spi->dev);
-}
-
static const struct spi_device_id bma400_spi_ids[] = {
{ "bma400", 0 },
{ }
@@ -110,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] 28+ messages in thread* Re: [PATCH v3 3/9] iio: accel: bma400: conversion to device-managed function
2022-04-11 20:31 ` [PATCH v3 3/9] iio: accel: bma400: conversion to device-managed function Jagath Jog J
@ 2022-04-12 9:04 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2022-04-12 9:04 UTC (permalink / raw)
To: Jagath Jog J
Cc: Dan Robertson, Jonathan Cameron, linux-iio,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> This is a conversion to device-managed by using devm_iio_device_register
devm_iio_device_register()
> inside probe function. 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
devm_iio_device_register()
> used to automatically unregister on driver detach.
LGTM,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 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 | 6 ---
> 4 files changed, 38 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 5d6a1976503f..8dbf85eeb005 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -113,6 +113,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 25ad1f7339bc..07674d89d978 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);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> + ERR_PTR(ret));
> +}
> +
> 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_NS(bma400_probe, IIO_BMA400);
>
> -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_NS(bma400_remove, IIO_BMA400);
> -
> 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 da104ffd3fe0..4f6e01a3b3a1 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 51f23bdc0ea5..28e240400a3f 100644
> --- a/drivers/iio/accel/bma400_spi.c
> +++ b/drivers/iio/accel/bma400_spi.c
> @@ -87,11 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
> return bma400_probe(&spi->dev, regmap, id->name);
> }
>
> -static void bma400_spi_remove(struct spi_device *spi)
> -{
> - bma400_remove(&spi->dev);
> -}
> -
> static const struct spi_device_id bma400_spi_ids[] = {
> { "bma400", 0 },
> { }
> @@ -110,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
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (2 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 3/9] iio: accel: bma400: conversion to device-managed function Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-12 9:12 ` Andy Shevchenko
2022-04-16 16:38 ` Jonathan Cameron
2022-04-11 20:31 ` [PATCH v3 5/9] iio: accel: bma400: Add separate channel for step counter Jagath Jog J
` (4 subsequent siblings)
8 siblings, 2 replies; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 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 | 153 ++++++++++++++++++++++++++++++--
drivers/iio/accel/bma400_i2c.c | 2 +-
drivers/iio/accel/bma400_spi.c | 2 +-
5 files changed, 161 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index eac3f02662ae..958097814232 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -204,6 +204,8 @@ config BMA220
config BMA400
tristate "Bosch BMA400 3-Axis Accelerometer Driver"
select REGMAP
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
select BMA400_I2C if I2C
select BMA400_SPI if SPI
help
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 8dbf85eeb005..a7482a66b36b 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -62,6 +62,13 @@
#define BMA400_ACC_CONFIG2_REG 0x1b
#define BMA400_CMD_REG 0x7e
+/* Interrupt registers */
+#define BMA400_INT_CONFIG0_REG 0x1f
+#define BMA400_INT_CONFIG1_REG 0x20
+#define BMA400_INT1_MAP_REG 0x21
+#define BMA400_INT_IO_CTRL_REG 0x24
+#define BMA400_INT_DRDY_MSK BIT(7)
+
/* Chip ID of BMA 400 devices found in the chip ID register. */
#define BMA400_ID_REG_VAL 0x90
@@ -111,6 +118,7 @@
extern const struct regmap_config bma400_regmap_config;
-int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
+int bma400_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name);
#endif
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 07674d89d978..b7b2b67aef31 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -11,6 +11,7 @@
* - Create channel for sensor time
*/
+#include <linux/bitfield.h>
#include <linux/bitops.h>
#include <linux/device.h>
#include <linux/kernel.h>
@@ -20,6 +21,10 @@
#include <linux/regulator/consumer.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#include "bma400.h"
@@ -61,6 +66,13 @@ struct bma400_data {
struct bma400_sample_freq sample_freq;
int oversampling_ratio;
int scale;
+ struct iio_trigger *trig;
+ /* Correct time stamp alignment */
+ struct {
+ __le16 buff[3];
+ u8 temperature;
+ s64 ts __aligned(8);
+ } buffer ____cacheline_aligned;
};
static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
@@ -152,7 +164,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
{ }
};
-#define BMA400_ACC_CHANNEL(_axis) { \
+#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
.channel2 = IIO_MOD_##_axis, \
@@ -164,17 +176,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
.ext_info = bma400_ext_info, \
+ .scan_index = _index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
}
static const struct iio_chan_spec bma400_channels[] = {
- BMA400_ACC_CHANNEL(X),
- BMA400_ACC_CHANNEL(Y),
- BMA400_ACC_CHANNEL(Z),
+ BMA400_ACC_CHANNEL(0, X),
+ BMA400_ACC_CHANNEL(1, Y),
+ BMA400_ACC_CHANNEL(2, Z),
{
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_index = 3,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 8,
+ .storagebits = 8,
+ .endianness = IIO_LE,
+ },
},
+ IIO_CHAN_SOFT_TIMESTAMP(4),
};
static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
@@ -659,6 +686,10 @@ static int bma400_init(struct bma400_data *data)
if (ret)
return ret;
+ /* Configure INT1 pin to open drain */
+ ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
+ if (ret)
+ return ret;
/*
* Once the interrupt engine is supported we might use the
* data_src_reg, but for now ensure this is set to the
@@ -807,6 +838,29 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
}
}
+static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
+ bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
+ BMA400_INT_DRDY_MSK,
+ FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
+ BMA400_INT_DRDY_MSK,
+ FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+}
+
+static const unsigned long bma400_avail_scan_masks[] = {
+ GENMASK(3, 0),
+ 0
+};
+
static const struct iio_info bma400_info = {
.read_raw = bma400_read_raw,
.read_avail = bma400_read_avail,
@@ -814,7 +868,64 @@ static const struct iio_info bma400_info = {
.write_raw_get_fmt = bma400_write_raw_get_fmt,
};
-int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
+static const struct iio_trigger_ops bma400_trigger_ops = {
+ .set_trigger_state = &bma400_data_rdy_trigger_set_state,
+ .validate_device = &iio_trigger_validate_own_device,
+};
+
+static irqreturn_t bma400_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret, temp;
+
+ mutex_lock(&data->mutex);
+
+ /* bulk read six registers, with the base being the LSB register */
+ ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
+ &data->buffer.buff, sizeof(data->buffer.buff));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return IRQ_NONE;
+
+ ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
+ if (ret)
+ return IRQ_NONE;
+
+ data->buffer.temperature = temp;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+ iio_get_time_ns(indio_dev));
+
+ iio_trigger_notify_done(indio_dev->trig);
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t bma400_interrupt(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+ __le16 status;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
+ sizeof(status));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return IRQ_NONE;
+
+ if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) {
+ iio_trigger_poll_chained(data->trig);
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
+int bma400_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name)
{
struct iio_dev *indio_dev;
struct bma400_data *data;
@@ -841,8 +952,40 @@ int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
indio_dev->info = &bma400_info;
indio_dev->channels = bma400_channels;
indio_dev->num_channels = ARRAY_SIZE(bma400_channels);
+ indio_dev->available_scan_masks = bma400_avail_scan_masks;
indio_dev->modes = INDIO_DIRECT_MODE;
+ if (irq > 0) {
+ data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->trig)
+ return -ENOMEM;
+
+ data->trig->ops = &bma400_trigger_ops;
+ iio_trigger_set_drvdata(data->trig, indio_dev);
+
+ ret = devm_iio_trigger_register(data->dev, data->trig);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio trigger register fail\n");
+
+ indio_dev->trig = iio_trigger_get(data->trig);
+ ret = devm_request_threaded_irq(dev, irq, NULL,
+ &bma400_interrupt,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ indio_dev->name, indio_dev);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "request irq %d failed\n", irq);
+ }
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ &bma400_trigger_handler, NULL);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio triggered buffer setup failed\n");
+
return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL_NS(bma400_probe, IIO_BMA400);
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index 4f6e01a3b3a1..1ba2a982ea73 100644
--- a/drivers/iio/accel/bma400_i2c.c
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -24,7 +24,7 @@ static int bma400_i2c_probe(struct i2c_client *client,
return PTR_ERR(regmap);
}
- return bma400_probe(&client->dev, regmap, id->name);
+ return bma400_probe(&client->dev, regmap, client->irq, id->name);
}
static const struct i2c_device_id bma400_i2c_ids[] = {
diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
index 28e240400a3f..ec13c044b304 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -84,7 +84,7 @@ static int bma400_spi_probe(struct spi_device *spi)
if (ret)
dev_err(&spi->dev, "Failed to read chip id register\n");
- return bma400_probe(&spi->dev, regmap, id->name);
+ return bma400_probe(&spi->dev, regmap, spi->irq, id->name);
}
static const struct spi_device_id bma400_spi_ids[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support
2022-04-11 20:31 ` [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support Jagath Jog J
@ 2022-04-12 9:12 ` Andy Shevchenko
2022-04-12 19:30 ` Jagath Jog J
2022-04-16 16:38 ` Jonathan Cameron
1 sibling, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2022-04-12 9:12 UTC (permalink / raw)
To: Jagath Jog J
Cc: Dan Robertson, Jonathan Cameron, linux-iio,
Linux Kernel Mailing List
On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Added trigger buffer support to read continuous acceleration
> data from device with data ready interrupt which is mapped
> to INT1 pin.
Can you explain the locking schema in this driver?
> + /* Configure INT1 pin to open drain */
> + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> + if (ret)
> + return ret;
No locking (or regmap only locking).
...
> +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> + BMA400_INT_DRDY_MSK,
> + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> + BMA400_INT_DRDY_MSK,
> + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> +}
Ditto.
...
> + mutex_lock(&data->mutex);
> +
> + /* bulk read six registers, with the base being the LSB register */
> + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> + &data->buffer.buff, sizeof(data->buffer.buff));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return IRQ_NONE;
But here only above with locking...
> + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> + if (ret)
> + return IRQ_NONE;
...followed by no locking.
...
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> + sizeof(status));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return IRQ_NONE;
And again with locking.
...
So,
1) Does regmap is configured with locking? What for?
2) What's the role of data->mutex?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support
2022-04-12 9:12 ` Andy Shevchenko
@ 2022-04-12 19:30 ` Jagath Jog J
[not found] ` <CAHp75Vc9MO2GxX81JQfzGRjM=nWLaQ-Uy9bV-dR1GMj1oQwjSQ@mail.gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-12 19:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dan Robertson, Jonathan Cameron, linux-iio,
Linux Kernel Mailing List
Hello Andy,
On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > Added trigger buffer support to read continuous acceleration
> > data from device with data ready interrupt which is mapped
> > to INT1 pin.
>
> Can you explain the locking schema in this driver?
>
> > + /* Configure INT1 pin to open drain */
> > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> > + if (ret)
> > + return ret;
>
> No locking (or regmap only locking).
This is bma400_init() function which will run when probe runs so there is no
locking in this bma400_init().
>
> ...
>
> > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > + bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> > + BMA400_INT_DRDY_MSK,
> > + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > + BMA400_INT_DRDY_MSK,
> > + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> > +}
>
> Ditto.
Sorry, I missed this.
I will add lock and unlocking in the next patch.
>
> ...
>
> > + mutex_lock(&data->mutex);
> > +
> > + /* bulk read six registers, with the base being the LSB register */
> > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> > + &data->buffer.buff, sizeof(data->buffer.buff));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return IRQ_NONE;
>
> But here only above with locking...
>
> > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> > + if (ret)
> > + return IRQ_NONE;
>
> ...followed by no locking.
Okay I will add lock in the next patch.
>
> ...
>
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> > + sizeof(status));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return IRQ_NONE;
>
> And again with locking.
>
> ...
>
> So,
> 1) Does regmap is configured with locking? What for?
> 2) What's the role of data->mutex?
1.
NO, regmap is not configured with locking.
In the regmap_config structure variable below these members are not defined
in the driver.
struct regmap_config {
regmap_lock lock;
regmap_unlock unlock;
void *lock_arg;
2.
data->mutex is used to protect the register read, write access from the device.
Is the regmap functions handle locking and unlocking internally if these below
struct members are not defined?
regmap_lock lock;
regmap_unlock unlock;
void *lock_arg;
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support
2022-04-11 20:31 ` [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support Jagath Jog J
2022-04-12 9:12 ` Andy Shevchenko
@ 2022-04-16 16:38 ` Jonathan Cameron
1 sibling, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:38 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 12 Apr 2022 02:01:28 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Added trigger buffer support to read continuous acceleration
> data from device with data ready interrupt which is mapped
> to INT1 pin.
>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
A few comments inline.
Jonathan
> ---
> drivers/iio/accel/Kconfig | 2 +
> drivers/iio/accel/bma400.h | 10 ++-
> drivers/iio/accel/bma400_core.c | 153 ++++++++++++++++++++++++++++++--
> drivers/iio/accel/bma400_i2c.c | 2 +-
> drivers/iio/accel/bma400_spi.c | 2 +-
> 5 files changed, 161 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index eac3f02662ae..958097814232 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -204,6 +204,8 @@ config BMA220
> config BMA400
> tristate "Bosch BMA400 3-Axis Accelerometer Driver"
> select REGMAP
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> select BMA400_I2C if I2C
> select BMA400_SPI if SPI
> help
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 8dbf85eeb005..a7482a66b36b 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -62,6 +62,13 @@
> #define BMA400_ACC_CONFIG2_REG 0x1b
> #define BMA400_CMD_REG 0x7e
>
> +/* Interrupt registers */
> +#define BMA400_INT_CONFIG0_REG 0x1f
> +#define BMA400_INT_CONFIG1_REG 0x20
> +#define BMA400_INT1_MAP_REG 0x21
> +#define BMA400_INT_IO_CTRL_REG 0x24
> +#define BMA400_INT_DRDY_MSK BIT(7)
> +
> /* Chip ID of BMA 400 devices found in the chip ID register. */
> #define BMA400_ID_REG_VAL 0x90
>
> @@ -111,6 +118,7 @@
>
> extern const struct regmap_config bma400_regmap_config;
>
> -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
> +int bma400_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name);
>
> #endif
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 07674d89d978..b7b2b67aef31 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -11,6 +11,7 @@
> * - Create channel for sensor time
> */
>
> +#include <linux/bitfield.h>
> #include <linux/bitops.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> @@ -20,6 +21,10 @@
> #include <linux/regulator/consumer.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> #include "bma400.h"
>
> @@ -61,6 +66,13 @@ struct bma400_data {
> struct bma400_sample_freq sample_freq;
> int oversampling_ratio;
> int scale;
> + struct iio_trigger *trig;
> + /* Correct time stamp alignment */
> + struct {
> + __le16 buff[3];
> + u8 temperature;
> + s64 ts __aligned(8);
> + } buffer ____cacheline_aligned;
See below, but I'd suggest adding
__le16 status;
here to be in the same cacheline as the buffer and hence also DMA safe (as it's
not in the same line as anything else which could be modified concurrently.)
> };
>
> static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> @@ -152,7 +164,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> { }
> };
>
> -#define BMA400_ACC_CHANNEL(_axis) { \
> +#define BMA400_ACC_CHANNEL(_index, _axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> .channel2 = IIO_MOD_##_axis, \
> @@ -164,17 +176,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
> BIT(IIO_CHAN_INFO_SCALE) | \
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> .ext_info = bma400_ext_info, \
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + }, \
> }
>
> static const struct iio_chan_spec bma400_channels[] = {
> - BMA400_ACC_CHANNEL(X),
> - BMA400_ACC_CHANNEL(Y),
> - BMA400_ACC_CHANNEL(Z),
> + BMA400_ACC_CHANNEL(0, X),
> + BMA400_ACC_CHANNEL(1, Y),
> + BMA400_ACC_CHANNEL(2, Z),
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = 3,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 8,
> + .storagebits = 8,
> + .endianness = IIO_LE,
> + },
> },
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> };
>
> static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> @@ -659,6 +686,10 @@ static int bma400_init(struct bma400_data *data)
> if (ret)
> return ret;
>
> + /* Configure INT1 pin to open drain */
> + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x06);
> + if (ret)
> + return ret;
> /*
> * Once the interrupt engine is supported we might use the
> * data_src_reg, but for now ensure this is set to the
> @@ -807,6 +838,29 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
> }
> }
>
> +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> + BMA400_INT_DRDY_MSK,
> + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> + if (ret)
> + return ret;
> +
> + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> + BMA400_INT_DRDY_MSK,
> + FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> +}
> +
> +static const unsigned long bma400_avail_scan_masks[] = {
> + GENMASK(3, 0),
> + 0
> +};
> +
> static const struct iio_info bma400_info = {
> .read_raw = bma400_read_raw,
> .read_avail = bma400_read_avail,
> @@ -814,7 +868,64 @@ static const struct iio_info bma400_info = {
> .write_raw_get_fmt = bma400_write_raw_get_fmt,
> };
>
> -int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
> +static const struct iio_trigger_ops bma400_trigger_ops = {
> + .set_trigger_state = &bma400_data_rdy_trigger_set_state,
> + .validate_device = &iio_trigger_validate_own_device,
> +};
> +
> +static irqreturn_t bma400_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret, temp;
> +
> + mutex_lock(&data->mutex);
> +
> + /* bulk read six registers, with the base being the LSB register */
> + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> + &data->buffer.buff, sizeof(data->buffer.buff));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return IRQ_NONE;
> +
> + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> + if (ret)
> + return IRQ_NONE;
> +
> + data->buffer.temperature = temp;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> + iio_get_time_ns(indio_dev));
> +
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t bma400_interrupt(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> + __le16 status;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> + sizeof(status));
regmap_bulk_read() (may) need a DMA safe buffer. Which means you can't use
a variable on the stack. Look at using the carefully aligned and padded
data->buffer if you can as that is DMA safe.
Note that then you will need that lock as it protects that buffer...
You could also just add a suitable buffer after that instead
of reusing that particular structure.
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return IRQ_NONE;
> +
> + if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) {
> + iio_trigger_poll_chained(data->trig);
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 5/9] iio: accel: bma400: Add separate channel for step counter
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (3 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 4/9] iio: accel: bma400: Add triggered buffer support Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-16 16:41 ` Jonathan Cameron
2022-04-11 20:31 ` [PATCH v3 6/9] iio: accel: bma400: Add step change event Jagath Jog J
` (3 subsequent siblings)
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 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 | 48 ++++++++++++++++++++++++++++++---
2 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index a7482a66b36b..52f9ea95de81 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 b7b2b67aef31..c8f147163d3c 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -20,6 +20,8 @@
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
+#include <asm/unaligned.h>
+
#include <linux/iio/iio.h>
#include <linux/iio/buffer.h>
#include <linux/iio/trigger.h>
@@ -67,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];
@@ -201,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),
};
@@ -705,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);
@@ -752,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;
}
@@ -817,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;
}
@@ -833,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] 28+ messages in thread* Re: [PATCH v3 5/9] iio: accel: bma400: Add separate channel for step counter
2022-04-11 20:31 ` [PATCH v3 5/9] iio: accel: bma400: Add separate channel for step counter Jagath Jog J
@ 2022-04-16 16:41 ` Jonathan Cameron
0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:41 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 12 Apr 2022 02:01:29 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 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 | 48 ++++++++++++++++++++++++++++++---
> 2 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index a7482a66b36b..52f9ea95de81 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 b7b2b67aef31..c8f147163d3c 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -20,6 +20,8 @@
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
>
> +#include <asm/unaligned.h>
> +
> #include <linux/iio/iio.h>
> #include <linux/iio/buffer.h>
> #include <linux/iio/trigger.h>
> @@ -67,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];
> @@ -201,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),
> };
>
> @@ -705,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));
Bulk read to a non DMA safe buffer. Similar fix to before needed.
Or given this is a slow path, just use a local kmalloc() for the buffer.
The reality is you are probably fine today without such care, but last
time we discussed this the conclusion was that it would be a perfectly
valid optimisation in regmap to return to requiring DMA safe buffers
for bulk accesses.
> + 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);
> @@ -752,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;
> }
> @@ -817,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));
Why the lock for this one?
> + mutex_unlock(&data->mutex);
> + data->steps_enabled = val;
> + return ret;
> default:
> return -EINVAL;
> }
> @@ -833,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;
> }
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 6/9] iio: accel: bma400: Add step change event
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (4 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 5/9] iio: accel: bma400: Add separate channel for step counter Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-11 20:31 ` [PATCH v3 7/9] iio: accel: bma400: Add activity recognition support Jagath Jog J
` (2 subsequent siblings)
8 siblings, 0 replies; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 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 | 76 +++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 52f9ea95de81..bc4641279be3 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 c8f147163d3c..37f38626a9aa 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/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,60 @@ 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 (chan->type) {
+ case IIO_STEPS:
+ 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 (chan->type) {
+ case IIO_STEPS:
+ 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) {
+ mutex_unlock(&data->mutex);
+ 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)
{
@@ -906,6 +970,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 = {
@@ -946,6 +1012,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
{
struct iio_dev *indio_dev = private;
struct bma400_data *data = iio_priv(indio_dev);
+ s64 timestamp = iio_get_time_ns(indio_dev);
int ret;
__le16 status;
@@ -956,6 +1023,15 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
if (ret)
return IRQ_NONE;
+ 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),
+ timestamp);
+ return IRQ_HANDLED;
+ }
+
if (FIELD_GET(BMA400_INT_DRDY_MSK, le16_to_cpu(status))) {
iio_trigger_poll_chained(data->trig);
return IRQ_HANDLED;
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v3 7/9] iio: accel: bma400: Add activity recognition support
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (5 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 6/9] iio: accel: bma400: Add step change event Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-16 16:47 ` Jonathan Cameron
2022-04-11 20:31 ` [PATCH v3 8/9] iio: accel: bma400: Add debugfs register access support Jagath Jog J
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Add support for activity recognition like STILL, WALKING, RUNNING
and these events are pushed to the userspace whenever the STEP
interrupt occurs.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/bma400_core.c | 104 ++++++++++++++++++++++++++++++++
1 file changed, 104 insertions(+)
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 37f38626a9aa..69d2caa4ed18 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -59,6 +59,12 @@ struct bma400_sample_freq {
int uhz;
};
+enum bma400_activity {
+ BMA400_STILL,
+ BMA400_WALKING,
+ BMA400_RUNNING,
+};
+
struct bma400_data {
struct device *dev;
struct regmap *regmap;
@@ -72,6 +78,7 @@ struct bma400_data {
struct iio_trigger *trig;
int steps_enabled;
bool step_event_en;
+ bool activity_event_en;
/* Correct time stamp alignment */
struct {
__le16 buff[3];
@@ -175,6 +182,12 @@ static const struct iio_event_spec bma400_step_detect_event = {
.mask_separate = BIT(IIO_EV_INFO_ENABLE),
};
+static const struct iio_event_spec bma400_activity_event = {
+ .type = IIO_EV_TYPE_CHANGE,
+ .dir = IIO_EV_DIR_NONE,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
+};
+
#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -196,6 +209,16 @@ static const struct iio_event_spec bma400_step_detect_event = {
}, \
}
+#define BMA400_ACTIVITY_CHANNEL(_chan2) { \
+ .type = IIO_ACTIVITY, \
+ .modified = 1, \
+ .channel2 = _chan2, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+ .scan_index = -1, /* No buffer support */ \
+ .event_spec = &bma400_activity_event, \
+ .num_event_specs = 1, \
+}
+
static const struct iio_chan_spec bma400_channels[] = {
BMA400_ACC_CHANNEL(0, X),
BMA400_ACC_CHANNEL(1, Y),
@@ -220,6 +243,9 @@ static const struct iio_chan_spec bma400_channels[] = {
.event_spec = &bma400_step_detect_event,
.num_event_specs = 1,
},
+ BMA400_ACTIVITY_CHANNEL(IIO_MOD_STILL),
+ BMA400_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
+ BMA400_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
IIO_CHAN_SOFT_TIMESTAMP(4),
};
@@ -626,6 +652,20 @@ static void bma400_power_disable(void *data_ptr)
ERR_PTR(ret));
}
+static enum iio_modifier bma400_act_to_mod(enum bma400_activity activity)
+{
+ switch (activity) {
+ case BMA400_STILL:
+ return IIO_MOD_STILL;
+ case BMA400_WALKING:
+ return IIO_MOD_WALKING;
+ case BMA400_RUNNING:
+ return IIO_MOD_RUNNING;
+ default:
+ return IIO_NO_MOD;
+ }
+}
+
static int bma400_init(struct bma400_data *data)
{
unsigned int val;
@@ -725,6 +765,7 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
struct bma400_data *data = iio_priv(indio_dev);
int ret;
u8 steps_raw[3];
+ unsigned int activity;
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
@@ -743,6 +784,23 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
return ret;
*val = get_unaligned_le24(steps_raw);
return IIO_VAL_INT;
+ case IIO_ACTIVITY:
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
+ &activity);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ /*
+ * The device does not support confidence value levels,
+ * so we will always have 100% for current activity and
+ * 0% for the others.
+ */
+ if (chan->channel2 == bma400_act_to_mod(activity))
+ *val = 100;
+ else
+ *val = 0;
+ return IIO_VAL_INT;
default:
return -EINVAL;
}
@@ -898,6 +956,8 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_STEPS:
return data->step_event_en;
+ case IIO_ACTIVITY:
+ return data->activity_event_en;
default:
return -EINVAL;
}
@@ -937,6 +997,32 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
return ret;
data->step_event_en = state;
return 0;
+ case IIO_ACTIVITY:
+ if (!data->step_event_en) {
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap,
+ BMA400_INT_CONFIG1_REG,
+ BMA400_STEP_INT_MSK,
+ FIELD_PREP(BMA400_STEP_INT_MSK,
+ 1));
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ 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,
+ 1));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ data->step_event_en = 1;
+ }
+ data->activity_event_en = state;
+ return 0;
default:
return -EINVAL;
}
@@ -1015,6 +1101,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
s64 timestamp = iio_get_time_ns(indio_dev);
int ret;
__le16 status;
+ unsigned int act;
mutex_lock(&data->mutex);
ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
@@ -1029,6 +1116,23 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
IIO_EV_DIR_NONE,
IIO_EV_TYPE_CHANGE, 0, 0, 0),
timestamp);
+
+ if (data->activity_event_en) {
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
+ &act);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return IRQ_NONE;
+
+ iio_push_event(indio_dev,
+ IIO_EVENT_CODE(IIO_ACTIVITY, 0,
+ bma400_act_to_mod(act),
+ IIO_EV_DIR_NONE,
+ IIO_EV_TYPE_CHANGE, 0,
+ 0, 0),
+ timestamp);
+ }
return IRQ_HANDLED;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 7/9] iio: accel: bma400: Add activity recognition support
2022-04-11 20:31 ` [PATCH v3 7/9] iio: accel: bma400: Add activity recognition support Jagath Jog J
@ 2022-04-16 16:47 ` Jonathan Cameron
0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:47 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 12 Apr 2022 02:01:31 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Add support for activity recognition like STILL, WALKING, RUNNING
> and these events are pushed to the userspace whenever the STEP
> interrupt occurs.
>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
Hi Jagath,
Other than more questions on locking and a suggestion for some
code deduplication this looks good to me.
Jonathan
> ---
> drivers/iio/accel/bma400_core.c | 104 ++++++++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 37f38626a9aa..69d2caa4ed18 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -59,6 +59,12 @@ struct bma400_sample_freq {
> int uhz;
> };
>
> +enum bma400_activity {
> + BMA400_STILL,
> + BMA400_WALKING,
> + BMA400_RUNNING,
> +};
> +
> struct bma400_data {
> struct device *dev;
> struct regmap *regmap;
> @@ -72,6 +78,7 @@ struct bma400_data {
> struct iio_trigger *trig;
> int steps_enabled;
> bool step_event_en;
> + bool activity_event_en;
> /* Correct time stamp alignment */
> struct {
> __le16 buff[3];
> @@ -175,6 +182,12 @@ static const struct iio_event_spec bma400_step_detect_event = {
> .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> };
>
> +static const struct iio_event_spec bma400_activity_event = {
> + .type = IIO_EV_TYPE_CHANGE,
> + .dir = IIO_EV_DIR_NONE,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> +};
> +
> #define BMA400_ACC_CHANNEL(_index, _axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -196,6 +209,16 @@ static const struct iio_event_spec bma400_step_detect_event = {
> }, \
> }
>
> +#define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> + .type = IIO_ACTIVITY, \
> + .modified = 1, \
> + .channel2 = _chan2, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .scan_index = -1, /* No buffer support */ \
> + .event_spec = &bma400_activity_event, \
> + .num_event_specs = 1, \
> +}
> +
> static const struct iio_chan_spec bma400_channels[] = {
> BMA400_ACC_CHANNEL(0, X),
> BMA400_ACC_CHANNEL(1, Y),
> @@ -220,6 +243,9 @@ static const struct iio_chan_spec bma400_channels[] = {
> .event_spec = &bma400_step_detect_event,
> .num_event_specs = 1,
> },
> + BMA400_ACTIVITY_CHANNEL(IIO_MOD_STILL),
> + BMA400_ACTIVITY_CHANNEL(IIO_MOD_WALKING),
> + BMA400_ACTIVITY_CHANNEL(IIO_MOD_RUNNING),
> IIO_CHAN_SOFT_TIMESTAMP(4),
> };
>
> @@ -626,6 +652,20 @@ static void bma400_power_disable(void *data_ptr)
> ERR_PTR(ret));
> }
>
> +static enum iio_modifier bma400_act_to_mod(enum bma400_activity activity)
> +{
> + switch (activity) {
> + case BMA400_STILL:
> + return IIO_MOD_STILL;
> + case BMA400_WALKING:
> + return IIO_MOD_WALKING;
> + case BMA400_RUNNING:
> + return IIO_MOD_RUNNING;
> + default:
> + return IIO_NO_MOD;
> + }
> +}
> +
> static int bma400_init(struct bma400_data *data)
> {
> unsigned int val;
> @@ -725,6 +765,7 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
> struct bma400_data *data = iio_priv(indio_dev);
> int ret;
> u8 steps_raw[3];
> + unsigned int activity;
>
> switch (mask) {
> case IIO_CHAN_INFO_PROCESSED:
> @@ -743,6 +784,23 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
> return ret;
> *val = get_unaligned_le24(steps_raw);
> return IIO_VAL_INT;
> + case IIO_ACTIVITY:
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
> + &activity);
More locking in here to think about. Not clear what you are protecting here.
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + /*
> + * The device does not support confidence value levels,
> + * so we will always have 100% for current activity and
> + * 0% for the others.
I was thinking ahead when I came up with confidence for these. One day someone
will implement a sensor that is honest that it isn't that sure what the activity is.
:) When they do we'll be ready!
> + */
> + if (chan->channel2 == bma400_act_to_mod(activity))
> + *val = 100;
> + else
> + *val = 0;
> + return IIO_VAL_INT;
> default:
> return -EINVAL;
> }
> @@ -898,6 +956,8 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> switch (chan->type) {
> case IIO_STEPS:
> return data->step_event_en;
> + case IIO_ACTIVITY:
> + return data->activity_event_en;
> default:
> return -EINVAL;
> }
> @@ -937,6 +997,32 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> return ret;
> data->step_event_en = state;
> return 0;
> + case IIO_ACTIVITY:
> + if (!data->step_event_en) {
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap,
> + BMA400_INT_CONFIG1_REG,
> + BMA400_STEP_INT_MSK,
> + FIELD_PREP(BMA400_STEP_INT_MSK,
> + 1));
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + 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,
> + 1));
This looks like some code that could be sensibly factored out in the earlier
patch to provide a function to call here rather than having another copy
of more or less the same code.
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + data->step_event_en = 1;
I can understand logic of using lock to keep the internal tracking in sync
with the device, but here you are doing that outside the lock which doesn't seem
to do that.
> + }
> + data->activity_event_en = state;
> + return 0;
> default:
> return -EINVAL;
> }
> @@ -1015,6 +1101,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> s64 timestamp = iio_get_time_ns(indio_dev);
> int ret;
> __le16 status;
> + unsigned int act;
>
> mutex_lock(&data->mutex);
> ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
> @@ -1029,6 +1116,23 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> IIO_EV_DIR_NONE,
> IIO_EV_TYPE_CHANGE, 0, 0, 0),
> timestamp);
> +
> + if (data->activity_event_en) {
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, BMA400_STEP_STAT_REG,
> + &act);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return IRQ_NONE;
> +
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_ACTIVITY, 0,
> + bma400_act_to_mod(act),
> + IIO_EV_DIR_NONE,
> + IIO_EV_TYPE_CHANGE, 0,
> + 0, 0),
> + timestamp);
> + }
> return IRQ_HANDLED;
> }
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 8/9] iio: accel: bma400: Add debugfs register access support
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (6 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 7/9] iio: accel: bma400: Add activity recognition support Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-16 16:48 ` Jonathan Cameron
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
8 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Add support to read/write byte from/to bma400 device from the userspace
using debugfs interface.
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
drivers/iio/accel/bma400_core.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 69d2caa4ed18..b6c79cfabaa4 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -1028,6 +1028,23 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
}
}
+static int bma400_debugfs_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg,
+ unsigned int writeval,
+ unsigned int *readval)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->mutex);
+ if (readval)
+ ret = regmap_read(data->regmap, reg, readval);
+ else
+ ret = regmap_write(data->regmap, reg, writeval);
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
bool state)
{
@@ -1058,6 +1075,7 @@ static const struct iio_info bma400_info = {
.write_raw_get_fmt = bma400_write_raw_get_fmt,
.read_event_config = bma400_read_event_config,
.write_event_config = bma400_write_event_config,
+ .debugfs_reg_access = bma400_debugfs_reg_access,
};
static const struct iio_trigger_ops bma400_trigger_ops = {
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 8/9] iio: accel: bma400: Add debugfs register access support
2022-04-11 20:31 ` [PATCH v3 8/9] iio: accel: bma400: Add debugfs register access support Jagath Jog J
@ 2022-04-16 16:48 ` Jonathan Cameron
0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:48 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 12 Apr 2022 02:01:32 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Add support to read/write byte from/to bma400 device from the userspace
> using debugfs interface.
>
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
> drivers/iio/accel/bma400_core.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 69d2caa4ed18..b6c79cfabaa4 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -1028,6 +1028,23 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> }
> }
>
> +static int bma400_debugfs_reg_access(struct iio_dev *indio_dev,
> + unsigned int reg,
> + unsigned int writeval,
> + unsigned int *readval)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
Guess what? :) Can't see what purpose locking this has.
> + if (readval)
> + ret = regmap_read(data->regmap, reg, readval);
> + else
> + ret = regmap_write(data->regmap, reg, writeval);
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> bool state)
> {
> @@ -1058,6 +1075,7 @@ static const struct iio_info bma400_info = {
> .write_raw_get_fmt = bma400_write_raw_get_fmt,
> .read_event_config = bma400_read_event_config,
> .write_event_config = bma400_write_event_config,
> + .debugfs_reg_access = bma400_debugfs_reg_access,
> };
>
> static const struct iio_trigger_ops bma400_trigger_ops = {
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-11 20:31 [PATCH v3 0/9] iio: accel: bma400: Add buffer, step and activity/inactivity Jagath Jog J
` (7 preceding siblings ...)
2022-04-11 20:31 ` [PATCH v3 8/9] iio: accel: bma400: Add debugfs register access support Jagath Jog J
@ 2022-04-11 20:31 ` Jagath Jog J
2022-04-12 5:21 ` kernel test robot
` (3 more replies)
8 siblings, 4 replies; 28+ messages in thread
From: Jagath Jog J @ 2022-04-11 20:31 UTC (permalink / raw)
To: dan, jic23, andy.shevchenko; +Cc: linux-iio, linux-kernel
Add support for activity and inactivity events for all axis based on the
threshold, duration and hysteresis value set from the userspace. 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 | 11 ++
drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
2 files changed, 240 insertions(+)
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index bc4641279be3..cbf8035c817e 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -93,6 +93,17 @@
#define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
#define BMA400_ACC_ODR_MIN_HZ 12
+/* Generic interrupts register */
+#define BMA400_GEN1INT_CONFIG0 0x3f
+#define BMA400_GEN2INT_CONFIG0 0x4A
+#define BMA400_GEN_CONFIG1_OFF 0x01
+#define BMA400_GEN_CONFIG2_OFF 0x02
+#define BMA400_GEN_CONFIG3_OFF 0x03
+#define BMA400_GEN_CONFIG31_OFF 0x04
+#define BMA400_INT_GEN1_MSK BIT(2)
+#define BMA400_INT_GEN2_MSK BIT(3)
+#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
+
/*
* BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
* converting to micro values for +-2g range.
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index b6c79cfabaa4..226a5f63d1a6 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -79,6 +79,7 @@ struct bma400_data {
int steps_enabled;
bool step_event_en;
bool activity_event_en;
+ u8 generic_event_en;
/* Correct time stamp alignment */
struct {
__le16 buff[3];
@@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
};
+static const struct iio_event_spec bma400_accel_event[] = {
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD) |
+ BIT(IIO_EV_INFO_HYSTERESIS) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+ {
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_PERIOD) |
+ BIT(IIO_EV_INFO_HYSTERESIS) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
#define BMA400_ACC_CHANNEL(_index, _axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .event_spec = bma400_accel_event, \
+ .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
}
#define BMA400_ACTIVITY_CHANNEL(_chan2) { \
@@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
struct bma400_data *data = iio_priv(indio_dev);
switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return FIELD_GET(BMA400_INT_GEN1_MSK,
+ data->generic_event_en);
+ case IIO_EV_DIR_FALLING:
+ return FIELD_GET(BMA400_INT_GEN2_MSK,
+ data->generic_event_en);
+ default:
+ return -EINVAL;
+ }
case IIO_STEPS:
return data->step_event_en;
case IIO_ACTIVITY:
@@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
{
int ret;
struct bma400_data *data = iio_priv(indio_dev);
+ int reg, msk, value, field_value;
switch (chan->type) {
+ case IIO_ACCEL:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ reg = BMA400_GEN1INT_CONFIG0;
+ msk = BMA400_INT_GEN1_MSK;
+ value = 2;
+ field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
+ break;
+ case IIO_EV_DIR_FALLING:
+ reg = BMA400_GEN2INT_CONFIG0;
+ msk = BMA400_INT_GEN2_MSK;
+ value = 0;
+ field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ mutex_lock(&data->mutex);
+ /* Enabling all axis for interrupt evaluation */
+ ret = regmap_write(data->regmap, reg, 0xF8);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* OR combination of all axis for interrupt evaluation */
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
+ value);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* Initial value to avoid interrupts while enabling*/
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ 0x0A);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ /* Initial duration value to avoid interrupts while enabling*/
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
+ 0x0F);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
+ msk, field_value);
+ if (ret) {
+ mutex_unlock(&data->mutex);
+ return ret;
+ }
+
+ ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
+ msk, field_value);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+
+ set_mask_bits(&data->generic_event_en, msk, field_value);
+ return 0;
case IIO_STEPS:
mutex_lock(&data->mutex);
if (!data->steps_enabled) {
@@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
}
}
+static int get_gen_config_reg(enum iio_event_direction dir)
+{
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ return BMA400_GEN2INT_CONFIG0;
+ case IIO_EV_DIR_RISING:
+ return BMA400_GEN1INT_CONFIG0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bma400_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 reg, duration[2];
+
+ reg = get_gen_config_reg(dir);
+ if (reg < 0)
+ return -EINVAL;
+
+ *val2 = 0;
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_PERIOD:
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_read(data->regmap,
+ reg + BMA400_GEN_CONFIG3_OFF,
+ duration, sizeof(duration));
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ *val = get_unaligned_be16(duration);
+ return IIO_VAL_INT;
+ case IIO_EV_INFO_HYSTERESIS:
+ mutex_lock(&data->mutex);
+ ret = regmap_read(data->regmap, reg, val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int bma400_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct bma400_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 reg, duration[2];
+
+ reg = get_gen_config_reg(dir);
+ if (reg < 0)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ if (val < 1 || val > 255)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
+ val);
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_INFO_PERIOD:
+ if (val < 1 || val > 65535)
+ return -EINVAL;
+
+ put_unaligned_be16(val, duration);
+
+ mutex_lock(&data->mutex);
+ ret = regmap_bulk_write(data->regmap,
+ reg + BMA400_GEN_CONFIG3_OFF,
+ duration, sizeof(duration));
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_INFO_HYSTERESIS:
+ if (val < 0 || val > 3)
+ return -EINVAL;
+
+ mutex_lock(&data->mutex);
+ ret = regmap_update_bits(data->regmap, reg,
+ BMA400_GEN_HYST_MSK,
+ FIELD_PREP(BMA400_GEN_HYST_MSK, val));
+ mutex_unlock(&data->mutex);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
static int bma400_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int reg,
unsigned int writeval,
@@ -1076,6 +1287,8 @@ static const struct iio_info bma400_info = {
.read_event_config = bma400_read_event_config,
.write_event_config = bma400_write_event_config,
.debugfs_reg_access = bma400_debugfs_reg_access,
+ .write_event_value = bma400_write_event_value,
+ .read_event_value = bma400_read_event_value,
};
static const struct iio_trigger_ops bma400_trigger_ops = {
@@ -1120,6 +1333,7 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
int ret;
__le16 status;
unsigned int act;
+ unsigned int ev_dir = IIO_EV_DIR_NONE;
mutex_lock(&data->mutex);
ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
@@ -1128,6 +1342,21 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
if (ret)
return IRQ_NONE;
+ if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(status)))
+ ev_dir = IIO_EV_DIR_RISING;
+
+ if (FIELD_GET(BMA400_INT_GEN2_MSK, le16_to_cpu(status)))
+ ev_dir = IIO_EV_DIR_FALLING;
+
+ if (ev_dir != IIO_EV_DIR_NONE) {
+ iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+ IIO_MOD_X_OR_Y_OR_Z,
+ IIO_EV_TYPE_MAG, ev_dir),
+ timestamp);
+ return 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,
--
2.17.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
@ 2022-04-12 5:21 ` kernel test robot
2022-04-12 7:38 ` Dan Carpenter
` (2 subsequent siblings)
3 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2022-04-12 5:21 UTC (permalink / raw)
To: Jagath Jog J, dan, jic23, andy.shevchenko
Cc: llvm, kbuild-all, linux-iio, linux-kernel
Hi Jagath,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on v5.18-rc2 next-20220411]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: riscv-randconfig-c006-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121327.f3Svxeg1-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/15ee6de45ed7a028569638c198e170bb98cef4ab
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
git checkout 15ee6de45ed7a028569638c198e170bb98cef4ab
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/iio/accel/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/iio/accel/bma400_core.c:1072:3: error: call to __compiletime_assert_272 declared with 'error' attribute: BUILD_BUG failed
set_mask_bits(&data->generic_event_en, msk, field_value);
^
include/linux/bitops.h:283:11: note: expanded from macro 'set_mask_bits'
} while (cmpxchg(ptr, old__, new__) != old__); \
^
include/linux/atomic/atomic-instrumented.h:1916:2: note: expanded from macro 'cmpxchg'
arch_cmpxchg(__ai_ptr, __VA_ARGS__); \
^
arch/riscv/include/asm/cmpxchg.h:344:23: note: expanded from macro 'arch_cmpxchg'
(__typeof__(*(ptr))) __cmpxchg((ptr), \
^
note: (skipping 4 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:340:2: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
^
include/linux/compiler_types.h:333:4: note: expanded from macro '__compiletime_assert'
prefix ## suffix(); \
^
<scratch space>:64:1: note: expanded from here
__compiletime_assert_272
^
1 error generated.
vim +/error +1072 drivers/iio/accel/bma400_core.c
998
999 static int bma400_write_event_config(struct iio_dev *indio_dev,
1000 const struct iio_chan_spec *chan,
1001 enum iio_event_type type,
1002 enum iio_event_direction dir, int state)
1003 {
1004 int ret;
1005 struct bma400_data *data = iio_priv(indio_dev);
1006 int reg, msk, value, field_value;
1007
1008 switch (chan->type) {
1009 case IIO_ACCEL:
1010 switch (dir) {
1011 case IIO_EV_DIR_RISING:
1012 reg = BMA400_GEN1INT_CONFIG0;
1013 msk = BMA400_INT_GEN1_MSK;
1014 value = 2;
1015 field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
1016 break;
1017 case IIO_EV_DIR_FALLING:
1018 reg = BMA400_GEN2INT_CONFIG0;
1019 msk = BMA400_INT_GEN2_MSK;
1020 value = 0;
1021 field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
1022 break;
1023 default:
1024 return -EINVAL;
1025 }
1026
1027 mutex_lock(&data->mutex);
1028 /* Enabling all axis for interrupt evaluation */
1029 ret = regmap_write(data->regmap, reg, 0xF8);
1030 if (ret) {
1031 mutex_unlock(&data->mutex);
1032 return ret;
1033 }
1034
1035 /* OR combination of all axis for interrupt evaluation */
1036 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
1037 value);
1038 if (ret) {
1039 mutex_unlock(&data->mutex);
1040 return ret;
1041 }
1042
1043 /* Initial value to avoid interrupts while enabling*/
1044 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
1045 0x0A);
1046 if (ret) {
1047 mutex_unlock(&data->mutex);
1048 return ret;
1049 }
1050
1051 /* Initial duration value to avoid interrupts while enabling*/
1052 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
1053 0x0F);
1054 if (ret) {
1055 mutex_unlock(&data->mutex);
1056 return ret;
1057 }
1058
1059 ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
1060 msk, field_value);
1061 if (ret) {
1062 mutex_unlock(&data->mutex);
1063 return ret;
1064 }
1065
1066 ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
1067 msk, field_value);
1068 mutex_unlock(&data->mutex);
1069 if (ret)
1070 return ret;
1071
> 1072 set_mask_bits(&data->generic_event_en, msk, field_value);
1073 return 0;
1074 case IIO_STEPS:
1075 mutex_lock(&data->mutex);
1076 if (!data->steps_enabled) {
1077 ret = regmap_update_bits(data->regmap,
1078 BMA400_INT_CONFIG1_REG,
1079 BMA400_STEP_INT_MSK,
1080 FIELD_PREP(BMA400_STEP_INT_MSK,
1081 1));
1082 if (ret) {
1083 mutex_unlock(&data->mutex);
1084 return ret;
1085 }
1086 data->steps_enabled = 1;
1087 }
1088
1089 ret = regmap_update_bits(data->regmap,
1090 BMA400_INT12_MAP_REG,
1091 BMA400_STEP_INT_MSK,
1092 FIELD_PREP(BMA400_STEP_INT_MSK,
1093 state));
1094 mutex_unlock(&data->mutex);
1095 if (ret)
1096 return ret;
1097 data->step_event_en = state;
1098 return 0;
1099 case IIO_ACTIVITY:
1100 if (!data->step_event_en) {
1101 mutex_lock(&data->mutex);
1102 ret = regmap_update_bits(data->regmap,
1103 BMA400_INT_CONFIG1_REG,
1104 BMA400_STEP_INT_MSK,
1105 FIELD_PREP(BMA400_STEP_INT_MSK,
1106 1));
1107 if (ret) {
1108 mutex_unlock(&data->mutex);
1109 return ret;
1110 }
1111 data->steps_enabled = 1;
1112
1113 ret = regmap_update_bits(data->regmap,
1114 BMA400_INT12_MAP_REG,
1115 BMA400_STEP_INT_MSK,
1116 FIELD_PREP(BMA400_STEP_INT_MSK,
1117 1));
1118 mutex_unlock(&data->mutex);
1119 if (ret)
1120 return ret;
1121 data->step_event_en = 1;
1122 }
1123 data->activity_event_en = state;
1124 return 0;
1125 default:
1126 return -EINVAL;
1127 }
1128 }
1129
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
2022-04-12 5:21 ` kernel test robot
@ 2022-04-12 7:38 ` Dan Carpenter
2022-04-12 10:41 ` kernel test robot
2022-04-16 16:55 ` Jonathan Cameron
3 siblings, 0 replies; 28+ messages in thread
From: Dan Carpenter @ 2022-04-12 7:38 UTC (permalink / raw)
To: kbuild, Jagath Jog J, dan, jic23, andy.shevchenko
Cc: lkp, kbuild-all, linux-iio, linux-kernel
Hi Jagath,
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-m001-20220411 (https://download.01.org/0day-ci/archive/20220412/202204121411.snZVqiMy-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/iio/accel/bma400_core.c:1154 bma400_read_event_value() warn: impossible condition '(reg < 0) => (0-255 < 0)'
drivers/iio/accel/bma400_core.c:1202 bma400_write_event_value() warn: impossible condition '(reg < 0) => (0-255 < 0)'
vim +1154 drivers/iio/accel/bma400_core.c
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1142 static int bma400_read_event_value(struct iio_dev *indio_dev,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1143 const struct iio_chan_spec *chan,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1144 enum iio_event_type type,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1145 enum iio_event_direction dir,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1146 enum iio_event_info info,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1147 int *val, int *val2)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1148 {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1149 struct bma400_data *data = iio_priv(indio_dev);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1150 int ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1151 u8 reg, duration[2];
^^^^^^
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1152
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1153 reg = get_gen_config_reg(dir);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 @1154 if (reg < 0)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1155 return -EINVAL;
Condition is impossible. Also propagate the return? if (ret < 0) return ret;?
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1156
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1157 *val2 = 0;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1158 switch (info) {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1159 case IIO_EV_INFO_VALUE:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1160 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1161 ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1162 val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1163 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1164 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1165 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1166 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1167 case IIO_EV_INFO_PERIOD:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1168 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1169 ret = regmap_bulk_read(data->regmap,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1170 reg + BMA400_GEN_CONFIG3_OFF,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1171 duration, sizeof(duration));
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1172 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1173 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1174 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1175 *val = get_unaligned_be16(duration);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1176 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1177 case IIO_EV_INFO_HYSTERESIS:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1178 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1179 ret = regmap_read(data->regmap, reg, val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1180 mutex_unlock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1181 if (ret)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1182 return ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1183 *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1184 return IIO_VAL_INT;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1185 default:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1186 return -EINVAL;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1187 }
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1188 }
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1189
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1190 static int bma400_write_event_value(struct iio_dev *indio_dev,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1191 const struct iio_chan_spec *chan,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1192 enum iio_event_type type,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1193 enum iio_event_direction dir,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1194 enum iio_event_info info,
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1195 int val, int val2)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1196 {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1197 struct bma400_data *data = iio_priv(indio_dev);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1198 int ret;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1199 u8 reg, duration[2];
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1200
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1201 reg = get_gen_config_reg(dir);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 @1202 if (reg < 0)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1203 return -EINVAL;
Same.
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1204
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1205 switch (info) {
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1206 case IIO_EV_INFO_VALUE:
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1207 if (val < 1 || val > 255)
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1208 return -EINVAL;
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1209
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1210 mutex_lock(&data->mutex);
15ee6de45ed7a02 Jagath Jog J 2022-04-12 1211 ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
2022-04-12 5:21 ` kernel test robot
2022-04-12 7:38 ` Dan Carpenter
@ 2022-04-12 10:41 ` kernel test robot
2022-04-16 16:55 ` Jonathan Cameron
3 siblings, 0 replies; 28+ messages in thread
From: kernel test robot @ 2022-04-12 10:41 UTC (permalink / raw)
To: Jagath Jog J, dan, jic23, andy.shevchenko
Cc: kbuild-all, linux-iio, linux-kernel
Hi Jagath,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on v5.18-rc2 next-20220412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20220412/202204121837.e1RdwIqu-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/15ee6de45ed7a028569638c198e170bb98cef4ab
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-buffer-step-and-activity-inactivity/20220412-043436
git checkout 15ee6de45ed7a028569638c198e170bb98cef4ab
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "__cmpxchg_small" [drivers/iio/accel/bma400_core.ko] undefined!
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-11 20:31 ` [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events Jagath Jog J
` (2 preceding siblings ...)
2022-04-12 10:41 ` kernel test robot
@ 2022-04-16 16:55 ` Jonathan Cameron
2022-04-18 22:09 ` Jagath Jog J
3 siblings, 1 reply; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-16 16:55 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 12 Apr 2022 02:01:33 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Add support for activity and inactivity events for all axis based on the
> threshold, duration and hysteresis value set from the userspace. 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 | 11 ++
> drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> 2 files changed, 240 insertions(+)
>
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index bc4641279be3..cbf8035c817e 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -93,6 +93,17 @@
> #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> #define BMA400_ACC_ODR_MIN_HZ 12
>
> +/* Generic interrupts register */
> +#define BMA400_GEN1INT_CONFIG0 0x3f
> +#define BMA400_GEN2INT_CONFIG0 0x4A
> +#define BMA400_GEN_CONFIG1_OFF 0x01
> +#define BMA400_GEN_CONFIG2_OFF 0x02
> +#define BMA400_GEN_CONFIG3_OFF 0x03
> +#define BMA400_GEN_CONFIG31_OFF 0x04
> +#define BMA400_INT_GEN1_MSK BIT(2)
> +#define BMA400_INT_GEN2_MSK BIT(3)
> +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> +
> /*
> * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> * converting to micro values for +-2g range.
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index b6c79cfabaa4..226a5f63d1a6 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -79,6 +79,7 @@ struct bma400_data {
> int steps_enabled;
> bool step_event_en;
> bool activity_event_en;
> + u8 generic_event_en;
> /* Correct time stamp alignment */
> struct {
> __le16 buff[3];
> @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> };
>
> +static const struct iio_event_spec bma400_accel_event[] = {
> + {
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD) |
> + BIT(IIO_EV_INFO_HYSTERESIS) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_PERIOD) |
> + BIT(IIO_EV_INFO_HYSTERESIS) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> #define BMA400_ACC_CHANNEL(_index, _axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .event_spec = bma400_accel_event, \
> + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> }
>
> #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> struct bma400_data *data = iio_priv(indio_dev);
>
> switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return FIELD_GET(BMA400_INT_GEN1_MSK,
> + data->generic_event_en);
> + case IIO_EV_DIR_FALLING:
> + return FIELD_GET(BMA400_INT_GEN2_MSK,
> + data->generic_event_en);
> + default:
> + return -EINVAL;
> + }
> case IIO_STEPS:
> return data->step_event_en;
> case IIO_ACTIVITY:
> @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> {
> int ret;
> struct bma400_data *data = iio_priv(indio_dev);
> + int reg, msk, value, field_value;
>
> switch (chan->type) {
> + case IIO_ACCEL:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + reg = BMA400_GEN1INT_CONFIG0;
> + msk = BMA400_INT_GEN1_MSK;
> + value = 2;
> + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
Hopefully you can use msk in here and the compiler can tell it's constant...
> + break;
> + case IIO_EV_DIR_FALLING:
> + reg = BMA400_GEN2INT_CONFIG0;
> + msk = BMA400_INT_GEN2_MSK;
> + value = 0;
> + field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + mutex_lock(&data->mutex);
> + /* Enabling all axis for interrupt evaluation */
> + ret = regmap_write(data->regmap, reg, 0xF8);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* OR combination of all axis for interrupt evaluation */
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
> + value);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* Initial value to avoid interrupts while enabling*/
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + 0x0A);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + /* Initial duration value to avoid interrupts while enabling*/
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
> + 0x0F);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> + msk, field_value);
> + if (ret) {
> + mutex_unlock(&data->mutex);
> + return ret;
> + }
> +
> + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> + msk, field_value);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
This whole stack or mutex_unlock() error handling is a good hint that you should
just factor out this case as a separate function then you can use a goto to
deal with the unlock cleanly.
> +
> + set_mask_bits(&data->generic_event_en, msk, field_value);
> + return 0;
> case IIO_STEPS:
> mutex_lock(&data->mutex);
> if (!data->steps_enabled) {
> @@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> }
> }
>
> +static int get_gen_config_reg(enum iio_event_direction dir)
> +{
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + return BMA400_GEN2INT_CONFIG0;
> + case IIO_EV_DIR_RISING:
> + return BMA400_GEN1INT_CONFIG0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bma400_read_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 reg, duration[2];
> +
> + reg = get_gen_config_reg(dir);
> + if (reg < 0)
> + return -EINVAL;
> +
> + *val2 = 0;
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_PERIOD:
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_read(data->regmap,
> + reg + BMA400_GEN_CONFIG3_OFF,
> + duration, sizeof(duration));
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + *val = get_unaligned_be16(duration);
As well as dma safety question, you could just have used a __be16 for
duration then you can use be16_to_cpu() as you know it is aligned.
> + return IIO_VAL_INT;
> + case IIO_EV_INFO_HYSTERESIS:
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, reg, val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int bma400_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct bma400_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 reg, duration[2];
> +
> + reg = get_gen_config_reg(dir);
> + if (reg < 0)
> + return -EINVAL;
> +
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + if (val < 1 || val > 255)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> + val);
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_INFO_PERIOD:
> + if (val < 1 || val > 65535)
> + return -EINVAL;
> +
> + put_unaligned_be16(val, duration);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_bulk_write(data->regmap,
> + reg + BMA400_GEN_CONFIG3_OFF,
> + duration, sizeof(duration));
I can't remember if we are safe or not with bulk_writes but at least
in theory we might not be and should be using a dma safe buffer.
Also locking not necessary in various places in here.
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_INFO_HYSTERESIS:
> + if (val < 0 || val > 3)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_update_bits(data->regmap, reg,
> + BMA400_GEN_HYST_MSK,
> + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-16 16:55 ` Jonathan Cameron
@ 2022-04-18 22:09 ` Jagath Jog J
2022-04-24 15:40 ` Jonathan Cameron
0 siblings, 1 reply; 28+ messages in thread
From: Jagath Jog J @ 2022-04-18 22:09 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
Hello Jonathan,
Thanks for your suggestions, I will fix the locking and unlocking for all
patches in the next series.
Please can you guide me for auto build test error reported by kernel test
robot for set_mask_bits(&data->generic_event_en, msk, field_value);
in this patch.
On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> On Tue, 12 Apr 2022 02:01:33 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Add support for activity and inactivity events for all axis based on the
> > threshold, duration and hysteresis value set from the userspace. 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 | 11 ++
> > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > 2 files changed, 240 insertions(+)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index bc4641279be3..cbf8035c817e 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -93,6 +93,17 @@
> > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > #define BMA400_ACC_ODR_MIN_HZ 12
> >
> > +/* Generic interrupts register */
> > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > +#define BMA400_INT_GEN1_MSK BIT(2)
> > +#define BMA400_INT_GEN2_MSK BIT(3)
> > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > +
> > /*
> > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > * converting to micro values for +-2g range.
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index b6c79cfabaa4..226a5f63d1a6 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -79,6 +79,7 @@ struct bma400_data {
> > int steps_enabled;
> > bool step_event_en;
> > bool activity_event_en;
> > + u8 generic_event_en;
> > /* Correct time stamp alignment */
> > struct {
> > __le16 buff[3];
> > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > };
> >
> > +static const struct iio_event_spec bma400_accel_event[] = {
> > + {
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
> > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
> > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > .type = IIO_ACCEL, \
> > .modified = 1, \
> > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > .storagebits = 16, \
> > .endianness = IIO_LE, \
> > }, \
> > + .event_spec = bma400_accel_event, \
> > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > }
> >
> > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > struct bma400_data *data = iio_priv(indio_dev);
> >
> > switch (chan->type) {
> > + case IIO_ACCEL:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > + data->generic_event_en);
> > + case IIO_EV_DIR_FALLING:
> > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > + data->generic_event_en);
> > + default:
> > + return -EINVAL;
> > + }
> > case IIO_STEPS:
> > return data->step_event_en;
> > case IIO_ACTIVITY:
> > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > {
> > int ret;
> > struct bma400_data *data = iio_priv(indio_dev);
> > + int reg, msk, value, field_value;
> >
> > switch (chan->type) {
> > + case IIO_ACCEL:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + reg = BMA400_GEN1INT_CONFIG0;
> > + msk = BMA400_INT_GEN1_MSK;
> > + value = 2;
> > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
>
> Hopefully you can use msk in here and the compiler can tell it's constant...
field_value = FIELD_PREP(msk, state);
is this the fix for error reported by kernel test robot?
>
> > + break;
> > + case IIO_EV_DIR_FALLING:
> > + reg = BMA400_GEN2INT_CONFIG0;
> > + msk = BMA400_INT_GEN2_MSK;
> > + value = 0;
> > + field_value = FIELD_PREP(BMA400_INT_GEN2_MSK, state);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + mutex_lock(&data->mutex);
> > + /* Enabling all axis for interrupt evaluation */
> > + ret = regmap_write(data->regmap, reg, 0xF8);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* OR combination of all axis for interrupt evaluation */
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG1_OFF,
> > + value);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* Initial value to avoid interrupts while enabling*/
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + 0x0A);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + /* Initial duration value to avoid interrupts while enabling*/
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG31_OFF,
> > + 0x0F);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > + msk, field_value);
> > + if (ret) {
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + }
> > +
> > + ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> > + msk, field_value);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
>
> This whole stack or mutex_unlock() error handling is a good hint that you should
> just factor out this case as a separate function then you can use a goto to
> deal with the unlock cleanly.
Sure, I will fix the error handling in the proper way in the next patch.
>
> > +
> > + set_mask_bits(&data->generic_event_en, msk, field_value);
> > + return 0;
> > case IIO_STEPS:
> > mutex_lock(&data->mutex);
> > if (!data->steps_enabled) {
> > @@ -1028,6 +1127,118 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > }
> > }
> >
> > +static int get_gen_config_reg(enum iio_event_direction dir)
> > +{
> > + switch (dir) {
> > + case IIO_EV_DIR_FALLING:
> > + return BMA400_GEN2INT_CONFIG0;
> > + case IIO_EV_DIR_RISING:
> > + return BMA400_GEN1INT_CONFIG0;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int *val, int *val2)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 reg, duration[2];
> > +
> > + reg = get_gen_config_reg(dir);
> > + if (reg < 0)
> > + return -EINVAL;
> > +
> > + *val2 = 0;
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + val);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_PERIOD:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_read(data->regmap,
> > + reg + BMA400_GEN_CONFIG3_OFF,
> > + duration, sizeof(duration));
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + *val = get_unaligned_be16(duration);
>
> As well as dma safety question, you could just have used a __be16 for
> duration then you can use be16_to_cpu() as you know it is aligned.
For dma safety, do I need to allocate memory by using local kmalloc() or
I can use __be16 local variable for regmap_bulk_read()?
>
> > + return IIO_VAL_INT;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + mutex_lock(&data->mutex);
> > + ret = regmap_read(data->regmap, reg, val);
> > + mutex_unlock(&data->mutex);
> > + if (ret)
> > + return ret;
> > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + enum iio_event_info info,
> > + int val, int val2)
> > +{
> > + struct bma400_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 reg, duration[2];
> > +
> > + reg = get_gen_config_reg(dir);
> > + if (reg < 0)
> > + return -EINVAL;
> > +
> > + switch (info) {
> > + case IIO_EV_INFO_VALUE:
> > + if (val < 1 || val > 255)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > + val);
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_EV_INFO_PERIOD:
> > + if (val < 1 || val > 65535)
> > + return -EINVAL;
> > +
> > + put_unaligned_be16(val, duration);
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_bulk_write(data->regmap,
> > + reg + BMA400_GEN_CONFIG3_OFF,
> > + duration, sizeof(duration));
>
> I can't remember if we are safe or not with bulk_writes but at least
> in theory we might not be and should be using a dma safe buffer.
Here also for regmap_bulk_write() can I allocate the memory locally by using
kmalloc().
>
> Also locking not necessary in various places in here.
I will fix the locking in all the patches in the next series.
>
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + case IIO_EV_INFO_HYSTERESIS:
> > + if (val < 0 || val > 3)
> > + return -EINVAL;
> > +
> > + mutex_lock(&data->mutex);
> > + ret = regmap_update_bits(data->regmap, reg,
> > + BMA400_GEN_HYST_MSK,
> > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > + mutex_unlock(&data->mutex);
> > + return ret;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
>
Thank you,
Jagath
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-18 22:09 ` Jagath Jog J
@ 2022-04-24 15:40 ` Jonathan Cameron
2022-04-25 12:03 ` jagath jogj
0 siblings, 1 reply; 28+ messages in thread
From: Jonathan Cameron @ 2022-04-24 15:40 UTC (permalink / raw)
To: Jagath Jog J; +Cc: dan, andy.shevchenko, linux-iio, linux-kernel
On Tue, 19 Apr 2022 03:39:43 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:
> Hello Jonathan,
>
> Thanks for your suggestions, I will fix the locking and unlocking for all
> patches in the next series.
>
> Please can you guide me for auto build test error reported by kernel test
> robot for set_mask_bits(&data->generic_event_en, msk, field_value);
> in this patch.
>
> On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> > On Tue, 12 Apr 2022 02:01:33 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > > Add support for activity and inactivity events for all axis based on the
> > > threshold, duration and hysteresis value set from the userspace. 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 | 11 ++
> > > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > > 2 files changed, 240 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index bc4641279be3..cbf8035c817e 100644
> > > --- a/drivers/iio/accel/bma400.h
> > > +++ b/drivers/iio/accel/bma400.h
> > > @@ -93,6 +93,17 @@
> > > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > > #define BMA400_ACC_ODR_MIN_HZ 12
> > >
> > > +/* Generic interrupts register */
> > > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > > +#define BMA400_INT_GEN1_MSK BIT(2)
> > > +#define BMA400_INT_GEN2_MSK BIT(3)
> > > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > > +
> > > /*
> > > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > > * converting to micro values for +-2g range.
> > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > index b6c79cfabaa4..226a5f63d1a6 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -79,6 +79,7 @@ struct bma400_data {
> > > int steps_enabled;
> > > bool step_event_en;
> > > bool activity_event_en;
> > > + u8 generic_event_en;
> > > /* Correct time stamp alignment */
> > > struct {
> > > __le16 buff[3];
> > > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > };
> > >
> > > +static const struct iio_event_spec bma400_accel_event[] = {
> > > + {
> > > + .type = IIO_EV_TYPE_MAG,
> > > + .dir = IIO_EV_DIR_FALLING,
> > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_PERIOD) |
> > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > + BIT(IIO_EV_INFO_ENABLE),
> > > + },
> > > + {
> > > + .type = IIO_EV_TYPE_MAG,
> > > + .dir = IIO_EV_DIR_RISING,
> > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > + BIT(IIO_EV_INFO_PERIOD) |
> > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > + BIT(IIO_EV_INFO_ENABLE),
> > > + },
> > > +};
> > > +
> > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > .type = IIO_ACCEL, \
> > > .modified = 1, \
> > > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > > .storagebits = 16, \
> > > .endianness = IIO_LE, \
> > > }, \
> > > + .event_spec = bma400_accel_event, \
> > > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > > }
> > >
> > > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > struct bma400_data *data = iio_priv(indio_dev);
> > >
> > > switch (chan->type) {
> > > + case IIO_ACCEL:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > > + data->generic_event_en);
> > > + case IIO_EV_DIR_FALLING:
> > > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > > + data->generic_event_en);
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > case IIO_STEPS:
> > > return data->step_event_en;
> > > case IIO_ACTIVITY:
> > > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > {
> > > int ret;
> > > struct bma400_data *data = iio_priv(indio_dev);
> > > + int reg, msk, value, field_value;
> > >
> > > switch (chan->type) {
> > > + case IIO_ACCEL:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + reg = BMA400_GEN1INT_CONFIG0;
> > > + msk = BMA400_INT_GEN1_MSK;
> > > + value = 2;
> > > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
> >
> > Hopefully you can use msk in here and the compiler can tell it's constant...
>
> field_value = FIELD_PREP(msk, state);
> is this the fix for error reported by kernel test robot?
No. That issue seems to be triggered by the size of parameters passed
to the underlying cmpxchg behind set_mask_bits.
Specifically riscv cmpxchg only support 32 or 64 bit inputs.
https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/cmpxchg.h#L302
Easiest fix is probably just to make generic_event_en 32 bits.
..
> > > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int *val, int *val2)
> > > +{
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > + int ret;
> > > + u8 reg, duration[2];
> > > +
> > > + reg = get_gen_config_reg(dir);
> > > + if (reg < 0)
> > > + return -EINVAL;
> > > +
> > > + *val2 = 0;
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > + val);
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + return IIO_VAL_INT;
> > > + case IIO_EV_INFO_PERIOD:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_bulk_read(data->regmap,
> > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > + duration, sizeof(duration));
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + *val = get_unaligned_be16(duration);
> >
> > As well as dma safety question, you could just have used a __be16 for
> > duration then you can use be16_to_cpu() as you know it is aligned.
>
> For dma safety, do I need to allocate memory by using local kmalloc() or
> I can use __be16 local variable for regmap_bulk_read()?
Ah. i've been unclear there. Was pointing out that if we didn't need
to force alignment larger for DMA safety then using __be16 would have
ensured that this was correctly aligned. However we do need to
force it so either use a kmalloc'd buffer as you suggest or
play games with an aligned buffer in the iio_priv() region.
Note however that we have a bug in IIO currently as we have
been forcing alignment to L1 cache size which is wrong (not enough in some cases
and far too much in others). I'll be posting
some patches to fix that in the next few days.
>
> >
> > > + return IIO_VAL_INT;
> > > + case IIO_EV_INFO_HYSTERESIS:
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_read(data->regmap, reg, val);
> > > + mutex_unlock(&data->mutex);
> > > + if (ret)
> > > + return ret;
> > > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> > > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > + const struct iio_chan_spec *chan,
> > > + enum iio_event_type type,
> > > + enum iio_event_direction dir,
> > > + enum iio_event_info info,
> > > + int val, int val2)
> > > +{
> > > + struct bma400_data *data = iio_priv(indio_dev);
> > > + int ret;
> > > + u8 reg, duration[2];
> > > +
> > > + reg = get_gen_config_reg(dir);
> > > + if (reg < 0)
> > > + return -EINVAL;
> > > +
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + if (val < 1 || val > 255)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > + val);
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + case IIO_EV_INFO_PERIOD:
> > > + if (val < 1 || val > 65535)
> > > + return -EINVAL;
> > > +
> > > + put_unaligned_be16(val, duration);
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_bulk_write(data->regmap,
> > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > + duration, sizeof(duration));
> >
> > I can't remember if we are safe or not with bulk_writes but at least
> > in theory we might not be and should be using a dma safe buffer.
>
> Here also for regmap_bulk_write() can I allocate the memory locally by using
> kmalloc().
Yes though that's usually a pain to handle in comparison with a buffer in iio_priv()
as you have to free it again.
>
> >
> > Also locking not necessary in various places in here.
>
> I will fix the locking in all the patches in the next series.
>
> >
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + case IIO_EV_INFO_HYSTERESIS:
> > > + if (val < 0 || val > 3)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&data->mutex);
> > > + ret = regmap_update_bits(data->regmap, reg,
> > > + BMA400_GEN_HYST_MSK,
> > > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > > + mutex_unlock(&data->mutex);
> > > + return ret;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +}
> > > +
> >
>
> Thank you,
> Jagath
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 9/9] iio: accel: bma400: Add support for activity and inactivity events
2022-04-24 15:40 ` Jonathan Cameron
@ 2022-04-25 12:03 ` jagath jogj
0 siblings, 0 replies; 28+ messages in thread
From: jagath jogj @ 2022-04-25 12:03 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dan Robertson, Andy Shevchenko, linux-iio,
Linux Kernel Mailing List
Hello Jonathan,
On Sun, Apr 24, 2022 at 9:02 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 19 Apr 2022 03:39:43 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Hello Jonathan,
> >
> > Thanks for your suggestions, I will fix the locking and unlocking for all
> > patches in the next series.
> >
> > Please can you guide me for auto build test error reported by kernel test
> > robot for set_mask_bits(&data->generic_event_en, msk, field_value);
> > in this patch.
> >
> > On Sat, Apr 16, 2022 at 05:55:37PM +0100, Jonathan Cameron wrote:
> > > On Tue, 12 Apr 2022 02:01:33 +0530
> > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > >
> > > > Add support for activity and inactivity events for all axis based on the
> > > > threshold, duration and hysteresis value set from the userspace. 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 | 11 ++
> > > > drivers/iio/accel/bma400_core.c | 229 ++++++++++++++++++++++++++++++++
> > > > 2 files changed, 240 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > index bc4641279be3..cbf8035c817e 100644
> > > > --- a/drivers/iio/accel/bma400.h
> > > > +++ b/drivers/iio/accel/bma400.h
> > > > @@ -93,6 +93,17 @@
> > > > #define BMA400_ACC_ODR_MIN_WHOLE_HZ 25
> > > > #define BMA400_ACC_ODR_MIN_HZ 12
> > > >
> > > > +/* Generic interrupts register */
> > > > +#define BMA400_GEN1INT_CONFIG0 0x3f
> > > > +#define BMA400_GEN2INT_CONFIG0 0x4A
> > > > +#define BMA400_GEN_CONFIG1_OFF 0x01
> > > > +#define BMA400_GEN_CONFIG2_OFF 0x02
> > > > +#define BMA400_GEN_CONFIG3_OFF 0x03
> > > > +#define BMA400_GEN_CONFIG31_OFF 0x04
> > > > +#define BMA400_INT_GEN1_MSK BIT(2)
> > > > +#define BMA400_INT_GEN2_MSK BIT(3)
> > > > +#define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> > > > +
> > > > /*
> > > > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > > > * converting to micro values for +-2g range.
> > > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > > index b6c79cfabaa4..226a5f63d1a6 100644
> > > > --- a/drivers/iio/accel/bma400_core.c
> > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > @@ -79,6 +79,7 @@ struct bma400_data {
> > > > int steps_enabled;
> > > > bool step_event_en;
> > > > bool activity_event_en;
> > > > + u8 generic_event_en;
> > > > /* Correct time stamp alignment */
> > > > struct {
> > > > __le16 buff[3];
> > > > @@ -188,6 +189,25 @@ static const struct iio_event_spec bma400_activity_event = {
> > > > .mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE),
> > > > };
> > > >
> > > > +static const struct iio_event_spec bma400_accel_event[] = {
> > > > + {
> > > > + .type = IIO_EV_TYPE_MAG,
> > > > + .dir = IIO_EV_DIR_FALLING,
> > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > > + BIT(IIO_EV_INFO_PERIOD) |
> > > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > > + BIT(IIO_EV_INFO_ENABLE),
> > > > + },
> > > > + {
> > > > + .type = IIO_EV_TYPE_MAG,
> > > > + .dir = IIO_EV_DIR_RISING,
> > > > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > > + BIT(IIO_EV_INFO_PERIOD) |
> > > > + BIT(IIO_EV_INFO_HYSTERESIS) |
> > > > + BIT(IIO_EV_INFO_ENABLE),
> > > > + },
> > > > +};
> > > > +
> > > > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > > .type = IIO_ACCEL, \
> > > > .modified = 1, \
> > > > @@ -207,6 +227,8 @@ static const struct iio_event_spec bma400_activity_event = {
> > > > .storagebits = 16, \
> > > > .endianness = IIO_LE, \
> > > > }, \
> > > > + .event_spec = bma400_accel_event, \
> > > > + .num_event_specs = ARRAY_SIZE(bma400_accel_event) \
> > > > }
> > > >
> > > > #define BMA400_ACTIVITY_CHANNEL(_chan2) { \
> > > > @@ -954,6 +976,17 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > > > struct bma400_data *data = iio_priv(indio_dev);
> > > >
> > > > switch (chan->type) {
> > > > + case IIO_ACCEL:
> > > > + switch (dir) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + return FIELD_GET(BMA400_INT_GEN1_MSK,
> > > > + data->generic_event_en);
> > > > + case IIO_EV_DIR_FALLING:
> > > > + return FIELD_GET(BMA400_INT_GEN2_MSK,
> > > > + data->generic_event_en);
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > case IIO_STEPS:
> > > > return data->step_event_en;
> > > > case IIO_ACTIVITY:
> > > > @@ -970,8 +1003,74 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > {
> > > > int ret;
> > > > struct bma400_data *data = iio_priv(indio_dev);
> > > > + int reg, msk, value, field_value;
> > > >
> > > > switch (chan->type) {
> > > > + case IIO_ACCEL:
> > > > + switch (dir) {
> > > > + case IIO_EV_DIR_RISING:
> > > > + reg = BMA400_GEN1INT_CONFIG0;
> > > > + msk = BMA400_INT_GEN1_MSK;
> > > > + value = 2;
> > > > + field_value = FIELD_PREP(BMA400_INT_GEN1_MSK, state);
> > >
> > > Hopefully you can use msk in here and the compiler can tell it's constant...
> >
> > field_value = FIELD_PREP(msk, state);
> > is this the fix for error reported by kernel test robot?
> No. That issue seems to be triggered by the size of parameters passed
> to the underlying cmpxchg behind set_mask_bits.
> Specifically riscv cmpxchg only support 32 or 64 bit inputs.
> https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/cmpxchg.h#L302
>
> Easiest fix is probably just to make generic_event_en 32 bits.
In the patch series v4 I have declared generic_event_en as unsigned int.
> ..
>
> > > > +static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir,
> > > > + enum iio_event_info info,
> > > > + int *val, int *val2)
> > > > +{
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 reg, duration[2];
> > > > +
> > > > + reg = get_gen_config_reg(dir);
> > > > + if (reg < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + *val2 = 0;
> > > > + switch (info) {
> > > > + case IIO_EV_INFO_VALUE:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_read(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > > + val);
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + return IIO_VAL_INT;
> > > > + case IIO_EV_INFO_PERIOD:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_bulk_read(data->regmap,
> > > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > > + duration, sizeof(duration));
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + *val = get_unaligned_be16(duration);
> > >
> > > As well as dma safety question, you could just have used a __be16 for
> > > duration then you can use be16_to_cpu() as you know it is aligned.
> >
> > For dma safety, do I need to allocate memory by using local kmalloc() or
> > I can use __be16 local variable for regmap_bulk_read()?
>
> Ah. i've been unclear there. Was pointing out that if we didn't need
> to force alignment larger for DMA safety then using __be16 would have
> ensured that this was correctly aligned. However we do need to
> force it so either use a kmalloc'd buffer as you suggest or
> play games with an aligned buffer in the iio_priv() region.
In v4 series, I have used a __be16 buffer in the iio_priv() region for
duration so
that it can be used for both regmap_bulk_read() and regmap_bulk_write().
>
> Note however that we have a bug in IIO currently as we have
> been forcing alignment to L1 cache size which is wrong (not enough in some cases
> and far too much in others). I'll be posting
> some patches to fix that in the next few days.
>
>
> >
> > >
> > > > + return IIO_VAL_INT;
> > > > + case IIO_EV_INFO_HYSTERESIS:
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_read(data->regmap, reg, val);
> > > > + mutex_unlock(&data->mutex);
> > > > + if (ret)
> > > > + return ret;
> > > > + *val = FIELD_GET(BMA400_GEN_HYST_MSK, *val);
> > > > + return IIO_VAL_INT;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > > > +static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > > + const struct iio_chan_spec *chan,
> > > > + enum iio_event_type type,
> > > > + enum iio_event_direction dir,
> > > > + enum iio_event_info info,
> > > > + int val, int val2)
> > > > +{
> > > > + struct bma400_data *data = iio_priv(indio_dev);
> > > > + int ret;
> > > > + u8 reg, duration[2];
> > > > +
> > > > + reg = get_gen_config_reg(dir);
> > > > + if (reg < 0)
> > > > + return -EINVAL;
> > > > +
> > > > + switch (info) {
> > > > + case IIO_EV_INFO_VALUE:
> > > > + if (val < 1 || val > 255)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_write(data->regmap, reg + BMA400_GEN_CONFIG2_OFF,
> > > > + val);
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + case IIO_EV_INFO_PERIOD:
> > > > + if (val < 1 || val > 65535)
> > > > + return -EINVAL;
> > > > +
> > > > + put_unaligned_be16(val, duration);
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_bulk_write(data->regmap,
> > > > + reg + BMA400_GEN_CONFIG3_OFF,
> > > > + duration, sizeof(duration));
> > >
> > > I can't remember if we are safe or not with bulk_writes but at least
> > > in theory we might not be and should be using a dma safe buffer.
> >
> > Here also for regmap_bulk_write() can I allocate the memory locally by using
> > kmalloc().
>
> Yes though that's usually a pain to handle in comparison with a buffer in iio_priv()
> as you have to free it again.
>
> >
> > >
> > > Also locking not necessary in various places in here.
> >
> > I will fix the locking in all the patches in the next series.
> >
> > >
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + case IIO_EV_INFO_HYSTERESIS:
> > > > + if (val < 0 || val > 3)
> > > > + return -EINVAL;
> > > > +
> > > > + mutex_lock(&data->mutex);
> > > > + ret = regmap_update_bits(data->regmap, reg,
> > > > + BMA400_GEN_HYST_MSK,
> > > > + FIELD_PREP(BMA400_GEN_HYST_MSK, val));
> > > > + mutex_unlock(&data->mutex);
> > > > + return ret;
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > > +
> > >
> >
> > Thank you,
> > Jagath
>
> Thanks,
>
> Jonathan
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread