Switch to regmap API.
Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
---
drivers/iio/accel/Kconfig | 2 +
drivers/iio/accel/bma220.h | 5 +-
drivers/iio/accel/bma220_core.c | 417 ++++++++++++++++++++++++++------
drivers/iio/accel/bma220_spi.c | 12 +-
4 files changed, 354 insertions(+), 82 deletions(-)
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 2cc3075e2688..9b6c35b75948 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -218,6 +218,7 @@ config BMA180
config BMA220
tristate "Bosch BMA220 3-Axis Accelerometer Driver"
+ select REGMAP
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
select BMA220_SPI if SPI
@@ -231,6 +232,7 @@ config BMA220
config BMA220_SPI
tristate
+ select REGMAP_SPI
depends on BMA220
config BMA400
diff --git a/drivers/iio/accel/bma220.h b/drivers/iio/accel/bma220.h
index 0606cf478f5f..5eefa9749d33 100644
--- a/drivers/iio/accel/bma220.h
+++ b/drivers/iio/accel/bma220.h
@@ -8,10 +8,13 @@
#ifndef _BMA220_H
#define _BMA220_H
+#include <linux/regmap.h>
+
#include <linux/iio/iio.h>
+extern const struct regmap_config bma220_spi_regmap_config;
extern const struct dev_pm_ops bma220_pm_ops;
-int bma220_common_probe(struct spi_device *dev);
+int bma220_common_probe(struct device *dev, struct regmap *regmap, int irq);
#endif
diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
index 60fd35637d2d..e6dac2e1cf4d 100644
--- a/drivers/iio/accel/bma220_core.c
+++ b/drivers/iio/accel/bma220_core.c
@@ -3,31 +3,133 @@
* BMA220 Digital triaxial acceleration sensor driver
*
* Copyright (c) 2016,2020 Intel Corporation.
+ * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.ro>
*/
#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/types.h>
-#include <linux/spi/spi.h>
-#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
+#include "bma220.h"
+
+/*
+ * Read-Only Registers
+ */
+
+/* ID registers */
#define BMA220_REG_ID 0x00
+#define BMA220_REG_REVISION_ID 0x01
+
+/* Acceleration registers */
#define BMA220_REG_ACCEL_X 0x02
#define BMA220_REG_ACCEL_Y 0x03
#define BMA220_REG_ACCEL_Z 0x04
+
+/*
+ * Read-write configuration registers
+ */
+#define BMA220_REG_CONF0 0x05
+#define BMA220_HIGH_DUR_MSK GENMASK(5, 0)
+#define BMA220_HIGH_HY_MSK GENMASK(7, 6)
+#define BMA220_REG_CONF1 0x06
+#define BMA220_HIGH_TH_MSK GENMASK(3, 0)
+#define BMA220_LOW_TH_MSK GENMASK(7, 4)
+#define BMA220_REG_CONF2 0x07
+#define BMA220_LOW_DUR_MSK GENMASK(5, 0)
+#define BMA220_LOW_HY_MSK GENMASK(7, 6)
+#define BMA220_REG_CONF3 0x08
+#define BMA220_TT_DUR_MSK GENMASK(2, 0)
+#define BMA220_TT_TH_MSK GENMASK(6, 3)
+#define BMA220_TT_FILT_MSK BIT(7)
+#define BMA220_REG_CONF4 0x09
+#define BMA220_SLOPE_DUR_MSK GENMASK(1, 0)
+#define BMA220_SLOPE_TH_MSK GENMASK(5, 2)
+#define BMA220_SLOPE_FILT_MSK BIT(6)
+#define BMA220_ORIENT_EX_MSK BIT(7)
+#define BMA220_REG_CONF5 0x0a
+#define BMA220_TT_SAMP_MSK GENMASK(1, 0)
+#define BMA220_ORIENT_BLOCKING_MSK GENMASK(3, 2)
+#define BMA220_TIP_EN_MSK BIT(4)
+
+/*
+ * Read-only interrupt flags
+ */
+#define BMA220_REG_IF0 0x0b
+/* interrupt flags */
+#define BMA220_IF_HIGH_SIGN BIT(0)
+#define BMA220_IF_HIGH_FIRST_Z BIT(1)
+#define BMA220_IF_HIGH_FIRST_Y BIT(2)
+#define BMA220_IF_HIGH_FIRST_X BIT(3)
+#define BMA220_IF_ORIENT_INT BIT(7)
+
+#define BMA220_REG_IF1 0x0c
+/* interrupt flags */
+#define BMA220_IF_SLOPE BIT(0)
+#define BMA220_IF_DRDY BIT(1)
+#define BMA220_IF_HIGH BIT(2)
+#define BMA220_IF_LOW BIT(3)
+#define BMA220_IF_TT BIT(4)
+
+/*
+ * Read-write interrupt enable configuration registers
+ */
+#define BMA220_REG_IE0 0x0d
+#define BMA220_INT_EN_TAP_Z_MSK BIT(0)
+#define BMA220_INT_EN_TAP_Y_MSK BIT(1)
+#define BMA220_INT_EN_TAP_X_MSK BIT(2)
+#define BMA220_INT_EN_SLOPE_Z_MSK BIT(3)
+#define BMA220_INT_EN_SLOPE_Y_MSK BIT(4)
+#define BMA220_INT_EN_SLOPE_X_MSK BIT(5)
+#define BMA220_INT_EN_ORIENT_MSK BIT(6)
+#define BMA220_INT_EN_DRDY_MSK BIT(7)
+#define BMA220_REG_IE1 0x0e
+#define BMA220_INT_EN_HIGH_Z_MSK BIT(0)
+#define BMA220_INT_EN_HIGH_Y_MSK BIT(1)
+#define BMA220_INT_EN_HIGH_X_MSK BIT(2)
+#define BMA220_INT_EN_LOW_MSK BIT(3)
+#define BMA220_INT_LATCH_MSK GENMASK(6, 4)
+#define BMA220_INT_LATCH_MAX 0x7
+#define BMA220_INT_RST_MSK BIT(7)
+#define BMA220_INT_LATCH_LEN 8
+#define BMA220_REG_IE2 0x0f
+
+/*
+ * Read-write configuration registers
+ */
+#define BMA220_REG_FILTER 0x10
#define BMA220_REG_RANGE 0x11
+#define BMA220_REG_WDT 0x17
+#define BMA220_WDT_MASK GENMASK(2, 1)
+#define BMA220_WDT_OFF 0x0
+#define BMA220_WDT_1MS BIT(1)
+#define BMA220_WDT_10MS GENMASK(1, 0)
+/*
+ * Read-only state change registers
+ */
#define BMA220_REG_SUSPEND 0x18
+#define BMA220_REG_SOFTRESET 0x19
#define BMA220_CHIP_ID 0xDD
-#define BMA220_READ_MASK BIT(7)
#define BMA220_RANGE_MASK GENMASK(1, 0)
+#define BMA220_FILTER_MASK GENMASK(3, 0)
#define BMA220_SUSPEND_SLEEP 0xFF
#define BMA220_SUSPEND_WAKE 0x00
@@ -61,14 +163,16 @@ static const int bma220_scale_table[][2] = {
};
struct bma220_data {
- struct spi_device *spi_device;
+ struct device *dev;
+ struct regmap *regmap;
struct mutex lock;
+ u8 range_idx;
+ struct iio_trigger *trig;
struct {
s8 chans[3];
/* Ensure timestamp is naturally aligned. */
aligned_s64 timestamp;
- } scan;
- u8 tx_buf[2] __aligned(IIO_DMA_MINALIGN);
+ } scan __aligned(IIO_DMA_MINALIGN);
};
static const struct iio_chan_spec bma220_channels[] = {
@@ -78,35 +182,81 @@ static const struct iio_chan_spec bma220_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(3),
};
-static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
-{
- return spi_w8r8(spi, reg | BMA220_READ_MASK);
-}
-
static const unsigned long bma220_accel_scan_masks[] = {
BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
0
};
+static bool bma220_is_writable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case BMA220_REG_CONF0:
+ case BMA220_REG_CONF1:
+ case BMA220_REG_CONF2:
+ case BMA220_REG_CONF3:
+ case BMA220_REG_CONF4:
+ case BMA220_REG_CONF5:
+ case BMA220_REG_IE0:
+ case BMA220_REG_IE1:
+ case BMA220_REG_IE2:
+ case BMA220_REG_FILTER:
+ case BMA220_REG_RANGE:
+ case BMA220_REG_WDT:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool bma220_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ /* Don't cache any registers. */
+ return true;
+}
+
+const struct regmap_config bma220_spi_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .read_flag_mask = BIT(7),
+ .max_register = BMA220_REG_SOFTRESET,
+ .cache_type = REGCACHE_MAPLE,
+ .writeable_reg = bma220_is_writable_reg,
+ .volatile_reg = bma220_is_volatile_reg,
+};
+EXPORT_SYMBOL_NS(bma220_spi_regmap_config, "IIO_BOSCH_BMA220");
+
+static int bma220_data_rdy_trigger_set_state(struct iio_trigger *trig,
+ bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct bma220_data *data = iio_priv(indio_dev);
+
+ guard(mutex)(&data->lock);
+ return regmap_update_bits(data->regmap, BMA220_REG_IE0,
+ BMA220_INT_EN_DRDY_MSK,
+ FIELD_PREP(BMA220_INT_EN_DRDY_MSK, state));
+}
+
+static const struct iio_trigger_ops bma220_trigger_ops = {
+ .set_trigger_state = &bma220_data_rdy_trigger_set_state,
+ .validate_device = &iio_trigger_validate_own_device,
+};
+
static irqreturn_t bma220_trigger_handler(int irq, void *p)
{
int ret;
struct iio_poll_func *pf = p;
struct iio_dev *indio_dev = pf->indio_dev;
struct bma220_data *data = iio_priv(indio_dev);
- struct spi_device *spi = data->spi_device;
- mutex_lock(&data->lock);
- data->tx_buf[0] = BMA220_REG_ACCEL_X | BMA220_READ_MASK;
- ret = spi_write_then_read(spi, data->tx_buf, 1, &data->scan.chans,
- ARRAY_SIZE(bma220_channels) - 1);
+ ret = regmap_bulk_read(data->regmap, BMA220_REG_ACCEL_X,
+ &data->scan.chans,
+ sizeof(data->scan.chans));
if (ret < 0)
- goto err;
+ return IRQ_NONE;
iio_push_to_buffers_with_ts(indio_dev, &data->scan, sizeof(data->scan),
- pf->timestamp);
-err:
- mutex_unlock(&data->lock);
+ iio_get_time_ns(indio_dev));
iio_trigger_notify_done(indio_dev->trig);
return IRQ_HANDLED;
@@ -117,59 +267,66 @@ static int bma220_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
int ret;
- u8 range_idx;
+ u8 index;
+ unsigned int reg;
struct bma220_data *data = iio_priv(indio_dev);
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = bma220_read_reg(data->spi_device, chan->address);
+ ret = regmap_read(data->regmap, chan->address, ®);
if (ret < 0)
return -EINVAL;
- *val = sign_extend32(ret >> chan->scan_type.shift,
+ *val = sign_extend32(reg >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
- if (ret < 0)
- return ret;
- range_idx = ret & BMA220_RANGE_MASK;
- *val = bma220_scale_table[range_idx][0];
- *val2 = bma220_scale_table[range_idx][1];
+ index = data->range_idx;
+ *val = bma220_scale_table[index][0];
+ *val2 = bma220_scale_table[index][1];
return IIO_VAL_INT_PLUS_MICRO;
}
return -EINVAL;
}
+static int bma220_find_match(const int (*tbl)[2], const int n,
+ const int val, const int val2)
+{
+ int i;
+
+ for (i = 0; i < n; i++) {
+ if (tbl[i][0] == val && tbl[i][1] == val2)
+ return i;
+ }
+
+ return -EINVAL;
+}
+
static int bma220_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
{
- int i;
int ret;
int index = -1;
struct bma220_data *data = iio_priv(indio_dev);
+ guard(mutex)(&data->lock);
+
switch (mask) {
case IIO_CHAN_INFO_SCALE:
- for (i = 0; i < ARRAY_SIZE(bma220_scale_table); i++)
- if (val == bma220_scale_table[i][0] &&
- val2 == bma220_scale_table[i][1]) {
- index = i;
- break;
- }
+ index = bma220_find_match(bma220_scale_table,
+ ARRAY_SIZE(bma220_scale_table),
+ val, val2);
if (index < 0)
return -EINVAL;
- mutex_lock(&data->lock);
- data->tx_buf[0] = BMA220_REG_RANGE;
- data->tx_buf[1] = index;
- ret = spi_write(data->spi_device, data->tx_buf,
- sizeof(data->tx_buf));
+ ret = regmap_update_bits(data->regmap, BMA220_REG_RANGE,
+ BMA220_RANGE_MASK,
+ FIELD_PREP(BMA220_RANGE_MASK, index));
if (ret < 0)
- dev_err(&data->spi_device->dev,
+ dev_err(data->dev,
"failed to set measurement range\n");
- mutex_unlock(&data->lock);
+ data->range_idx = index;
return 0;
}
@@ -199,69 +356,150 @@ static const struct iio_info bma220_info = {
.read_avail = bma220_read_avail,
};
-static int bma220_init(struct spi_device *spi)
+static int bma220_reset(struct bma220_data *data, bool up)
{
- int ret;
+ int i, ret;
+ unsigned int val;
- ret = bma220_read_reg(spi, BMA220_REG_ID);
- if (ret != BMA220_CHIP_ID)
- return -ENODEV;
+ guard(mutex)(&data->lock);
- /* Make sure the chip is powered on */
- ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
- if (ret == BMA220_SUSPEND_WAKE)
- ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
- if (ret < 0)
- return ret;
- if (ret == BMA220_SUSPEND_WAKE)
- return -EBUSY;
+ /**
+ * The chip can be reset by a simple register read.
+ * We need up to 2 register reads of the softreset register
+ * to make sure that the device is in the desired state.
+ */
+ for (i = 0; i < 2; i++) {
+ ret = regmap_read(data->regmap, BMA220_REG_SOFTRESET, &val);
+ if (ret < 0)
+ return ret;
- return 0;
+ if (up && (val == BMA220_SUSPEND_SLEEP))
+ return 0;
+
+ if (!up && (val == BMA220_SUSPEND_WAKE))
+ return 0;
+ }
+
+ return -EBUSY;
}
-static int bma220_power(struct spi_device *spi, bool up)
+static int bma220_power(struct bma220_data *data, bool up)
{
int i, ret;
+ unsigned int val;
+ guard(mutex)(&data->lock);
/**
* The chip can be suspended/woken up by a simple register read.
* So, we need up to 2 register reads of the suspend register
* to make sure that the device is in the desired state.
*/
for (i = 0; i < 2; i++) {
- ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
+ ret = regmap_read(data->regmap, BMA220_REG_SUSPEND, &val);
if (ret < 0)
return ret;
- if (up && ret == BMA220_SUSPEND_SLEEP)
+ if (up && (val == BMA220_SUSPEND_SLEEP))
return 0;
- if (!up && ret == BMA220_SUSPEND_WAKE)
+ if (!up && (val == BMA220_SUSPEND_WAKE))
return 0;
}
return -EBUSY;
}
-static void bma220_deinit(void *spi)
+static int bma220_init(struct bma220_data *data)
+{
+ int ret;
+ unsigned int val;
+ static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
+
+ ret = devm_regulator_bulk_get_enable(data->dev,
+ ARRAY_SIZE(regulator_names),
+ regulator_names);
+ if (ret)
+ return dev_err_probe(data->dev, ret, "Failed to get regulators\n");
+
+ /* Try to read chip_id register. It must return 0xdd. */
+ ret = regmap_read(data->regmap, BMA220_REG_ID, &val);
+ if (ret) {
+ dev_err(data->dev, "Failed to read chip id register\n");
+ return ret;
+ }
+
+ if (val != BMA220_CHIP_ID)
+ return -ENODEV;
+
+ ret = bma220_power(data, true);
+ if (ret) {
+ dev_err(data->dev, "Failed to power-on chip\n");
+ return ret;
+ }
+
+ ret = bma220_reset(data, true);
+ if (ret) {
+ dev_err(data->dev, "Failed to soft reset chip\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void bma220_deinit(void *data_ptr)
{
- bma220_power(spi, false);
+ struct bma220_data *data = data_ptr;
+ int ret;
+
+ ret = bma220_power(data, false);
+ if (ret)
+ dev_warn(data->dev,
+ "Failed to put device into suspend mode (%pe)\n",
+ ERR_PTR(ret));
+}
+
+static irqreturn_t bma220_irq_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct bma220_data *data = iio_priv(indio_dev);
+ int rv;
+ u8 bma220_reg_if[2];
+
+ guard(mutex)(&data->lock);
+ rv = regmap_bulk_read(data->regmap, BMA220_REG_IF0, bma220_reg_if,
+ sizeof(bma220_reg_if));
+ if (rv)
+ return IRQ_NONE;
+
+ if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if[1])) {
+ iio_trigger_poll_nested(data->trig);
+ goto done;
+ }
+
+done:
+
+ return IRQ_HANDLED;
}
-int bma220_common_probe(struct spi_device *spi)
+int bma220_common_probe(struct device *dev, struct regmap *regmap, int irq)
{
int ret;
struct iio_dev *indio_dev;
struct bma220_data *data;
- indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
if (!indio_dev)
return -ENOMEM;
data = iio_priv(indio_dev);
- data->spi_device = spi;
- mutex_init(&data->lock);
+ data->regmap = regmap;
+ data->dev = dev;
+
+ ret = bma220_init(data);
+ if (ret)
+ return ret;
+ mutex_init(&data->lock);
indio_dev->info = &bma220_info;
indio_dev->name = BMA220_DEVICE_NAME;
indio_dev->modes = INDIO_DIRECT_MODE;
@@ -269,38 +507,59 @@ int bma220_common_probe(struct spi_device *spi)
indio_dev->num_channels = ARRAY_SIZE(bma220_channels);
indio_dev->available_scan_masks = bma220_accel_scan_masks;
- ret = bma220_init(data->spi_device);
- if (ret)
- return ret;
+ if (irq > 0) {
+ data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ iio_device_id(indio_dev));
+ if (!data->trig)
+ return -ENOMEM;
+
+ data->trig->ops = &bma220_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,
+ &bma220_irq_handler,
+ 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_add_action_or_reset(&spi->dev, bma220_deinit, spi);
+ ret = devm_add_action_or_reset(data->dev, bma220_deinit, data);
if (ret)
return ret;
- ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
- iio_pollfunc_store_time,
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
bma220_trigger_handler, NULL);
if (ret < 0) {
- dev_err(&spi->dev, "iio triggered buffer setup failed\n");
+ dev_err(dev, "iio triggered buffer setup failed\n");
return ret;
}
- return devm_iio_device_register(&spi->dev, indio_dev);
+ return devm_iio_device_register(dev, indio_dev);
}
EXPORT_SYMBOL_NS(bma220_common_probe, "IIO_BOSCH_BMA220");
static int bma220_suspend(struct device *dev)
{
- struct spi_device *spi = to_spi_device(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct bma220_data *data = iio_priv(indio_dev);
- return bma220_power(spi, false);
+ return bma220_power(data, false);
}
static int bma220_resume(struct device *dev)
{
- struct spi_device *spi = to_spi_device(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct bma220_data *data = iio_priv(indio_dev);
- return bma220_power(spi, true);
+ return bma220_power(data, true);
}
EXPORT_NS_SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume,
IIO_BOSCH_BMA220);
diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index be8348ad0a93..00e3fba9436d 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -9,17 +9,25 @@
#include <linux/errno.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
+#include <linux/regmap.h>
#include <linux/types.h>
#include <linux/spi/spi.h>
-#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include "bma220.h"
static int bma220_spi_probe(struct spi_device *spi)
{
- return bma220_common_probe(spi);
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_spi(spi, &bma220_spi_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(&spi->dev, "failed to create regmap\n");
+ return PTR_ERR(regmap);
+ }
+
+ return bma220_common_probe(&spi->dev, regmap, spi->irq);
}
static const struct spi_device_id bma220_spi_id[] = {
--
2.49.1On Mon, 1 Sep 2025 22:47:29 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:
> Switch to regmap API.
>
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
Hi Petre,
Various comments inline. Biggest one is that the addition of the stuff
for irqs doesn't belong in the patch adding regmap.
> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index 60fd35637d2d..e6dac2e1cf4d 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c
> @@ -3,31 +3,133 @@
> * BMA220 Digital triaxial acceleration sensor driver
> *
> * Copyright (c) 2016,2020 Intel Corporation.
> + * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.ro>
> */
>
> #include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
This feels like an unrelated change.
Good to fix up the headers but for this patch I'd just
expect to see regmap related ones. Do a precursor patch
just before this one to add the others.
> #include <linux/types.h>
> -#include <linux/spi/spi.h>
Can't you drop that in previous patch?
>
> -#include <linux/iio/buffer.h>
Why move this? We tend to keep these in alphabetical order.
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
This is related to the irq stuff that shouldn't be in this patch.
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
>
> +#include "bma220.h"
> +
> +/*
> + * Read-Only Registers
Here as well. Not seeing this as beneficial over the the look
up in the regmap callback that already tells use which are in what
state.
> + */
> +
> +/* ID registers */
> #define BMA220_REG_ID 0x00
> +#define BMA220_REG_REVISION_ID 0x01
> +
> +/* Acceleration registers */
> #define BMA220_REG_ACCEL_X 0x02
> #define BMA220_REG_ACCEL_Y 0x03
> #define BMA220_REG_ACCEL_Z 0x04
> +
> +/*
> + * Read-write configuration registers
I'm not sure we need the read-write part of these comments.
That should be obvious once the regmap config is in place.
> + */
> +static bool bma220_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + /* Don't cache any registers. */
I assume this changes in later patches as setting cache_type is a bit pointless
otherwise!
> + return true;
> +}
> +
> +const struct regmap_config bma220_spi_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .read_flag_mask = BIT(7),
> + .max_register = BMA220_REG_SOFTRESET,
> + .cache_type = REGCACHE_MAPLE,
> + .writeable_reg = bma220_is_writable_reg,
> + .volatile_reg = bma220_is_volatile_reg,
> +};
> +EXPORT_SYMBOL_NS(bma220_spi_regmap_config, "IIO_BOSCH_BMA220");
Any reason not to go NS_GPL? I'd prefer that ideally.
> @@ -199,69 +356,150 @@ static const struct iio_info bma220_info = {
> .read_avail = bma220_read_avail,
> };
>
> -static int bma220_init(struct spi_device *spi)
> +static int bma220_reset(struct bma220_data *data, bool up)
> {
> - int ret;
> + int i, ret;
> + unsigned int val;
>
> - ret = bma220_read_reg(spi, BMA220_REG_ID);
> - if (ret != BMA220_CHIP_ID)
> - return -ENODEV;
> + guard(mutex)(&data->lock);
>
> - /* Make sure the chip is powered on */
> - ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> - if (ret == BMA220_SUSPEND_WAKE)
> - ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> - if (ret < 0)
> - return ret;
> - if (ret == BMA220_SUSPEND_WAKE)
> - return -EBUSY;
> + /**
> + * The chip can be reset by a simple register read.
> + * We need up to 2 register reads of the softreset register
May need? Given you return early if the first one succeeds. If you actually
need two drop the loop and only check values on second read.
> + * to make sure that the device is in the desired state.
> + */
> + for (i = 0; i < 2; i++) {
> + ret = regmap_read(data->regmap, BMA220_REG_SOFTRESET, &val);
> + if (ret < 0)
> + return ret;
>
> - return 0;
> + if (up && (val == BMA220_SUSPEND_SLEEP))
> + return 0;
> +
> + if (!up && (val == BMA220_SUSPEND_WAKE))
> + return 0;
> + }
> +
> + return -EBUSY;
> }
>
> -static void bma220_deinit(void *spi)
> +static int bma220_init(struct bma220_data *data)
> +{
> + int ret;
> + unsigned int val;
> + static const char * const regulator_names[] = { "vddd", "vddio", "vdda" };
> +
> + ret = devm_regulator_bulk_get_enable(data->dev,
> + ARRAY_SIZE(regulator_names),
> + regulator_names);
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Failed to get regulators\n");
I'd have a local struct device *dev = data->dev;
just to shorten the various lines.
> +
> + /* Try to read chip_id register. It must return 0xdd. */
> + ret = regmap_read(data->regmap, BMA220_REG_ID, &val);
> + if (ret) {
> + dev_err(data->dev, "Failed to read chip id register\n");
Use return dev_err_probe(). For things that can't defer it just brings
prettier prints and simpler code. Still worth having!
> + return ret;
> + }
> +
> + if (val != BMA220_CHIP_ID)
> + return -ENODEV;
> +
> + ret = bma220_power(data, true);
> + if (ret) {
> + dev_err(data->dev, "Failed to power-on chip\n");
> + return ret;
return dev_err_probe() here as well..
> + }
> +
> + ret = bma220_reset(data, true);
> + if (ret) {
> + dev_err(data->dev, "Failed to soft reset chip\n");
> + return ret;
and here.
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t bma220_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct bma220_data *data = iio_priv(indio_dev);
> + int rv;
> + u8 bma220_reg_if[2];
> +
> + guard(mutex)(&data->lock);
> + rv = regmap_bulk_read(data->regmap, BMA220_REG_IF0, bma220_reg_if,
> + sizeof(bma220_reg_if));
> + if (rv)
> + return IRQ_NONE;
> +
> + if (FIELD_GET(BMA220_IF_DRDY, bma220_reg_if[1])) {
> + iio_trigger_poll_nested(data->trig);
> + goto done;
> + }
> +
> +done:
> +
> + return IRQ_HANDLED;
> }
>
> -int bma220_common_probe(struct spi_device *spi)
> +int bma220_common_probe(struct device *dev, struct regmap *regmap, int irq)
> {
> int ret;
> struct iio_dev *indio_dev;
> struct bma220_data *data;
>
> - indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> data = iio_priv(indio_dev);
> - data->spi_device = spi;
> - mutex_init(&data->lock);
> + data->regmap = regmap;
> + data->dev = dev;
> +
> + ret = bma220_init(data);
> + if (ret)
> + return ret;
>
> + mutex_init(&data->lock);
#Whilst you are here perhaps switch this to
ret = devm_mutex_init(dev, 7data->lock);
if (ret)
return ret;
It brings only a small benefit in lock debugging but doesn't cost much either
so I'm encouraging it's use in new code or code we are touching anyway.
Fine to just slip that in with this patch rather than spinning another one.
> indio_dev->info = &bma220_info;
> indio_dev->name = BMA220_DEVICE_NAME;
> indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -269,38 +507,59 @@ int bma220_common_probe(struct spi_device *spi)
> indio_dev->num_channels = ARRAY_SIZE(bma220_channels);
> indio_dev->available_scan_masks = bma220_accel_scan_masks;
>
> - ret = bma220_init(data->spi_device);
> - if (ret)
> - return ret;
> + if (irq > 0) {
This next block doesn't seem to have much to do with regmap API conversion.
Wrong patch?
> + 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 = &bma220_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,
> + &bma220_irq_handler,
> + 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_add_action_or_reset(&spi->dev, bma220_deinit, spi);
> + ret = devm_add_action_or_reset(data->dev, bma220_deinit, data);
> if (ret)
> return ret;
>
> - ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> - iio_pollfunc_store_time,
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> bma220_trigger_handler, NULL);
> if (ret < 0) {
> - dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> + dev_err(dev, "iio triggered buffer setup failed\n");
> return ret;
> }
>
> - return devm_iio_device_register(&spi->dev, indio_dev);
> + return devm_iio_device_register(dev, indio_dev);
> }
> EXPORT_SYMBOL_NS(bma220_common_probe, "IIO_BOSCH_BMA220");
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index be8348ad0a93..00e3fba9436d 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -9,17 +9,25 @@
> #include <linux/errno.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> +#include <linux/regmap.h>
> #include <linux/types.h>
> #include <linux/spi/spi.h>
>
> -#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
>
> #include "bma220.h"
>
> static int bma220_spi_probe(struct spi_device *spi)
> {
> - return bma220_common_probe(spi);
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_spi(spi, &bma220_spi_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&spi->dev, "failed to create regmap\n");
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "failed to create regmap\n");
If there are other similar cases in things only called from probe please
switch them to this interface as well (in a separate patch if touching
existing code)
Thanks,
Jonathan
> + return PTR_ERR(regmap);
> + }
> +
> + return bma220_common_probe(&spi->dev, regmap, spi->irq);
> }
On Sun, Sep 07, 2025 at 01:45:06PM +0100, Jonathan Cameron wrote:
> > +static int bma220_reset(struct bma220_data *data, bool up)
> > {
> > + int i, ret;
> > + unsigned int val;
> > + guard(mutex)(&data->lock);
> >
> > + /**
> > + * The chip can be reset by a simple register read.
> > + * We need up to 2 register reads of the softreset register
>
> May need? Given you return early if the first one succeeds. If you actually
> need two drop the loop and only check values on second read.
>
> > + * to make sure that the device is in the desired state.
> > + */
> > + for (i = 0; i < 2; i++) {
> > + ret = regmap_read(data->regmap, BMA220_REG_SOFTRESET, &val);
> > + if (ret < 0)
> > + return ret;
I'm not sure how eloquently I can explain this. the sensor can be in
sleep state / non-sleep state
reset state / non-reset state
(these overlap)
the sensor toggles between these states when the master reads the suspend and
the soft_reset registers respectively.
based on the value read one can tell what was the previous state the sensor was in.
bma220_init() simply places the sensor in the non-sleep AND non-reset modes (and
resets all configuration registers so that we start from a known initial condition)
'may need' is used because the sensor might have been left in an unexpected mode
in the previous session.
we need at most two reads of a register to make sure bma220 ends up in the state we need.
best regards,
peter
On Mon, 8 Sep 2025 06:27:05 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:
> On Sun, Sep 07, 2025 at 01:45:06PM +0100, Jonathan Cameron wrote:
> > > +static int bma220_reset(struct bma220_data *data, bool up)
> > > {
> > > + int i, ret;
> > > + unsigned int val;
> > > + guard(mutex)(&data->lock);
> > >
> > > + /**
> > > + * The chip can be reset by a simple register read.
> > > + * We need up to 2 register reads of the softreset register
> >
> > May need? Given you return early if the first one succeeds. If you actually
> > need two drop the loop and only check values on second read.
> >
> > > + * to make sure that the device is in the desired state.
> > > + */
> > > + for (i = 0; i < 2; i++) {
> > > + ret = regmap_read(data->regmap, BMA220_REG_SOFTRESET, &val);
> > > + if (ret < 0)
> > > + return ret;
>
> I'm not sure how eloquently I can explain this. the sensor can be in
>
> sleep state / non-sleep state
> reset state / non-reset state
> (these overlap)
>
> the sensor toggles between these states when the master reads the suspend and
> the soft_reset registers respectively.
> based on the value read one can tell what was the previous state the sensor was in.
>
> bma220_init() simply places the sensor in the non-sleep AND non-reset modes (and
> resets all configuration registers so that we start from a known initial condition)
>
> 'may need' is used because the sensor might have been left in an unexpected mode
> in the previous session.
> we need at most two reads of a register to make sure bma220 ends up in the state we need.
Fair enough. That is obscure and generally weird. 'may need' seems a valid short comment!
J
>
> best regards,
> peter
>
© 2016 - 2025 Red Hat, Inc.