Re: [PATCH v2 08/14] iio: accel: bma220: migrate to regmap API

Jonathan Cameron
2025-09-10T18:12:35Z

On Wed, 10 Sep 2025 10:57:13 +0300
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Switch to regmap API.
>
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>

Hi Petre

There are a few things in here that seem unrelated to the regmap change
and should be in separate patches where we can review and discuss them more easily.

Thanks,

Jonathan

> diff --git a/drivers/iio/accel/bma220_core.c b/drivers/iio/accel/bma220_core.c
> index 322df516c90a7c645eeca579cae9803eb31caad1..4d8b65ea737a2d5fe74f98da13a582a80874a5af 100644
> --- a/drivers/iio/accel/bma220_core.c
> +++ b/drivers/iio/accel/bma220_core.c

> +#define BMA220_INT_LATCH_MSK GENMASK(6, 4)
> +#define BMA220_INT_LATCH_MAX 0x7

Not particular important here but if this is used in later patches how I'd expect
you can use FIELD_FIT() to achieve the same without an extra define.

>> 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);

Why the move to grabbing timestamps in the thread rather than the top half?

I don't necessarily mind that change but doesn't feel appropriate to have
it in the same patch as the regmap change.

> -err:
> - mutex_unlock(&data->lock);
> + iio_get_time_ns(indio_dev));
> iio_trigger_notify_done(indio_dev->trig);
>
> return IRQ_HANDLED;

> 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_2dt(bma220_scale_table,
> + ARRAY_SIZE(bma220_scale_table),
> + val, val2);

This feels like an unrelated change that belongs in a different patch.

> 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)
> return ret;
> - mutex_unlock(&data->lock);
> + data->range_idx = index;
>
> return 0;
> }
> @@ -206,9 +288,12 @@ static const struct iio_info bma220_info = {
>
> -static int bma220_init(struct spi_device *spi)
> +

one blank line enough and will tidy up the diff a little.

> +static int bma220_init(struct bma220_data *data)
...

> - 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);

As above, I wasn't expecting the switch from iio_pollfunc_store_time in this
patch.

> if (ret < 0) {
> - dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> + dev_err(dev, "iio triggered buffer setup failed\n");
> return ret;

Can use dev_err_probe() to shorten this a little.

> }
>
> - 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");
> +EXPORT_SYMBOL_NS_GPL(bma220_common_probe, "IIO_BOSCH_BMA220");