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, 19 Mar 2022 15:26:41 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org, Gwendal Grignou <gwendal@chromium.org>
Subject: Re: [PATCH] iio:proximity:sx9324: Fix hardware gain read/write
Message-ID: <20220319152641.49d8b3e1@jic23-huawei>
In-Reply-To: <20220318204808.3404542-1-swboyd@chromium.org>
References: <20220318204808.3404542-1-swboyd@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, 18 Mar 2022 13:48:08 -0700
Stephen Boyd <swboyd@chromium.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@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.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