iio:adc:ad7280a Cleanup and proposed staging graduation.

From: Jonathan Cameron <[email protected]>

The bit reversal was wrong for bits 1 and 3 of the 5 bits. Result is driver failure to probe if you have more than 2 daisy-chained devices. Discovered via QEMU based device emulation.

Fixes tag is for when this moved from a macro to a function, but it was broken before that.

Signed-off-by Jonathan Cameron <[email protected]>

Fixes 065a7c0b1fec ("Staging: iio: adc: ad7280a.c: Fixed Macro argument reuse")

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

drivers/staging/iio/adc/ad7280a.c +2 -2

index fef0055b8990..20183b2ea127 100644
@@ -107,9 +107,9 @@
 static unsigned int ad7280a_devaddr(unsigned int addr)
 {
 	return ((addr & 0x1) << 4) |
-	       ((addr & 0x2) << 3) |
+	       ((addr & 0x2) << 2) |
 	       (addr & 0x4) |
-	       ((addr & 0x8) >> 3) |
+	       ((addr & 0x8) >> 2) |
 	       ((addr & 0x10) >> 4);
 }
 

Signature · 1 line

2.34.1

From: Jonathan Cameron <[email protected]>

1. Postfix register addresses with _REG to distinguish them from fields within the registers
2. Switch to using FIELD_PREP and masks to aid readability.
3. Shorten a few defines to make the lines remain a sensible length.
4. Fix an issue whether where an CTRL_LB field is set in CTRL_HB.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 299 ++++++++++++++++--------------
 1 file changed, 161 insertions(+), 138 deletions(-)

drivers/staging/iio/adc/ad7280a.c +9 -63

index 20183b2ea127..d169c8a7b5f1 100644
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>

@@ -23,78 +24,86 @@
 #include "ad7280a.h"
 
 /* Registers */
-#define AD7280A_CELL_VOLTAGE_1		0x0  /* D11 to D0, Read only */
-#define AD7280A_CELL_VOLTAGE_2		0x1  /* D11 to D0, Read only */
-#define AD7280A_CELL_VOLTAGE_3		0x2  /* D11 to D0, Read only */
-#define AD7280A_CELL_VOLTAGE_4		0x3  /* D11 to D0, Read only */
-#define AD7280A_CELL_VOLTAGE_5		0x4  /* D11 to D0, Read only */
-#define AD7280A_CELL_VOLTAGE_6		0x5  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_1		0x6  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_2		0x7  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_3		0x8  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_4		0x9  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_5		0xA  /* D11 to D0, Read only */
-#define AD7280A_AUX_ADC_6		0xB  /* D11 to D0, Read only */
-#define AD7280A_SELF_TEST		0xC  /* D11 to D0, Read only */
-#define AD7280A_CONTROL_HB		0xD  /* D15 to D8, Read/write */
-#define AD7280A_CONTROL_LB		0xE  /* D7 to D0, Read/write */
-#define AD7280A_CELL_OVERVOLTAGE	0xF  /* D7 to D0, Read/write */
-#define AD7280A_CELL_UNDERVOLTAGE	0x10 /* D7 to D0, Read/write */
-#define AD7280A_AUX_ADC_OVERVOLTAGE	0x11 /* D7 to D0, Read/write */
-#define AD7280A_AUX_ADC_UNDERVOLTAGE	0x12 /* D7 to D0, Read/write */
-#define AD7280A_ALERT			0x13 /* D7 to D0, Read/write */
-#define AD7280A_CELL_BALANCE		0x14 /* D7 to D0, Read/write */
-#define AD7280A_CB1_TIMER		0x15 /* D7 to D0, Read/write */
-#define AD7280A_CB2_TIMER		0x16 /* D7 to D0, Read/write */
-#define AD7280A_CB3_TIMER		0x17 /* D7 to D0, Read/write */
-#define AD7280A_CB4_TIMER		0x18 /* D7 to D0, Read/write */
-#define AD7280A_CB5_TIMER		0x19 /* D7 to D0, Read/write */
-#define AD7280A_CB6_TIMER		0x1A /* D7 to D0, Read/write */
-#define AD7280A_PD_TIMER		0x1B /* D7 to D0, Read/write */
-#define AD7280A_READ			0x1C /* D7 to D0, Read/write */
-#define AD7280A_CNVST_CONTROL		0x1D /* D7 to D0, Read/write */
-
-/* Bits and Masks */
-#define AD7280A_CTRL_HB_CONV_INPUT_ALL			0
-#define AD7280A_CTRL_HB_CONV_INPUT_6CELL_AUX1_3_4	BIT(6)
-#define AD7280A_CTRL_HB_CONV_INPUT_6CELL		BIT(7)
-#define AD7280A_CTRL_HB_CONV_INPUT_SELF_TEST		(BIT(7) | BIT(6))
-#define AD7280A_CTRL_HB_CONV_RES_READ_ALL		0
-#define AD7280A_CTRL_HB_CONV_RES_READ_6CELL_AUX1_3_4	BIT(4)
-#define AD7280A_CTRL_HB_CONV_RES_READ_6CELL		BIT(5)
-#define AD7280A_CTRL_HB_CONV_RES_READ_NO		(BIT(5) | BIT(4))
-#define AD7280A_CTRL_HB_CONV_START_CNVST		0
-#define AD7280A_CTRL_HB_CONV_START_CS			BIT(3)
-#define AD7280A_CTRL_HB_CONV_AVG_DIS			0
-#define AD7280A_CTRL_HB_CONV_AVG_2			BIT(1)
-#define AD7280A_CTRL_HB_CONV_AVG_4			BIT(2)
-#define AD7280A_CTRL_HB_CONV_AVG_8			(BIT(2) | BIT(1))
-#define AD7280A_CTRL_HB_CONV_AVG(x)			((x) << 1)
-#define AD7280A_CTRL_HB_PWRDN_SW			BIT(0)
-
-#define AD7280A_CTRL_LB_SWRST				BIT(7)
-#define AD7280A_CTRL_LB_ACQ_TIME_400ns			0
-#define AD7280A_CTRL_LB_ACQ_TIME_800ns			BIT(5)
-#define AD7280A_CTRL_LB_ACQ_TIME_1200ns			BIT(6)
-#define AD7280A_CTRL_LB_ACQ_TIME_1600ns			(BIT(6) | BIT(5))
-#define AD7280A_CTRL_LB_ACQ_TIME(x)			((x) << 5)
-#define AD7280A_CTRL_LB_MUST_SET			BIT(4)
-#define AD7280A_CTRL_LB_THERMISTOR_EN			BIT(3)
-#define AD7280A_CTRL_LB_LOCK_DEV_ADDR			BIT(2)
-#define AD7280A_CTRL_LB_INC_DEV_ADDR			BIT(1)
-#define AD7280A_CTRL_LB_DAISY_CHAIN_RB_EN		BIT(0)
-
-#define AD7280A_ALERT_GEN_STATIC_HIGH			BIT(6)
-#define AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN		(BIT(7) | BIT(6))
 
+#define AD7280A_CELL_VOLTAGE_1_REG		0x0  /* D11 to D0, Read only */
+#define AD7280A_CELL_VOLTAGE_2_REG		0x1  /* D11 to D0, Read only */
+#define AD7280A_CELL_VOLTAGE_3_REG		0x2  /* D11 to D0, Read only */
+#define AD7280A_CELL_VOLTAGE_4_REG		0x3  /* D11 to D0, Read only */
+#define AD7280A_CELL_VOLTAGE_5_REG		0x4  /* D11 to D0, Read only */
+#define AD7280A_CELL_VOLTAGE_6_REG		0x5  /* D11 to D0, Read only */
+#define AD7280A_AUX_ADC_1_REG			0x6  /* D11 to D0, Read only */
+#define AD7280A_AUX_ADC_2_REG			0x7  /* D11 to D0, Read only */

drivers/staging/iio/adc/ad7280a.c +118 -55

