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, 5 Jan 2019 17:57:12 +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,
        Shreeya Patel <shreeya.patel23498@gmail.com>
Subject: Re: [PATCH v2 4/4] staging: iio: adt7316: fix the dac write
 calculation
Message-ID: <20190105175712.7e5075e1@archlinux>
In-Reply-To: <20181223045743.10716-5-jeremyfertic@gmail.com>
References: <20181223045743.10716-1-jeremyfertic@gmail.com>
        <20181223045743.10716-5-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 Sat, 22 Dec 2018 21:57:43 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> The lsb calculation is not masking the correct bits from the user input.
> Subtract 1 from (1 << offset) to correctly set up the mask to be applied
> to user input.
> 
> The lsb register stores its value starting at the bit 7 position.
> adt7316_store_DAC() currently assumes the value is at the other end of the
> register. Shift the lsb value before storing it in a new variable lsb_reg,
> and write this variable to the lsb register.
> 
> Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
Applied to the togreg branch fo iio.git and pushed out as testing for the
autobuilders to play with it.

I cc'd Shreeya on all of these.   It would be helpful if you both cc'd
the other one on any future series. Cross review also good given you are both
working with this part!

thanks,

Jonathan

> ---
>  drivers/staging/iio/addac/adt7316.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index d7f3d68e525b..6f7891b567b9 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -1431,7 +1431,7 @@ static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
>  static ssize_t adt7316_store_DAC(struct adt7316_chip_info *chip,
>  				 int channel, const char *buf, size_t len)
>  {
> -	u8 msb, lsb, offset;
> +	u8 msb, lsb, lsb_reg, offset;
>  	u16 data;
>  	int ret;
>  
> @@ -1449,9 +1449,13 @@ static ssize_t adt7316_store_DAC(struct adt7316_chip_info *chip,
>  		return -EINVAL;
>  
>  	if (chip->dac_bits > 8) {
> -		lsb = data & (1 << offset);
> +		lsb = data & ((1 << offset) - 1);
> +		if (chip->dac_bits == 12)
> +			lsb_reg = lsb << ADT7316_DA_12_BIT_LSB_SHIFT;
> +		else
> +			lsb_reg = lsb << ADT7316_DA_10_BIT_LSB_SHIFT;
>  		ret = chip->bus.write(chip->bus.client,
> -			ADT7316_DA_DATA_BASE + channel * 2, lsb);
> +			ADT7316_DA_DATA_BASE + channel * 2, lsb_reg);
>  		if (ret)
>  			return -EIO;
>  	}