| Message ID | 1bc4bdf6342d4bc0c2fea17fb3bcd79fabf0e1d1.1541082656.git.renatogeh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: ad7780: correct driver read | expand |
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 <renatogeh@gmail.com> > --- > 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, 1 Nov 2018 15:20:55 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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 <renatogeh@gmail.com> > > --- > > 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:20:55 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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 <renatogeh@gmail.com> wrote: > On Thu, 1 Nov 2018 15:20:55 +0000 > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> 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
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:
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 <renatogeh@gmail.com> --- 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(-)