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:   Sat, 22 Dec 2018 18:03:16 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jeremy Fertic <jeremyfertic@gmail.com>
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: <20181222180310.4ad9a1c6@archlinux>
In-Reply-To: <20181217213059.GA11856@r2700x.localdomain>
References: <20181212005503.28054-1-jeremyfertic@gmail.com>
        <20181212005503.28054-4-jeremyfertic@gmail.com>
        <20181216113756.70d76d0a@archlinux>
        <20181217213059.GA11856@r2700x.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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