[PATCH v1 0/5] iio: accel: bma400: Add support for buffer and step

[PATCH v1 0/5] iio: accel: bma400: Add support for buffer and step

[Jagath Jog J]

This patch series adds trigger buffer support with data ready interrupt,
separate channel for step counter and an event for step change interrupt.

Jagath Jog J (5):
  iio: accel: bma400: conversion to device-managed function
  iio: accel: bma400: changing scale min and max macro values
  iio: accel: bma400: Add triggered buffer support
  iio: accel: bma400: Add separate channel for step counter
  iio: accel: bma400: Add step change event

 drivers/iio/accel/Kconfig       |   2 +
 drivers/iio/accel/bma400.h      |  19 +-
 drivers/iio/accel/bma400_core.c | 315 +++++++++++++++++++++++++++++---
 drivers/iio/accel/bma400_i2c.c  |  10 +-
 drivers/iio/accel/bma400_spi.c  |  10 +-
 5 files changed, 304 insertions(+), 52 deletions(-)

-- 
2.17.1

[PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Jagath Jog J]

This is a conversion to device-managed by using devm_iio_device_register
inside probe function, now disabling the regulator and putting bma400 to
power down via a devm_add_action_or_reset() hook.

The dev_set_drvdata() call, bma400_remove() function and hooks in the I2C
and SPI driver struct is removed as devm_iio_device_register function is
used to automatically unregister on driver detach.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/accel/bma400.h      |  2 --
 drivers/iio/accel/bma400_core.c | 39 ++++++++++++++-------------------
 drivers/iio/accel/bma400_i2c.c  |  8 -------
 drivers/iio/accel/bma400_spi.c  |  8 -------
 4 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index c4c8d74155c2..e938da5a57b4 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -94,6 +94,4 @@ extern const struct regmap_config bma400_regmap_config;
 
 int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
 
-void bma400_remove(struct device *dev);
-
 #endif
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index fd2647b728d3..dcc7549c7a0e 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -793,6 +793,19 @@ static const struct iio_info bma400_info = {
 	.write_raw_get_fmt = bma400_write_raw_get_fmt,
 };
 
+static void bma400_disable(void *data_ptr)
+{
+	struct bma400_data *data = data_ptr;
+	int ret;
+
+	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
+	if (ret)
+		dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
+			 ERR_PTR(ret));
+
+	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
 int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
 {
 	struct iio_dev *indio_dev;
@@ -822,31 +835,13 @@ 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);
-}
-EXPORT_SYMBOL(bma400_probe);
-
-void bma400_remove(struct device *dev)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct bma400_data *data = iio_priv(indio_dev);
-	int ret;
-
-	mutex_lock(&data->mutex);
-	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
-	mutex_unlock(&data->mutex);
-
+	ret = devm_add_action_or_reset(dev, bma400_disable, data);
 	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);
+		return ret;
 
-	iio_device_unregister(indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
-EXPORT_SYMBOL(bma400_remove);
+EXPORT_SYMBOL(bma400_probe);
 
 MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
 MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index f50df5310beb..56da06537562 100644
--- a/drivers/iio/accel/bma400_i2c.c
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -27,13 +27,6 @@ static int bma400_i2c_probe(struct i2c_client *client,
 	return bma400_probe(&client->dev, regmap, id->name);
 }
 
-static int bma400_i2c_remove(struct i2c_client *client)
-{
-	bma400_remove(&client->dev);
-
-	return 0;
-}
-
 static const struct i2c_device_id bma400_i2c_ids[] = {
 	{ "bma400", 0 },
 	{ }
@@ -52,7 +45,6 @@ static struct i2c_driver bma400_i2c_driver = {
 		.of_match_table = bma400_of_i2c_match,
 	},
 	.probe    = bma400_i2c_probe,
-	.remove   = bma400_i2c_remove,
 	.id_table = bma400_i2c_ids,
 };
 
diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
index 9f622e37477b..96dc9c215401 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -87,13 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
 	return bma400_probe(&spi->dev, regmap, id->name);
 }
 
-static int bma400_spi_remove(struct spi_device *spi)
-{
-	bma400_remove(&spi->dev);
-
-	return 0;
-}
-
 static const struct spi_device_id bma400_spi_ids[] = {
 	{ "bma400", 0 },
 	{ }
@@ -112,7 +105,6 @@ static struct spi_driver bma400_spi_driver = {
 		.of_match_table = bma400_of_spi_match,
 	},
 	.probe    = bma400_spi_probe,
-	.remove   = bma400_spi_remove,
 	.id_table = bma400_spi_ids,
 };
 
-- 
2.17.1

Re: [PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Jonathan Cameron]

On Sat, 19 Mar 2022 23:40:19 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> This is a conversion to device-managed by using devm_iio_device_register
> inside probe function, now disabling the regulator and putting bma400 to
> power down via a devm_add_action_or_reset() hook.
> 
> 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>

Hi Jagath,

There is an oddity in the existing driver that has lead this in
what I think is the wrong direction.  See below.

> ---
>  drivers/iio/accel/bma400.h      |  2 --
>  drivers/iio/accel/bma400_core.c | 39 ++++++++++++++-------------------
>  drivers/iio/accel/bma400_i2c.c  |  8 -------
>  drivers/iio/accel/bma400_spi.c  |  8 -------
>  4 files changed, 17 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index c4c8d74155c2..e938da5a57b4 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -94,6 +94,4 @@ extern const struct regmap_config bma400_regmap_config;
>  
>  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
>  
> -void bma400_remove(struct device *dev);
> -
>  #endif
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index fd2647b728d3..dcc7549c7a0e 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -793,6 +793,19 @@ static const struct iio_info bma400_info = {
>  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
>  };
>  
> +static void bma400_disable(void *data_ptr)
> +{
> +	struct bma400_data *data = data_ptr;
> +	int ret;
> +
> +	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> +	if (ret)
> +		dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> +			 ERR_PTR(ret));
> +
> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);

So this raised alarm bells.  You almost never want a devm callback to do two things.

The reason it 'looks' like this might be ok is that the driver is currently calling
bma400_set_power_mode(data, POWER_MODE_SLEEP) in error paths during probe.
I think it should be.  If you make that modification first you'll see that to
keep a clean: "only undo things you have done" approach you'll then need
to have a pair of devm_add_action_or_reset() callbacks so as to cover the
disabling of the regulators when the power enabling fails and then to
cover the change to sleep mode if anything else fails.


Jonathan