+#define     AD7280A_CTRL_HB_CONV_RREAD_6CELL			2
+#define     AD7280A_CTRL_HB_CONV_RREAD_NO		        3
+#define   AD7280A_CTRL_HB_CONV_START_MSK		BIT(3)
+#define     AD7280A_CTRL_HB_CONV_START_CNVST			0
+#define     AD7280A_CTRL_HB_CONV_START_CS			1
+#define   AD7280A_CTRL_HB_CONV_AVG_MSK			GENMASK(2, 1)
+#define     AD7280A_CTRL_HB_CONV_AVG_DIS			0
+#define     AD7280A_CTRL_HB_CONV_AVG_2				1
+#define     AD7280A_CTRL_HB_CONV_AVG_4			        2
+#define     AD7280A_CTRL_HB_CONV_AVG_8			        3
+#define   AD7280A_CTRL_HB_PWRDN_SW			BIT(0)
+
+#define AD7280A_CTRL_LB_REG			0xE  /* D7 to D0, Read/write */
+#define   AD7280A_CTRL_LB_SWRST_MSK			BIT(7)
+#define   AD7280A_CTRL_LB_ACQ_TIME_MSK			GENMASK(6, 5)
+#define     AD7280A_CTRL_LB_ACQ_TIME_400ns			0
+#define     AD7280A_CTRL_LB_ACQ_TIME_800ns			1
+#define     AD7280A_CTRL_LB_ACQ_TIME_1200ns			2
+#define     AD7280A_CTRL_LB_ACQ_TIME_1600ns			3
+#define   AD7280A_CTRL_LB_MUST_SET			BIT(4)
+#define   AD7280A_CTRL_LB_THERMISTOR_MSK		BIT(3)
+#define   AD7280A_CTRL_LB_LOCK_DEV_ADDR_MSK		BIT(2)
+#define   AD7280A_CTRL_LB_INC_DEV_ADDR_MSK		BIT(1)
+#define   AD7280A_CTRL_LB_DAISY_CHAIN_RB_MSK		BIT(0)
+
+#define AD7280A_CELL_OVERVOLTAGE_REG		0xF  /* D7 to D0, Read/write */
+#define AD7280A_CELL_UNDERVOLTAGE_REG		0x10 /* D7 to D0, Read/write */
+#define AD7280A_AUX_ADC_OVERVOLTAGE_REG		0x11 /* D7 to D0, Read/write */
+#define AD7280A_AUX_ADC_UNDERVOLTAGE_REG	0x12 /* D7 to D0, Read/write */
+
+#define AD7280A_ALERT_REG			0x13 /* D7 to D0, Read/write */
+#define   AD7280A_ALERT_GEN_STATIC_HIGH			BIT(6)
+#define   AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN		(BIT(7) | BIT(6))
+
+#define AD7280A_CELL_BALANCE_REG		0x14 /* D7 to D0, Read/write */
+#define AD7280A_CB1_TIMER_REG			0x15 /* D7 to D0, Read/write */
+#define  AD7280A_CB_TIMER_VAL_MSK			GENMASK(7, 3)
+#define AD7280A_CB2_TIMER_REG			0x16 /* D7 to D0, Read/write */
+#define AD7280A_CB3_TIMER_REG			0x17 /* D7 to D0, Read/write */
+#define AD7280A_CB4_TIMER_REG			0x18 /* D7 to D0, Read/write */
+#define AD7280A_CB5_TIMER_REG			0x19 /* D7 to D0, Read/write */
+#define AD7280A_CB6_TIMER_REG			0x1A /* D7 to D0, Read/write */
+#define AD7280A_PD_TIMER_REG			0x1B /* D7 to D0, Read/write */
+#define AD7280A_READ_REG			0x1C /* D7 to D0, Read/write */
+#define   AD7280A_READ_ADDR_MSK				GENMASK(7, 2)
+#define AD7280A_CNVST_CTRL_REG			0x1D /* D7 to D0, Read/write */
+
+/* Magic value used to indicate this special case */
 #define AD7280A_ALL_CELLS				(0xAD << 16)
 
 #define AD7280A_MAX_SPI_CLK_HZ		700000 /* < 1MHz */
 #define AD7280A_MAX_CHAIN		8
 #define AD7280A_CELLS_PER_DEV		6
 #define AD7280A_BITS			12
-#define AD7280A_NUM_CH			(AD7280A_AUX_ADC_6 - \
-					AD7280A_CELL_VOLTAGE_1 + 1)
+#define AD7280A_NUM_CH			(AD7280A_AUX_ADC_6_REG - \
+					AD7280A_CELL_VOLTAGE_1_REG + 1)
 
 #define AD7280A_CALC_VOLTAGE_CHAN_NUM(d, c) (((d) * AD7280A_CELLS_PER_DEV) + \
 					     (c))

@@ -222,23 +231,28 @@ static int ad7280_read(struct ad7280_state *st, unsigned int devaddr,
 	unsigned int tmp;
 
 	/* turns off the read operation on all parts */
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
-			   AD7280A_CTRL_HB_CONV_INPUT_ALL |
-			   AD7280A_CTRL_HB_CONV_RES_READ_NO |
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_HB_REG, 1,
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_INPUT_MSK,
+				      AD7280A_CTRL_HB_CONV_INPUT_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_RREAD_MSK,
+				      AD7280A_CTRL_HB_CONV_RREAD_NO) |
 			   st->ctrl_hb);
 	if (ret)
 		return ret;
 
 	/* turns on the read operation on the addressed part */
-	ret = ad7280_write(st, devaddr, AD7280A_CONTROL_HB, 0,
-			   AD7280A_CTRL_HB_CONV_INPUT_ALL |
-			   AD7280A_CTRL_HB_CONV_RES_READ_ALL |
+	ret = ad7280_write(st, devaddr, AD7280A_CTRL_HB_REG, 0,
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_INPUT_MSK,
+				      AD7280A_CTRL_HB_CONV_INPUT_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_RREAD_MSK,
+				      AD7280A_CTRL_HB_CONV_RREAD_ALL) |
 			   st->ctrl_hb);
 	if (ret)
 		return ret;
 
 	/* Set register address on the part to be read from */
-	ret = ad7280_write(st, devaddr, AD7280A_READ, 0, addr << 2);
+	ret = ad7280_write(st, devaddr, AD7280A_READ_REG, 0,
+			   FIELD_PREP(AD7280A_READ_ADDR_MSK, addr));
 	if (ret)
 		return ret;
 

@@ -261,21 +275,27 @@ static int ad7280_read_channel(struct ad7280_state *st, unsigned int devaddr,
 	int ret;
 	unsigned int tmp;
 
-	ret = ad7280_write(st, devaddr, AD7280A_READ, 0, addr << 2);
+	ret = ad7280_write(st, devaddr, AD7280A_READ_REG, 0,
+			   FIELD_PREP(AD7280A_READ_ADDR_MSK, addr));
 	if (ret)
 		return ret;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
-			   AD7280A_CTRL_HB_CONV_INPUT_ALL |
-			   AD7280A_CTRL_HB_CONV_RES_READ_NO |
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_HB_REG, 1,
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_INPUT_MSK,
+				      AD7280A_CTRL_HB_CONV_INPUT_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_RREAD_MSK,
+				      AD7280A_CTRL_HB_CONV_RREAD_NO) |
 			   st->ctrl_hb);
 	if (ret)
 		return ret;
 
-	ret = ad7280_write(st, devaddr, AD7280A_CONTROL_HB, 0,
-			   AD7280A_CTRL_HB_CONV_INPUT_ALL |
-			   AD7280A_CTRL_HB_CONV_RES_READ_ALL |
-			   AD7280A_CTRL_HB_CONV_START_CS |
+	ret = ad7280_write(st, devaddr, AD7280A_CTRL_HB_REG, 0,
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_INPUT_MSK,
+				      AD7280A_CTRL_HB_CONV_INPUT_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_RREAD_MSK,
+				      AD7280A_CTRL_HB_CONV_RREAD_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_START_MSK,
+				      AD7280A_CTRL_HB_CONV_START_CS) |
 			   st->ctrl_hb);
 	if (ret)
 		return ret;

@@ -301,15 +321,18 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
 	int i, ret;
 	unsigned int tmp, sum = 0;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_READ, 1,
-			   AD7280A_CELL_VOLTAGE_1 << 2);
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_READ_REG, 1,
+			   AD7280A_CELL_VOLTAGE_1_REG << 2);
 	if (ret)
 		return ret;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
-			   AD7280A_CTRL_HB_CONV_INPUT_ALL |
-			   AD7280A_CTRL_HB_CONV_RES_READ_ALL |
-			   AD7280A_CTRL_HB_CONV_START_CS |
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_HB_REG, 1,
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_INPUT_MSK,
+				      AD7280A_CTRL_HB_CONV_INPUT_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_RREAD_MSK,
+				      AD7280A_CTRL_HB_CONV_RREAD_ALL) |
+			   FIELD_PREP(AD7280A_CTRL_HB_CONV_START_MSK,
+				      AD7280A_CTRL_HB_CONV_START_CS) |
 			   st->ctrl_hb);
 	if (ret)
 		return ret;

@@ -327,7 +350,7 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
 		if (array)
 			array[i] = tmp;
 		/* only sum cell voltages */
-		if (((tmp >> 23) & 0xF) <= AD7280A_CELL_VOLTAGE_6)
+		if (((tmp >> 23) & 0xF) <= AD7280A_CELL_VOLTAGE_6_REG)
 			sum += ((tmp >> 11) & 0xFFF);
 	}
 

@@ -338,7 +361,7 @@ static void ad7280_sw_power_down(void *data)
 {
 	struct ad7280_state *st = data;
 
-	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
+	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_HB_REG, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
 }
 

@@ -347,25 +370,26 @@ static int ad7280_chain_setup(struct ad7280_state *st)
 	unsigned int val, n;
 	int ret;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_LB, 1,
-			   AD7280A_CTRL_LB_DAISY_CHAIN_RB_EN |
-			   AD7280A_CTRL_LB_LOCK_DEV_ADDR |
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_LB_REG, 1,
+			   FIELD_PREP(AD7280A_CTRL_LB_DAISY_CHAIN_RB_MSK, 1) |
+			   FIELD_PREP(AD7280A_CTRL_LB_LOCK_DEV_ADDR_MSK, 1) |
 			   AD7280A_CTRL_LB_MUST_SET |
-			   AD7280A_CTRL_LB_SWRST |
+			   FIELD_PREP(AD7280A_CTRL_LB_SWRST_MSK, 1) |
 			   st->ctrl_lb);
 	if (ret)
 		return ret;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_LB, 1,
-			   AD7280A_CTRL_LB_DAISY_CHAIN_RB_EN |
-			   AD7280A_CTRL_LB_LOCK_DEV_ADDR |
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_LB_REG, 1,
+			   FIELD_PREP(AD7280A_CTRL_LB_DAISY_CHAIN_RB_MSK, 1) |
+			   FIELD_PREP(AD7280A_CTRL_LB_LOCK_DEV_ADDR_MSK, 1) |
 			   AD7280A_CTRL_LB_MUST_SET |
