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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 17 Dec 2018 14:30:59 -0700
Jeremy Fertic <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com>  
> > 
> > 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);  
> >