Changes in v2:
- Patches 1-2: reworded commit messages and added Fixes tags
- Patches 3-4: added Fixes tags
Jeremy Fertic (4):
staging: iio: adt7316: fix dac_bits assignment
staging: iio: adt7316: fix handling of dac high resolution option
staging: iio: adt7316: fix the dac read calculation
staging: iio: adt7316: fix the dac write calculation
drivers/staging/iio/addac/adt7316.c | 46 ++++++++++++++++++-----------
1 file changed, 28 insertions(+), 18 deletions(-)
--
2.19.1
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 <[email protected]>
---
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;
}
--
2.19.1
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 <[email protected]>
---
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);
--
2.19.1
The calculation of the current dac value is using the wrong bits of the
dac lsb register. Create two macros to shift the lsb register value into
lsb position, depending on whether the dac is 10 or 12 bit. Initialize
data to 0 so, with an 8 bit dac, the msb register value can be bitwise
ORed with data.
Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
Signed-off-by: Jeremy Fertic <[email protected]>
---
drivers/staging/iio/addac/adt7316.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 80cc3420036b..d7f3d68e525b 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -47,6 +47,8 @@
#define ADT7516_MSB_AIN3 0xA
#define ADT7516_MSB_AIN4 0xB
#define ADT7316_DA_DATA_BASE 0x10
+#define ADT7316_DA_10_BIT_LSB_SHIFT 6
+#define ADT7316_DA_12_BIT_LSB_SHIFT 4
#define ADT7316_DA_MSB_DATA_REGS 4
#define ADT7316_LSB_DAC_A 0x10
#define ADT7316_MSB_DAC_A 0x11
@@ -1392,7 +1394,7 @@ static IIO_DEVICE_ATTR(ex_analog_temp_offset, 0644,
static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
int channel, char *buf)
{
- u16 data;
+ u16 data = 0;
u8 msb, lsb, offset;
int ret;
@@ -1417,7 +1419,11 @@ static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
if (ret)
return -EIO;
- data = (msb << offset) + (lsb & ((1 << offset) - 1));
+ if (chip->dac_bits == 12)
+ data = lsb >> ADT7316_DA_12_BIT_LSB_SHIFT;
+ else if (chip->dac_bits == 10)
+ data = lsb >> ADT7316_DA_10_BIT_LSB_SHIFT;
+ data |= msb << offset;
return sprintf(buf, "%d\n", data);
}
--
2.19.1
The adt7316/7 and adt7516/7 have the option to output voltage proportional
to temperature on dac a and/or dac b. The default dac resolution in this
mode is 8 bits with the dac high resolution option enabling 10 bits. None
of these settings affect dacs c and d. Remove the "1 (12 bits)" output from
the show function since that is not an option for this mode. Return
"1 (10 bits)" if the device is one of the above mentioned chips and the dac
high resolution mode is enabled.
In the store function, the driver currently allows the user to write to the
ADT7316_DA_HIGH_RESOLUTION bit regardless of the device in use. Add a check
to return an error in the case of an adt7318 or adt7519. Remove the else
statement that clears the ADT7316_DA_HIGH_RESOLUTION bit. Instead, clear it
before conditionally enabling it, depending on user input. This matches the
typical pattern in the driver when an attribute is a boolean.
Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
Signed-off-by: Jeremy Fertic <[email protected]>
---
drivers/staging/iio/addac/adt7316.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index e17c1cb12c94..80cc3420036b 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -633,9 +633,7 @@ static ssize_t adt7316_show_da_high_resolution(struct device *dev,
struct adt7316_chip_info *chip = iio_priv(dev_info);
if (chip->config3 & ADT7316_DA_HIGH_RESOLUTION) {
- if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
- return sprintf(buf, "1 (12 bits)\n");
- if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517)
+ if (chip->id != ID_ADT7318 && chip->id != ID_ADT7519)
return sprintf(buf, "1 (10 bits)\n");
}
@@ -652,10 +650,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
u8 config3;
int ret;
+ if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519)
+ return -EPERM;
+
+ config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
if (buf[0] == '1')
- config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION;
- else
- config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
+ config3 |= ADT7316_DA_HIGH_RESOLUTION;
ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
if (ret)
--
2.19.1
+ CC Shreeya who is working on the same driver.
On Sat, 22 Dec 2018 21:57:40 -0700
Jeremy Fertic <[email protected]> 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 <[email protected]>
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);
On Sat, 22 Dec 2018 21:57:42 -0700
Jeremy Fertic <[email protected]> wrote:
> The calculation of the current dac value is using the wrong bits of the
> dac lsb register. Create two macros to shift the lsb register value into
> lsb position, depending on whether the dac is 10 or 12 bit. Initialize
> data to 0 so, with an 8 bit dac, the msb register value can be bitwise
> ORed with data.
>
> Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> Signed-off-by: Jeremy Fertic <[email protected]>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/staging/iio/addac/adt7316.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 80cc3420036b..d7f3d68e525b 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -47,6 +47,8 @@
> #define ADT7516_MSB_AIN3 0xA
> #define ADT7516_MSB_AIN4 0xB
> #define ADT7316_DA_DATA_BASE 0x10
> +#define ADT7316_DA_10_BIT_LSB_SHIFT 6
> +#define ADT7316_DA_12_BIT_LSB_SHIFT 4
> #define ADT7316_DA_MSB_DATA_REGS 4
> #define ADT7316_LSB_DAC_A 0x10
> #define ADT7316_MSB_DAC_A 0x11
> @@ -1392,7 +1394,7 @@ static IIO_DEVICE_ATTR(ex_analog_temp_offset, 0644,
> static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
> int channel, char *buf)
> {
> - u16 data;
> + u16 data = 0;
> u8 msb, lsb, offset;
> int ret;
>
> @@ -1417,7 +1419,11 @@ static ssize_t adt7316_show_DAC(struct adt7316_chip_info *chip,
> if (ret)
> return -EIO;
>
> - data = (msb << offset) + (lsb & ((1 << offset) - 1));
> + if (chip->dac_bits == 12)
> + data = lsb >> ADT7316_DA_12_BIT_LSB_SHIFT;
> + else if (chip->dac_bits == 10)
> + data = lsb >> ADT7316_DA_10_BIT_LSB_SHIFT;
> + data |= msb << offset;
>
> return sprintf(buf, "%d\n", data);
> }
On Sat, 22 Dec 2018 21:57:43 -0700
Jeremy Fertic <[email protected]> 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 <[email protected]>
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;
> }
On Sat, 22 Dec 2018 21:57:41 -0700
Jeremy Fertic <[email protected]> wrote:
> The adt7316/7 and adt7516/7 have the option to output voltage proportional
> to temperature on dac a and/or dac b. The default dac resolution in this
> mode is 8 bits with the dac high resolution option enabling 10 bits. None
> of these settings affect dacs c and d. Remove the "1 (12 bits)" output from
> the show function since that is not an option for this mode. Return
> "1 (10 bits)" if the device is one of the above mentioned chips and the dac
> high resolution mode is enabled.
>
> In the store function, the driver currently allows the user to write to the
> ADT7316_DA_HIGH_RESOLUTION bit regardless of the device in use. Add a check
> to return an error in the case of an adt7318 or adt7519. Remove the else
> statement that clears the ADT7316_DA_HIGH_RESOLUTION bit. Instead, clear it
> before conditionally enabling it, depending on user input. This matches the
> typical pattern in the driver when an attribute is a boolean.
>
> Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> Signed-off-by: Jeremy Fertic <[email protected]>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.
Thanks,
Jonathan
> ---
> drivers/staging/iio/addac/adt7316.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index e17c1cb12c94..80cc3420036b 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -633,9 +633,7 @@ static ssize_t adt7316_show_da_high_resolution(struct device *dev,
> struct adt7316_chip_info *chip = iio_priv(dev_info);
>
> if (chip->config3 & ADT7316_DA_HIGH_RESOLUTION) {
> - if (chip->id == ID_ADT7316 || chip->id == ID_ADT7516)
> - return sprintf(buf, "1 (12 bits)\n");
> - if (chip->id == ID_ADT7317 || chip->id == ID_ADT7517)
> + if (chip->id != ID_ADT7318 && chip->id != ID_ADT7519)
> return sprintf(buf, "1 (10 bits)\n");
> }
>
> @@ -652,10 +650,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
> u8 config3;
> int ret;
>
> + if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519)
> + return -EPERM;
> +
> + config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
> if (buf[0] == '1')
> - config3 = chip->config3 | ADT7316_DA_HIGH_RESOLUTION;
> - else
> - config3 = chip->config3 & (~ADT7316_DA_HIGH_RESOLUTION);
> + config3 |= ADT7316_DA_HIGH_RESOLUTION;
>
> ret = chip->bus.write(chip->bus.client, ADT7316_CONFIG3, config3);
> if (ret)