> +}
> +
>  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
>  {
>  	struct iio_dev *indio_dev;
> @@ -822,31 +835,13 @@ 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);
> -}
> -EXPORT_SYMBOL(bma400_probe);
> -
> -void bma400_remove(struct device *dev)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct bma400_data *data = iio_priv(indio_dev);
> -	int ret;
> -
> -	mutex_lock(&data->mutex);
> -	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> -	mutex_unlock(&data->mutex);
> -
> +	ret = devm_add_action_or_reset(dev, bma400_disable, data);
>  	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);
> +		return ret;
>  
> -	iio_device_unregister(indio_dev);
> +	return devm_iio_device_register(dev, indio_dev);
>  }
> -EXPORT_SYMBOL(bma400_remove);
> +EXPORT_SYMBOL(bma400_probe);
>  
>  MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
>  MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
> diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> index f50df5310beb..56da06537562 100644
> --- a/drivers/iio/accel/bma400_i2c.c
> +++ b/drivers/iio/accel/bma400_i2c.c
> @@ -27,13 +27,6 @@ static int bma400_i2c_probe(struct i2c_client *client,
>  	return bma400_probe(&client->dev, regmap, id->name);
>  }
>  
> -static int bma400_i2c_remove(struct i2c_client *client)
> -{
> -	bma400_remove(&client->dev);
> -
> -	return 0;
> -}
> -
>  static const struct i2c_device_id bma400_i2c_ids[] = {
>  	{ "bma400", 0 },
>  	{ }
> @@ -52,7 +45,6 @@ static struct i2c_driver bma400_i2c_driver = {
>  		.of_match_table = bma400_of_i2c_match,
>  	},
>  	.probe    = bma400_i2c_probe,
> -	.remove   = bma400_i2c_remove,
>  	.id_table = bma400_i2c_ids,
>  };
>  
> diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
> index 9f622e37477b..96dc9c215401 100644
> --- a/drivers/iio/accel/bma400_spi.c
> +++ b/drivers/iio/accel/bma400_spi.c
> @@ -87,13 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
>  	return bma400_probe(&spi->dev, regmap, id->name);
>  }
>  
> -static int bma400_spi_remove(struct spi_device *spi)
> -{
> -	bma400_remove(&spi->dev);
> -
> -	return 0;
> -}
> -
>  static const struct spi_device_id bma400_spi_ids[] = {
>  	{ "bma400", 0 },
>  	{ }
> @@ -112,7 +105,6 @@ static struct spi_driver bma400_spi_driver = {
>  		.of_match_table = bma400_of_spi_match,
>  	},
>  	.probe    = bma400_spi_probe,
> -	.remove   = bma400_spi_remove,
>  	.id_table = bma400_spi_ids,
>  };
>  

Re: [PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Jagath Jog J]

On Sun, Mar 20, 2022 at 05:14:22PM +0000, Jonathan Cameron wrote:
> On Sat, 19 Mar 2022 23:40:19 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 
> > This is a conversion to device-managed by using devm_iio_device_register
> > inside probe function, now disabling the regulator and putting bma400 to
> > power down via a devm_add_action_or_reset() hook.
> > 
> > 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>
> 
> Hi Jagath,
> 
> There is an oddity in the existing driver that has lead this in
> what I think is the wrong direction.  See below.
> 
> > ---
> >  drivers/iio/accel/bma400.h      |  2 --
> >  drivers/iio/accel/bma400_core.c | 39 ++++++++++++++-------------------
> >  drivers/iio/accel/bma400_i2c.c  |  8 -------
> >  drivers/iio/accel/bma400_spi.c  |  8 -------
> >  4 files changed, 17 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index c4c8d74155c2..e938da5a57b4 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -94,6 +94,4 @@ extern const struct regmap_config bma400_regmap_config;
> >  
> >  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
> >  
> > -void bma400_remove(struct device *dev);
> > -
> >  #endif
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index fd2647b728d3..dcc7549c7a0e 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -793,6 +793,19 @@ static const struct iio_info bma400_info = {
> >  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> >  };
> >  
> > +static void bma400_disable(void *data_ptr)
> > +{
> > +	struct bma400_data *data = data_ptr;
> > +	int ret;
> > +
> > +	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > +	if (ret)
> > +		dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> > +			 ERR_PTR(ret));
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> 
> So this raised alarm bells.  You almost never want a devm callback to do two things.
> 
> The reason it 'looks' like this might be ok is that the driver is currently calling
> bma400_set_power_mode(data, POWER_MODE_SLEEP) in error paths during probe.
> I think it should be.  If you make that modification first you'll see that to
> keep a clean: "only undo things you have done" approach you'll then need
> to have a pair of devm_add_action_or_reset() callbacks so as to cover the
> disabling of the regulators when the power enabling fails and then to
> cover the change to sleep mode if anything else fails.

Sure I will add separate functions for regulators disable and power disable
then use them with help of two devm_add_action_or_reset() callbacks in bma400_init
function. Is below is correct?

static void bma400_regulators_disable(void *data_ptr)
{
        struct bma400_data *data = data_ptr;

        regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
}

static void bma400_power_disable(void *data_ptr)
{
        struct bma400_data *data = data_ptr;
        int ret;

        mutex_lock(&data->mutex);
        ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
        if (ret)
                dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
                         ERR_PTR(ret));
        mutex_unlock(&data->mutex);
}

static int bma400_init(struct bma400_data *data)
{
        unsigned int val;
        int 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;
        }

        ret = devm_add_action_or_reset(data->dev, bma400_regulators_disable, data);
        if (ret)
		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;
                }
                /*
                 * TODO: The datasheet waits 1500us here in the example, but
                 * lists 2/ODR as the wakeup time.
                 */
                usleep_range(1500, 2000);
        }

        ret = devm_add_action_or_reset(data->dev, bma400_power_disable, data);
        if (ret)
		return ret;
....

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

> 
> 
> Jonathan
> 
> > +}
> > +
> >  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
> >  {
> >  	struct iio_dev *indio_dev;
> > @@ -822,31 +835,13 @@ 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);
> > -}
> > -EXPORT_SYMBOL(bma400_probe);
> > -
> > -void bma400_remove(struct device *dev)
> > -{
> > -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > -	struct bma400_data *data = iio_priv(indio_dev);
> > -	int ret;
> > -
> > -	mutex_lock(&data->mutex);
> > -	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > -	mutex_unlock(&data->mutex);
> > -
> > +	ret = devm_add_action_or_reset(dev, bma400_disable, data);
> >  	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);
> > +		return ret;
> >  
> > -	iio_device_unregister(indio_dev);
> > +	return devm_iio_device_register(dev, indio_dev);
> >  }
> > -EXPORT_SYMBOL(bma400_remove);
> > +EXPORT_SYMBOL(bma400_probe);
> >  
> >  MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> >  MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
> > diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> > index f50df5310beb..56da06537562 100644
> > --- a/drivers/iio/accel/bma400_i2c.c
> > +++ b/drivers/iio/accel/bma400_i2c.c
> > @@ -27,13 +27,6 @@ static int bma400_i2c_probe(struct i2c_client *client,
> >  	return bma400_probe(&client->dev, regmap, id->name);
> >  }
> >  
> > -static int bma400_i2c_remove(struct i2c_client *client)
> > -{
> > -	bma400_remove(&client->dev);
> > -
> > -	return 0;
> > -}
> > -
> >  static const struct i2c_device_id bma400_i2c_ids[] = {
> >  	{ "bma400", 0 },
> >  	{ }
> > @@ -52,7 +45,6 @@ static struct i2c_driver bma400_i2c_driver = {
> >  		.of_match_table = bma400_of_i2c_match,
> >  	},
> >  	.probe    = bma400_i2c_probe,
> > -	.remove   = bma400_i2c_remove,
> >  	.id_table = bma400_i2c_ids,
> >  };
> >  
> > diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
> > index 9f622e37477b..96dc9c215401 100644
> > --- a/drivers/iio/accel/bma400_spi.c
> > +++ b/drivers/iio/accel/bma400_spi.c
> > @@ -87,13 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
> >  	return bma400_probe(&spi->dev, regmap, id->name);
> >  }
> >  
> > -static int bma400_spi_remove(struct spi_device *spi)
> > -{
> > -	bma400_remove(&spi->dev);
> > -
> > -	return 0;
> > -}
> > -
> >  static const struct spi_device_id bma400_spi_ids[] = {
> >  	{ "bma400", 0 },
> >  	{ }
> > @@ -112,7 +105,6 @@ static struct spi_driver bma400_spi_driver = {
> >  		.of_match_table = bma400_of_spi_match,
> >  	},
> >  	.probe    = bma400_spi_probe,
> > -	.remove   = bma400_spi_remove,
> >  	.id_table = bma400_spi_ids,
> >  };
> >  
> 

