[PATCH 00/11] staging: iio: adt7316: dac fixes

[PATCH 00/11] staging: iio: adt7316: dac fixes

[Jeremy Fertic]

Here are some dac related fixes for adt7316. I'm testing with an adt7516
over i2c to an orange pi pc. I've attempted to test any functionality that
these patches are touching.

Jeremy Fertic (11):
  staging: iio: adt7316: fix register and bit definitions
  staging: iio: adt7316: invert the logic of the check for an ldac pin
  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
  staging: iio: adt7316: use correct variable in DAC_internal_Vref read
  staging: iio: adt7316: allow adt751x to use internal vref for all dacs
  staging: iio: adt7316: remove dac vref buffer bypass from adt751x
  staging: iio: adt7316: change interpretation of write to dac update mode
  staging: iio: adt7316: correct spelling of ADT7316_DA_EN_VIA_DAC_LDCA

 drivers/staging/iio/addac/adt7316.c | 89 ++++++++++++++---------------
 1 file changed, 43 insertions(+), 46 deletions(-)

-- 
2.19.1

[PATCH 01/11] staging: iio: adt7316: fix register and bit definitions

[Jeremy Fertic]

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

Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index dc93e85808e0..1fa4a4c2b4f3 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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
@@ -117,7 +117,7 @@
  */
 #define ADT7316_ADCLK_22_5		0x1
 #define ADT7316_DA_HIGH_RESOLUTION	0x2
-#define ADT7316_DA_EN_VIA_DAC_LDCA	0x4
+#define ADT7316_DA_EN_VIA_DAC_LDCA	0x8
 #define ADT7516_AIN_IN_VREF		0x10
 #define ADT7316_EN_IN_TEMP_PROP_DACA	0x20
 #define ADT7316_EN_EX_TEMP_PROP_DACB	0x40
-- 
2.19.1

[PATCH 02/11] staging: iio: adt7316: invert the logic of the check for an ldac pin

[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 <jeremyfertic@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 1fa4a4c2b4f3..e5e1f9d6357f 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -2130,7 +2130,7 @@ int adt7316_probe(struct device *dev, struct adt7316_bus *bus,
 		return ret;
 	}
 
-	if (chip->ldac_pin) {
+	if (!chip->ldac_pin) {
 		chip->config3 |= ADT7316_DA_EN_VIA_DAC_LDCA;
 		if ((chip->id & ID_FAMILY_MASK) == ID_ADT75XX)
 			chip->config1 |= ADT7516_SEL_AIN3;
-- 
2.19.1

[PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

[Jeremy Fertic]

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 <jeremyfertic@gmail.com>
---
 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 e5e1f9d6357f..a9990e7f2a4d 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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);
-- 
2.19.1

[PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

[Jeremy Fertic]

The dac high resolution option 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 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, return an error if the
device does not support this mode.

Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
---
 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 a9990e7f2a4d..eee7c04f93f4 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -632,9 +632,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");
 	}
 
@@ -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;
+
+	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

[PATCH 05/11] staging: iio: adt7316: fix the dac read calculation

[Jeremy Fertic]

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 <jeremyfertic@gmail.com>
---
 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 eee7c04f93f4..b7d12d003ddc 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
@@ -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);
 }
-- 
2.19.1

[PATCH 06/11] staging: iio: adt7316: fix the dac write calculation

[Jeremy Fertic]

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(-)

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;
 	}
-- 
2.19.1

[PATCH 07/11] staging: iio: adt7316: use correct variable in DAC_internal_Vref read

[Jeremy Fertic]

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 <jeremyfertic@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 77ef3c209b67..98101a7157d2 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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,
-- 
2.19.1

[PATCH 08/11] staging: iio: adt7316: allow adt751x to use internal vref for all dacs

[Jeremy Fertic]

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 <jeremyfertic@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 98101a7157d2..3348fdf08f2e 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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);
-- 
2.19.1

[PATCH 09/11] staging: iio: adt7316: remove dac vref buffer bypass from adt751x