+			   FIELD_PREP(AD7280A_CTRL_LB_SWRST_MSK, 0) |
 			   st->ctrl_lb);
 	if (ret)
 		goto error_power_down;
 
-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_READ, 1,
-			   AD7280A_CONTROL_LB << 2);
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_READ_REG, 1,
+			   FIELD_PREP(AD7280A_READ_ADDR_MSK, AD7280A_CTRL_LB_REG));
 	if (ret)
 		goto error_power_down;
 

@@ -390,7 +414,7 @@ static int ad7280_chain_setup(struct ad7280_state *st)
 	ret = -EFAULT;
 
 error_power_down:
-	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
+	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CTRL_HB_REG, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);
 
 	return ret;

@@ -434,7 +458,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev,
 	else
 		st->cb_mask[devaddr] &= ~(1 << (ch + 2));
 
-	ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE,
+	ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE_REG,
 			   0, st->cb_mask[devaddr]);
 	mutex_unlock(&st->lock);
 

@@ -459,7 +483,7 @@ static ssize_t ad7280_show_balance_timer(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	msecs = (ret >> 3) * 71500;
+	msecs = FIELD_GET(AD7280A_CB_TIMER_VAL_MSK, ret) * 71500;
 
 	return sprintf(buf, "%u\n", msecs);
 }

@@ -486,8 +510,8 @@ static ssize_t ad7280_store_balance_timer(struct device *dev,
 
 	mutex_lock(&st->lock);
 	ret = ad7280_write(st, this_attr->address >> 8,
-			   this_attr->address & 0xFF,
-			   0, (val & 0x1F) << 3);
+			   this_attr->address & 0xFF, 0,
+			   FIELD_PREP(AD7280A_CB_TIMER_VAL_MSK, val));
 	mutex_unlock(&st->lock);
 
 	return ret ? ret : len;

@@ -559,10 +583,10 @@ static void ad7280_init_dev_channels(struct ad7280_state *st, int dev, int *cnt)
 	int addr, ch, i;
 	struct iio_chan_spec *chan;
 
-	for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6; ch++) {
+	for (ch = AD7280A_CELL_VOLTAGE_1_REG; ch <= AD7280A_AUX_ADC_6_REG; ch++) {
 		chan = &st->channels[*cnt];
 
-		if (ch < AD7280A_AUX_ADC_1) {
+		if (ch < AD7280A_AUX_ADC_1_REG) {
 			i = AD7280A_CALC_VOLTAGE_CHAN_NUM(dev, ch);
 			ad7280_voltage_channel_init(chan, i);
 		} else {

@@ -634,7 +658,7 @@ static int ad7280_init_dev_attrs(struct ad7280_state *st, int dev, int *cnt)
 	struct iio_dev_attr *iio_attr;
 	struct device *sdev = &st->spi->dev;
 
-	for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6; ch++) {
+	for (ch = AD7280A_CELL_VOLTAGE_1_REG; ch <= AD7280A_CELL_VOLTAGE_6_REG; ch++) {
 		iio_attr = &st->iio_attr[*cnt];
 		addr = ad7280a_devaddr(dev) << 8 | ch;
 		i = dev * AD7280A_CELLS_PER_DEV + ch;

@@ -647,7 +671,7 @@ static int ad7280_init_dev_attrs(struct ad7280_state *st, int dev, int *cnt)
 
 		(*cnt)++;
 		iio_attr = &st->iio_attr[*cnt];
-		addr = ad7280a_devaddr(dev) << 8 | (AD7280A_CB1_TIMER + ch);
+		addr = ad7280a_devaddr(dev) << 8 | (AD7280A_CB1_TIMER_REG + ch);
 
 		ret = ad7280_balance_timer_attr_init(iio_attr, sdev, addr, i);
 		if (ret < 0)

@@ -691,16 +715,16 @@ static ssize_t ad7280_read_channel_config(struct device *dev,
 	unsigned int val;
 
 	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE:
+	case AD7280A_CELL_OVERVOLTAGE_REG:
 		val = 1000 + (st->cell_threshhigh * 1568) / 100;
 		break;
-	case AD7280A_CELL_UNDERVOLTAGE:
+	case AD7280A_CELL_UNDERVOLTAGE_REG:
 		val = 1000 + (st->cell_threshlow * 1568) / 100;
 		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE:
+	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
 		val = (st->aux_threshhigh * 196) / 10;
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE:
+	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
 		val = (st->aux_threshlow * 196) / 10;
 		break;
 	default:

@@ -727,12 +751,12 @@ static ssize_t ad7280_write_channel_config(struct device *dev,
 		return ret;
 
 	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE:
-	case AD7280A_CELL_UNDERVOLTAGE:
+	case AD7280A_CELL_OVERVOLTAGE_REG:
+	case AD7280A_CELL_UNDERVOLTAGE_REG:
 		val = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
 		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE:
-	case AD7280A_AUX_ADC_UNDERVOLTAGE:
+	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
+	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
 		val = (val * 10) / 196; /* LSB 19.6mV */
 		break;
 	default:

@@ -743,16 +767,16 @@ static ssize_t ad7280_write_channel_config(struct device *dev,
 
 	mutex_lock(&st->lock);
 	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE:
+	case AD7280A_CELL_OVERVOLTAGE_REG:
 		st->cell_threshhigh = val;
 		break;
-	case AD7280A_CELL_UNDERVOLTAGE:
+	case AD7280A_CELL_UNDERVOLTAGE_REG:
 		st->cell_threshlow = val;
 		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE:
+	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
 		st->aux_threshhigh = val;
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE:
+	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
 		st->aux_threshlow = val;
 		break;
 	}

drivers/staging/iio/adc/ad7280a.c +8 -11

 				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
 							 IIO_EV_DIR_RISING,
 							 IIO_EV_TYPE_THRESH,
 							 0, 0, 0);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
-			} else if (((channels[i] >> 11) & 0xFFF) <=
-				   st->cell_threshlow) {
+			} else if (val <= st->cell_threshlow) {
 				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
 							 IIO_EV_DIR_FALLING,
 							 IIO_EV_TYPE_THRESH,

@@ -800,15 +826,13 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 					       iio_get_time_ns(indio_dev));
 			}
 		} else {
-			if (((channels[i] >> 11) & 0xFFF) >=
-			    st->aux_threshhigh) {
+			if (val >= st->aux_threshhigh) {
 				u64 tmp = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_RISING);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
-			} else if (((channels[i] >> 11) & 0xFFF) <=
-				st->aux_threshlow) {
+			} else if (val <= st->aux_threshlow) {
 				u64 tmp = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_FALLING);

@@ -833,26 +857,26 @@ static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
 			     0644,
 			     ad7280_read_channel_config,
 			     ad7280_write_channel_config,
-			     AD7280A_CELL_UNDERVOLTAGE);
+			     AD7280A_CELL_UNDERVOLTAGE_REG);
 
 static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
 			     in_voltage-voltage_thresh_high_value,
 			     0644,
 			     ad7280_read_channel_config,
 			     ad7280_write_channel_config,
-			     AD7280A_CELL_OVERVOLTAGE);
+			     AD7280A_CELL_OVERVOLTAGE_REG);
 
 static IIO_DEVICE_ATTR(in_temp_thresh_low_value,
 		       0644,
 		       ad7280_read_channel_config,
 		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_UNDERVOLTAGE);
+		       AD7280A_AUX_ADC_UNDERVOLTAGE_REG);
 
 static IIO_DEVICE_ATTR(in_temp_thresh_high_value,
 		       0644,
 		       ad7280_read_channel_config,
 		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_OVERVOLTAGE);