Re: [PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Jonathan Cameron]

On Tue, 22 Mar 2022 02:42:41 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> On Sun, Mar 20, 2022 at 05:14:22PM +0000, Jonathan Cameron wrote:
> > On Sat, 19 Mar 2022 23:40:19 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >   
> > > This is a conversion to device-managed by using devm_iio_device_register
> > > inside probe function, now disabling the regulator and putting bma400 to
> > > power down via a devm_add_action_or_reset() hook.
> > > 
> > > 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>  
> > 
> > Hi Jagath,
> > 
> > There is an oddity in the existing driver that has lead this in
> > what I think is the wrong direction.  See below.
> >   
> > > ---
> > >  drivers/iio/accel/bma400.h      |  2 --
> > >  drivers/iio/accel/bma400_core.c | 39 ++++++++++++++-------------------
> > >  drivers/iio/accel/bma400_i2c.c  |  8 -------
> > >  drivers/iio/accel/bma400_spi.c  |  8 -------
> > >  4 files changed, 17 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index c4c8d74155c2..e938da5a57b4 100644
> > > --- a/drivers/iio/accel/bma400.h
> > > +++ b/drivers/iio/accel/bma400.h
> > > @@ -94,6 +94,4 @@ extern const struct regmap_config bma400_regmap_config;
> > >  
> > >  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name);
> > >  
> > > -void bma400_remove(struct device *dev);
> > > -
> > >  #endif
> > > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > > index fd2647b728d3..dcc7549c7a0e 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -793,6 +793,19 @@ static const struct iio_info bma400_info = {
> > >  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
> > >  };
> > >  
> > > +static void bma400_disable(void *data_ptr)
> > > +{
> > > +	struct bma400_data *data = data_ptr;
> > > +	int ret;
> > > +
> > > +	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > > +	if (ret)
> > > +		dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> > > +			 ERR_PTR(ret));
> > > +
> > > +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);  
> > 
> > So this raised alarm bells.  You almost never want a devm callback to do two things.
> > 
> > The reason it 'looks' like this might be ok is that the driver is currently calling
> > bma400_set_power_mode(data, POWER_MODE_SLEEP) in error paths during probe.
> > I think it should be.  If you make that modification first you'll see that to
> > keep a clean: "only undo things you have done" approach you'll then need
> > to have a pair of devm_add_action_or_reset() callbacks so as to cover the
> > disabling of the regulators when the power enabling fails and then to
> > cover the change to sleep mode if anything else fails.  
> 
> Sure I will add separate functions for regulators disable and power disable
> then use them with help of two devm_add_action_or_reset() callbacks in bma400_init
> function. Is below is correct?
> 
> static void bma400_regulators_disable(void *data_ptr)
> {
>         struct bma400_data *data = data_ptr;
> 
>         regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> }
> 
> static void bma400_power_disable(void *data_ptr)
> {
>         struct bma400_data *data = data_ptr;
>         int ret;
> 
>         mutex_lock(&data->mutex);
>         ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
>         if (ret)
>                 dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
>                          ERR_PTR(ret));
>         mutex_unlock(&data->mutex);
> }
> 
> static int bma400_init(struct bma400_data *data)
> {
>         unsigned int val;
>         int 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;
>         }
> 
>         ret = devm_add_action_or_reset(data->dev, bma400_regulators_disable, data);
>         if (ret)
> 		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;
>                 }
>                 /*
>                  * TODO: The datasheet waits 1500us here in the example, but
>                  * lists 2/ODR as the wakeup time.
>                  */
>                 usleep_range(1500, 2000);
>         }
> 
>         ret = devm_add_action_or_reset(data->dev, bma400_power_disable, data);
>         if (ret)
> 		return ret;
> ....
> 
>         return regmap_write(data->regmap, BMA400_ACC_CONFIG2_REG, 0x00);
> 
> err_reg_disable:
>         regulator_bulk_disable(ARRAY_SIZE(data->regulators),
>                                data->regulators);

No route to these error paths any more. So drop this.

Otherwise looks good to me.



> out:
Note an out label is always a bad thing.  Just return directly
instead of goto out;

Thanks,

Jonathan

>         return ret;
> }
> 
> > 
> > 
> > Jonathan
> >   
> > > +}
> > > +
> > >  int bma400_probe(struct device *dev, struct regmap *regmap, const char *name)
> > >  {
> > >  	struct iio_dev *indio_dev;
> > > @@ -822,31 +835,13 @@ 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);
> > > -}
> > > -EXPORT_SYMBOL(bma400_probe);
> > > -
> > > -void bma400_remove(struct device *dev)
> > > -{
> > > -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > -	struct bma400_data *data = iio_priv(indio_dev);
> > > -	int ret;
> > > -
> > > -	mutex_lock(&data->mutex);
> > > -	ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > > -	mutex_unlock(&data->mutex);
> > > -
> > > +	ret = devm_add_action_or_reset(dev, bma400_disable, data);
> > >  	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);
> > > +		return ret;
> > >  
> > > -	iio_device_unregister(indio_dev);
> > > +	return devm_iio_device_register(dev, indio_dev);
> > >  }
> > > -EXPORT_SYMBOL(bma400_remove);
> > > +EXPORT_SYMBOL(bma400_probe);
> > >  
> > >  MODULE_AUTHOR("Dan Robertson <dan@dlrobertson.com>");
> > >  MODULE_DESCRIPTION("Bosch BMA400 triaxial acceleration sensor core");
> > > diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
> > > index f50df5310beb..56da06537562 100644
> > > --- a/drivers/iio/accel/bma400_i2c.c
> > > +++ b/drivers/iio/accel/bma400_i2c.c
> > > @@ -27,13 +27,6 @@ static int bma400_i2c_probe(struct i2c_client *client,
> > >  	return bma400_probe(&client->dev, regmap, id->name);
> > >  }
> > >  
> > > -static int bma400_i2c_remove(struct i2c_client *client)
> > > -{
> > > -	bma400_remove(&client->dev);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  static const struct i2c_device_id bma400_i2c_ids[] = {
> > >  	{ "bma400", 0 },
> > >  	{ }
> > > @@ -52,7 +45,6 @@ static struct i2c_driver bma400_i2c_driver = {
> > >  		.of_match_table = bma400_of_i2c_match,
> > >  	},
> > >  	.probe    = bma400_i2c_probe,
> > > -	.remove   = bma400_i2c_remove,
> > >  	.id_table = bma400_i2c_ids,
> > >  };
> > >  
> > > diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
> > > index 9f622e37477b..96dc9c215401 100644
> > > --- a/drivers/iio/accel/bma400_spi.c
> > > +++ b/drivers/iio/accel/bma400_spi.c
> > > @@ -87,13 +87,6 @@ static int bma400_spi_probe(struct spi_device *spi)
> > >  	return bma400_probe(&spi->dev, regmap, id->name);
> > >  }
> > >  
> > > -static int bma400_spi_remove(struct spi_device *spi)
> > > -{
> > > -	bma400_remove(&spi->dev);
> > > -
> > > -	return 0;
> > > -}
> > > -
> > >  static const struct spi_device_id bma400_spi_ids[] = {
> > >  	{ "bma400", 0 },
> > >  	{ }
> > > @@ -112,7 +105,6 @@ static struct spi_driver bma400_spi_driver = {
> > >  		.of_match_table = bma400_of_spi_match,
> > >  	},
> > >  	.probe    = bma400_spi_probe,
> > > -	.remove   = bma400_spi_remove,
> > >  	.id_table = bma400_spi_ids,
> > >  };
> > >    
> >   

