Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 16 Apr 2022 17:41:18 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jagath Jog J <jagathjog1996@gmail.com>
Cc:     dan@dlrobertson.com, andy.shevchenko@gmail.com,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/9] iio: accel: bma400: Add separate channel for
 step counter
Message-ID: <20220416174118.13f787be@jic23-huawei>
In-Reply-To: <20220411203133.19929-6-jagathjog1996@gmail.com>
References: <20220411203133.19929-1-jagathjog1996@gmail.com>
        <20220411203133.19929-6-jagathjog1996@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 12 Apr 2022 02:01:29 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Added channel for step counter which can be enable or disable
> through the sysfs interface.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
>  drivers/iio/accel/bma400.h      |  1 +
>  drivers/iio/accel/bma400_core.c | 48 ++++++++++++++++++++++++++++++---
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index a7482a66b36b..52f9ea95de81 100644
> --- a/drivers/iio/accel/bma400.h
> +++ b/drivers/iio/accel/bma400.h
> @@ -53,6 +53,7 @@
>  #define BMA400_STEP_CNT1_REG        0x16
>  #define BMA400_STEP_CNT3_REG        0x17
>  #define BMA400_STEP_STAT_REG        0x18
> +#define BMA400_STEP_INT_MSK	    BIT(0)
>  
>  /*
>   * Read-write configuration registers
> diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> index b7b2b67aef31..c8f147163d3c 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -20,6 +20,8 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <asm/unaligned.h>
> +
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/trigger.h>
> @@ -67,6 +69,7 @@ struct bma400_data {
>  	int oversampling_ratio;
>  	int scale;
>  	struct iio_trigger *trig;
> +	int steps_enabled;
>  	/* Correct time stamp alignment */
>  	struct {
>  		__le16 buff[3];
> @@ -201,6 +204,12 @@ static const struct iio_chan_spec bma400_channels[] = {
>  			.endianness = IIO_LE,
>  		},
>  	},
> +	{
> +		.type = IIO_STEPS,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				      BIT(IIO_CHAN_INFO_ENABLE),
> +		.scan_index = -1, /* No buffer support */
> +	},
>  	IIO_CHAN_SOFT_TIMESTAMP(4),
>  };
>  
> @@ -705,13 +714,28 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct bma400_data *data = iio_priv(indio_dev);
>  	int ret;
> +	u8 steps_raw[3];
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_PROCESSED:
> -		mutex_lock(&data->mutex);
> -		ret = bma400_get_temp_reg(data, val, val2);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			mutex_lock(&data->mutex);
> +			ret = bma400_get_temp_reg(data, val, val2);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		case IIO_STEPS:
> +			mutex_lock(&data->mutex);
> +			ret = regmap_bulk_read(data->regmap, BMA400_STEP_CNT0_REG,
> +					       &steps_raw, sizeof(steps_raw));

Bulk read to a non DMA safe buffer. Similar fix to before needed.
Or given this is a slow path, just use a local kmalloc() for the buffer.

The reality is you are probably fine today without such care, but last
time we discussed this the conclusion was that it would be a perfectly
valid optimisation in regmap to return to requiring DMA safe buffers
for bulk accesses.

> +			mutex_unlock(&data->mutex);
> +			if (ret)
> +				return ret;
> +			*val = get_unaligned_le24(steps_raw);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&data->mutex);
>  		ret = bma400_get_accel_reg(data, chan, val);
> @@ -752,6 +776,9 @@ static int bma400_read_raw(struct iio_dev *indio_dev,
>  
>  		*val = data->oversampling_ratio;
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = data->steps_enabled;
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -817,6 +844,17 @@ static int bma400_write_raw(struct iio_dev *indio_dev,
>  		ret = bma400_set_accel_oversampling_ratio(data, val);
>  		mutex_unlock(&data->mutex);
>  		return ret;
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (data->steps_enabled == val)
> +			return 0;
> +
> +		mutex_lock(&data->mutex);
> +		ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG,
> +					 BMA400_STEP_INT_MSK,
> +					 FIELD_PREP(BMA400_STEP_INT_MSK, !!val));

Why the lock for this one?

> +		mutex_unlock(&data->mutex);
> +		data->steps_enabled = val;
> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -833,6 +871,8 @@ static int bma400_write_raw_get_fmt(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_ENABLE:
> +		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}