[Jeremy Fertic]

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 <jeremyfertic@gmail.com>
---
 drivers/staging/iio/addac/adt7316.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index 3348fdf08f2e..bca599d8c51c 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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,
-- 
2.19.1

[PATCH 10/11] staging: iio: adt7316: change interpretation of write to dac update mode

[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
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 <jeremyfertic@gmail.com>
---
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(-)

diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
index bca599d8c51c..58b462ad0c83 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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)
-- 
2.19.1

[PATCH 11/11] staging: iio: adt7316: correct spelling of ADT7316_DA_EN_VIA_DAC_LDCA

[Jeremy Fertic]

Change LDCA to LDAC.

Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
---
 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 58b462ad0c83..020d695ded97 100644
--- a/drivers/staging/iio/addac/adt7316.c
+++ b/drivers/staging/iio/addac/adt7316.c
@@ -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;
 	}
-- 
2.19.1

Re: [PATCH 02/11] staging: iio: adt7316: invert the logic of the check for an ldac pin

[Dan Carpenter]

On Tue, Dec 11, 2018 at 05:54:54PM -0700, Jeremy Fertic wrote:
> 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 <jeremyfertic@gmail.com>

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

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

regards,
dan carpenter

Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

[Dan Carpenter]

On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote:
> @@ -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.

regards,
dan carpenter

Re: [PATCH 10/11] staging: iio: adt7316: change interpretation of write to dac update mode

[Dan Carpenter]

On Tue, Dec 11, 2018 at 05:55:02PM -0700, Jeremy Fertic wrote:
> 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 <jeremyfertic@gmail.com>
> ---
> 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.

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

Re: [PATCH 10/11] staging: iio: adt7316: change interpretation of write to dac update mode

[Jonathan Cameron]

On 12 December 2018 08:31:32 GMT, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>On Tue, Dec 11, 2018 at 05:55:02PM -0700, Jeremy Fertic wrote:
>> 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 <jeremyfertic@gmail.com>
>> ---
>> 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.
>
>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

This driver breaks the standard abi in loads of ways. It is going to change userspace interface
 a lot before it is ready to move out of staging. That includes this particular interface almost
 certainly being completely replaced.  Hence good to move towards something sensible.  Don't worry at all
 about userapace ABI breaks in this one!

Jonathan

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

[Jeremy Fertic]

On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote:
> On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote:
> > @@ -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.
> 
> 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. Is there any rule of thumb about
when -EPERM is appropriate for a driver, or is -EINVAL almost always the
better option?

Re: [PATCH 02/11] staging: iio: adt7316: invert the logic of the check for an ldac pin

[Jeremy Fertic]

On Wed, Dec 12, 2018 at 11:19:49AM +0300, Dan Carpenter wrote:
> On Tue, Dec 11, 2018 at 05:54:54PM -0700, Jeremy Fertic wrote:
> > 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 <jeremyfertic@gmail.com>
> 
> Huh...  This bug has always been there...
> 
> Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> 
> 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.

Re: [PATCH 02/11] staging: iio: adt7316: invert the logic of the check for an ldac pin

[Dan Carpenter]

On Thu, Dec 13, 2018 at 03:06:29PM -0700, Jeremy Fertic wrote:
> On Wed, Dec 12, 2018 at 11:19:49AM +0300, Dan Carpenter wrote:
> > On Tue, Dec 11, 2018 at 05:54:54PM -0700, Jeremy Fertic wrote:
> > > 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 <jeremyfertic@gmail.com>
> > 
> > Huh...  This bug has always been there...
> > 
> > Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> > 
> > 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

Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

[Dan Carpenter]

On Thu, Dec 13, 2018 at 03:01:46PM -0700, Jeremy Fertic wrote:
> On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote:
> > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote:
> > > @@ -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.
> > 
> > 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.

> 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

Re: [PATCH 04/11] staging: iio: adt7316: fix handling of dac high resolution option

[Jeremy Fertic]