+		       AD7280A_AUX_ADC_OVERVOLTAGE_REG);
 
 static struct attribute *ad7280_event_attributes[] = {
 	&iio_dev_attr_in_thresh_low_value.dev_attr.attr,

@@ -892,7 +916,7 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if ((chan->address & 0xFF) <= AD7280A_CELL_VOLTAGE_6)
+		if ((chan->address & 0xFF) <= AD7280A_CELL_VOLTAGE_6_REG)
 			*val = 4000;
 		else
 			*val = 5000;

drivers/staging/iio/adc/ad7280a.c +3 -2

+	st->ctrl_hb = FIELD_PREP(AD7280A_CTRL_HB_CONV_AVG_MSK, pdata->conversion_averaging);
 
 	ret = ad7280_chain_setup(st);
 	if (ret < 0)

@@ -998,13 +1021,13 @@ static int ad7280_probe(struct spi_device *spi)
 
 	if (spi->irq > 0) {
 		ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
-				   AD7280A_ALERT, 1,
+				   AD7280A_ALERT_REG, 1,
 				   AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN);
 		if (ret)
 			return ret;
 
 		ret = ad7280_write(st, ad7280a_devaddr(st->slave_num),
-				   AD7280A_ALERT, 0,
+				   AD7280A_ALERT_REG, 0,
 				   AD7280A_ALERT_GEN_STATIC_HIGH |
 				   (pdata->chain_last_alert_ignore & 0xF));
 		if (ret)

Signature · 1 line

2.34.1

Looks overall good.
A few comments inline.

Quoted 9 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

1. Postfix register addresses with _REG to distinguish them from fields within the registers
2. Switch to using FIELD_PREP and masks to aid readability.
3. Shorten a few defines to make the lines remain a sensible length.
4. Fix an issue whether where an CTRL_LB field is set in CTRL_HB.

Signed-off-by Jonathan Cameron <[email protected]>

Reviewed-by Marcelo Schmitt <[email protected]>

Quoted 19 lines

---
 drivers/staging/iio/adc/ad7280a.c | 299 ++++++++++++++++--------------
 1 file changed, 161 insertions(+), 138 deletions(-)

drivers/staging/iio/adc/ad7280a.c +1 -0

index 20183b2ea127..d169c8a7b5f1 100644
@@ -11,6 +11,7 @@
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>

@@ -23,78 +24,86 @@
 #include "ad7280a.h"
 

Quoted 7 lines

drivers/staging/iio/adc/ad7280a.c +0 -7

-/* Bits and Masks */
-#define AD7280A_CTRL_HB_CONV_INPUT_ALL			0
-#define AD7280A_CTRL_HB_CONV_INPUT_6CELL_AUX1_3_4	BIT(6)
-#define AD7280A_CTRL_HB_CONV_INPUT_6CELL		BIT(7)
-#define AD7280A_CTRL_HB_CONV_INPUT_SELF_TEST		(BIT(7) | BIT(6))
-#define AD7280A_CTRL_HB_CONV_RES_READ_ALL		0
-#define AD7280A_CTRL_HB_CONV_RES_READ_6CELL_AUX1_3_4	BIT(4)

drivers/staging/iio/adc/ad7280a.c line 45

Quoted 3 lines

drivers/staging/iio/adc/ad7280a.c +3 -0

+#define   AD7280A_CTRL_HB_CONV_INPUT_MSK		GENMASK(7, 6)
+#define     AD7280A_CTRL_HB_CONV_INPUT_ALL			0
+#define     AD7280A_CTRL_HB_CONV_INPUT_6CELL_AUX1_3_4		1  

typo, guess you meant
#define AD7280A_CTRL_HB_CONV_INPUT_6CELL_AUX1_3_5 1

drivers/staging/iio/adc/ad7280a.c line 50

Quoted 6 lines

drivers/staging/iio/adc/ad7280a.c +5 -0

+#define     AD7280A_CTRL_HB_CONV_INPUT_6CELL			2
+#define     AD7280A_CTRL_HB_CONV_INPUT_SELF_TEST		3
+#define   AD7280A_CTRL_HB_CONV_RREAD_MSK		GENMASK(5, 4)
+#define     AD7280A_CTRL_HB_CONV_RREAD_ALL			0
+#define     AD7280A_CTRL_HB_CONV_RREAD_6CELL_AUX1_3_4		1  

Same here
#define AD7280A_CTRL_HB_CONV_RREAD_6CELL_AUX1_3_5 1

drivers/staging/iio/adc/ad7280a.c static irqreturn_t ad7280_event_handler(int irq, void *private) · line 810

Quoted 10 lines

drivers/staging/iio/adc/ad7280a.c +3 -3

@@ -781,17 +805,19 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 		goto out;
 
 	for (i = 0; i < st->scan_cnt; i++) {
-		if (((channels[i] >> 23) & 0xF) <= AD7280A_CELL_VOLTAGE_6) {
-			if (((channels[i] >> 11) & 0xFFF) >=
-			    st->cell_threshhigh) {
+		unsigned int val;
+
+		val = FIELD_GET(AD7280A_TRANS_READ_CONV_DATA_MSK, channels[i]);  

AD7280A_TRANS_READ_CONV_DATA_MSK is defined in patch 5. Guess its simpler to delay this change to patch 5.

drivers/staging/iio/adc/ad7280a.c static irqreturn_t ad7280_event_handler(int irq, void *private) · line 811

Quoted 2 lines

drivers/staging/iio/adc/ad7280a.c +1 -0

+		if (FIELD_GET(AD7280A_TRANS_READ_CONV_CHANADDR_MSK, channels[i])  

Same for AD7280A_TRANS_READ_CONV_CHANADDR_MSK.

drivers/staging/iio/adc/ad7280a.c static irqreturn_t ad7280_event_handler(int irq, void *private) · line 813

Quoted 2 lines

drivers/staging/iio/adc/ad7280a.c +2 -0

+			<= AD7280A_CELL_VOLTAGE_6_REG) {
+			if (val >= st->cell_threshhigh) {  

These uses of val would also be delayed to patch 5

Quoted 30 lines

drivers/staging/iio/adc/ad7280a.c +3 -6

 				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
 							 IIO_EV_DIR_RISING,
 							 IIO_EV_TYPE_THRESH,
 							 0, 0, 0);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
-			} else if (((channels[i] >> 11) & 0xFFF) <=
-				   st->cell_threshlow) {
+			} else if (val <= st->cell_threshlow) {
 				u64 tmp = IIO_EVENT_CODE(IIO_VOLTAGE, 1, 0,
 							 IIO_EV_DIR_FALLING,
 							 IIO_EV_TYPE_THRESH,

@@ -800,15 +826,13 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 					       iio_get_time_ns(indio_dev));
 			}
 		} else {
-			if (((channels[i] >> 11) & 0xFFF) >=
-			    st->aux_threshhigh) {
+			if (val >= st->aux_threshhigh) {
 				u64 tmp = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_RISING);
 				iio_push_event(indio_dev, tmp,
 					       iio_get_time_ns(indio_dev));
-			} else if (((channels[i] >> 11) & 0xFFF) <=
-				st->aux_threshlow) {
+			} else if (val <= st->aux_threshlow) {
 				u64 tmp = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
 							IIO_EV_TYPE_THRESH,
 							IIO_EV_DIR_FALLING);

Quoted 4 lines

drivers/staging/iio/adc/ad7280a.c +0 -0

@@ -942,10 +966,9 @@ static int ad7280_probe(struct spi_device *spi)
 	st->spi->mode = SPI_MODE_1;
 	spi_setup(st->spi);
 

drivers/staging/iio/adc/ad7280a.c static int ad7280_probe(struct spi_device *spi) · line 970

Quoted 6 lines

drivers/staging/iio/adc/ad7280a.c +2 -4

-	st->ctrl_lb = AD7280A_CTRL_LB_ACQ_TIME(pdata->acquisition_time & 0x3);
-	st->ctrl_hb = AD7280A_CTRL_HB_CONV_AVG(pdata->conversion_averaging
-			& 0x3) | (pdata->thermistor_term_en ?
-			AD7280A_CTRL_LB_THERMISTOR_EN : 0);
+	st->ctrl_lb = FIELD_PREP(AD7280A_CTRL_LB_ACQ_TIME_MSK, pdata->acquisition_time) |
+		FIELD_PREP(AD7280A_CTRL_LB_THERMISTOR_MSK, pdata->thermistor_term_en);  

Writes to the CTRL_LB_REG already set it, but could also set the must_set bit here if want FIELD_PREP(AD7280A_CTRL_LB_MUST_SET, 1)

From: Jonathan Cameron <[email protected]>

This avoids possible confusion with read back of the channel conversions. These two types of reads are of difference sizes with resulting differences in the data layout of the response from the hardware.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/staging/iio/adc/ad7280a.c +4 -4

index d169c8a7b5f1..4d866e5fe43c 100644
@@ -224,8 +224,8 @@ static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
 	return spi_write(st->spi, &st->buf[0], 4);
 }
 
-static int ad7280_read(struct ad7280_state *st, unsigned int devaddr,
-		       unsigned int addr)
+static int ad7280_read_reg(struct ad7280_state *st, unsigned int devaddr,
+			   unsigned int addr)
 {
 	int ret;
 	unsigned int tmp;

@@ -476,8 +476,8 @@ static ssize_t ad7280_show_balance_timer(struct device *dev,
 	unsigned int msecs;
 
 	mutex_lock(&st->lock);
-	ret = ad7280_read(st, this_attr->address >> 8,
-			  this_attr->address & 0xFF);
+	ret = ad7280_read_reg(st, this_attr->address >> 8,
+			      this_attr->address & 0xFF);
 	mutex_unlock(&st->lock);
 
 	if (ret < 0)

Signature · 1 line

2.34.1

From: Jonathan Cameron <[email protected]>

As the __cacheline_aligned will ensure that the first of these two buffers is appropriate aligned, there is no need to keep them as a single array which is confusing given the first element is always tx and the second rx. Hence let us just have two parts and name them separately.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/staging/iio/adc/ad7280a.c +9 -8

index 4d866e5fe43c..1f7ea5fb1e20 100644
@@ -153,7 +153,8 @@ struct ad7280_state {
 	unsigned char			cb_mask[AD7280A_MAX_CHAIN];
 	struct mutex			lock; /* protect sensor state */
 
-	__be32				buf[2] ____cacheline_aligned;
+	__be32				tx ____cacheline_aligned;
+	__be32				rx;
 };
 
 static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned int val)

@@ -196,18 +197,18 @@ static int __ad7280_read32(struct ad7280_state *st, unsigned int *val)
 {
 	int ret;
 	struct spi_transfer t = {
-		.tx_buf	= &st->buf[0],
-		.rx_buf = &st->buf[1],
-		.len = 4,
+		.tx_buf	= &st->tx,
+		.rx_buf = &st->rx,
+		.len = sizeof(st->tx),
 	};
 
-	st->buf[0] = cpu_to_be32(AD7280A_READ_TXVAL);
+	st->tx = cpu_to_be32(AD7280A_READ_TXVAL);
 
 	ret = spi_sync_transfer(st->spi, &t, 1);
 	if (ret)
 		return ret;
 
-	*val = be32_to_cpu(st->buf[1]);
+	*val = be32_to_cpu(st->rx);
 
 	return 0;
 }

@@ -219,9 +220,9 @@ static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
 			(val & 0xFF) << 13 | all << 12;
 
 	reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
-	st->buf[0] = cpu_to_be32(reg);
+	st->tx = cpu_to_be32(reg);
 
-	return spi_write(st->spi, &st->buf[0], 4);
+	return spi_write(st->spi, &st->tx, sizeof(st->tx));
 }
 
 static int ad7280_read_reg(struct ad7280_state *st, unsigned int devaddr,

Signature · 1 line

2.34.1

From: Jonathan Cameron <[email protected]>

The write and two types of read transfer are sufficiently complex that they benefit from the clarity of using FIELD_PREP() and FIELD_GET()

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 46 ++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/staging/iio/adc/ad7280a.c +17 -0

index 1f7ea5fb1e20..158a792c0bf8 100644
@@ -95,6 +95,23 @@
 #define   AD7280A_READ_ADDR_MSK				GENMASK(7, 2)
 #define AD7280A_CNVST_CTRL_REG			0x1D /* D7 to D0, Read/write */
 
+/* Transfer fields */
+#define AD7280A_TRANS_WRITE_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_WRITE_ADDR_MSK		GENMASK(26, 21)
+#define AD7280A_TRANS_WRITE_VAL_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_WRITE_ALL_MSK		BIT(12)
+#define AD7280A_TRANS_WRITE_CRC_MSK		GENMASK(10, 3)
+#define AD7280A_TRANS_WRITE_RES_PATTERN		0x2
+
+/* Layouts differ for channel vs other registers */
+#define AD7280A_TRANS_READ_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_READ_CONV_CHANADDR_MSK	GENMASK(26, 23)
+#define AD7280A_TRANS_READ_CONV_DATA_MSK	GENMASK(22, 11)
+#define AD7280A_TRANS_READ_REG_REGADDR_MSK	GENMASK(26, 21)
+#define AD7280A_TRANS_READ_REG_DATA_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_READ_WRITE_ACK_MSK	BIT(10)
+#define AD7280A_TRANS_READ_CRC_MSK		GENMASK(9, 2)
+
 /* Magic value used to indicate this special case */
 #define AD7280A_ALL_CELLS				(0xAD << 16)
 

drivers/staging/iio/adc/ad7280a.c +15 -8

+
+	reg |= FIELD_PREP(AD7280A_TRANS_WRITE_CRC_MSK,
+			ad7280_calc_crc8(st->crc_tab, reg >> 11));
+	/* Reserved b010 pattern not included crc calc */
+	reg |= AD7280A_TRANS_WRITE_RES_PATTERN;
 
-	reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
 	st->tx = cpu_to_be32(reg);
 
 	return spi_write(st->spi, &st->tx, sizeof(st->tx));

@@ -264,10 +287,11 @@ static int ad7280_read_reg(struct ad7280_state *st, unsigned int devaddr,
 	if (ad7280_check_crc(st, tmp))
 		return -EIO;
 
-	if (((tmp >> 27) != devaddr) || (((tmp >> 21) & 0x3F) != addr))
+	if ((FIELD_GET(AD7280A_TRANS_READ_DEVADDR_MSK, tmp) != devaddr) ||
+	    (FIELD_GET(AD7280A_TRANS_READ_REG_REGADDR_MSK, tmp) != addr))
 		return -EFAULT;
 
-	return (tmp >> 13) & 0xFF;
+	return FIELD_GET(AD7280A_TRANS_READ_REG_DATA_MSK, tmp);
 }
 
 static int ad7280_read_channel(struct ad7280_state *st, unsigned int devaddr,

@@ -310,10 +334,11 @@ static int ad7280_read_channel(struct ad7280_state *st, unsigned int devaddr,
 	if (ad7280_check_crc(st, tmp))
 		return -EIO;
 
-	if (((tmp >> 27) != devaddr) || (((tmp >> 23) & 0xF) != addr))
+	if ((FIELD_GET(AD7280A_TRANS_READ_DEVADDR_MSK, tmp) != devaddr) ||
+	    (FIELD_GET(AD7280A_TRANS_READ_CONV_CHANADDR_MSK, tmp) != addr))
 		return -EFAULT;
 
-	return (tmp >> 11) & 0xFFF;
+	return FIELD_GET(AD7280A_TRANS_READ_CONV_DATA_MSK, tmp);
 }
 
 static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,

@@ -351,8 +376,9 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
 		if (array)
 			array[i] = tmp;
 		/* only sum cell voltages */
-		if (((tmp >> 23) & 0xF) <= AD7280A_CELL_VOLTAGE_6_REG)
-			sum += ((tmp >> 11) & 0xFFF);
+		if (FIELD_GET(AD7280A_TRANS_READ_CONV_CHANADDR_MSK, tmp) <=
+		    AD7280A_CELL_VOLTAGE_6_REG)
+			sum += FIELD_GET(AD7280A_TRANS_READ_CONV_DATA_MSK, tmp);
 	}
 
 	return sum;

@@ -407,7 +433,7 @@ static int ad7280_chain_setup(struct ad7280_state *st)
 			goto error_power_down;
 		}
 
-		if (n != ad7280a_devaddr(val >> 27)) {
+		if (n != ad7280a_devaddr(FIELD_GET(AD7280A_TRANS_READ_DEVADDR_MSK, val))) {
 			ret = -EIO;
 			goto error_power_down;
 		}

Signature · 1 line

2.34.1

Minor bit inline.

Quoted 6 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

The write and two types of read transfer are sufficiently complex that they benefit from the clarity of using FIELD_PREP() and FIELD_GET()

Signed-off-by Jonathan Cameron <[email protected]>

Reviewed-by Marcelo Schmitt <[email protected]>

Quoted 33 lines

---
 drivers/staging/iio/adc/ad7280a.c | 46 ++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/staging/iio/adc/ad7280a.c +17 -0

index 1f7ea5fb1e20..158a792c0bf8 100644
@@ -95,6 +95,23 @@
 #define   AD7280A_READ_ADDR_MSK				GENMASK(7, 2)
 #define AD7280A_CNVST_CTRL_REG			0x1D /* D7 to D0, Read/write */
 
+/* Transfer fields */
+#define AD7280A_TRANS_WRITE_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_WRITE_ADDR_MSK		GENMASK(26, 21)
+#define AD7280A_TRANS_WRITE_VAL_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_WRITE_ALL_MSK		BIT(12)
+#define AD7280A_TRANS_WRITE_CRC_MSK		GENMASK(10, 3)
+#define AD7280A_TRANS_WRITE_RES_PATTERN		0x2
+
+/* Layouts differ for channel vs other registers */
+#define AD7280A_TRANS_READ_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_READ_CONV_CHANADDR_MSK	GENMASK(26, 23)
+#define AD7280A_TRANS_READ_CONV_DATA_MSK	GENMASK(22, 11)
+#define AD7280A_TRANS_READ_REG_REGADDR_MSK	GENMASK(26, 21)
+#define AD7280A_TRANS_READ_REG_DATA_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_READ_WRITE_ACK_MSK	BIT(10)
+#define AD7280A_TRANS_READ_CRC_MSK		GENMASK(9, 2)
+
 /* Magic value used to indicate this special case */
 #define AD7280A_ALL_CELLS				(0xAD << 16)
 

drivers/staging/iio/adc/ad7280a.c static int __ad7280_read32(struct ad7280_state *st, unsigned int *val) · line 239

Quoted 10 lines

drivers/staging/iio/adc/ad7280a.c +4 -2

@@ -216,10 +233,16 @@ static int __ad7280_read32(struct ad7280_state *st, unsigned int *val)
 static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
 			unsigned int addr, bool all, unsigned int val)
 {
-	unsigned int reg = devaddr << 27 | addr << 21 |
-			(val & 0xFF) << 13 | all << 12;
+	unsigned int reg = FIELD_PREP(AD7280A_TRANS_WRITE_DEVADDR_MSK, devaddr) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_ADDR_MSK, addr) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_VAL_MSK, val) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_ALL_MSK, all);  

Does reg get initialized to 0? If not, we should take care of the reserved bit D11

Quoted 10 lines

drivers/staging/iio/adc/ad7280a.c +5 -1

+
+	reg |= FIELD_PREP(AD7280A_TRANS_WRITE_CRC_MSK,
+			ad7280_calc_crc8(st->crc_tab, reg >> 11));
+	/* Reserved b010 pattern not included crc calc */
+	reg |= AD7280A_TRANS_WRITE_RES_PATTERN;
 
-	reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
 	st->tx = cpu_to_be32(reg);
 
 	return spi_write(st->spi, &st->tx, sizeof(st->tx));

Quoted 10 lines · On Tue, 14 Dec 2021 14:12:24 -0300 Marcelo Schmitt…

Minor bit inline.

Quoted 6 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

The write and two types of read transfer are sufficiently complex that they benefit from the clarity of using FIELD_PREP() and FIELD_GET()

Signed-off-by Jonathan Cameron <[email protected]>

Reviewed-by Marcelo Schmitt <[email protected]>

Those hunks you highlighted in patch 2 now in this patch and I've added a note to the description to say a similar arguement applies to the event_handler()

Reply to your comment inline. I didn't understand this one.

Quoted 33 lines

Quoted 33 lines

---
 drivers/staging/iio/adc/ad7280a.c | 46 ++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 10 deletions(-)

drivers/staging/iio/adc/ad7280a.c +17 -0

index 1f7ea5fb1e20..158a792c0bf8 100644
@@ -95,6 +95,23 @@
 #define   AD7280A_READ_ADDR_MSK				GENMASK(7, 2)
 #define AD7280A_CNVST_CTRL_REG			0x1D /* D7 to D0, Read/write */
 
+/* Transfer fields */
+#define AD7280A_TRANS_WRITE_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_WRITE_ADDR_MSK		GENMASK(26, 21)
+#define AD7280A_TRANS_WRITE_VAL_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_WRITE_ALL_MSK		BIT(12)
+#define AD7280A_TRANS_WRITE_CRC_MSK		GENMASK(10, 3)
+#define AD7280A_TRANS_WRITE_RES_PATTERN		0x2
+
+/* Layouts differ for channel vs other registers */
+#define AD7280A_TRANS_READ_DEVADDR_MSK		GENMASK(31, 27)
+#define AD7280A_TRANS_READ_CONV_CHANADDR_MSK	GENMASK(26, 23)
+#define AD7280A_TRANS_READ_CONV_DATA_MSK	GENMASK(22, 11)
+#define AD7280A_TRANS_READ_REG_REGADDR_MSK	GENMASK(26, 21)
+#define AD7280A_TRANS_READ_REG_DATA_MSK		GENMASK(20, 13)
+#define AD7280A_TRANS_READ_WRITE_ACK_MSK	BIT(10)
+#define AD7280A_TRANS_READ_CRC_MSK		GENMASK(9, 2)
+
 /* Magic value used to indicate this special case */
 #define AD7280A_ALL_CELLS				(0xAD << 16)
 

Quoted 12 lines

drivers/staging/iio/adc/ad7280a.c static int __ad7280_read32(struct ad7280_state *st, unsigned int *val) · line 239

Quoted 10 lines

drivers/staging/iio/adc/ad7280a.c +4 -2

@@ -216,10 +233,16 @@ static int __ad7280_read32(struct ad7280_state *st, unsigned int *val)
 static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
 			unsigned int addr, bool all, unsigned int val)
 {
-	unsigned int reg = devaddr << 27 | addr << 21 |
-			(val & 0xFF) << 13 | all << 12;
+	unsigned int reg = FIELD_PREP(AD7280A_TRANS_WRITE_DEVADDR_MSK, devaddr) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_ADDR_MSK, addr) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_VAL_MSK, val) |
+		FIELD_PREP(AD7280A_TRANS_WRITE_ALL_MSK, all);  