Re: [PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> This is a conversion to device-managed by using devm_iio_device_register
> inside probe function, now disabling the regulator and putting bma400 to
> power down via a devm_add_action_or_reset() hook.
>
> 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.

...

> +static void bma400_disable(void *data_ptr)
> +{
> +       struct bma400_data *data = data_ptr;
> +       int ret;

> +       ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> +       if (ret)
> +               dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> +                        ERR_PTR(ret));

By what reason did you remove mutex around this call?

> +       regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] iio: accel: bma400: conversion to device-managed function

[Jagath Jog J]

Hello Andy,

On Mon, Mar 21, 2022 at 10:30:26AM +0200, Andy Shevchenko wrote:
> On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > This is a conversion to device-managed by using devm_iio_device_register
> > inside probe function, now disabling the regulator and putting bma400 to
> > power down via a devm_add_action_or_reset() hook.
> >
> > 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.
> 
> ...
> 
> > +static void bma400_disable(void *data_ptr)
> > +{
> > +       struct bma400_data *data = data_ptr;
> > +       int ret;
> 
> > +       ret = bma400_set_power_mode(data, POWER_MODE_SLEEP);
> > +       if (ret)
> > +               dev_warn(data->dev, "Failed to put device into sleep mode (%pe)\n",
> > +                        ERR_PTR(ret));
> 
> By what reason did you remove mutex around this call?
> 
> > +       regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> > +}
Sorry my mistake, I missed this.
I will include the mutex calls in next patch version.
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[PATCH v1 2/5] iio: accel: bma400: changing scale min and max macro values

[Jagath Jog J]

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index e938da5a57b4..cfc2c9bacec8 100644
--- a/drivers/iio/accel/bma400.h
+++ b/drivers/iio/accel/bma400.h
@@ -83,8 +83,8 @@
 #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
+#define BMA400_SCALE_MIN            9583
+#define BMA400_SCALE_MAX            76669
 
 #define BMA400_NUM_REGULATORS       2
 #define BMA400_VDD_REGULATOR        0
-- 
2.17.1

Re: [PATCH v1 2/5] iio: accel: bma400: changing scale min and max macro values

[Jonathan Cameron]

On Sat, 19 Mar 2022 23:40:20 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Changing the scale macro values to match the bma400 sensitivity
> for 1 LSB of all the available ranges.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
This needs a fixes tag and should be the first patch in the series.
I'd also like the maths to be described as these numbers are (I assume)
calculated based on the range and the bit depth of the sensor?
If you can add a comment in the code, then we'll have a convenient
reference alongside the numbers.

> ---
>  drivers/iio/accel/bma400.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index e938da5a57b4..cfc2c9bacec8 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -83,8 +83,8 @@
>  #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
> +#define BMA400_SCALE_MIN            9583
> +#define BMA400_SCALE_MAX            76669
>  
>  #define BMA400_NUM_REGULATORS       2
>  #define BMA400_VDD_REGULATOR        0

[PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Jagath Jog J]

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 | 161 +++++++++++++++++++++++++++++++-
 drivers/iio/accel/bma400_i2c.c  |   2 +-
 drivers/iio/accel/bma400_spi.c  |   2 +-
 5 files changed, 169 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 49587c992a6d..0eb379578e00 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -177,6 +177,8 @@ config BMA220
 config BMA400
 	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
 	select REGMAP
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	select BMA400_I2C if I2C
 	select BMA400_SPI if SPI
 	help
diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index cfc2c9bacec8..b306a5ad513a 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
 
@@ -92,6 +99,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 dcc7549c7a0e..797403c7dd85 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -20,6 +20,12 @@
 #include <linux/mutex.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #include "bma400.h"
 
@@ -61,6 +67,13 @@ struct bma400_data {
 	struct bma400_sample_freq sample_freq;
 	int oversampling_ratio;
 	int scale;
+	struct iio_trigger *trig;
+	/* Correct time stamp alignment */
+	struct {
+		__be16 buff[3];
+		u8 temperature;
+		s64 ts __aligned(8);
+	} buffer;
 };
 
 static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
@@ -152,7 +165,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
 	{ }
 };
 
-#define BMA400_ACC_CHANNEL(_axis) { \
+#define BMA400_ACC_CHANNEL(_index, _axis) { \
 	.type = IIO_ACCEL, \
 	.modified = 1, \
 	.channel2 = IIO_MOD_##_axis, \
@@ -164,17 +177,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
 		BIT(IIO_CHAN_INFO_SCALE) | \
 		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
 	.ext_info = bma400_ext_info, \
