by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

On Mon, 17 Dec 2018 14:30:59 -0700
Jeremy Fertic <[email protected]> wrote:

> On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote:
> > On Tue, 11 Dec 2018 17:54:55 -0700
> > Jeremy Fertic <[email protected]> wrote:
> >
> > > The only assignment to dac_bits is in adt7316_store_da_high_resolution().
> > > This function enables or disables 10 bit dac resolution for the adt7316/7
> > > and adt7516/7 when they're set to output voltage proportional to
> > > temperature. Remove these assignments since they're unnecessary for the
> > > dac high resolution functionality.
> > >
> > > Instead, assign a value to dac_bits in adt7316_probe() since the number
> > > of dac bits might be needed as soon as the device is registered and
> > > available to userspace.
> > >
> > > Signed-off-by: Jeremy Fertic <[email protected]>
> >
> > I'm a little confused as it seems to me that in the orignal code
> > if we were setting high resolution 'off' we would fall back to 8
> > bits for either type of device. Now you just have a check on the
> > device type?
> >
> > The result will be that we read more bytes than we want to
> > in show_DAC.
> >
> > I'd need a pretty strong argument to persuade me it is worth
> > supporting the 8 bit mode at all btw on devices that will go
> > higher. It adds interface complexity and the number of usecases
> > where the saving in bus traffic is worthwhile are probably fairly
> > few!
> >
> > Jonathan
>
> Thanks for the feedback Jonathan, and sorry for the confusion on this one.
> My commit message should have been clearer. This is not a general purpose
> option to change the dac resolution. It is specific to an optional feature
> where one or two of the four dacs can be set to output voltage proportional
> to temperature. If the user chooses to set dac a and/or dac b to ouput
> voltage proportional to temperature, this da_high_resolution attribute can
> optionally be enabled to use 10 bit resolution rather than the default 8
> bits. It is only available on the 10 and 12 bit dac devices. If the user
> attempts to read or write dacs a or b under these settings, the driver's
> current behaviour is to return an error. The dacs c and d continue to
> operate normally under these conditions.
>
> With the above in mind, dac_bits should have never been assigned to in this
> da_high_resolution attribute. Before this patch, if the user didn't write
> to this optional attribute, dac_bits would be 0 since it was never assigned
> to anywhere else. This would result in incorrect calculations in show_DAC
> and store_DAC.
>
Good explanation. Send me a v2 with that in the description.

Thanks,

Jonathan

> Jeremy
>
> > > ---
> > > drivers/staging/iio/addac/adt7316.c | 18 +++++++++---------
> > > 1 file changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> > > index e5e1f9d6357f..a9990e7f2a4d 100644
> > > --- a/drivers/staging/iio/addac/adt7316.c
> > > +++ b/drivers/staging/iio/addac/adt7316.c
> > > @@ -651,17 +651,10 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
> > > u8 config3;
> > > int ret;
> > >
> > > - chip->dac_bits = 8;
> > > -
> > > - if (buf[0] == '1') {
> > > + if (buf[0] == '1')
> > > config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION;
> > > - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
> > > - chip->dac_bits = 12;
> > > - else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517)
> > > - chip->dac_bits = 10;
> > > - } else {
> > > + else
> > > config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
> > > - }
> > >
> > > ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
> > > if (ret)
> > > @@ -2123,6 +2116,13 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
> > > else
> > > return -ENODEV;
> > >
> > > + if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
> > > + chip->dac_bits = 12;
> > > + else if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517)
> > > + chip->dac_bits = 10;
> > > + else
> > > + chip->dac_bits = 8;
> > > +
> > > chip->ldac_pin = devm_gpiod_get_optional(dev, "adi,ldac", GPIOD_OUT_LOW);
> > > if (IS_ERR(chip->ldac_pin)) {
> > > ret = PTR_ERR(chip->ldac_pin);
> >