linux-kernel - Re: [PATCH 2/5] iio: imu: adis: Add DIAG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241028170422.00001865@Huawei.com>
Date: Mon, 28 Oct 2024 17:04:22 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Robert Budai <robert.budai@...log.com>
CC: Nuno Sa <nuno.sa@...log.com>, Ramona Gradinariu
	<ramona.gradinariu@...log.com>, Antoniu Miclaus <antoniu.miclaus@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>, "Michael Hennerich"
	<Michael.Hennerich@...log.com>, Jonathan Cameron <jic23@...nel.org>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>, Jagath Jog J
	<jagathjog1996@...il.com>, <linux-iio@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <robi_budai@...oo.com>
Subject: Re: [PATCH 2/5] iio: imu: adis: Add DIAG_STAT register size

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

> From: Nuno Sá <nuno.sa@...log.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@...log.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> Signed-off-by: Nuno Sá <nuno.sa@...log.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;