staging: iio: adt7316: dac fixes

Change two register addresses and one bit definition to match the datasheet.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/staging/iio/addac/adt7316.c +2 -2

index dc93e85808e0..1fa4a4c2b4f3 100644
@@ -59,8 +59,8 @@
 #define ADT7316_CONFIG1			0x18
 #define ADT7316_CONFIG2			0x19
 #define ADT7316_CONFIG3			0x1A
-#define ADT7316_LDAC_CONFIG		0x1B
-#define ADT7316_DAC_CONFIG		0x1C
+#define ADT7316_DAC_CONFIG		0x1B
+#define ADT7316_LDAC_CONFIG		0x1C
 #define ADT7316_INT_MASK1		0x1D
 #define ADT7316_INT_MASK2		0x1E
 #define ADT7316_IN_TEMP_OFFSET		0x1F

drivers/staging/iio/addac/adt7316.c +0 -0

 #define ADT7516_AIN_IN_VREF		0x10
 #define ADT7316_EN_IN_TEMP_PROP_DACA	0x20
 #define ADT7316_EN_EX_TEMP_PROP_DACB	0x40

ADT7316_DA_EN_VIA_DAC_LDCA is set when the dac and ldac registers are being used to update the dacs instead of the ldac pin. ADT7516_SEL_AIN3 is an adc input that shares the ldac pin. Only set these bits if an ldac pin is not being used.

Signed-off-by Jeremy Fertic <[email protected]>

Quoted 5 lines · Jeremy Fertic

ADT7316_DA_EN_VIA_DAC_LDCA is set when the dac and ldac registers are being used to update the dacs instead of the ldac pin. ADT7516_SEL_AIN3 is an adc input that shares the ldac pin. Only set these bits if an ldac pin is not being used.

Quoted 1 line

Signed-off-by Jeremy Fertic <[email protected]>

Huh... This bug has always been there...

Fixes 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")

regards,
dan carpenter

Quoted 12 lines · Dan Carpenter

Quoted 6 lines · Jeremy Fertic

ADT7316_DA_EN_VIA_DAC_LDCA is set when the dac and ldac registers are being used to update the dacs instead of the ldac pin. ADT7516_SEL_AIN3 is an adc input that shares the ldac pin. Only set these bits if an ldac pin is not being used.

Signed-off-by Jeremy Fertic <[email protected]>

Huh... This bug has always been there...

Fixes 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")

Quoted 3 lines

regards,
dan carpenter

Should I include this Fixes tag in v2? I wasn't sure how important this was in staging. I think most of the patches in this series fix bugs that date back to the introduction of the driver.

Quoted 13 lines · Jeremy Fertic

Quoted 12 lines · Dan Carpenter

Quoted 6 lines · Jeremy Fertic

ADT7316_DA_EN_VIA_DAC_LDCA is set when the dac and ldac registers are being used to update the dacs instead of the ldac pin. ADT7516_SEL_AIN3 is an adc input that shares the ldac pin. Only set these bits if an ldac pin is not being used.

Signed-off-by Jeremy Fertic <[email protected]>

Huh... This bug has always been there...

Fixes 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")

Quoted 7 lines

Quoted 3 lines

regards,
dan carpenter

Should I include this Fixes tag in v2? I wasn't sure how important this was in staging. I think most of the patches in this series fix bugs that date back to the introduction of the driver.

I was just curious to see if it was a cleanup which introduced the inverted if statement.

I think the Fixes tag is always useful. For example, it would be interesting to do some data mining to see how many bugs drivers normally have when they're first merged.

regards,
dan carpenter

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 <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/staging/iio/addac/adt7316.c +9 -9

index e5e1f9d6357f..a9990e7f2a4d 100644
@@ -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);

Quoted 11 lines · On Tue, 11 Dec 2018 17:54:55 -0700 Jeremy Fertic <[email protected]> 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 <[email protected]>

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

Quoted 42 lines

---
 drivers/staging/iio/addac/adt7316.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/staging/iio/addac/adt7316.c +9 -9

index e5e1f9d6357f..a9990e7f2a4d 100644
@@ -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);

Quoted 23 lines · Jonathan Cameron

Quoted 11 lines · On Tue, 11 Dec 2018 17:54:55 -0700 Jeremy Fertic <[email protected]> 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 <[email protected]>

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.

Quoted 7 lines

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

Thanks for the feedback Jonathan, and sorry for the confusion on this one. My commit message should have been clearer. This is not a general purpose option to change the dac resolution. 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 ouput voltage proportional to temperature, this da_high_resolution attribute can optionally be enabled to use 10 bit resolution rather than the default 8 bits. It 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. The dacs c and d continue to operate normally under these conditions.