Does reg get initialized to 0? If not, we should take care of the reserved bit D11

You have lost me here. We are writing the whole register and this first assignment of reg uses = not |= so any bits not set explicitly will be zero. We could do a FIELD_PREP(BIT(11), 0) with an appropriate name for the BIT(11) define but that would be a bit odd.

Quoted 10 lines

Quoted 10 lines

drivers/staging/iio/adc/ad7280a.c +5 -1

+
+	reg |= FIELD_PREP(AD7280A_TRANS_WRITE_CRC_MSK,
+			ad7280_calc_crc8(st->crc_tab, reg >> 11));
+	/* Reserved b010 pattern not included crc calc */
+	reg |= AD7280A_TRANS_WRITE_RES_PATTERN;
 
-	reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
 	st->tx = cpu_to_be32(reg);
 
 	return spi_write(st->spi, &st->tx, sizeof(st->tx));

From: Jonathan Cameron <[email protected]>

This driver had a slightly non standard events ABI but there seems to be no reason for not doing it with the core support for rising and falling events on the two types of channels.

In theory the events on different daisy chained chips could be at different levels, but the driver has never supported this and it doesn't seem likely to be used so let us ignore that option.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 198 ++++++++++++++----------------
 1 file changed, 95 insertions(+), 103 deletions(-)

