[PATCH] iio:proximity:sx9324: Fix hardware gain read/write

[PATCH] iio:proximity:sx9324: Fix hardware gain read/write

[Stephen Boyd]

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.

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)
+		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
-- 
https://chromeos.dev

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

[Jonathan Cameron]

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

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

[Stephen Boyd]

Quoting Jonathan Cameron (2022-03-19 08:26:41)
> 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.

Yes 0 is reserved. The top of this driver's C file has the datasheet
link[1]

>
> >
> > 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)?
>

That sounds OK to me. The driver is only being introduced now so we can
still fix it to reject a gain of 0. Unless 0 should mean "off", i.e.
hardware gain of 1?

[1] https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf

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

[Jonathan Cameron]

On Mon, 21 Mar 2022 19:36:33 +0100
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2022-03-19 08:26:41)
> > 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.  
> 
> Yes 0 is reserved. The top of this driver's C file has the datasheet
> link[1]
Ah. Thanks ;)

> 
> >  
> > >
> > > 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)?
> >  
> 
> That sounds OK to me. The driver is only being introduced now so we can
> still fix it to reject a gain of 0. Unless 0 should mean "off", i.e.
> hardware gain of 1?
No.  I don't think we want to add that sort of fiddly definition.
So error is the way to go - I'd forgotten we only just introduced this
so no ABI breakage risk.


Jonathan

> 
> [1] https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf

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

[Stephen Boyd]

Quoting Jonathan Cameron (2022-03-22 13:38:44)
> On Mon, 21 Mar 2022 19:36:33 +0100
> Stephen Boyd <swboyd@chromium.org> wrote:
> > Quoting Jonathan Cameron (2022-03-19 08:26:41)
> > > On Fri, 18 Mar 2022 13:48:08 -0700
> > > Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > 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)?
> > >
> >
> > That sounds OK to me. The driver is only being introduced now so we can
> > still fix it to reject a gain of 0. Unless 0 should mean "off", i.e.
> > hardware gain of 1?
> No.  I don't think we want to add that sort of fiddly definition.
> So error is the way to go - I'd forgotten we only just introduced this
> so no ABI breakage risk.
>

Ok got it. Does the write_gain function also need to reject values
greater than 8 and less than or equal to 0?

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

[Jonathan Cameron]

On Tue, 22 Mar 2022 17:57:26 -0400
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Jonathan Cameron (2022-03-22 13:38:44)
> > On Mon, 21 Mar 2022 19:36:33 +0100
> > Stephen Boyd <swboyd@chromium.org> wrote:  
> > > Quoting Jonathan Cameron (2022-03-19 08:26:41)  
> > > > On Fri, 18 Mar 2022 13:48:08 -0700
> > > > Stephen Boyd <swboyd@chromium.org> wrote:  
> > > > >
> > > > > 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)?
> > > >  
> > >
> > > That sounds OK to me. The driver is only being introduced now so we can
> > > still fix it to reject a gain of 0. Unless 0 should mean "off", i.e.
> > > hardware gain of 1?  
> > No.  I don't think we want to add that sort of fiddly definition.
> > So error is the way to go - I'd forgotten we only just introduced this
> > so no ABI breakage risk.
> >  
> 
> Ok got it. Does the write_gain function also need to reject values
> greater than 8 and less than or equal to 0?

Ah. Yes, it should indeed as the value is coming from userspace
so we shouldn't trust it to be sensible.

Jonathan

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

[Andy Shevchenko]

On Sat, Mar 19, 2022 at 5:58 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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.

...

> +       *val = 1 << regval;

I see it's similar in the original code, but this is still problematic
from C standard point of view, i.e. if regval = 31, the C standard
calls it UB (Undefined Behaviour).

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Mon, 28 Mar 2022 01:16:10 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, Mar 19, 2022 at 5:58 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > 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.  
> 
> ...
> 
> > +       *val = 1 << regval;  
> 
> I see it's similar in the original code, but this is still problematic
> from C standard point of view, i.e. if regval = 31, the C standard
> calls it UB (Undefined Behaviour).
> 

I don't see that as a problem as regval is coming from a FIELD_GET() with a 3 bit mask
so we can't hit the UB case (can only be up to 7 - well 6 because of the --)

Jonathan