RFC: Does separate types for differential signals make sense?

PATCH New Jonathan Cameron 2 participants 4 messages Aug 18, 2011

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Jonathan Cameron

author

2011-08-18 10:10 UTC #0 lore raw

Hi All,

Whilst working on some of the capacitance adc drivers yesterday I added a second
differential type. Whilst we only had IIO_VOlTAGE_DIFF, it seemed easier to
handle it as a special case - now we have IIO_CAPACITANCE_DIFF, I'm not so sure.

The alternative is to add another flag to struct iio_chan_spec and build the differential names automatically (easy enough I think). Nastier is how to handle the related event codes. As we haven't pushed out the new 64 bit codes, this is a perfect time to slip in a change there (rather than two changes back to back.)

Current macro to generate codes is:

define IIO_EVENT_CODE(chan_type, modifier, direction,			\
		       type, chan, chan1, chan2)			\
	(((u64)type << 56) | ((u64)direction << 48) | ((u64)modifier << 40) | \
	 ((u64)chan_type << 32) | (chan2 << 16) | chan1 | chan)

So if we were to steal a bit to mark channels as differential, where would we do it? Obvious choice is in type - reducing max number of types to 128 - can't see that being a problem any time soon. Could pinch the top bit off direction instead - that one has way more values than I can think of uses for...

This is going to be an annoyingly invasive patch to do. I'll probably scrap the use of the IIO_CHAN macro for all differential channels rather than adding another parameter to it. Plan was to scrap that macro entirely eventually, so not such a bad thing.

Jonathan

Michael Hennerich

maintainer

2011-08-18 12:09 UTC #1 lore raw

Quoted 5 lines · Jonathan Cameron

Hi All,
Whilst working on some of the capacitance adc drivers yesterday I added a second
differential type. Whilst we only had IIO_VOlTAGE_DIFF, it seemed easier to
handle it as a special case - now we have IIO_CAPACITANCE_DIFF, I'm not so sure.

IIO_CURRENT_DIFF is also likely to be added in future...

Quoted 11 lines

The alternative is to add another flag to struct iio_chan_spec and build the differential names automatically (easy enough I think). Nastier is how to handle the related event codes. As we haven't pushed out the new 64 bit codes, this is a perfect time to slip in a change there (rather than two changes back to back.) Current macro to generate codes is:
define IIO_EVENT_CODE(chan_type, modifier, direction,			\
		       type, chan, chan1, chan2)			\
	(((u64)type<<  56) | ((u64)direction<<  48) | ((u64)modifier<<  40) | \
	 ((u64)chan_type<<  32) | (chan2<<  16) | chan1 | chan)

Quoted 4 lines

So if we were to steal a bit to mark channels as differential, where would we do it? Obvious choice is in type - reducing max number of types to 128 - can't see that being a problem any time soon. Could pinch the top bit off direction instead - that one has way more values than I can think of uses for...

In the way we use chan_type today - I think it would be the best spot.

Quoted 8 lines

This is going to be an annoyingly invasive patch to do. I'll probably scrap the use of the IIO_CHAN macro for all differential channels rather than adding another parameter to it. Plan was to scrap that macro entirely eventually, so not such a bad thing.
Jonathan

Signature · 8 lines

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Jonathan Cameron

author

2011-08-18 12:21 UTC #2 lore raw

PATCH 1/1 staging:iio: Differential channel handling - use explicit flag rather than types.

Straight forward change in the core, but required some drivers to not use the IIO_CHAN macro as that doesn't allow setting this bit (and is going away anyway). Hence the churn.

Tested on max1363 with a couple of supported parts.

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

---
 drivers/staging/iio/adc/ad7152.c        |    8 +-
 drivers/staging/iio/adc/ad7192.c        |    8 +-
 drivers/staging/iio/adc/ad7793.c        |  176 ++++++++++++++++++++--------
 drivers/staging/iio/adc/max1363_core.c  |  198 ++++++++++++++-----------------
 drivers/staging/iio/iio.h               |    4 +-
 drivers/staging/iio/industrialio-core.c |   40 ++++---
 drivers/staging/iio/sysfs.h             |    8 +-
 7 files changed, 253 insertions(+), 189 deletions(-)

drivers/staging/iio/adc/ad7152.c +5 -3

