Re: [PATCH 2/5] iio: imu: adis: Add DIAG_STAT register size

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 28 Oct 2024 14:25:34 +0200
Robert Budai <robert.budai@analog.com> wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> Some devices may have more than 16 bits of status. This patch allows the
> user to specify the size of the DIAG_STAT register. It defaults to 2 if
> not specified. This is mainly for backward compatibility.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
I'd rather we didn't use a default for this one.
The value 2 isn't obvious.  So just update all existing cases
in this patch and drop the check on whether it is set.

Again, remember to add your SoB on this.

One other minor comment inline.

Jonathan

> ---
>  drivers/iio/imu/adis.c       | 12 +++++++++---
>  include/linux/iio/imu/adis.h |  3 +++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 504d18a36f90..f03f35c94f76 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -304,11 +304,17 @@ EXPORT_SYMBOL_NS(__adis_enable_irq, IIO_ADISLIB);
>   */
>  int __adis_check_status(struct adis *adis)
>  {
> -	u16 status;
> +	unsigned int status = 0;
>  	int ret;
>  	int i;
> +	/* default to 2 bytes */
> +	unsigned int reg_size = 2;
>  
> -	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
> +	if (adis->data->diag_stat_size)
> +		reg_size = adis->data->diag_stat_size;
> +
> +	ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status,
> +			      reg_size);
>  	if (ret)
>  		return ret;
>  
> @@ -317,7 +323,7 @@ int __adis_check_status(struct adis *adis)
>  	if (status == 0)
>  		return 0;
>  
> -	for (i = 0; i < 16; ++i) {
> +	for (i = 0; i < (reg_size * 8); ++i) {
BITS_PER_BYTE instead of 8 and no need for the brackets around (reg_size * BITS_PER_BYTE)

>  		if (status & BIT(i)) {
>  			dev_err(&adis->spi->dev, "%s.\n",
>  				adis->data->status_error_msgs[i]);
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 7b589cc83380..fae31042a622 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -44,6 +44,8 @@ struct adis_timeout {
>   * @glob_cmd_reg: Register address of the GLOB_CMD register
>   * @msc_ctrl_reg: Register address of the MSC_CTRL register
>   * @diag_stat_reg: Register address of the DIAG_STAT register
> + * @diag_stat_size: Length (in bytes) of the DIAG_STAT register.
> + *		    Defaults to 2 if not set.
>   * @prod_id_reg: Register address of the PROD_ID register
>   * @prod_id: Product ID code that should be expected when reading @prod_id_reg
>   * @self_test_mask: Bitmask of supported self-test operations
> @@ -70,6 +72,7 @@ struct adis_data {
>  	unsigned int glob_cmd_reg;
>  	unsigned int msc_ctrl_reg;
>  	unsigned int diag_stat_reg;
> +	unsigned int diag_stat_size;
>  	unsigned int prod_id_reg;
>  
>  	unsigned int prod_id;