| Message ID | 20181212005503.28054-7-jeremyfertic@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: adt7316: dac fixes | expand |
On Tue, 11 Dec 2018 17:54:58 -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. > > Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> Looks right to me. Wow this driver had some impressively wrong maths in it ;) Glad you are fixing this up. I'll pick up in v2. 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 b7d12d003ddc..77ef3c209b67 100644 > --- a/drivers/staging/iio/addac/adt7316.c > +++ b/drivers/staging/iio/addac/adt7316.c > @@ -1442,7 +1442,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; > > @@ -1460,9 +1460,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; > }
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c index b7d12d003ddc..77ef3c209b67 100644 --- a/drivers/staging/iio/addac/adt7316.c +++ b/drivers/staging/iio/addac/adt7316.c @@ -1442,7 +1442,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; @@ -1460,9 +1460,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; }
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. Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com> --- drivers/staging/iio/addac/adt7316.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)