index 0809bb1..debcea5 100644
@@ -241,7 +241,7 @@ static int ad7152_read_raw(struct iio_dev *dev_info,
 	switch (mask) {
 	case 0:
 		/* First set whether in differential mode */
-		if (chan->type == IIO_CAPACITANCE_DIFF)
+		if (chan->differential)
 			regval = (1 << 5);
 
 		/* Make sure the channel is enabled */

@@ -319,7 +319,8 @@ static const struct iio_chan_spec ad7152_channels[] = {
 		(1 << IIO_CHAN_INFO_CALIBBIAS_SEPARATE) |
 		(1 << IIO_CHAN_INFO_SCALE_SEPARATE),
 	}, {
-		.type = IIO_CAPACITANCE_DIFF,
+		.type = IIO_CAPACITANCE,
+		.differential = 1,
 		.indexed = 1,
 		.channel = 0,
 		.channel2 = 2,

@@ -327,7 +328,8 @@ static const struct iio_chan_spec ad7152_channels[] = {
 		(1 << IIO_CHAN_INFO_CALIBBIAS_SEPARATE) |
 		(1 << IIO_CHAN_INFO_SCALE_SEPARATE),
 	}, {
-		.type = IIO_CAPACITANCE_DIFF,
+		.type = IIO_CAPACITANCE,
+		.differential = 1,
 		.indexed = 1,
 		.channel = 1,
 		.channel2 = 3,

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

index c67da6a..5bc1051 100644
@@ -886,8 +886,7 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
 			((1 << (chan->scan_type.realbits)) - 1);
 
 		switch (chan->type) {
-		case IIO_IN:
-		case IIO_IN_DIFF:
+		case IIO_VOLTAGE:
 			if (!unipolar)
 				*val -= (1 << (chan->scan_type.realbits - 1));
 			break;

@@ -986,7 +985,8 @@ static const struct iio_info ad7192_info = {
 };
 
 #define AD7192_CHAN_DIFF(_chan, _chan2, _name, _address, _si)		\
-	{ .type = IIO_IN_DIFF,						\
+	{ .type = IIO_VOLTAGE,						\
+	  .differential = 1,						\
 	  .indexed = 1,							\
 	  .extend_name = _name,						\
 	  .channel = _chan,						\

@@ -997,7 +997,7 @@ static const struct iio_info ad7192_info = {
 	  .scan_type =  IIO_ST('s', 24, 32, 0)}
 
 #define AD7192_CHAN(_chan, _address, _si)				\
-	{ .type = IIO_IN,						\
+	{ .type = IIO_VOLTAGE,						\
 	  .indexed = 1,							\
 	  .channel = _chan,						\
 	  .info_mask = (1 << IIO_CHAN_INFO_SCALE_SHARED),		\

drivers/staging/iio/adc/ad7793.c +128 ~~-48~~

185 lines not on this page · raw message

drivers/staging/iio/adc/max1363_core.c +89 ~~-109~~

226 lines not on this page · raw message

drivers/staging/iio/iio.h +2 -2

index 53315c1..6902745 100644
@@ -31,7 +31,6 @@ enum iio_chan_type {
 	IIO_CURRENT,
 	IIO_POWER,
 	IIO_ACCEL,
-	IIO_VOLTAGE_DIFF,
 	IIO_GYRO,
 	IIO_MAGN,
 	IIO_LIGHT,

@@ -43,7 +42,6 @@ enum iio_chan_type {
 	IIO_ANGL,
 	IIO_TIMESTAMP,
 	IIO_CAPACITANCE,
-	IIO_CAPACITANCE_DIFF,
 };
 
 /* Nasty hack to avoid massive churn */

@@ -125,6 +123,7 @@ enum iio_endian {
  *			the value in channel will be suppressed for attribute
  *			but not for event codes. Typically set it to 0 when
  *			the index is false.
+ * @differential:	Channel is differential.
  */
 struct iio_chan_spec {
 	enum iio_chan_type	type;

@@ -146,6 +145,7 @@ struct iio_chan_spec {
 	unsigned		modified:1;
 	unsigned		indexed:1;
 	unsigned		output:1;
+	unsigned		differential:1;
 };
 
 #define IIO_ST(si, rb, sb, sh)						\

drivers/staging/iio/industrialio-core.c +21 ~~-19~~

index 07a5066..d4d13f4 100644
@@ -54,7 +54,6 @@ static const char * const iio_chan_type_name_spec_shared[] = {
 	[IIO_CURRENT] = "current",
 	[IIO_POWER] = "power",
 	[IIO_ACCEL] = "accel",
-	[IIO_VOLTAGE_DIFF] = "voltage-voltage",
 	[IIO_GYRO] = "gyro",
 	[IIO_MAGN] = "magn",
 	[IIO_LIGHT] = "illuminance",

@@ -66,12 +65,6 @@ static const char * const iio_chan_type_name_spec_shared[] = {
 	[IIO_ANGL] = "angl",
 	[IIO_TIMESTAMP] = "timestamp",
 	[IIO_CAPACITANCE] = "capacitance",
-	[IIO_CAPACITANCE_DIFF] = "capacitance-capacitance",
-};
-
-static const char * const iio_chan_type_name_spec_complex[] = {
-	[IIO_VOLTAGE_DIFF] = "voltage%d-voltage%d",
-	[IIO_CAPACITANCE_DIFF] = "capacitance%d-capacitance%d",
 };
 
 static const char * const iio_modifier_names[] = {

@@ -449,13 +442,21 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 		goto error_ret;
 
 	/* Special case for types that uses both channel numbers in naming */
-	if (((chan->type == IIO_VOLTAGE_DIFF) ||
-	     (chan->type == IIO_CAPACITANCE_DIFF)) &&
-	    !generic)
+	if (chan->differential && !generic)
 		name_format
-			= kasprintf(GFP_KERNEL, "%s_%s_%s",
+			= kasprintf(GFP_KERNEL, "%s_%s%d-%s%d_%s",
 				    iio_direction[chan->output],
-				    iio_chan_type_name_spec_complex[chan->type],
+				    iio_chan_type_name_spec_shared[chan->type],
+				    chan->channel,
+				    iio_chan_type_name_spec_shared[chan->type],
+				    chan->channel2,
+				    full_postfix);
+	else if (chan->differential && generic)
+		name_format
+			= kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
+				    iio_direction[chan->output],
+				    iio_chan_type_name_spec_shared[chan->type],
+				    iio_chan_type_name_spec_shared[chan->type],
 				    full_postfix);
 	else if (generic || !chan->indexed)
 		name_format

@@ -797,13 +798,14 @@ static int iio_device_add_event_sysfs(struct iio_dev *dev_info,
 			mask = IIO_MOD_EVENT_CODE(chan->type, 0, chan->channel,
 						  i/IIO_EV_DIR_MAX,
 						  i%IIO_EV_DIR_MAX);
-		else if ((chan->type == IIO_VOLTAGE_DIFF) ||
-			 (chan->type == IIO_CAPACITANCE_DIFF))
-			mask = IIO_MOD_EVENT_CODE(chan->type,
-						  chan->channel,
-						  chan->channel2,
-						  i/IIO_EV_DIR_MAX,
-						  i%IIO_EV_DIR_MAX);
+		else if (chan->differential)
+			mask = IIO_EVENT_CODE(chan->type,
+					      0, 0,
+					      i%IIO_EV_DIR_MAX,
+					      i/IIO_EV_DIR_MAX,
+					      0,
+					      chan->channel,
+					      chan->channel2);
 		else
 			mask = IIO_UNMOD_EVENT_CODE(chan->type,
 						    chan->channel,

drivers/staging/iio/sysfs.h +4 -4

index 3388b8c..0c04d93 100644
@@ -128,9 +128,9 @@ enum iio_event_direction {
 	IIO_EV_DIR_FALLING,
 };
 
-#define IIO_EVENT_CODE(chan_type, modifier, direction,			\
+#define IIO_EVENT_CODE(chan_type, diff, modifier, direction,	\
 		       type, chan, chan1, chan2)			\
-	(((u64)type << 56) | ((u64)direction << 48) | ((u64)modifier << 40) | \
+	(((u64)type << 56) | ((u64)diff << 48) | ((u64)direction << 47) | ((u64)modifier << 40) | \
 	 ((u64)chan_type << 32) | (chan2 << 16) | chan1 | chan)
 
 #define IIO_EV_DIR_MAX 4

@@ -139,10 +139,10 @@ enum iio_event_direction {
 
 #define IIO_MOD_EVENT_CODE(channelclass, number, modifier,		\
 			   type, direction)				\
-	IIO_EVENT_CODE(channelclass, modifier, direction, type, number, 0, 0)
+	IIO_EVENT_CODE(channelclass, 0, modifier, direction, type, number, 0, 0)
 
 #define IIO_UNMOD_EVENT_CODE(channelclass, number, type, direction)	\
-	IIO_EVENT_CODE(channelclass, 0, direction, type, number, 0, 0)
+	IIO_EVENT_CODE(channelclass, 0, 0, direction, type, number, 0, 0)
 
 #define IIO_EVENT_CODE_EXTRACT_TYPE(mask) ((mask >> 56) & 0xFF)
 #define IIO_EVENT_CODE_EXTRACT_DIR(mask) ((mask >> 48) & 0xFF)

Signature · 1 line

1.7.3.4

Jonathan Cameron

author

2011-08-18 12:29 UTC #3 lore raw

PATCH 1/1 IIO: Scrap the _DIFF types in favour of a flag in chan spec.

In reply to Michael Hennerich

This also resorts in a small change to the event codes.

Sits on the capfun branch (because it effects some stuff there).

If people are happy I'll back port it to before the introduction of the capacitance version thus cleaning the series up.

Jonathan

Jonathan Cameron (1):
staging:iio: Differential channel handling - use explicit flag rather
than types.

 drivers/staging/iio/adc/ad7152.c        |    8 +-
 drivers/staging/iio/adc/ad7192.c        |    8 +-
 drivers/staging/iio/adc/ad7793.c        |  176 ++++++++++++++++++++--------
 drivers/staging/iio/adc/max1363_core.c  |  198 ++++++++++++++-----------------
 drivers/staging/iio/iio.h               |    4 +-
 drivers/staging/iio/industrialio-core.c |   40 ++++---
 drivers/staging/iio/sysfs.h             |    8 +-
 7 files changed, 253 insertions(+), 189 deletions(-)

Signature · 1 line

1.7.3.4