drivers/staging/iio/adc/ad7280a.c +21 -10

index 158a792c0bf8..0aeca39388cf 100644
@@ -551,18 +551,34 @@ static const struct attribute_group ad7280_attrs_group = {
 	.attrs = ad7280_attributes,
 };
 
+static const struct iio_event_spec ad7280_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 static void ad7280_voltage_channel_init(struct iio_chan_spec *chan, int i)
 {
 	chan->type = IIO_VOLTAGE;
 	chan->differential = 1;
 	chan->channel = i;
 	chan->channel2 = chan->channel + 1;
+	chan->event_spec = ad7280_events;
+	chan->num_event_specs = ARRAY_SIZE(ad7280_events);
 }
 
 static void ad7280_temp_channel_init(struct iio_chan_spec *chan, int i)
 {
 	chan->type = IIO_TEMP;
 	chan->channel = i;
+	chan->event_spec = ad7280_events;
+	chan->num_event_specs = ARRAY_SIZE(ad7280_events);
 }
 
 static void ad7280_common_fields_init(struct iio_chan_spec *chan, int addr,

@@ -732,88 +748,105 @@ static int ad7280_attr_init(struct ad7280_state *st)
 	return 0;
 }
 
-static ssize_t ad7280_read_channel_config(struct device *dev,
-					  struct device_attribute *attr,
-					  char *buf)
+static int ad7280a_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info, int *val, int *val2)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	unsigned int val;
 
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-		val = 1000 + (st->cell_threshhigh * 1568) / 100;
-		break;

drivers/staging/iio/adc/ad7280a.c +61 -41

+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			*val = 1000 + (st->cell_threshlow * 1568L) / 100;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		val = (st->aux_threshlow * 196) / 10;
+	case IIO_TEMP:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			*val = ((st->aux_threshhigh) * 196L) / 10;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			*val = (st->aux_threshlow * 196L) / 10;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 		break;
 	default:
 		return -EINVAL;
 	}
-
-	return sprintf(buf, "%u\n", val);
 }
 
-static ssize_t ad7280_write_channel_config(struct device *dev,
-					   struct device_attribute *attr,
-					   const char *buf,
-					   size_t len)
+static int ad7280a_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-
-	long val;
+	unsigned int addr;
+	long value;
 	int ret;
 
-	ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-	case AD7280A_CELL_UNDERVOLTAGE_REG:
-		val = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
-		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		val = (val * 10) / 196; /* LSB 19.6mV */
-		break;
-	default:
-		return -EFAULT;
-	}
-
-	val = clamp(val, 0L, 0xFFL);
+	if (val2 != 0)
+		return -EINVAL;
 
 	mutex_lock(&st->lock);
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-		st->cell_threshhigh = val;
-		break;
-	case AD7280A_CELL_UNDERVOLTAGE_REG:
-		st->cell_threshlow = val;
-		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
-		st->aux_threshhigh = val;
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
+		value = clamp(value, 0L, 0xFFL);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = AD7280A_CELL_OVERVOLTAGE_REG;
+			st->cell_threshhigh = value;
+			break;
+		case IIO_EV_DIR_FALLING:
+			addr = AD7280A_CELL_UNDERVOLTAGE_REG;
+			st->cell_threshlow = value;
+			break;
+		default:
+			ret = -EINVAL;
+			goto err_unlock;
+		}
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		st->aux_threshlow = val;
+	case IIO_TEMP:
+		value = (val * 10) / 196; /* LSB 19.6mV */
+		value = clamp(value, 0L, 0xFFL);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = AD7280A_AUX_ADC_OVERVOLTAGE_REG;
+			st->aux_threshhigh = val;
+			break;
+		case IIO_EV_DIR_FALLING:
+			addr = AD7280A_AUX_ADC_UNDERVOLTAGE_REG;
+			st->aux_threshlow = val;
+			break;
+		default:
+			ret = -EINVAL;
+			goto err_unlock;
+		}

drivers/staging/iio/adc/ad7280a.c +4 -44

+err_unlock:
 	mutex_unlock(&st->lock);
 
-	return ret ? ret : len;
+	return ret;
 }
 
 static irqreturn_t ad7280_event_handler(int irq, void *private)

