linux-kernel - Re: [PATCH] iio:proximity:sx9324: Fix hardware gain read/write

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220319152641.49d8b3e1@jic23-huawei>
Date:   Sat, 19 Mar 2022 15:26:41 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org, Gwendal Grignou <gwendal@...omium.org>
Subject: Re: [PATCH] iio:proximity:sx9324: Fix hardware gain read/write

On Fri, 18 Mar 2022 13:48:08 -0700
Stephen Boyd <swboyd@...omium.org> wrote:

Hi Stephen,


> There are four possible gain values according to sx9324_gain_vals[]: 1,
> 2, 4, and 8. When writing and reading the register the values are off by
> one. 
> The bits should be set according to this equation:
> 
> 	ilog2(<gain>) + 1
> 
> so that a gain of 8 is 0x3 in the register field and a gain of 4 is 0x2
> in the register field, etc. Fix up the functions.

So is the 0 value reserved?  I can't find an sx9324 datasheet but he
9320 is online and that seems to be the case there.  If so please state
that in this description as well.

> 
> Fixes: 4c18a890dff8 ("iio:proximity:sx9324: Add SX9324 support")
> Cc: Gwendal Grignou <gwendal@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> ---
>  drivers/iio/proximity/sx9324.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index 0d9bbbb50cb4..a3c8e02f5a56 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -379,7 +379,10 @@ static int sx9324_read_gain(struct sx_common_data *data,
>  	if (ret)
>  		return ret;
>  
> -	*val = 1 << FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
> +	regval = FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval);
> +	if (regval)

If 0 is reserved then I'd return and error code here to indicate
we don't know what the gain is rather than carrying on regardless.
Or is this going to cause problems as it will be an ABI change (error
return possible when it wasn't really before)?

> +		regval--;

> +	*val = 1 << regval;



>  
>  	return IIO_VAL_INT;
>  }
> @@ -725,7 +728,7 @@ static int sx9324_write_gain(struct sx_common_data *data,
>  	unsigned int gain, reg;
>  	int ret;
>  
> -	gain = ilog2(val);
> +	gain = ilog2(val) + 1;
>  	reg = SX9324_REG_PROX_CTRL0 + chan->channel / 2;
>  	gain = FIELD_PREP(SX9324_REG_PROX_CTRL0_GAIN_MASK, gain);
>  
> 
> base-commit: a8ee3b32f5da6c77a5ccc0e42c2250d61ba54fe0