Re: [PATCH V2 2/6] iio: accel: bmi088: Make it possible to config scales

From: Jonathan Cameron <jic23@kernel.org>
To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Cc: lars@metafoo.de, mchehab+huawei@kernel.org,
	ardeleanalex@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	mike.looijmans@topic.nl, devicetree@vger.kernel.org
Subject: Re: [PATCH V2 2/6] iio: accel: bmi088: Make it possible to config scales
Date: Sat, 14 May 2022 16:35:46 +0100	[thread overview]
Message-ID: <20220514163546.59ac8512@jic23-huawei> (raw)
In-Reply-To: <20220510141753.3878390-3-Qing-wu.Li@leica-geosystems.com.cn>

On Tue, 10 May 2022 14:17:49 +0000
LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:

> The sensor can set the scales by writing the range register 0x41,
> The current driver has no interface to configure it.
> The commit adds the interface for config the scales.
> 
> Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
Hi.

A few minor requested changes inline,

Thanks,

Jonathan

> ---
>  drivers/iio/accel/bmi088-accel-core.c | 32 ++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/bmi088-accel-core.c b/drivers/iio/accel/bmi088-accel-core.c
> index 9300313b63cb..8fee1d02e773 100644
> --- a/drivers/iio/accel/bmi088-accel-core.c
> +++ b/drivers/iio/accel/bmi088-accel-core.c
> @@ -237,6 +237,21 @@ static int bmi088_accel_set_sample_freq(struct bmi088_accel_data *data, int val)
>  				  BMI088_ACCEL_MODE_ODR_MASK, regval);
>  }
>  
> +static int bmi088_accel_set_scale(struct bmi088_accel_data *data, int val, int val2)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < 4; i++)
> +		if (val == data->chip_info->scale_table[i][0] &&
> +		    val2 == data->chip_info->scale_table[i][1])
> +			break;
> +
> +	if (i >= 4)
== 4

If it's > 4 something very odd happened :)

> +		return -EINVAL;
> +
> +	return regmap_write(data->regmap, BMI088_ACCEL_REG_ACC_RANGE, i);
> +}
> +
>  static int bmi088_accel_get_temp(struct bmi088_accel_data *data, int *val)
>  {
>  	int ret;
> @@ -368,7 +383,13 @@ static int bmi088_accel_read_avail(struct iio_dev *indio_dev,
>  			     const int **vals, int *type, int *length,
>  			     long mask)
>  {
> +	struct bmi088_accel_data *data = iio_priv(indio_dev);
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*vals = (const int *)data->chip_info->scale_table;
> +		*length = 8;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*type = IIO_VAL_INT_PLUS_MICRO;
>  		*vals = bmi088_sample_freqs;
> @@ -388,6 +409,14 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			return ret;

Blank line here.  That separates a functional call and it's error handling from
what happens next and makes the code a tiny bit easier to read.

Also consistent with SAMP_FREQ block that follows.

> +		ret = bmi088_accel_set_scale(data, val, val2);
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		return ret;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = pm_runtime_resume_and_get(dev);
>  		if (ret)
> @@ -409,7 +438,8 @@ static int bmi088_accel_write_raw(struct iio_dev *indio_dev,
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>  				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> -	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				BIT(IIO_CHAN_INFO_SCALE), \
>  	.scan_index = AXIS_##_axis, \
>  }
>  