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:   Sun, 16 Dec 2018 11:37:56 +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: <20181216113756.70d76d0a@archlinux>
In-Reply-To: <20181212005503.28054-4-jeremyfertic@gmail.com>
References: <20181212005503.28054-1-jeremyfertic@gmail.com>
        <20181212005503.28054-4-jeremyfertic@gmail.com>
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 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
> ---
>  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);