+	.scan_index = _index,	\
+	.scan_type = {		\
+		.sign = 's',	\
+		.realbits = 12,		\
+		.storagebits = 16,	\
+		.endianness = IIO_LE,	\
+	},				\
 }
 
 static const struct iio_chan_spec bma400_channels[] = {
-	BMA400_ACC_CHANNEL(X),
-	BMA400_ACC_CHANNEL(Y),
-	BMA400_ACC_CHANNEL(Z),
+	BMA400_ACC_CHANNEL(0, X),
+	BMA400_ACC_CHANNEL(1, Y),
+	BMA400_ACC_CHANNEL(2, Z),
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 8,
+			.storagebits = 8,
+			.endianness = IIO_LE,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
 static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
@@ -632,6 +660,11 @@ static int bma400_init(struct bma400_data *data)
 	if (ret)
 		goto err_reg_disable;
 
+	/* Configure INT-1 pin to push pull */
+	ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x02);
+	if (ret)
+		goto err_reg_disable;
+
 	/*
 	 * Once the interrupt engine is supported we might use the
 	 * data_src_reg, but for now ensure this is set to the
@@ -786,6 +819,33 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct bma400_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
+				 BMA400_INT_DRDY_MSK,
+				 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
+				 BMA400_INT_DRDY_MSK,
+				 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const unsigned long bma400_avail_scan_masks[] = {
+	GENMASK(3, 0),
+	0,
+};
+
 static const struct iio_info bma400_info = {
 	.read_raw          = bma400_read_raw,
 	.read_avail        = bma400_read_avail,
@@ -793,6 +853,63 @@ static const struct iio_info bma400_info = {
 	.write_raw_get_fmt = bma400_write_raw_get_fmt,
 };
 
+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, 3 * sizeof(__be16));
+	mutex_unlock(&data->mutex);
+	if (ret)
+		goto out;
+
+	ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
+	if (ret)
+		goto out;
+
+	data->buffer.temperature = temp;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+					   iio_get_time_ns(indio_dev));
+
+out:
+	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)
+		goto out;
+
+	if (status & BMA400_INT_DRDY_MSK)
+		iio_trigger_poll_chained(data->trig);
+
+out:
+	return IRQ_HANDLED;
+}
+
 static void bma400_disable(void *data_ptr)
 {
 	struct bma400_data *data = data_ptr;
@@ -806,7 +923,8 @@ static void bma400_disable(void *data_ptr)
 	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
 }
 
-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)
 {
 	struct iio_dev *indio_dev;
 	struct bma400_data *data;
@@ -833,12 +951,45 @@ 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;
 
 	ret = devm_add_action_or_reset(dev, bma400_disable, data);
 	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 = &bma400_trigger_ops;
+		iio_trigger_set_drvdata(data->trig, indio_dev);
+
+		ret = devm_iio_trigger_register(data->dev, data->trig);
+		if (ret) {
+			dev_err(dev, "iio trigger register failed\n");
+			return ret;
+		}
+		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) {
+			dev_err(dev, "request irq %d failed\n", irq);
+			return ret;
+		}
+	}
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+					      &bma400_trigger_handler, NULL);
+	if (ret) {
+		dev_err(dev, "iio triggered buffer setup failed\n");
+		return ret;
+	}
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL(bma400_probe);
diff --git a/drivers/iio/accel/bma400_i2c.c b/drivers/iio/accel/bma400_i2c.c
index 56da06537562..49b0ae13fdc8 100644
--- a/drivers/iio/accel/bma400_i2c.c
+++ b/drivers/iio/accel/bma400_i2c.c
@@ -24,7 +24,7 @@ static int bma400_i2c_probe(struct i2c_client *client,
 		return PTR_ERR(regmap);
 	}
 
-	return bma400_probe(&client->dev, regmap, id->name);
+	return bma400_probe(&client->dev, regmap, client->irq, id->name);
 }
 
 static const struct i2c_device_id bma400_i2c_ids[] = {
diff --git a/drivers/iio/accel/bma400_spi.c b/drivers/iio/accel/bma400_spi.c
index 96dc9c215401..2957ebc51543 100644
--- a/drivers/iio/accel/bma400_spi.c
+++ b/drivers/iio/accel/bma400_spi.c
@@ -84,7 +84,7 @@ static int bma400_spi_probe(struct spi_device *spi)
 	if (ret)
 		dev_err(&spi->dev, "Failed to read chip id register\n");
 
-	return bma400_probe(&spi->dev, regmap, id->name);
+	return bma400_probe(&spi->dev, regmap, spi->irq, id->name);
 }
 
 static const struct spi_device_id bma400_spi_ids[] = {
-- 
2.17.1

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[kernel test robot]

Hi Jagath,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.17-rc8]
[cannot apply to jic23-iio/togreg next-20220318]
[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/0day-ci/linux/commits/Jagath-Jog-J/iio-accel-bma400-Add-support-for-buffer-and-step/20220320-021114
base:    09688c0166e76ce2fb85e86b9d99be8b0084cdf9
config: i386-randconfig-s001 (https://download.01.org/0day-ci/archive/20220320/202203201124.OLMstZaW-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/8dc9a46d5af9a53917108ce2b103b3d9a50986a5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jagath-Jog-J/iio-accel-bma400-Add-support-for-buffer-and-step/20220320-021114
        git checkout 8dc9a46d5af9a53917108ce2b103b3d9a50986a5
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 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>


sparse warnings: (new ones prefixed by >>)
>> drivers/iio/accel/bma400_core.c:906:13: sparse: sparse: restricted __le16 degrades to integer

vim +906 drivers/iio/accel/bma400_core.c

   891	
   892	static irqreturn_t bma400_interrupt(int irq, void *private)
   893	{
   894		struct iio_dev *indio_dev = private;
   895		struct bma400_data *data = iio_priv(indio_dev);
   896		int ret;
   897		__le16 status;
   898	
   899		mutex_lock(&data->mutex);
   900		ret = regmap_bulk_read(data->regmap, BMA400_INT_STAT0_REG, &status,
   901				       sizeof(status));
   902		mutex_unlock(&data->mutex);
   903		if (ret)
   904			goto out;
   905	
 > 906		if (status & BMA400_INT_DRDY_MSK)
   907			iio_trigger_poll_chained(data->trig);
   908	
   909	out:
   910		return IRQ_HANDLED;
   911	}
   912	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Jonathan Cameron]

On Sat, 19 Mar 2022 23:40:21 +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>
> ---
>  drivers/iio/accel/Kconfig       |   2 +
>  drivers/iio/accel/bma400.h      |  10 +-
>  drivers/iio/accel/bma400_core.c | 161 +++++++++++++++++++++++++++++++-
>  drivers/iio/accel/bma400_i2c.c  |   2 +-
>  drivers/iio/accel/bma400_spi.c  |   2 +-
>  5 files changed, 169 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 49587c992a6d..0eb379578e00 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -177,6 +177,8 @@ config BMA220
>  config BMA400
>  	tristate "Bosch BMA400 3-Axis Accelerometer Driver"
>  	select REGMAP
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	select BMA400_I2C if I2C
>  	select BMA400_SPI if SPI
>  	help
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index cfc2c9bacec8..b306a5ad513a 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
>  
> @@ -92,6 +99,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 dcc7549c7a0e..797403c7dd85 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -20,6 +20,12 @@
>  #include <linux/mutex.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #include "bma400.h"
>  
> @@ -61,6 +67,13 @@ struct bma400_data {
>  	struct bma400_sample_freq sample_freq;
>  	int oversampling_ratio;
>  	int scale;
> +	struct iio_trigger *trig;
> +	/* Correct time stamp alignment */
> +	struct {
> +		__be16 buff[3];
> +		u8 temperature;
> +		s64 ts __aligned(8);
> +	} buffer;
you are doing bulk reads from an spi device into here.
There is a long running discussion about what we can assume about need for DMA
safety when regmap is involved.  Current state is we can't assume we don't need
to be DMA safe.  As such this should be in a separate cacheline from anything
that might be touched concurrently with DMA.

Mark buffer ___cacheline_aligned; and we should be fine.

If curious, Wolfram Sang did a good talk on this at ELCE a few years back that
google should find for you. It's an interesting little corner of horribleness :)

>  };
>  
>  static bool bma400_is_writable_reg(struct device *dev, unsigned int reg)
> @@ -152,7 +165,7 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
>  	{ }
>  };
>  
> -#define BMA400_ACC_CHANNEL(_axis) { \
> +#define BMA400_ACC_CHANNEL(_index, _axis) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
>  	.channel2 = IIO_MOD_##_axis, \
> @@ -164,17 +177,32 @@ static const struct iio_chan_spec_ext_info bma400_ext_info[] = {
>  		BIT(IIO_CHAN_INFO_SCALE) | \
>  		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>  	.ext_info = bma400_ext_info, \
> +	.scan_index = _index,	\
> +	.scan_type = {		\
> +		.sign = 's',	\
> +		.realbits = 12,		\
> +		.storagebits = 16,	\
> +		.endianness = IIO_LE,	\
> +	},				\
>  }
>  
>  static const struct iio_chan_spec bma400_channels[] = {
> -	BMA400_ACC_CHANNEL(X),
> -	BMA400_ACC_CHANNEL(Y),
> -	BMA400_ACC_CHANNEL(Z),
> +	BMA400_ACC_CHANNEL(0, X),
> +	BMA400_ACC_CHANNEL(1, Y),
> +	BMA400_ACC_CHANNEL(2, Z),
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = 3,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 8,
> +			.storagebits = 8,
> +			.endianness = IIO_LE,
> +		},
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
>  static int bma400_get_temp_reg(struct bma400_data *data, int *val, int *val2)
> @@ -632,6 +660,11 @@ static int bma400_init(struct bma400_data *data)
>  	if (ret)
>  		goto err_reg_disable;
>  
> +	/* Configure INT-1 pin to push pull */

Hmm. We should be getting the requirements for using this pin from DT, though
I can't immediately think how to do it.  If the interrupt controller
is happy with open drain, then we should do that as well. Ultimately I think
this code will be happy with shared interrupts so lets not make it harder
than we need to.

