| Message ID | 20220318204808.3404542-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio:proximity:sx9324: Fix hardware gain read/write | expand |
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
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
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
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?
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
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).
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
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);
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(-) base-commit: a8ee3b32f5da6c77a5ccc0e42c2250d61ba54fe0