Re: [PATCH v2 1/4] staging: iio: adt7316: fix dac_bits assignment

[Jonathan Cameron]

+ CC Shreeya who is working on the same driver.

On Sat, 22 Dec 2018 21:57:40 -0700
Jeremy Fertic <jeremyfertic@xxxxxxxxx> wrote:

> The value of dac_bits is used in adt7316_show_DAC() and adt7316_store_DAC(),
> and it should be either 8, 10, or 12 bits depending on the device in use. The
> driver currently only assigns a value to dac_bits in
> adt7316_store_da_high_resolution(). The purpose of the dac high resolution
> option is not to change dac resolution for normal operation. Instead, 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 output voltage proportional to temperature,
> the da_high_resolution attribute can optionally be enabled to use 10 bit
> resolution rather than the default 8 bits. This 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. Dacs c and d continue to operate normally under these conditions.
> With the above in mind, remove the dac_bits assignments from this function
> since the value of dac_bits as used in the driver is not dependent on this
> dac high resolution option.
> 
> Since the dac_bits assignments discussed above are currently the only ones
> in this driver, the default value of dac_bits is 0. This results in incorrect
> calculations when the dacs are read or written in adt7316_show_DAC() and
> adt7316_store_DAC(). To correct this, assign a value to dac_bits in
> adt7316_probe() to ensure correct operation as soon as the device is
> registered and available to userspace.
> 
> Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> Signed-off-by: Jeremy Fertic <jeremyfertic@xxxxxxxxx>

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with.  The driver has been broken a long time and
is undergoing a lot of churn at the moment, so I'm not going to rush
this in even though it's a fix.

Thanks,

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 9db49aa186bb..e17c1cb12c94 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -652,17 +652,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)
> @@ -2145,6 +2138,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);