> +	ret = regmap_write(data->regmap, BMA400_INT_IO_CTRL_REG, 0x02);
> +	if (ret)
> +		goto err_reg_disable;
> +
>  	/*
>  	 * Once the interrupt engine is supported we might use the
>  	 * data_src_reg, but for now ensure this is set to the
> @@ -786,6 +819,33 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct bma400_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG0_REG,
> +				 BMA400_INT_DRDY_MSK,
> +				 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(data->regmap, BMA400_INT1_MAP_REG,
> +				 BMA400_INT_DRDY_MSK,
> +				 FIELD_PREP(BMA400_INT_DRDY_MSK, state));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const unsigned long bma400_avail_scan_masks[] = {
> +	GENMASK(3, 0),
> +	0,
> +};
> +
>  static const struct iio_info bma400_info = {
>  	.read_raw          = bma400_read_raw,
>  	.read_avail        = bma400_read_avail,
> @@ -793,6 +853,63 @@ static const struct iio_info bma400_info = {
>  	.write_raw_get_fmt = bma400_write_raw_get_fmt,
>  };
>  
> +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, 3 * sizeof(__be16));
> +	mutex_unlock(&data->mutex);
> +	if (ret)
> +		goto out;
> +
> +	ret = regmap_read(data->regmap, BMA400_TEMP_DATA_REG, &temp);
> +	if (ret)
> +		goto out;
> +
> +	data->buffer.temperature = temp;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> +					   iio_get_time_ns(indio_dev));
> +
> +out:
> +	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)
> +		goto out;
> +
> +	if (status & BMA400_INT_DRDY_MSK)

0-day pointed this out. You need an le16_to_cpu()

> +		iio_trigger_poll_chained(data->trig);
> +
> +out:
> +	return IRQ_HANDLED;
> +}
> +
>  static void bma400_disable(void *data_ptr)
>  {
>  	struct bma400_data *data = data_ptr;
> @@ -806,7 +923,8 @@ static void bma400_disable(void *data_ptr)
>  	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>  }
>  
> -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)
>  {
>  	struct iio_dev *indio_dev;
>  	struct bma400_data *data;
> @@ -833,12 +951,45 @@ 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;
>  
>  	ret = devm_add_action_or_reset(dev, bma400_disable, data);
>  	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 = &bma400_trigger_ops;
> +		iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(data->dev, data->trig);
> +		if (ret) {
> +			dev_err(dev, "iio trigger register failed\n");
> +			return ret;
> +		}
> +		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) {
> +			dev_err(dev, "request irq %d failed\n", irq);
> +			return ret;
> +		}
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> +					      &bma400_trigger_handler, NULL);
> +	if (ret) {
> +		dev_err(dev, "iio triggered buffer setup failed\n");
> +		return ret;
> +	}
>  	return devm_iio_device_register(dev, indio_dev);
>  }

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 8:10 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.

...

>  #include <linux/mutex.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>

It would be nice to keep the above in order.

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>

These ones, possibly including iio headers from the above piece, are
good to be grouped together here with a blank line in between the
above part and iio/*.

...

> +static const unsigned long bma400_avail_scan_masks[] = {
> +       GENMASK(3, 0),

> +       0,

No need to have a comma in terminator entry.

> +};

...

> +       ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> +                              &data->buffer.buff, 3 * sizeof(__be16));

sizeof(buff)

...

> +out:

A useless label. Moreover this raises a question: why is it okay to
always mark IRQ as handled?

> +       return IRQ_HANDLED;

...

> +                       dev_err(dev, "iio trigger register failed\n");
> +                       return ret;

return dev_err_probe();

...

> +                       dev_err(dev, "request irq %d failed\n", irq);
> +                       return ret;

Ditto.

...

> +               dev_err(dev, "iio triggered buffer setup failed\n");
> +               return ret;

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Jagath Jog J]

Hello Andy,

On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote:
> On Sat, Mar 19, 2022 at 8:10 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.
> 
> ...
> 
> >  #include <linux/mutex.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> 
> It would be nice to keep the above in order.
> 
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> 
> These ones, possibly including iio headers from the above piece, are
> good to be grouped together here with a blank line in between the
> above part and iio/*.
> 
> ...
> 
> > +static const unsigned long bma400_avail_scan_masks[] = {
> > +       GENMASK(3, 0),
> 
> > +       0,
> 
> No need to have a comma in terminator entry.
> 
> > +};
> 
> ...
> 
> > +       ret = regmap_bulk_read(data->regmap, BMA400_X_AXIS_LSB_REG,
> > +                              &data->buffer.buff, 3 * sizeof(__be16));
> 
> sizeof(buff)
> 
> ...
> 
> > +out:

Just to skip the below "if()" if error occurs in previous regmap read,
I used this label.
       if (status & BMA400_INT_DRDY_MSK)
             iio_trigger_poll_chained(data->trig);

I will remove the label in next patch
> 
> A useless label. Moreover this raises a question: why is it okay to
> always mark IRQ as handled?
> 
> > +       return IRQ_HANDLED;

Since I was not using top-half of the interrupt so I marked IRQ as handled
even for error case in the handler.

> 
> ...
> 
> > +                       dev_err(dev, "iio trigger register failed\n");
> > +                       return ret;
> 
> return dev_err_probe();
> 
> ...
> 
> > +                       dev_err(dev, "request irq %d failed\n", irq);
> > +                       return ret;
> 
> Ditto.
> 
> ...
> 
> > +               dev_err(dev, "iio triggered buffer setup failed\n");
> > +               return ret;
> 
> Ditto.

I will change this in the next patch version.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Andy Shevchenko]

On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote:
> > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:

First of all, you left many uncommented comments. I assume you agree
with my comments and are going to address them. If it's not the case,
please elaborate.

...

> > > +out:
>
> Just to skip the below "if()" if error occurs in previous regmap read,
> I used this label.
>        if (status & BMA400_INT_DRDY_MSK)
>              iio_trigger_poll_chained(data->trig);
>
> I will remove the label in next patch

Just return directly.

...

> > A useless label. Moreover this raises a question: why is it okay to
> > always mark IRQ as handled?
> >
> > > +       return IRQ_HANDLED;
>
> Since I was not using top-half of the interrupt so I marked IRQ as handled
> even for error case in the handler.

Yes, but why? Isn't it an erroneous state? Does it mean spurious
interrupt? Does it mean interrupt is unserviced?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Jagath Jog J]

Hello Andy,

On Tue, Mar 22, 2022 at 10:54:53AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote:
> > > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 
> First of all, you left many uncommented comments. I assume you agree
> with my comments and are going to address them. If it's not the case,
> please elaborate.

Yes Andy, I agree with your comments and I will address them in the next v2 series.

> 
> ...
> 
> > > > +out:
> >
> > Just to skip the below "if()" if error occurs in previous regmap read,
> > I used this label.
> >        if (status & BMA400_INT_DRDY_MSK)
> >              iio_trigger_poll_chained(data->trig);
> >
> > I will remove the label in next patch
> 
> Just return directly.
> 
> ...
> 
> > > A useless label. Moreover this raises a question: why is it okay to
> > > always mark IRQ as handled?
> > >
> > > > +       return IRQ_HANDLED;
> >
> > Since I was not using top-half of the interrupt so I marked IRQ as handled
> > even for error case in the handler.
> 
> Yes, but why? Isn't it an erroneous state? Does it mean spurious
> interrupt? Does it mean interrupt is unserviced?

Sorry, even for erroneous state I was returning IRQ_HANDLED.
As shown below, now for erroneous state and spurious interrupt I will return
IRQ_NONE and for valid interrupt IRQ_HANDLED will be returned.

Is below method is correct?

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 (le16_to_cpu(status) & BMA400_INT_DRDY_MSK) {
               iio_trigger_poll_chained(data->trig);
	       return IRQ_HANDLED;
	}

        return IRQ_NONE;
}

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v1 3/5] iio: accel: bma400: Add triggered buffer support

[Andy Shevchenko]

On Tue, Mar 22, 2022 at 5:40 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> On Tue, Mar 22, 2022 at 10:54:53AM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 22, 2022 at 12:21 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > > On Mon, Mar 21, 2022 at 10:39:22AM +0200, Andy Shevchenko wrote:
> > > > On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:

...

> > > > A useless label. Moreover this raises a question: why is it okay to
> > > > always mark IRQ as handled?
> > > >
> > > > > +       return IRQ_HANDLED;
> > >
> > > Since I was not using top-half of the interrupt so I marked IRQ as handled
> > > even for error case in the handler.
> >
> > Yes, but why? Isn't it an erroneous state? Does it mean spurious
> > interrupt? Does it mean interrupt is unserviced?
>
> Sorry, even for erroneous state I was returning IRQ_HANDLED.
> As shown below, now for erroneous state and spurious interrupt I will return
> IRQ_NONE and for valid interrupt IRQ_HANDLED will be returned.
>
> Is below method is correct?

The thing is that I don't know. I am not familiar with this hardware.
So, you have to investigate and decide.

> 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 (le16_to_cpu(status) & BMA400_INT_DRDY_MSK) {
>                iio_trigger_poll_chained(data->trig);
>                return IRQ_HANDLED;
>         }
>
>         return IRQ_NONE;

If you are going with this approach, try to handle errors first, i.e.

    if (...)
        return IRQ_NONE;
    ...
    return IRQ_HANDLED;

> }

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/5] iio: accel: bma400: Add separate channel for step counter

[Jagath Jog J]

Added channel for step counter which can be enable or disable
through the sysfs interface.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/accel/bma400.h      |  1 +
 drivers/iio/accel/bma400_core.c | 47 ++++++++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index b306a5ad513a..65bbc80cbb7f 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 797403c7dd85..305643e99eb5 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -68,6 +68,7 @@ struct bma400_data {
 	int oversampling_ratio;
 	int scale;
 	struct iio_trigger *trig;
+	int steps_enabled;
 	/* Correct time stamp alignment */
 	struct {
 		__be16 buff[3];
@@ -202,6 +203,13 @@ 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),
 };
 