On Fri, Dec 14, 2018 at 09:26:18AM +0300, Dan Carpenter wrote:
> On Thu, Dec 13, 2018 at 03:01:46PM -0700, Jeremy Fertic wrote:
> > On Wed, Dec 12, 2018 at 11:23:16AM +0300, Dan Carpenter wrote:
> > > On Tue, Dec 11, 2018 at 05:54:56PM -0700, Jeremy Fertic wrote:
> > > > @@ -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.
> > > 
> > > 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.
> 
> > 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
> 

I'll wait to deal with these error values since some of them might go away
with all the changes necessary to get the driver out of staging. Thanks
for clarifying things for me.

Re: [PATCH 01/11] staging: iio: adt7316: fix register and bit definitions

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:54:53 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> Change two register addresses and one bit definition to match the
> datasheet.
> 
> Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
One comment inline.  I added a fixes tag but also a note saying I would
not suggest backporting to stable.

There are too many things wrong with this driver for any backports
to really be worthwhile.  It didn't work at all for i2c until last cycle
for example!

Jonathan

> ---
>  drivers/staging/iio/addac/adt7316.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index dc93e85808e0..1fa4a4c2b4f3 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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
> @@ -117,7 +117,7 @@
>   */
>  #define ADT7316_ADCLK_22_5		0x1
>  #define ADT7316_DA_HIGH_RESOLUTION	0x2
> -#define ADT7316_DA_EN_VIA_DAC_LDCA	0x4
> +#define ADT7316_DA_EN_VIA_DAC_LDCA	0x8
This looks to be called LDAC not LDCA?  

Different error so I don't mind that being fixed later!

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

Re: [PATCH 02/11] staging: iio: adt7316: invert the logic of the check for an ldac pin

[Jonathan Cameron]

On Fri, 14 Dec 2018 09:18:20 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Thu, Dec 13, 2018 at 03:06:29PM -0700, Jeremy Fertic wrote:
> > On Wed, Dec 12, 2018 at 11:19:49AM +0300, Dan Carpenter wrote:  
> > > On Tue, Dec 11, 2018 at 05:54:54PM -0700, Jeremy Fertic wrote:  
> > > > 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 <jeremyfertic@gmail.com>  
> > > 
> > > Huh...  This bug has always been there...
> > > 
> > > Fixes: 35f6b6b86ede ("staging: iio: new ADT7316/7/8 and ADT7516/7/9 driver")
> > > 
> > > 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.
> 
Absolutely agreed. It's useful information even if we don't plan on
backporting.  Note that some staging fixes do get backported but
I'm adding a note to most things on this driver to say don't bother!

It's too far from 'good'.  Great to have multiple people working on
sorting that though!

If you and Shreeya could review each others patches that would be
cool.  I do have one of these, but I'm usually too lazy to actually
dig it out to test if there are others who are working with it
more normally!

Jonathan

> regards,
> dan carpenter

Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:54:55 -0700
Jeremy Fertic <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com>

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
> ---
>  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 e5e1f9d6357f..a9990e7f2a4d 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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);

Re: [PATCH 05/11] staging: iio: adt7316: fix the dac read calculation

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:54:57 -0700
Jeremy Fertic <jeremyfertic@gmail.com> 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.
> 
> Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
This looks good, but with the previous 2 patches under discussion I'll
hold this one for v2.

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 eee7c04f93f4..b7d12d003ddc 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
> @@ -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);
>  }

Re: [PATCH 06/11] staging: iio: adt7316: fix the dac write calculation

[Jonathan Cameron]

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;
>  	}

Re: [PATCH 07/11] staging: iio: adt7316: use correct variable in DAC_internal_Vref read

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:54:59 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> 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 <jeremyfertic@gmail.com>
Is this a follow through from the earlier 'correcting' the register
addresses?  Looks like it to me, so should really have been in that
same patch.

Oh well, it's staging and horribly broken so let's ignore that ;)

It's separated enough from the earlier patches that I will apply it now
though.  Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Again I have added a do not backport note as far too much chance of
it going wrong.

Jonathan