With the above in mind, dac_bits should have never been assigned to in this da_high_resolution attribute. Before this patch, if the user didn't write to this optional attribute, dac_bits would be 0 since it was never assigned to anywhere else. This would result in incorrect calculations in show_DAC and store_DAC.

Jeremy

Quoted 43 lines

Quoted 42 lines

---
 drivers/staging/iio/addac/adt7316.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/staging/iio/addac/adt7316.c +9 -9

index e5e1f9d6357f..a9990e7f2a4d 100644
@@ -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);  

Quoted 4 lines · Jeremy Fertic

drivers/staging/iio/addac/adt7316.c +0 -0

@@ -651,10 +649,12 @@ static ssize_t adt7316_store_da_high_resolution(struct device *dev,
 	u8 config3;
 	int ret;
 

drivers/staging/iio/addac/adt7316.c static ssize_t adt7316_store_da_high_resolution(struct device *dev, · line 653

Quoted 2 lines

drivers/staging/iio/addac/adt7316.c +2 -0

+	if (chip->id == ID_ADT7318 || chip->id == ID_ADT7519)
+		return -EPERM;

return -EINVAL is more appropriate than -EPERM.

regards,
dan carpenter

Quoted 10 lines · Dan Carpenter

drivers/staging/iio/addac/adt7316.c static ssize_t adt7316_store_da_high_resolution(struct device *dev, · line 653

Quoted 6 lines · Jeremy Fertic

drivers/staging/iio/addac/adt7316.c +2 -0

@@ -651,10 +649,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;

return -EINVAL is more appropriate than -EPERM.

Quoted 3 lines

regards,
dan carpenter

I chose -EPERM because the driver uses it quite a few times in similar circumstances.

At least with this driver, -EINVAL is used when the user attempts to write data that would never be valid. -EPERM is used when either the current device settings prevent some functionality from being used, or the device never supports that functionality. This patch is the latter, that these two chip ids never support this function.

I'll change to -EINVAL in a v2 series, but I wonder if I should hold off on a separate patch for other instances in this driver since it will be undergoing a substantial refactoring.

Quoted 11 lines · Jeremy Fertic

Quoted 10 lines · Dan Carpenter

drivers/staging/iio/addac/adt7316.c static ssize_t adt7316_store_da_high_resolution(struct device *dev, · line 653

Quoted 6 lines · Jeremy Fertic

drivers/staging/iio/addac/adt7316.c +2 -0

@@ -651,10 +649,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;

return -EINVAL is more appropriate than -EPERM.

Quoted 6 lines

Quoted 3 lines

regards,
dan carpenter

I chose -EPERM because the driver uses it quite a few times in similar circumstances.

Yeah. I saw that when I reviewed the later patches in this series.

It's really not doing it right. -EPERM means permission checks like access_ok() failed so it's not appropriate. -EINVAL is sort of general purpose for invalid commands so it's probably the correct thing.

Quoted 9 lines

At least with this driver, -EINVAL is used when the user attempts to write data that would never be valid. -EPERM is used when either the current device settings prevent some functionality from being used, or the device never supports that functionality. This patch is the latter, that these two chip ids never support this function.

I'll change to -EINVAL in a v2 series, but I wonder if I should hold off on a separate patch for other instances in this driver since it will be undergoing a substantial refactoring.

Generally, you should prefer kernel standards over driver standards and especially for staging. But it doesn't matter. When I reviewed this patch, I hadn't seen that the driver was doing it like this but now I know so it's fine. We can clean it all at once later if you want.

regards,
dan carpenter

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.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/staging/iio/addac/adt7316.c +8 -2

index eee7c04f93f4..b7d12d003ddc 100644
@@ -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

@@ -1403,7 +1405,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;
 

@@ -1428,7 +1430,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);
 }

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 <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/staging/iio/addac/adt7316.c +7 -3

index b7d12d003ddc..77ef3c209b67 100644
@@ -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 dac internal vref settings are part of the ldac config register rather than the dac config register. Change the variable being used so the read returns the correct result.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/iio/addac/adt7316.c +2 -2

index 77ef3c209b67..98101a7157d2 100644
@@ -1056,10 +1056,10 @@ static ssize_t adt7316_show_DAC_internal_Vref(struct device *dev,
 
 	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
 		return sprintf(buf, "0x%x\n",
-			(chip->dac_config & ADT7516_DAC_IN_VREF_MASK) >>
+			(chip->ldac_config & ADT7516_DAC_IN_VREF_MASK) >>
 			ADT7516_DAC_IN_VREF_OFFSET);
 	return sprintf(buf, "%d\n",
-		       !!(chip->dac_config & ADT7316_DAC_IN_VREF));
+		       !!(chip->ldac_config & ADT7316_DAC_IN_VREF));
 }
 
 static ssize_t adt7316_store_DAC_internal_Vref(struct device *dev,

With adt7516/7/9, internal vref is available for dacs a and b, dacs c and d, or all dacs. The driver doesn't currently support internal vref for all dacs. Change the else if to an if so both bits are checked rather than just one or the other.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/addac/adt7316.c +1 -1

index 98101a7157d2..3348fdf08f2e 100644
@@ -1081,7 +1081,7 @@ static ssize_t adt7316_store_DAC_internal_Vref(struct device *dev,
 		ldac_config = chip->ldac_config & (~ADT7516_DAC_IN_VREF_MASK);
 		if (data & 0x1)
 			ldac_config |= ADT7516_DAC_AB_IN_VREF;
-		else if (data & 0x2)
+		if (data & 0x2)
 			ldac_config |= ADT7516_DAC_CD_IN_VREF;
 	} else {
 		ret = kstrtou8(buf, 16, &data);

The option to allow the external vref to bypass the reference buffer is only available for adt7316/7/8. Remove the attributes for adt751x as well as the chip->id checks from the show and store functions.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 14 --------------
 1 file changed, 14 deletions(-)

drivers/staging/iio/addac/adt7316.c +0 -14

index 3348fdf08f2e..bca599d8c51c 100644
@@ -964,9 +964,6 @@ static ssize_t adt7316_show_DA_AB_Vref_bypass(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-		return -EPERM;
-
 	return sprintf(buf, "%d\n",
 		!!(chip->dac_config & ADT7316_VREF_BYPASS_DAC_AB));
 }

@@ -981,9 +978,6 @@ static ssize_t adt7316_store_DA_AB_Vref_bypass(struct device *dev,
 	u8 dac_config;
 	int ret;
 
-	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-		return -EPERM;
-
 	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_AB);
 	if (buf[0] == '1')
 		dac_config |= ADT7316_VREF_BYPASS_DAC_AB;

@@ -1009,9 +1003,6 @@ static ssize_t adt7316_show_DA_CD_Vref_bypass(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-		return -EPERM;
-
 	return sprintf(buf, "%d\n",
 		!!(chip->dac_config & ADT7316_VREF_BYPASS_DAC_CD));
 }

@@ -1026,9 +1017,6 @@ static ssize_t adt7316_store_DA_CD_Vref_bypass(struct device *dev,
 	u8 dac_config;
 	int ret;
 
-	if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
-		return -EPERM;
-
 	dac_config = chip->dac_config & (~ADT7316_VREF_BYPASS_DAC_CD);
 	if (buf[0] == '1')
 		dac_config |= ADT7316_VREF_BYPASS_DAC_CD;

@@ -1713,8 +1701,6 @@ static struct attribute *adt7516_attributes[] = {
 	&iio_dev_attr_DAC_update_mode.dev_attr.attr,
 	&iio_dev_attr_all_DAC_update_modes.dev_attr.attr,
 	&iio_dev_attr_update_DAC.dev_attr.attr,
-	&iio_dev_attr_DA_AB_Vref_bypass.dev_attr.attr,
-	&iio_dev_attr_DA_CD_Vref_bypass.dev_attr.attr,
 	&iio_dev_attr_DAC_internal_Vref.dev_attr.attr,
 	&iio_dev_attr_VDD.dev_attr.attr,
 	&iio_dev_attr_in_temp.dev_attr.attr,

Based on the output of adt7316_show_all_DAC_update_modes() and adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() should expect the user to enter an integer input from 0 to 3. The user input is currently expected to account for the actual bit positions in the register. For example, choosing option 3 would require a write of 0x30 (actually 48 since it expects base 10). To address this inconsistency, create a shift macro to be used in the valid input check as well as the calculation for the register write.

Signed-off-by Jeremy Fertic <[email protected]>

---
I'm not sure if this patch is appropriate since it's making a user visible change. I've included it since the driver is still in staging.

Quoted 2 lines · Jeremy Fertic

Based on the output of adt7316_show_all_DAC_update_modes() and adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode()

should

Quoted 1 line

expect the user to enter an integer input from 0 to 3. The user input

is

Quoted 1 line

currently expected to account for the actual bit positions in the

register.

Quoted 1 line

For example, choosing option 3 would require a write of 0x30

(actually 48

Quoted 1 line

since it expects base 10). To address this inconsistency, create a

shift

Quoted 1 line

macro to be used in the valid input check as well as the calculation

for

Quoted 5 lines

the register write.

Signed-off-by Jeremy Fertic <[email protected]>

---
I'm not sure if this patch is appropriate since it's making a user

visible

Quoted 1 line

change. I've included it since the driver is still in staging.

We don't want to break user space, but I agree with you that applying this patch is probably the right thing.

regards,
dan carpenter

Based on the output of adt7316_show_all_DAC_update_modes() and adt7316_show_DAC_update_mode(), adt7316_store_DAC_update_mode() should expect the user to enter an integer input from 0 to 3. The user input is currently expected to account for the actual bit positions in the register. For example, choosing option 3 would require a write of 0x30 (actually 48 since it expects base 10). To address this inconsistency, create a shift macro to be used in the valid input check as well as the calculation for the register write.

Signed-off-by Jeremy Fertic <[email protected]>

---
I'm not sure if this patch is appropriate since it's making a user visible change. I've included it since the driver is still in staging.

 drivers/staging/iio/addac/adt7316.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/staging/iio/addac/adt7316.c +3 -2

index bca599d8c51c..58b462ad0c83 100644
@@ -129,6 +129,7 @@
  */
 #define ADT7316_DA_2VREF_CH_MASK	0xF
 #define ADT7316_DA_EN_MODE_MASK		0x30
+#define ADT7316_DA_EN_MODE_SHIFT	4
 #define ADT7316_DA_EN_MODE_SINGLE	0x00
 #define ADT7316_DA_EN_MODE_AB_CD	0x10
 #define ADT7316_DA_EN_MODE_ABCD		0x20

@@ -879,11 +880,11 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev,
 		return -EPERM;
 
 	ret = kstrtou8(buf, 10, &data);
-	if (ret || data > ADT7316_DA_EN_MODE_MASK)
+	if (ret || data > (ADT7316_DA_EN_MODE_MASK >> ADT7316_DA_EN_MODE_SHIFT))
 		return -EINVAL;
 
 	dac_config = chip->dac_config & (~ADT7316_DA_EN_MODE_MASK);
-	dac_config |= data;
+	dac_config |= data << ADT7316_DA_EN_MODE_SHIFT;
 
 	ret = chip->bus.write(chip->bus.client, ADT7316_DAC_CONFIG, dac_config);
 	if (ret)

Change LDCA to LDAC.

Signed-off-by Jeremy Fertic <[email protected]>

---
 drivers/staging/iio/addac/adt7316.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/staging/iio/addac/adt7316.c +6 -6

index 58b462ad0c83..020d695ded97 100644
@@ -119,7 +119,7 @@
  */
 #define ADT7316_ADCLK_22_5		0x1
 #define ADT7316_DA_HIGH_RESOLUTION	0x2
-#define ADT7316_DA_EN_VIA_DAC_LDCA	0x8
+#define ADT7316_DA_EN_VIA_DAC_LDAC	0x8
 #define ADT7516_AIN_IN_VREF		0x10
 #define ADT7316_EN_IN_TEMP_PROP_DACA	0x20
 #define ADT7316_EN_EX_TEMP_PROP_DACB	0x40

@@ -847,7 +847,7 @@ static ssize_t adt7316_show_DAC_update_mode(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDCA))
+	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC))
 		return sprintf(buf, "manual\n");
 
 	switch (chip->dac_config & ADT7316_DA_EN_MODE_MASK) {

@@ -876,7 +876,7 @@ static ssize_t adt7316_store_DAC_update_mode(struct device *dev,
 	u8 data;
 	int ret;
 
-	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDCA))
+	if (!(chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC))
 		return -EPERM;
 
 	ret = kstrtou8(buf, 10, &data);

@@ -907,7 +907,7 @@ static ssize_t adt7316_show_all_DAC_update_modes(struct device *dev,
 	struct iio_dev *dev_info = dev_to_iio_dev(dev);
 	struct adt7316_chip_info *chip = iio_priv(dev_info);
 
-	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDCA)
+	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC)
 		return sprintf(buf, "0 - auto at any MSB DAC writing\n"
 				"1 - auto at MSB DAC AB and CD writing\n"
 				"2 - auto at MSB DAC ABCD writing\n"

@@ -929,7 +929,7 @@ static ssize_t adt7316_store_update_DAC(struct device *dev,
 	u8 data;
 	int ret;
 
-	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDCA) {
+	if (chip->config3 & ADT7316_DA_EN_VIA_DAC_LDAC) {
 		if ((chip->dac_config & ADT7316_DA_EN_MODE_MASK) !=
 			ADT7316_DA_EN_MODE_LDAC)
 			return -EPERM;

@@ -2128,7 +2128,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 	}
 
 	if (!chip->ldac_pin) {
-		chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDCA;
+		chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDAC;
 		if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
 			chip->config1 |= ADT7516_SEL_AIN3;
 	}