@@ -686,13 +694,28 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
 {
 	struct bma400_data *data = iio_priv(indio_dev);
 	int ret;
+	u32 steps_raw;
 
 	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_STEPS:
+			mutex_lock(&data->mutex);
+			ret = regmap_bulk_read(data->regmap, BMA400_STEP_CNT0_REG,
+					       &steps_raw, 3 * sizeof(u8));
+			mutex_unlock(&data->mutex);
+			if (ret)
+				return ret;
+			*val = steps_raw & 0x00FFFFFF;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			mutex_lock(&data->mutex);
+			ret = bma400_get_temp_reg(data, val, val2);
+			mutex_unlock(&data->mutex);
+			return ret;
+		default:
+			return -EINVAL;
+		}
 	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&data->mutex);
 		ret = bma400_get_accel_reg(data, chan, val);
@@ -733,6 +756,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;
 	}
@@ -798,6 +824,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;
 	}
@@ -814,6 +851,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

Re: [PATCH v1 4/5] iio: accel: bma400: Add separate channel for step counter

[Jonathan Cameron]

On Sat, 19 Mar 2022 23:40:22 +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>
hi Jagath,

Been a long time since we had a steps counter. Good to have 
support on another device.

One minor thing inline, but otherwise looks good to me.

Jonathan

> ---
>  drivers/iio/accel/bma400.h      |  1 +
>  drivers/iio/accel/bma400_core.c | 47 ++++++++++++++++++++++++++++++---
>  2 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index b306a5ad513a..65bbc80cbb7f 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 797403c7dd85..305643e99eb5 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -68,6 +68,7 @@ struct bma400_data {
>  	int oversampling_ratio;
>  	int scale;
>  	struct iio_trigger *trig;
> +	int steps_enabled;
>  	/* Correct time stamp alignment */
>  	struct {
>  		__be16 buff[3];
> @@ -202,6 +203,13 @@ 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),
>  };
>  
> @@ -686,13 +694,28 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct bma400_data *data = iio_priv(indio_dev);
>  	int ret;
> +	u32 steps_raw;
>  
>  	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_STEPS:
> +			mutex_lock(&data->mutex);
> +			ret = regmap_bulk_read(data->regmap, BMA400_STEP_CNT0_REG,
> +					       &steps_raw, 3 * sizeof(u8));
> +			mutex_unlock(&data->mutex);
> +			if (ret)
> +				return ret;
> +			*val = steps_raw & 0x00FFFFFF;

No need for an endian conversion in here?  I'd also intialize steps_raw = 0 before
the bulk read then you shouldn't need to mask as you only read 3 bytes in.

> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			mutex_lock(&data->mutex);
> +			ret = bma400_get_temp_reg(data, val, val2);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&data->mutex);
>  		ret = bma400_get_accel_reg(data, chan, val);
> @@ -733,6 +756,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;
>  	}
> @@ -798,6 +824,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;
>  	}
> @@ -814,6 +851,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;
>  	}

Re: [PATCH v1 4/5] iio: accel: bma400: Add separate channel for step counter

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Added channel for step counter which can be enable or disable
> through the sysfs interface.

...

> +       u32 steps_raw;

I would expect this to be u8 ...[3].

...

> +                       ret = regmap_bulk_read(data->regmap, BMA400_STEP_CNT0_REG,
> +                                              &steps_raw, 3 * sizeof(u8));

sizeof(&steps_raw);

...

> +                       *val = steps_raw & 0x00FFFFFF;

And here it seems to be be24_to_cpu() like Jonathan mentioned,


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] iio: accel: bma400: Add separate channel for step counter

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:

> +       {
> +               .type = IIO_STEPS,
> +               .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +                                     BIT(IIO_CHAN_INFO_ENABLE),
> +               .scan_index = -1, /* No buffer support */
> +       },

> +

One more thing, seems like a stray blank line addition.

>         IIO_CHAN_SOFT_TIMESTAMP(4),
>  };

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/5] iio: accel: bma400: Add step change event

[Jagath Jog J]