@@ -875,48 +908,6 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-/* Note: No need to fix checkpatch warning that reads:
- *	CHECK: spaces preferred around that '-' (ctx:VxV)
- * The function argument is stringified and doesn't need a fix
- */
-static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-			     in_voltage-voltage_thresh_low_value,
-			     0644,
-			     ad7280_read_channel_config,
-			     ad7280_write_channel_config,
-			     AD7280A_CELL_UNDERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
-			     in_voltage-voltage_thresh_high_value,
-			     0644,
-			     ad7280_read_channel_config,
-			     ad7280_write_channel_config,
-			     AD7280A_CELL_OVERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR(in_temp_thresh_low_value,
-		       0644,
-		       ad7280_read_channel_config,
-		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_UNDERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR(in_temp_thresh_high_value,
-		       0644,
-		       ad7280_read_channel_config,
-		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_OVERVOLTAGE_REG);
-
-static struct attribute *ad7280_event_attributes[] = {
-	&iio_dev_attr_in_thresh_low_value.dev_attr.attr,
-	&iio_dev_attr_in_thresh_high_value.dev_attr.attr,
-	&iio_dev_attr_in_temp_thresh_low_value.dev_attr.attr,
-	&iio_dev_attr_in_temp_thresh_high_value.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7280_event_attrs_group = {
-	.attrs = ad7280_event_attributes,
-};
-
 static int ad7280_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val,

@@ -956,7 +947,8 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_info ad7280_info = {
 	.read_raw = ad7280_read_raw,
-	.event_attrs = &ad7280_event_attrs_group,
+	.read_event_value = &ad7280a_read_thresh,
+	.write_event_value = &ad7280a_write_thresh,
 	.attrs = &ad7280_attrs_group,
 };
 

Signature · 1 line

2.34.1

Neat, though some bits can be improved after moving out of staging.

Quoted 11 lines · Jonathan Cameron

From: Jonathan Cameron <[email protected]>

This driver had a slightly non standard events ABI but there seems to be no reason for not doing it with the core support for rising and falling events on the two types of channels.

In theory the events on different daisy chained chips could be at different levels, but the driver has never supported this and it doesn't seem likely to be used so let us ignore that option.

Signed-off-by Jonathan Cameron <[email protected]>

Reviewed-by Marcelo Schmitt <[email protected]>

Quoted 70 lines

---
 drivers/staging/iio/adc/ad7280a.c | 198 ++++++++++++++----------------
 1 file changed, 95 insertions(+), 103 deletions(-)

drivers/staging/iio/adc/ad7280a.c +21 -15

index 158a792c0bf8..0aeca39388cf 100644
@@ -551,18 +551,34 @@ static const struct attribute_group ad7280_attrs_group = {
 	.attrs = ad7280_attributes,
 };
 
+static const struct iio_event_spec ad7280_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 static void ad7280_voltage_channel_init(struct iio_chan_spec *chan, int i)
 {
 	chan->type = IIO_VOLTAGE;
 	chan->differential = 1;
 	chan->channel = i;
 	chan->channel2 = chan->channel + 1;
+	chan->event_spec = ad7280_events;
+	chan->num_event_specs = ARRAY_SIZE(ad7280_events);
 }
 
 static void ad7280_temp_channel_init(struct iio_chan_spec *chan, int i)
 {
 	chan->type = IIO_TEMP;
 	chan->channel = i;
+	chan->event_spec = ad7280_events;
+	chan->num_event_specs = ARRAY_SIZE(ad7280_events);
 }
 
 static void ad7280_common_fields_init(struct iio_chan_spec *chan, int addr,

@@ -732,88 +748,105 @@ static int ad7280_attr_init(struct ad7280_state *st)
 	return 0;
 }
 
-static ssize_t ad7280_read_channel_config(struct device *dev,
-					  struct device_attribute *attr,
-					  char *buf)
+static int ad7280a_read_thresh(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info, int *val, int *val2)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	unsigned int val;
 
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-		val = 1000 + (st->cell_threshhigh * 1568) / 100;
-		break;
-	case AD7280A_CELL_UNDERVOLTAGE_REG:
-		val = 1000 + (st->cell_threshlow * 1568) / 100;
-		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
-		val = (st->aux_threshhigh * 196) / 10;

drivers/staging/iio/adc/ad7280a.c static int ad7280_attr_init(struct ad7280_state *st) · line 763

Quoted 5 lines

drivers/staging/iio/adc/ad7280a.c +5 -0

+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			*val = 1000 + (st->cell_threshhigh * 1568L) / 100;  

Shouldn't the range multiplier for voltage threshhold be 1562L? This of course has nothing to do with this patch and should make no practical difference so can be changed after moving out of staging.

Quoted 123 lines

drivers/staging/iio/adc/ad7280a.c +64 -41

+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			*val = 1000 + (st->cell_threshlow * 1568L) / 100;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		val = (st->aux_threshlow * 196) / 10;
+	case IIO_TEMP:
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			*val = ((st->aux_threshhigh) * 196L) / 10;
+			return IIO_VAL_INT;
+		case IIO_EV_DIR_FALLING:
+			*val = (st->aux_threshlow * 196L) / 10;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 		break;
 	default:
 		return -EINVAL;
 	}
-
-	return sprintf(buf, "%u\n", val);
 }
 
-static ssize_t ad7280_write_channel_config(struct device *dev,
-					   struct device_attribute *attr,
-					   const char *buf,
-					   size_t len)
+static int ad7280a_write_thresh(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info,
+				int val, int val2)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-
-	long val;
+	unsigned int addr;
+	long value;
 	int ret;
 
-	ret = kstrtol(buf, 10, &val);
-	if (ret)
-		return ret;
-
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-	case AD7280A_CELL_UNDERVOLTAGE_REG:
-		val = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
-		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		val = (val * 10) / 196; /* LSB 19.6mV */
-		break;
-	default:
-		return -EFAULT;
-	}
-
-	val = clamp(val, 0L, 0xFFL);
+	if (val2 != 0)
+		return -EINVAL;
 
 	mutex_lock(&st->lock);
-	switch (this_attr->address) {
-	case AD7280A_CELL_OVERVOLTAGE_REG:
-		st->cell_threshhigh = val;
-		break;
-	case AD7280A_CELL_UNDERVOLTAGE_REG:
-		st->cell_threshlow = val;
-		break;
-	case AD7280A_AUX_ADC_OVERVOLTAGE_REG:
-		st->aux_threshhigh = val;
+	switch (chan->type) {
+	case IIO_VOLTAGE:
+		value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
+		value = clamp(value, 0L, 0xFFL);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = AD7280A_CELL_OVERVOLTAGE_REG;
+			st->cell_threshhigh = value;
+			break;
+		case IIO_EV_DIR_FALLING:
+			addr = AD7280A_CELL_UNDERVOLTAGE_REG;
+			st->cell_threshlow = value;
+			break;
+		default:
+			ret = -EINVAL;
+			goto err_unlock;
+		}
 		break;
-	case AD7280A_AUX_ADC_UNDERVOLTAGE_REG:
-		st->aux_threshlow = val;
+	case IIO_TEMP:
+		value = (val * 10) / 196; /* LSB 19.6mV */
+		value = clamp(value, 0L, 0xFFL);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			addr = AD7280A_AUX_ADC_OVERVOLTAGE_REG;
+			st->aux_threshhigh = val;
+			break;
+		case IIO_EV_DIR_FALLING:
+			addr = AD7280A_AUX_ADC_UNDERVOLTAGE_REG;
+			st->aux_threshlow = val;
+			break;
+		default:
+			ret = -EINVAL;
+			goto err_unlock;
+		}
 		break;
+	default:
+		ret = -EINVAL;
+		goto err_unlock;
 	}
 

drivers/staging/iio/adc/ad7280a.c static int ad7280_attr_init(struct ad7280_state *st) · line 845

Quoted 4 lines

drivers/staging/iio/adc/ad7280a.c +1 -3

-	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
-			   this_attr->address, 1, val);
-
+	ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr, 1, val);  

Can't ad7280_state end up with inconsistent threshold value if device write fails for some reason?
Should work well on normal operation so I guess there's no need to change it before graduation.

Quoted 71 lines

drivers/staging/iio/adc/ad7280a.c +4 -44

+err_unlock:
 	mutex_unlock(&st->lock);
 
-	return ret ? ret : len;
+	return ret;
 }
 
 static irqreturn_t ad7280_event_handler(int irq, void *private)

@@ -875,48 +908,6 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
-/* Note: No need to fix checkpatch warning that reads:
- *	CHECK: spaces preferred around that '-' (ctx:VxV)
- * The function argument is stringified and doesn't need a fix
- */
-static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
-			     in_voltage-voltage_thresh_low_value,
-			     0644,
-			     ad7280_read_channel_config,
-			     ad7280_write_channel_config,
-			     AD7280A_CELL_UNDERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value,
-			     in_voltage-voltage_thresh_high_value,
-			     0644,
-			     ad7280_read_channel_config,
-			     ad7280_write_channel_config,
-			     AD7280A_CELL_OVERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR(in_temp_thresh_low_value,
-		       0644,
-		       ad7280_read_channel_config,
-		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_UNDERVOLTAGE_REG);
-
-static IIO_DEVICE_ATTR(in_temp_thresh_high_value,
-		       0644,
-		       ad7280_read_channel_config,
-		       ad7280_write_channel_config,
-		       AD7280A_AUX_ADC_OVERVOLTAGE_REG);
-
-static struct attribute *ad7280_event_attributes[] = {
-	&iio_dev_attr_in_thresh_low_value.dev_attr.attr,
-	&iio_dev_attr_in_thresh_high_value.dev_attr.attr,
-	&iio_dev_attr_in_temp_thresh_low_value.dev_attr.attr,
-	&iio_dev_attr_in_temp_thresh_high_value.dev_attr.attr,
-	NULL,
-};
-
-static const struct attribute_group ad7280_event_attrs_group = {
-	.attrs = ad7280_event_attributes,
-};
-
 static int ad7280_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val,

@@ -956,7 +947,8 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
 
 static const struct iio_info ad7280_info = {
 	.read_raw = ad7280_read_raw,
-	.event_attrs = &ad7280_event_attrs_group,
+	.read_event_value = &ad7280a_read_thresh,
+	.write_event_value = &ad7280a_write_thresh,
 	.attrs = &ad7280_attrs_group,
 };
 

Signature · 1 line

2.34.1

From: Jonathan Cameron <[email protected]>

The *_balance_switch_en and *_balance_switch_timer attributes had non standard prefixes. Use the ext_info framework to automatically create then with in_voltageX-voltageY_ prefix.

Documentation for these two unusual attributes to follow.

Signed-off-by Jonathan Cameron <[email protected]>

---
 drivers/staging/iio/adc/ad7280a.c | 174 ++++++++----------------------
 1 file changed, 43 insertions(+), 131 deletions(-)

drivers/staging/iio/adc/ad7280a.c +43 -131