> ---
>  drivers/staging/iio/addac/adt7316.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 77ef3c209b67..98101a7157d2 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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,

Re: [PATCH 08/11] staging: iio: adt7316: allow adt751x to use internal vref for all dacs

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:55:00 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> 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 <jeremyfertic@gmail.com>
This one is nice and separated from the earlier patches and 'obviously' right
I think.  Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

It's also fine to backport though given how broken the driver was before
patches that aren't, I'm not going to mark it for stable!

Thanks,

Jonathan

> ---
>  drivers/staging/iio/addac/adt7316.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 98101a7157d2..3348fdf08f2e 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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);

Re: [PATCH 09/11] staging: iio: adt7316: remove dac vref buffer bypass from adt751x

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:55:01 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> 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 <jeremyfertic@gmail.com>
Another good little cleanup, well separated from the other bits so
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 | 14 --------------
>  1 file changed, 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index 3348fdf08f2e..bca599d8c51c 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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,

Re: [PATCH 10/11] staging: iio: adt7316: change interpretation of write to dac update mode

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:55:02 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> 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 <jeremyfertic@gmail.com>
As I mentioned, long term this interface is going to need to be replaced
with something more generic.  Probably something like the power down
modes where we use a string to describe what is going on on.

Still this is a step in the right direction even if we may go further
shortly!

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it,

Thanks,

Jonathan

> ---
> 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(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c b/drivers/staging/iio/addac/adt7316.c
> index bca599d8c51c..58b462ad0c83 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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)

Re: [PATCH 11/11] staging: iio: adt7316: correct spelling of ADT7316_DA_EN_VIA_DAC_LDCA

[Jonathan Cameron]

On Tue, 11 Dec 2018 17:55:03 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> Change LDCA to LDAC.
> 
> Signed-off-by: Jeremy Fertic <jeremyfertic@gmail.com>
Ah.  Here it is ;)  As commented on earlier.

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 58b462ad0c83..020d695ded97 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -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;
>  	}

Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

[Jeremy Fertic]

On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote:
> On Tue, 11 Dec 2018 17:54:55 -0700
> Jeremy Fertic <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com>
> 
> 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

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

> > ---
> >  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 e5e1f9d6357f..a9990e7f2a4d 100644
> > --- a/drivers/staging/iio/addac/adt7316.c
> > +++ b/drivers/staging/iio/addac/adt7316.c
> > @@ -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);
> 

Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

[Jonathan Cameron]

On Mon, 17 Dec 2018 14:30:59 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote:
> > On Tue, 11 Dec 2018 17:54:55 -0700
> > Jeremy Fertic <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com>  
> > 
> > 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  
> 
> 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.
> 
Good explanation.  Send me a v2 with that in the description.

Thanks,

Jonathan

> Jeremy
> 
> > > ---
> > >  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 e5e1f9d6357f..a9990e7f2a4d 100644
> > > --- a/drivers/staging/iio/addac/adt7316.c
> > > +++ b/drivers/staging/iio/addac/adt7316.c
> > > @@ -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);  
> >   

Re: [PATCH 03/11] staging: iio: adt7316: fix dac_bits assignment

[Jonathan Cameron]

On Mon, 17 Dec 2018 14:30:59 -0700
Jeremy Fertic <jeremyfertic@gmail.com> wrote:

> On Sun, Dec 16, 2018 at 11:37:56AM +0000, Jonathan Cameron wrote:
> > On Tue, 11 Dec 2018 17:54:55 -0700
> > Jeremy Fertic <jeremyfertic@gmail.com> 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 <jeremyfertic@gmail.com>  
> > 
> > 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  
> 
> 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.
> 
Good explanation.  Send me a v2 with that in the description.

Thanks,

Jonathan

> Jeremy
> 
> > > ---
> > >  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 e5e1f9d6357f..a9990e7f2a4d 100644
> > > --- a/drivers/staging/iio/addac/adt7316.c
> > > +++ b/drivers/staging/iio/addac/adt7316.c
> > > @@ -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);  
> >