Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 17 Dec 2018 14:30:59 -0700
From:   Jeremy Fertic <jeremyfertic@gmail.com>
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Hartmut Knaack <knaack.h@gmx.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-iio@vger.kernel.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment
Message-ID: <20181217213059.GA11856@r2700x.localdomain>
References: <20181212005503.28054-1-jeremyfertic@gmail.com>
 <20181212005503.28054-4-jeremyfertic@gmail.com>
 <20181216113756.70d76d0a@archlinux>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181216113756.70d76d0a@archlinux>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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);
>