The purpose of this series is to correct two issues in the driver's raw
read function and remove unnecessary struct fields.
Changelog:
*v2
- separated original patch into two patches
(https://marc.info/?l=linux-iio&m=154047435605492)
*v3
- reordered patches so that fixes go first
- removed unnecessary initialization
- removed unnecessary voltage field variable
- dropped reading voltage on probe
- returns -EINVAL error on null voltage
Renato Lui Geh (3):
staging: iio: ad7780: fix offset read value
staging: iio: ad7780: update voltage on read
staging: iio: ad7780: remove unnecessary stashed voltage value
drivers/staging/iio/adc/ad7780.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
--
2.19.1
The ad7780 driver previously did not read the correct device output, as
it read an outdated value set at initialization. It now updates its
voltage on read.
Signed-off-by: Renato Lui Geh <[email protected]>
---
Changes in v3:
- removed initialization (int voltage_uv = 0)
- returns error when voltage_uv is null
drivers/staging/iio/adc/ad7780.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 91e016d534ed..f2a11e9424cd 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
long m)
{
struct ad7780_state *st = iio_priv(indio_dev);
+ int voltage_uv;
switch (m) {
case IIO_CHAN_INFO_RAW:
return ad_sigma_delta_single_conversion(indio_dev, chan, val);
case IIO_CHAN_INFO_SCALE:
- *val = st->int_vref_mv * st->gain;
+ voltage_uv = regulator_get_voltage(st->reg);
+ if (!voltage_uv)
+ return -EINVAL;
+ *val = (voltage_uv / 1000) * st->gain;
*val2 = chan->scan_type.realbits - 1;
return IIO_VAL_FRACTIONAL_LOG2;
case IIO_CHAN_INFO_OFFSET:
--
2.19.1
This patch removes the unnecessary field int_vref_mv in ad7780_state
referring to the device's voltage.
Signed-off-by: Renato Lui Geh <[email protected]>
---
Changes in v3:
- removed unnecessary int_vref_mv from ad7780_state
drivers/staging/iio/adc/ad7780.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index f2a11e9424cd..f250370efcf7 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -42,7 +42,6 @@ struct ad7780_state {
struct regulator *reg;
struct gpio_desc *powerdown_gpio;
unsigned int gain;
- u16 int_vref_mv;
struct ad_sigma_delta sd;
};
@@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
st->chip_info =
&ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
- if (voltage_uv)
- st->int_vref_mv = voltage_uv / 1000;
- else
+ if (!voltage_uv)
dev_warn(&spi->dev, "Reference voltage unspecified\n");
spi_set_drvdata(spi, indio_dev);
--
2.19.1
Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
This was fixed by assigning the correct value instead.
Signed-off-by: Renato Lui Geh <[email protected]>
---
drivers/staging/iio/adc/ad7780.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index b67412db0318..91e016d534ed 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
*val2 = chan->scan_type.realbits - 1;
return IIO_VAL_FRACTIONAL_LOG2;
case IIO_CHAN_INFO_OFFSET:
- *val -= (1 << (chan->scan_type.realbits - 1));
+ *val = -(1 << (chan->scan_type.realbits - 1));
return IIO_VAL_INT;
}
--
2.19.1
Good catch.
Acked-by: Alexandru Ardelean <[email protected]>
On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
> This was fixed by assigning the correct value instead.
>
> Signed-off-by: Renato Lui Geh <[email protected]>
> ---
> drivers/staging/iio/adc/ad7780.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..91e016d534ed 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> *val2 = chan->scan_type.realbits - 1;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> - *val -= (1 << (chan->scan_type.realbits - 1));
> + *val = -(1 << (chan->scan_type.realbits - 1));
> return IIO_VAL_INT;
> }
>
On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
>
> Signed-off-by: Renato Lui Geh <[email protected]>
> ---
> Changes in v3:
> - removed initialization (int voltage_uv = 0)
> - returns error when voltage_uv is null
>
> drivers/staging/iio/adc/ad7780.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..f2a11e9424cd 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv;
>
> switch (m) {
> case IIO_CHAN_INFO_RAW:
> return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
> case IIO_CHAN_INFO_SCALE:
> - *val = st->int_vref_mv * st->gain;
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (!voltage_uv)
This looks wrong.
I admit this was done in the same way in the probe function, but that looks
a bit wrong as well.
Typically, the return value of `regulator_get_voltage()` would get checked
with:
ret = regulator_get_voltage(st->reg);
if (ret < 0)
return ret;
*val = ret / 1000;
So, negative values are errors and zero & positive values are valid voltage
values.
> + return -EINVAL;
> + *val = (voltage_uv / 1000) * st->gain;
> *val2 = chan->scan_type.realbits - 1;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
>
> Signed-off-by: Renato Lui Geh <[email protected]>
> ---
> Changes in v3:
> - removed unnecessary int_vref_mv from ad7780_state
>
> drivers/staging/iio/adc/ad7780.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index f2a11e9424cd..f250370efcf7 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
> struct regulator *reg;
> struct gpio_desc *powerdown_gpio;
> unsigned int gain;
> - u16 int_vref_mv;
>
> struct ad_sigma_delta sd;
> };
> @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
> st->chip_info =
> &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>
> - if (voltage_uv)
> - st->int_vref_mv = voltage_uv / 1000;
> - else
> + if (!voltage_uv)
> dev_warn(&spi->dev, "Reference voltage unspecified\n");
I think you could remove this altogether, and also remove the entire
`voltage_uv = regulator_get_voltage(st->reg);` assignment.
It doesn't make much sense to read the voltage here, since it won't be used
in the probe part anymore.
>
> spi_set_drvdata(spi, indio_dev);
On Thu, 1 Nov 2018 15:02:32 +0000
"Ardelean, Alexandru" <[email protected]> wrote:
> Good catch.
>
> Acked-by: Alexandru Ardelean <[email protected]>
On the basis this has been broken for a long time, and you are clearly
doing other nearby not fix work, I'm going to take this through the togreg
tree rather than via the quicker fix path. It makes my life
easier :)
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
>
> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
> > This was fixed by assigning the correct value instead.
> >
> > Signed-off-by: Renato Lui Geh <[email protected]>
> > ---
> > drivers/staging/iio/adc/ad7780.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index b67412db0318..91e016d534ed 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> > *val2 = chan->scan_type.realbits - 1;
> > return IIO_VAL_FRACTIONAL_LOG2;
> > case IIO_CHAN_INFO_OFFSET:
> > - *val -= (1 << (chan->scan_type.realbits - 1));
> > + *val = -(1 << (chan->scan_type.realbits - 1));
> > return IIO_VAL_INT;
> > }
> >
On Thu, 1 Nov 2018 15:20:55 +0000
"Ardelean, Alexandru" <[email protected]> wrote:
> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > The ad7780 driver previously did not read the correct device output, as
> > it read an outdated value set at initialization. It now updates its
> > voltage on read.
> >
> > Signed-off-by: Renato Lui Geh <[email protected]>
> > ---
> > Changes in v3:
> > - removed initialization (int voltage_uv = 0)
> > - returns error when voltage_uv is null
> >
> > drivers/staging/iio/adc/ad7780.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 91e016d534ed..f2a11e9424cd 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> > long m)
> > {
> > struct ad7780_state *st = iio_priv(indio_dev);
> > + int voltage_uv;
> >
> > switch (m) {
> > case IIO_CHAN_INFO_RAW:
> > return ad_sigma_delta_single_conversion(indio_dev, chan,
> > val);
> > case IIO_CHAN_INFO_SCALE:
> > - *val = st->int_vref_mv * st->gain;
> > + voltage_uv = regulator_get_voltage(st->reg);
> > + if (!voltage_uv)
>
> This looks wrong.
> I admit this was done in the same way in the probe function, but that looks
> a bit wrong as well.
>
> Typically, the return value of `regulator_get_voltage()` would get checked
> with:
>
> ret = regulator_get_voltage(st->reg);
> if (ret < 0)
> return ret;
> *val = ret / 1000;
>
> So, negative values are errors and zero & positive values are valid voltage
> values.
This one is a stylistic choice for readability. I ever so slightly
prefer how Alex has it but don't care enough that I'd have commented on it ;)
However, nice to tidy up as you'll be respinning patch 3 anyway!
Thanks,
Jonathan
>
> > + return -EINVAL;
> > + *val = (voltage_uv / 1000) * st->gain;
> > *val2 = chan->scan_type.realbits - 1;
> > return IIO_VAL_FRACTIONAL_LOG2;
> > case IIO_CHAN_INFO_OFFSET:
On Thu, 1 Nov 2018 15:28:19 +0000
"Ardelean, Alexandru" <[email protected]> wrote:
> On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> > This patch removes the unnecessary field int_vref_mv in ad7780_state
> > referring to the device's voltage.
> >
> > Signed-off-by: Renato Lui Geh <[email protected]>
> > ---
> > Changes in v3:
> > - removed unnecessary int_vref_mv from ad7780_state
> >
> > drivers/staging/iio/adc/ad7780.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index f2a11e9424cd..f250370efcf7 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -42,7 +42,6 @@ struct ad7780_state {
> > struct regulator *reg;
> > struct gpio_desc *powerdown_gpio;
> > unsigned int gain;
> > - u16 int_vref_mv;
> >
> > struct ad_sigma_delta sd;
> > };
> > @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
> > st->chip_info =
> > &ad7780_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> >
> > - if (voltage_uv)
> > - st->int_vref_mv = voltage_uv / 1000;
> > - else
> > + if (!voltage_uv)
> > dev_warn(&spi->dev, "Reference voltage unspecified\n");
>
> I think you could remove this altogether, and also remove the entire
> `voltage_uv = regulator_get_voltage(st->reg);` assignment.
>
> It doesn't make much sense to read the voltage here, since it won't be used
> in the probe part anymore.
>
Absolutely agree on this. There is no point in reading here at all.
Jonathan
> >
> > spi_set_drvdata(spi, indio_dev);
Hi,
>On Thu, 1 Nov 2018 15:02:32 +0000
>"Ardelean, Alexandru" <[email protected]> wrote:
>
>> Good catch.
That was actually Jonathan's catch. :)
>> Acked-by: Alexandru Ardelean <[email protected]>
I read up on Acked-by on the kernel docs, as I didn't know exactly what
it meant, but I'm not so sure on how to proceed once the patch has been
acked. For future patches, do I add this Acked-by line on the patch's
message body? Or is this just an informal way of approval?
>On the basis this has been broken for a long time, and you are clearly
>doing other nearby not fix work, I'm going to take this through the togreg
>tree rather than via the quicker fix path. It makes my life
>easier :)
>
>Applied to the togreg branch of iio.git and pushed out as testing for
>the autobuilders to play with it.
So this means I should skip this patch on v4, right?
Thanks,
Renato
On Thu, 1 Nov 2018 15:20:55 +0000
"Ardelean, Alexandru" <[email protected]> wrote:
>
> This looks wrong.
> I admit this was done in the same way in the probe function, but that looks
> a bit wrong as well.
>
> Typically, the return value of `regulator_get_voltage()` would get checked
> with:
>
> ret = regulator_get_voltage(st->reg);
> if (ret < 0)
> return ret;
> *val = ret / 1000;
>
> So, negative values are errors and zero & positive values are valid voltage
> values.
I see. So -EINVAL is only used when sent the wrong info type?
On Sat, 3 Nov 2018 13:06:19 -0300
Renato Lui Geh <[email protected]> wrote:
> On Thu, 1 Nov 2018 15:20:55 +0000
> "Ardelean, Alexandru" <[email protected]> wrote:
> >
> > This looks wrong.
> > I admit this was done in the same way in the probe function, but that looks
> > a bit wrong as well.
> >
> > Typically, the return value of `regulator_get_voltage()` would get checked
> > with:
> >
> > ret = regulator_get_voltage(st->reg);
> > if (ret < 0)
> > return ret;
> > *val = ret / 1000;
> >
> > So, negative values are errors and zero & positive values are valid voltage
> > values.
>
> I see. So -EINVAL is only used when sent the wrong info type?
yes. I actually misread what was there and thought we were just talking
about using a ret variable rather than returning the error via your
local variable.
Definitely want to pass on the error from regulator_get_voltage as
it may have more meaning than a simple -EINVAL.
Thanks,
Jonathan
On Sat, 3 Nov 2018 12:59:16 -0300
Renato Lui Geh <[email protected]> wrote:
> Hi,
>
> >On Thu, 1 Nov 2018 15:02:32 +0000
> >"Ardelean, Alexandru" <[email protected]> wrote:
> >
> >> Good catch.
>
> That was actually Jonathan's catch. :)
>
> >> Acked-by: Alexandru Ardelean <[email protected]>
>
> I read up on Acked-by on the kernel docs, as I didn't know exactly what
> it meant, but I'm not so sure on how to proceed once the patch has been
> acked. For future patches, do I add this Acked-by line on the patch's
> message body? Or is this just an informal way of approval?
It's formal. You put the line directly below your Signed-off-by: line
if you are resending. If I pick up the patch I paste it in there
whilst applying.
>
> >On the basis this has been broken for a long time, and you are clearly
> >doing other nearby not fix work, I'm going to take this through the togreg
> >tree rather than via the quicker fix path. It makes my life
> >easier :)
> >
> >Applied to the togreg branch of iio.git and pushed out as testing for
> >the autobuilders to play with it.
>
> So this means I should skip this patch on v4, right?
Yes. Already in the tree so don't send it again.
Thanks
Jonathan
>
> Thanks,
> Renato
>