index 0aeca39388cf..032d6430bebf 100644
@@ -156,7 +156,6 @@ static unsigned int ad7280a_devaddr(unsigned int addr)
 struct ad7280_state {
 	struct spi_device		*spi;
 	struct iio_chan_spec		*channels;
-	struct iio_dev_attr		*iio_attr;
 	int				slave_num;
 	int				scan_cnt;
 	int				readback_delay_us;

@@ -447,37 +446,33 @@ static int ad7280_chain_setup(struct ad7280_state *st)
 	return ret;
 }
 
-static ssize_t ad7280_show_balance_sw(struct device *dev,
-				      struct device_attribute *attr,
-				      char *buf)
+static ssize_t ad7280_show_balance_sw(struct iio_dev *indio_dev,
+				      uintptr_t private,
+				      const struct iio_chan_spec *chan, char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
-	return sprintf(buf, "%d\n",
-		       !!(st->cb_mask[this_attr->address >> 8] &
-		       (1 << ((this_attr->address & 0xFF) + 2))));
+	return sysfs_emit(buf, "%d\n",
+			  !!(st->cb_mask[chan->address >> 8] &
+			  (1 << ((chan->address & 0xFF) + 2))));
 }
 
-static ssize_t ad7280_store_balance_sw(struct device *dev,
-				       struct device_attribute *attr,
-				       const char *buf,
-				       size_t len)
+static ssize_t ad7280_store_balance_sw(struct iio_dev *indio_dev,
+				       uintptr_t private,
+				       const struct iio_chan_spec *chan,
+				       const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	unsigned int devaddr, ch;
 	bool readin;
 	int ret;
-	unsigned int devaddr, ch;
 
 	ret = strtobool(buf, &readin);
 	if (ret)
 		return ret;
 
-	devaddr = this_attr->address >> 8;
-	ch = this_attr->address & 0xFF;
+	devaddr = chan->address >> 8;
+	ch = chan->address & 0xFF;
 
 	mutex_lock(&st->lock);
 	if (readin)

@@ -492,19 +487,18 @@ static ssize_t ad7280_store_balance_sw(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t ad7280_show_balance_timer(struct device *dev,
-					 struct device_attribute *attr,
+static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
 					 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	int ret;
 	unsigned int msecs;
+	int ret;
 
 	mutex_lock(&st->lock);
-	ret = ad7280_read_reg(st, this_attr->address >> 8,
-			      this_attr->address & 0xFF);
+	ret = ad7280_read_reg(st, chan->address >> 8,
+			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
 	mutex_unlock(&st->lock);
 
 	if (ret < 0)

@@ -512,43 +506,50 @@ static ssize_t ad7280_show_balance_timer(struct device *dev,
 
 	msecs = FIELD_GET(AD7280A_CB_TIMER_VAL_MSK, ret) * 71500;
 
-	return sprintf(buf, "%u\n", msecs);
+	return sysfs_emit(buf, "%u.%u\n", msecs / 1000, msecs % 1000);
 }
 
-static ssize_t ad7280_store_balance_timer(struct device *dev,
-					  struct device_attribute *attr,
-					  const char *buf,
-					  size_t len)
+static ssize_t ad7280_store_balance_timer(struct iio_dev *indio_dev,
+					  uintptr_t private,
+					  const struct iio_chan_spec *chan,
+					  const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7280_state *st = iio_priv(indio_dev);
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
-	unsigned long val;
+	int val, val2;
 	int ret;
 
-	ret = kstrtoul(buf, 10, &val);
+	ret = iio_str_to_fixpoint(buf, 1000, &val, &val2);
 	if (ret)
 		return ret;
 
+	val = val * 1000 + val2;
 	val /= 71500;
 
 	if (val > 31)
 		return -EINVAL;
 
 	mutex_lock(&st->lock);
-	ret = ad7280_write(st, this_attr->address >> 8,
-			   this_attr->address & 0xFF, 0,
+	ret = ad7280_write(st, chan->address >> 8,
+			   (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG, 0,
 			   FIELD_PREP(AD7280A_CB_TIMER_VAL_MSK, val));
 	mutex_unlock(&st->lock);
 
 	return ret ? ret : len;
 }
 
-static struct attribute *ad7280_attributes[AD7280A_MAX_CHAIN *
-					   AD7280A_CELLS_PER_DEV * 2 + 1];
-
-static const struct attribute_group ad7280_attrs_group = {
-	.attrs = ad7280_attributes,
+static const struct iio_chan_spec_ext_info ad7280_cell_ext_info[] = {
+	{
+		.name = "balance_switch_en",
+		.read = ad7280_show_balance_sw,
+		.write = ad7280_store_balance_sw,
+		.shared = IIO_SEPARATE,
+	}, {
+		.name = "balance_switch_timer",
+		.read = ad7280_show_balance_timer,
+		.write = ad7280_store_balance_timer,
+		.shared = IIO_SEPARATE,
+	},
+	{}
 };
 
 static const struct iio_event_spec ad7280_events[] = {

@@ -571,6 +572,7 @@ static void ad7280_voltage_channel_init(struct iio_chan_spec *chan, int i)
 	chan->channel2 = chan->channel + 1;
 	chan->event_spec = ad7280_events;
 	chan->num_event_specs = ARRAY_SIZE(ad7280_events);
+	chan->ext_info = ad7280_cell_ext_info;
 }
 
 static void ad7280_temp_channel_init(struct iio_chan_spec *chan, int i)

@@ -663,91 +665,6 @@ static int ad7280_channel_init(struct ad7280_state *st)
 	return cnt + 1;
 }
 
-static int ad7280_balance_switch_attr_init(struct iio_dev_attr *attr,
-					   struct device *dev, int addr, int i)
-{
-	attr->address = addr;
-	attr->dev_attr.attr.mode = 0644;
-	attr->dev_attr.show = ad7280_show_balance_sw;
-	attr->dev_attr.store = ad7280_store_balance_sw;
-	attr->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL,
-						  "in%d-in%d_balance_switch_en",
-						  i, i + 1);
-	if (!attr->dev_attr.attr.name)
-		return -ENOMEM;
-
-	return 0;
-}
-
-static int ad7280_balance_timer_attr_init(struct iio_dev_attr *attr,
-					  struct device *dev, int addr, int i)
-{
-	attr->address = addr;
-	attr->dev_attr.attr.mode = 0644;
-	attr->dev_attr.show = ad7280_show_balance_timer;
-	attr->dev_attr.store = ad7280_store_balance_timer;
-	attr->dev_attr.attr.name = devm_kasprintf(dev, GFP_KERNEL,
-						  "in%d-in%d_balance_timer",
-						  i, i + 1);
-	if (!attr->dev_attr.attr.name)
-		return -ENOMEM;
-
-	return 0;
-}
-
-static int ad7280_init_dev_attrs(struct ad7280_state *st, int dev, int *cnt)
-{
-	int addr, ch, i, ret;
-	struct iio_dev_attr *iio_attr;
-	struct device *sdev = &st->spi->dev;
-
-	for (ch = AD7280A_CELL_VOLTAGE_1_REG; ch <= AD7280A_CELL_VOLTAGE_6_REG; ch++) {
-		iio_attr = &st->iio_attr[*cnt];
-		addr = ad7280a_devaddr(dev) << 8 | ch;
-		i = dev * AD7280A_CELLS_PER_DEV + ch;
-
-		ret = ad7280_balance_switch_attr_init(iio_attr, sdev, addr, i);
-		if (ret < 0)
-			return ret;
-
-		ad7280_attributes[*cnt] = &iio_attr->dev_attr.attr;
-
-		(*cnt)++;
-		iio_attr = &st->iio_attr[*cnt];
-		addr = ad7280a_devaddr(dev) << 8 | (AD7280A_CB1_TIMER_REG + ch);
-
-		ret = ad7280_balance_timer_attr_init(iio_attr, sdev, addr, i);
-		if (ret < 0)
-			return ret;
-
-		ad7280_attributes[*cnt] = &iio_attr->dev_attr.attr;
-		(*cnt)++;
-	}
-
-	ad7280_attributes[*cnt] = NULL;
-
-	return 0;
-}
-
-static int ad7280_attr_init(struct ad7280_state *st)
-{
-	int dev, cnt = 0, ret;
-
-	st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
-				    (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
-				    GFP_KERNEL);
-	if (!st->iio_attr)
-		return -ENOMEM;
-
-	for (dev = 0; dev <= st->slave_num; dev++) {
-		ret = ad7280_init_dev_attrs(st, dev, &cnt);
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int ad7280a_read_thresh(struct iio_dev *indio_dev,
 			       const struct iio_chan_spec *chan,
 			       enum iio_event_type type,

@@ -949,7 +866,6 @@ static const struct iio_info ad7280_info = {
 	.read_raw = ad7280_read_raw,
 	.read_event_value = &ad7280a_read_thresh,
 	.write_event_value = &ad7280a_write_thresh,
-	.attrs = &ad7280_attrs_group,
 };
 
 static const struct ad7280_platform_data ad7793_default_pdata = {

@@ -1030,10 +946,6 @@ static int ad7280_probe(struct spi_device *spi)
 	indio_dev->channels = st->channels;
 	indio_dev->info = &ad7280_info;
 
-	ret = ad7280_attr_init(st);
-	if (ret < 0)
-		return ret;
-
 	ret = devm_iio_device_register(&spi->dev, indio_dev);
 	if (ret)
 		return ret;

Signature · 1 line

2.34.1