This patch series adds trigger buffer support with data ready interrupt,
separate channel for step counter, an event for step change interrupt,
activity recognition and activity/inactivity event support.
changes since v2
1. Reordering of header includes in the separate patch.
2. Matching the IIO syntax for multiline comment.
3. Following the preference in the interrupt handler for returning.
4. Add support for activity recognition.
5. Add support for debugfs to access registers from userspace.
6. Add support for activity and inactivity events
changes since v1
1. Added comment section that describes the math for scale calculation.
2. Added separate devm_add_action_or_reset() calls to disable regulator
and to put the sensor in power down mode.
3. Remove the err_reg_disable and out, goto labels and returning directly
if error occurs.
4. Added mutex calls while putting sensor in power down.
5. Added ___cacheline_aligned for device data.
6. Ordering the header includes.
7. Handling erroneous and spurious interrupts in the interrupt handler
by returning IRQ_NONE.
8. Using dev_err_probe() instead of dev_err().
9. Configured the interrupt to open drain.
10. Using le16_to_cpu() to fix the sparse warning.
11. Checking the step change event is enabled or not.
12. Enabling the step change event will also enable the step channel.
13. Using FIELD_GET() instead of bitwise operation.
14. Removal of dead code in the _event_config().
Jagath Jog J (9):
iio: accel: bma400: Fix the scale min and max macro values
iio: accel: bma400: Reordering of header files
iio: accel: bma400: conversion to device-managed function
iio: accel: bma400: Add triggered buffer support
iio: accel: bma400: Add separate channel for step counter
iio: accel: bma400: Add step change event
iio: accel: bma400: Add activity recognition support
iio: accel: bma400: Add debugfs register access support
iio: accel: bma400: Add support for activity and inactivity events
drivers/iio/accel/Kconfig | 2 +
drivers/iio/accel/bma400.h | 49 ++-
drivers/iio/accel/bma400_core.c | 701 +++++++++++++++++++++++++++++---
drivers/iio/accel/bma400_i2c.c | 10 +-
drivers/iio/accel/bma400_spi.c | 8 +-
5 files changed, 703 insertions(+), 67 deletions(-)
--
2.17.1
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 <[email protected]>
---
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
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 <[email protected]>
---
drivers/iio/accel/bma400.h | 2 -
drivers/iio/accel/bma400_core.c | 77 ++++++++++++++++-----------------
drivers/iio/accel/bma400_i2c.c | 8 ----
drivers/iio/accel/bma400_spi.c | 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 <[email protected]>");
MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index 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
Added channel for step counter which can be enable or disable
through the sysfs interface.
Signed-off-by: Jagath Jog J <[email protected]>
---
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
On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <[email protected]> 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
On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote:
> On Wednesday, April 13, 2022, Andy Shevchenko <[email protected]>
> wrote:
>
> >
> >
> > On Tuesday, April 12, 2022, Jagath Jog J <[email protected]> wrote:
> >
> >> Hello Andy,
> >>
> >> On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote:
> >> > On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <[email protected]>
> >> wrote:
> >> > >
> >> > > Added trigger buffer support to read continuous acceleration
> >> > > data from device with data ready interrupt which is mapped
> >> > > to INT1 pin.
> >> >
> >> > Can you explain the locking schema in this driver?
> >> >
> >> > > + /* Configure INT1 pin to open drain */
> >> > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG,
> >> 0x06);
> >> > > + if (ret)
> >> > > + return ret;
> >> >
> >> > No locking (or regmap only locking).
> >>
> >> This is bma400_init() function which will run when probe runs so there is
> >> no
> >> locking in this bma400_init().
> >>
> >> >
> >> > ...
> >> >
> >> > > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger
> >> *trig,
> >> > > + bool state)
> >> > > +{
> >> > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> >> > > + struct bma400_data *data = iio_priv(indio_dev);
> >> > > + int ret;
> >> > > +
> >> > > + ret = regmap_update_bits(data->regmap,
> >> BMA400_INT_CONFIG0_REG,
> >> > > + BMA400_INT_DRDY_MSK,
> >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> >> state));
> >> > > + if (ret)
> >> > > + return ret;
> >> > > +
> >> > > + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> >> > > + BMA400_INT_DRDY_MSK,
> >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> >> state));
> >> > > +}
> >> >
> >> > Ditto.
> >>
> >> Sorry, I missed this.
> >> I will add lock and unlocking in the next patch.
> >>
> >> >
> >> > ...
> >> >
> >> > > + mutex_lock(&data->mutex);
> >> > > +
> >> > > + /* bulk read six registers, with the base being the LSB
> >> register */
> >> > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> >> > > + &data->buffer.buff,
> >> sizeof(data->buffer.buff));
> >> > > + mutex_unlock(&data->mutex);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > But here only above with locking...
> >> >
> >> > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > ...followed by no locking.
> >>
> >> Okay I will add lock in the next patch.
> >>
> >> >
> >> > ...
> >> >
> >> > > + mutex_lock(&data->mutex);
> >> > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG,
> >> &status,
> >> > > + sizeof(status));
> >> > > + mutex_unlock(&data->mutex);
> >> > > + if (ret)
> >> > > + return IRQ_NONE;
> >> >
> >> > And again with locking.
> >> >
> >> > ...
> >> >
> >> > So,
> >> > 1) Does regmap is configured with locking? What for?
> >> > 2) What's the role of data->mutex?
> >>
> >> 1.
> >> NO,
> >
> >
> > Are you sure?
> >
> >
> >> regmap is not configured with locking.
> >> In the remap_config structure variable below these members are not defined
> >> in the driver.
> >>
> >> struct regmap_config {
> >> regmap_lock lock;
> >> regmap_unlock unlock;
> >> void *lock_arg;
> >>
> >>
> > It means that default may be used.
> >
> >
> >> 2.
> >> data->mutex is used to protect the register read, write access from the
> >> device.
> >>
> >> Is the regmap functions handle locking and unlocking internally if these
> >> below
> >> struct members are not defined?
> >
> >
> > Yes. Look at this: https://elixir.bootlin.com/linux/latest/C/ident/
> > disable_locking
Please your advise will be very helpful for this.
I'm going through the same documentation. In this driver, disable_locking is
not initialized.
The functions which are called in the bma400_init() are not protected by mutex
for regmap since bma400_init() will run when the probe runs.
The functions which are called by read_raw() and write_raw() are protected by
mutex for regmap access.
There are some members in the device's private data structure and they are being
accessed in multiple places in the driver.
struct bma400_data {
enum bma400_power_mode power_mode;
struct bma400_sample_freq sample_freq;
int oversampling_ratio;
int scale;
.....
I think mutex is used to protect these above struct members since they are
critical resource, but in the struct bma400_data comment for mutex
is "data register lock".
> >
> >
> >>
> >> regmap_lock lock;
> >> regmap_unlock unlock;
> >> void *lock_arg;
> >>
> >>
> >> >
> >> > --
> >> > With Best Regards,
> >> > Andy Shevchenko
> >>
> >
> >>
>
> You may read the kernel documentation what those fields mean:
> https://elixir.bootlin.com/linux/latest/source/include/linux/regmap.h#L278
>
>
>
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >
>
> --
> With Best Regards,
> Andy Shevchenko
Thank you,
Jagath
Hi Andy,
On Wed, Apr 13, 2022 at 07:53:46PM +0530, Jagath Jog J wrote:
> On Wed, Apr 13, 2022 at 12:24:54AM +0300, Andy Shevchenko wrote:
> > On Wednesday, April 13, 2022, Andy Shevchenko <[email protected]>
> > wrote:
> >
> > >
> > >
> > > On Tuesday, April 12, 2022, Jagath Jog J <[email protected]> wrote:
> > >
> > >> Hello Andy,
> > >>
> > >> On Tue, Apr 12, 2022 at 12:12:21PM +0300, Andy Shevchenko wrote:
> > >> > On Mon, Apr 11, 2022 at 11:31 PM Jagath Jog J <[email protected]>
> > >> wrote:
> > >> > >
> > >> > > Added trigger buffer support to read continuous acceleration
> > >> > > data from device with data ready interrupt which is mapped
> > >> > > to INT1 pin.
> > >> >
> > >> > Can you explain the locking schema in this driver?
> > >> >
> > >> > > + /* Configure INT1 pin to open drain */
> > >> > > + ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG,
> > >> 0x06);
> > >> > > + if (ret)
> > >> > > + return ret;
> > >> >
> > >> > No locking (or regmap only locking).
> > >>
> > >> This is bma400_init() function which will run when probe runs so there is
> > >> no
> > >> locking in this bma400_init().
> > >>
> > >> >
> > >> > ...
> > >> >
> > >> > > +static int bma400_data_rdy_trigger_set_state(struct iio_trigger
> > >> *trig,
> > >> > > + bool state)
> > >> > > +{
> > >> > > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > >> > > + struct bma400_data *data = iio_priv(indio_dev);
> > >> > > + int ret;
> > >> > > +
> > >> > > + ret = regmap_update_bits(data->regmap,
> > >> BMA400_INT_CONFIG0_REG,
> > >> > > + BMA400_INT_DRDY_MSK,
> > >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> > >> state));
> > >> > > + if (ret)
> > >> > > + return ret;
> > >> > > +
> > >> > > + return regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> > >> > > + BMA400_INT_DRDY_MSK,
> > >> > > + FIELD_PREP(BMA400_INT_DRDY_MSK,
> > >> state));
> > >> > > +}
> > >> >
> > >> > Ditto.
> > >>
> > >> Sorry, I missed this.
> > >> I will add lock and unlocking in the next patch.
> > >>
> > >> >
> > >> > ...
> > >> >
> > >> > > + mutex_lock(&data->mutex);
> > >> > > +
> > >> > > + /* bulk read six registers, with the base being the LSB
> > >> register */
> > >> > > + ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> > >> > > + &data->buffer.buff,
> > >> sizeof(data->buffer.buff));
> > >> > > + mutex_unlock(&data->mutex);
> > >> > > + if (ret)
> > >> > > + return IRQ_NONE;
> > >> >
> > >> > But here only above with locking...
> > >> >
> > >> > > + ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> > >> > > + if (ret)
> > >> > > + return IRQ_NONE;
> > >> >
> > >> > ...followed by no locking.
> > >>
> > >> Okay I will add lock in the next patch.
> > >>
> > >> >
> > >> > ...
> > >> >
> > >> > > + mutex_lock(&data->mutex);
> > >> > > + ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG,
> > >> &status,
> > >> > > + sizeof(status));
> > >> > > + mutex_unlock(&data->mutex);
> > >> > > + if (ret)
> > >> > > + return IRQ_NONE;
> > >> >
> > >> > And again with locking.
> > >> >
> > >> > ...
> > >> >
> > >> > So,
> > >> > 1) Does regmap is configured with locking? What for?
> > >> > 2) What's the role of data->mutex?
> > >>
> > >> 1.
> > >> NO,
> > >
> > >
> > > Are you sure?
> > >
> > >
> > >> regmap is not configured with locking.
> > >> In the remap_config structure variable below these members are not defined
> > >> in the driver.
> > >>
> > >> struct regmap_config {
> > >> regmap_lock lock;
> > >> regmap_unlock unlock;
> > >> void *lock_arg;
> > >>
> > >>
> > > It means that default may be used.
> > >
> > >
> > >> 2.
> > >> data->mutex is used to protect the register read, write access from the
> > >> device.
> > >>
> > >> Is the regmap functions handle locking and unlocking internally if these
> > >> below
> > >> struct members are not defined?
> > >
> > >
> > > Yes. Look at this: https://elixir.bootlin.com/linux/latest/C/ident/
> > > disable_locking
>
> Please your advise will be very helpful for this.
>
> I'm going through the same documentation. In this driver, disable_locking is
> not initialized.
>
> The functions which are called in the bma400_init() are not protected by mutex
> for regmap since bma400_init() will run when the probe runs.
>
> The functions which are called by read_raw() and write_raw() are protected by
> mutex for regmap access.
>
> There are some members in the device's private data structure and they are being
> accessed in multiple places in the driver.
>
> struct bma400_data {
> enum bma400_power_mode power_mode;
> struct bma400_sample_freq sample_freq;
> int oversampling_ratio;
> int scale;
> .....
>
> I think mutex is used to protect these above struct members since they are
> critical resource, but in the struct bma400_data comment for mutex
> is "data register lock".
Sorry, I got my mistake about mutex lock and unlocking and I will correct those
in the next patch series.
Thank you
Jagath
>
>
> > >
> > >
> > >>
> > >> regmap_lock lock;
> > >> regmap_unlock unlock;
> > >> void *lock_arg;
> > >>
> > >>
> > >> >
> > >> > --
> > >> > With Best Regards,
> > >> > Andy Shevchenko
> > >>
> > >
> > >>
> >
> > You may read the kernel documentation what those fields mean:
> > https://elixir.bootlin.com/linux/latest/source/include/linux/regmap.h#L278
> >
> >
> >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> > >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
> Thank you,
> Jagath
On Tue, 12 Apr 2022 02:01:28 +0530
Jagath Jog J <[email protected]> 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 <[email protected]>
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;
> +}
On Tue, 12 Apr 2022 02:01:29 +0530
Jagath Jog J <[email protected]> wrote:
> Added channel for step counter which can be enable or disable
> through the sysfs interface.
>
> Signed-off-by: Jagath Jog J <[email protected]>
> ---
> 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;
> }