Added support for event when there is a detection of single step
or double 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 | 72 +++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index 65bbc80cbb7f..c833119bb42e 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(1, 0)
 
 /*
  * Read-write configuration registers
diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
index 305643e99eb5..79321e41df51 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -26,6 +26,7 @@
 #include <linux/iio/trigger.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
+#include <linux/iio/events.h>
 
 #include "bma400.h"
 
@@ -69,6 +70,7 @@ struct bma400_data {
 	int scale;
 	struct iio_trigger *trig;
 	int steps_enabled;
+	bool step_event_en;
 	/* Correct time stamp alignment */
 	struct {
 		__be16 buff[3];
@@ -166,6 +168,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, \
@@ -208,6 +216,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),
@@ -858,6 +868,59 @@ 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:
+		switch (type) {
+		case IIO_EV_TYPE_CHANGE:
+			return data->steps_enabled;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+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:
+		switch (type) {
+		case IIO_EV_TYPE_CHANGE:
+			mutex_lock(&data->mutex);
+			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->steps_enabled = state;
+			return 0;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
 					     bool state)
 {
@@ -890,6 +953,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 = {
@@ -945,6 +1010,13 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 	if (status & BMA400_INT_DRDY_MSK)
 		iio_trigger_poll_chained(data->trig);
 
+	if (status & (BMA400_STEP_STAT_MASK << 8)) {
+		iio_push_event(indio_dev,
+			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
+					      IIO_EV_DIR_NONE,
+					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
+			       iio_get_time_ns(indio_dev));
+		}
 out:
 	return IRQ_HANDLED;
 }
-- 
2.17.1

Re: [PATCH v1 5/5] iio: accel: bma400: Add step change event

[Jonathan Cameron]

On Sat, 19 Mar 2022 23:40:23 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Added support for event when there is a detection of single step
> or double step change.

This is new - do we have any way of saying if it was single or
double that was detected?
  
> INT1 pin is used to interrupt and event
> is pushed to userspace.

My main question in here is the interaction between the event
enable and the channel enable in the previous patch.

If the channel is enabled, the event will read as enabled but
it looks to me like the interrupt will never happen...

So I think you need to also check if the interrupt is enabled.

The other way around is fine as any IIO ABI is allowed to affect
any other so when you enable the interrupt it can also enable
the channel in general.

> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
>  drivers/iio/accel/bma400.h      |  2 +
>  drivers/iio/accel/bma400_core.c | 72 +++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index 65bbc80cbb7f..c833119bb42e 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(1, 0)
>  
>  /*
>   * Read-write configuration registers
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index 305643e99eb5..79321e41df51 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -26,6 +26,7 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/events.h>
>  
>  #include "bma400.h"
>  
> @@ -69,6 +70,7 @@ struct bma400_data {
>  	int scale;
>  	struct iio_trigger *trig;
>  	int steps_enabled;
> +	bool step_event_en;
>  	/* Correct time stamp alignment */
>  	struct {
>  		__be16 buff[3];
> @@ -166,6 +168,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, \
> @@ -208,6 +216,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),
> @@ -858,6 +868,59 @@ 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:
> +		switch (type) {
> +		case IIO_EV_TYPE_CHANGE:
> +			return data->steps_enabled;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +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:
> +		switch (type) {
> +		case IIO_EV_TYPE_CHANGE:
> +			mutex_lock(&data->mutex);
> +			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->steps_enabled = state;

How does this interact with the use in the previous patch?

> +			return 0;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
>  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
>  					     bool state)
>  {
> @@ -890,6 +953,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 = {
> @@ -945,6 +1010,13 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
>  	if (status & BMA400_INT_DRDY_MSK)
>  		iio_trigger_poll_chained(data->trig);
>  
> +	if (status & (BMA400_STEP_STAT_MASK << 8)) {

FIELD_GET() would be cleaner

> +		iio_push_event(indio_dev,
> +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> +					      IIO_EV_DIR_NONE,
> +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> +			       iio_get_time_ns(indio_dev));
> +		}
>  out:
>  	return IRQ_HANDLED;
>  }

Re: [PATCH v1 5/5] iio: accel: bma400: Add step change event

[Jagath Jog J]

Hello Jonathan,

On Sun, Mar 20, 2022 at 05:37:26PM +0000, Jonathan Cameron wrote:
> On Sat, 19 Mar 2022 23:40:23 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
> 
> > Added support for event when there is a detection of single step
> > or double step change.
> 
> This is new - do we have any way of saying if it was single or
> double that was detected?

Sorry, I could have mentioned this properly. Device will detect single step.
If the sensor misses the detection of single step then it will indicate
as double step for the next step. (Register - INT-STAT1)

I will correct this in the next patch.
>   
> > INT1 pin is used to interrupt and event
> > is pushed to userspace.
> 
> My main question in here is the interaction between the event
> enable and the channel enable in the previous patch.

Channel enable will start the counter when sensor detects the steps,
But there will be no interrupts for each step detection since INT pin
mapping is not done here.

> 
> If the channel is enabled, the event will read as enabled but
> it looks to me like the interrupt will never happen...
> 
> So I think you need to also check if the interrupt is enabled.

Sorry my mistake, I will maintain separate member in device
structure for event.
> 
> The other way around is fine as any IIO ABI is allowed to affect
> any other so when you enable the interrupt it can also enable
> the channel in general.

Then I will enable channel also while enabling interrupt.
> 
> > 
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > ---
> >  drivers/iio/accel/bma400.h      |  2 +
> >  drivers/iio/accel/bma400_core.c | 72 +++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+)
> > 
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index 65bbc80cbb7f..c833119bb42e 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(1, 0)
> >  
> >  /*
> >   * Read-write configuration registers
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index 305643e99eb5..79321e41df51 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/iio/trigger.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/events.h>
> >  
> >  #include "bma400.h"
> >  
> > @@ -69,6 +70,7 @@ struct bma400_data {
> >  	int scale;
> >  	struct iio_trigger *trig;
> >  	int steps_enabled;
> > +	bool step_event_en;
> >  	/* Correct time stamp alignment */
> >  	struct {
> >  		__be16 buff[3];
> > @@ -166,6 +168,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, \
> > @@ -208,6 +216,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),
> > @@ -858,6 +868,59 @@ 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:
> > +		switch (type) {
> > +		case IIO_EV_TYPE_CHANGE:
> > +			return data->steps_enabled;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +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:
> > +		switch (type) {
> > +		case IIO_EV_TYPE_CHANGE:
> > +			mutex_lock(&data->mutex);
> > +			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->steps_enabled = state;
> 
> How does this interact with the use in the previous patch?
> 
> > +			return 0;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int bma400_data_rdy_trigger_set_state(struct iio_trigger *trig,
> >  					     bool state)
> >  {
> > @@ -890,6 +953,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 = {
> > @@ -945,6 +1010,13 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> >  	if (status & BMA400_INT_DRDY_MSK)
> >  		iio_trigger_poll_chained(data->trig);
> >  
> > +	if (status & (BMA400_STEP_STAT_MASK << 8)) {
> 
> FIELD_GET() would be cleaner
> 
> > +		iio_push_event(indio_dev,
> > +			       IIO_EVENT_CODE(IIO_STEPS, 0, IIO_NO_MOD,
> > +					      IIO_EV_DIR_NONE,
> > +					      IIO_EV_TYPE_CHANGE, 0, 0, 0),
> > +			       iio_get_time_ns(indio_dev));
> > +		}
> >  out:
> >  	return IRQ_HANDLED;
> >  }
> 

Re: [PATCH v1 5/5] iio: accel: bma400: Add step change event

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 8:10 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Added support for event when there is a detection of single step
> or double step change. INT1 pin is used to interrupt and event
> is pushed to userspace.

...

>  #include <linux/iio/trigger.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/events.h>

Keep this block ordered,

...

> +       switch (chan->type) {
> +       case IIO_STEPS:
> +               switch (type) {
> +               case IIO_EV_TYPE_CHANGE:
> +                       return data->steps_enabled;
> +               default:
> +                       return -EINVAL;
> +               }
> +       default:
> +               return -EINVAL;
> +       }

> +       return 0;

Dead code.

...

> +       case IIO_STEPS:
> +               switch (type) {
> +               case IIO_EV_TYPE_CHANGE:
> +                       mutex_lock(&data->mutex);
> +                       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->steps_enabled = state;
> +                       return 0;
> +               default:
> +                       return -EINVAL;
> +               }
> +       default:
> +               return -EINVAL;
> +       }

> +       return 0;

Ditto.

-- 
With Best Regards,
Andy Shevchenko