[PATCH v2 0/2] iio: add support for signed conversions

[PATCH v2 0/2] iio: add support for signed conversions

[Ludovic Desroches]

Hi Jonathan,

I hope these patches are in adequacy with your comments. They are based on the
the previous two patches you have taken. I am not sure about the 
correctness of the documentation.

Ludovic Desroches (2):
  iio: core: introduce IIO_CHAN_INFO_SIGNED
  iio:adc:at91-sama5d2: add support for signed conversion

 Documentation/ABI/testing/sysfs-bus-iio |  10 +++
 drivers/iio/adc/at91-sama5d2_adc.c      | 125 ++++++++++++++++++++++++++------
 drivers/iio/industrialio-core.c         |   1 +
 include/linux/iio/iio.h                 |   1 +
 4 files changed, 114 insertions(+), 23 deletions(-)

-- 
2.5.0

[PATCH v2 1/2] iio: core: introduce IIO_CHAN_INFO_SIGNED

[Ludovic Desroches]

The same channel can be used to perform a signed or an unsigned
conversion. Add a new infomask element to be able to select the type of
conversion wanted: a raw one or a signed raw one.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 10 ++++++++++
 drivers/iio/industrialio-core.c         |  1 +
 include/linux/iio/iio.h                 |  1 +
 3 files changed, 12 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 3c66248..161733c 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1501,3 +1501,13 @@ Contact:	linux-iio at vger.kernel.org
 Description:
 		Raw (unscaled no offset etc.) pH reading of a substance as a negative
 		base-10 logarithm of hydrodium ions in a litre of water.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_signed
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_supply_signed
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_i_signed
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_q_signed
+KernelVersion:	4.7
+Contact:	linux-iio at vger.kernel.org
+Description:
+		Signed (unscaled no bias removal etc.) voltage measurement from
+		channel Y.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 70cb7eb..fb2ca27 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -147,6 +147,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
 	[IIO_CHAN_INFO_CALIBEMISSIVITY] = "calibemissivity",
 	[IIO_CHAN_INFO_OVERSAMPLING_RATIO] = "oversampling_ratio",
+	[IIO_CHAN_INFO_SIGNED] = "signed",
 };
 
 /**
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b2b1677..6f5eb24 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -46,6 +46,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_DEBOUNCE_TIME,
 	IIO_CHAN_INFO_CALIBEMISSIVITY,
 	IIO_CHAN_INFO_OVERSAMPLING_RATIO,
+	IIO_CHAN_INFO_SIGNED,
 };
 
 enum iio_shared_by {
-- 
2.5.0

[PATCH v2 1/2] iio: core: introduce IIO_CHAN_INFO_SIGNED

[Lars-Peter Clausen]

On 03/07/2016 03:29 PM, Ludovic Desroches wrote:
> The same channel can be used to perform a signed or an unsigned
> conversion. Add a new infomask element to be able to select the type of
> conversion wanted: a raw one or a signed raw one.

If this is the difference between offset binary and two's complement then it
makes no sense to expose this at this level. Both are the same number just
in a different representation and converting between them is cheap. A few
magnitudes cheaper than reading the result over sysfs. So, if your device
supports both, just pick one.

For the buffered interface it may make sense to expose this, since the per
sample overhead is a lot lower. But still doing the conversion should be
cheap enough that it does not really matter. Before this is implemented I'd
like to see hard performance numbers that this actually makes a difference.

- Lars

[PATCH v2 1/2] iio: core: introduce IIO_CHAN_INFO_SIGNED

[Jonathan Cameron]

On 07/03/16 20:09, Lars-Peter Clausen wrote:
> On 03/07/2016 03:29 PM, Ludovic Desroches wrote:
>> The same channel can be used to perform a signed or an unsigned
>> conversion. Add a new infomask element to be able to select the type of
>> conversion wanted: a raw one or a signed raw one.
> 
> If this is the difference between offset binary and two's complement then it
> makes no sense to expose this at this level. Both are the same number just
> in a different representation and converting between them is cheap. A few
> magnitudes cheaper than reading the result over sysfs. So, if your device
> supports both, just pick one.
> 
> For the buffered interface it may make sense to expose this, since the per
> sample overhead is a lot lower. But still doing the conversion should be
> cheap enough that it does not really matter. Before this is implemented I'd
> like to see hard performance numbers that this actually makes a difference.
> 
> - Lars
> 
Definitely looking for more detail on this.  I'd missed we were talking simply
about representation (which is also how I read 62.6.6 Conversion Results Format
in the datasheet). Not entirely sure what I imagined the difference between
signed and unsigned output would be!

Jonathan

[PATCH v2 1/2] iio: core: introduce IIO_CHAN_INFO_SIGNED

[Ludovic Desroches]

On Wed, Mar 09, 2016 at 09:04:21PM +0000, Jonathan Cameron wrote:
> On 07/03/16 20:09, Lars-Peter Clausen wrote:
> > On 03/07/2016 03:29 PM, Ludovic Desroches wrote:
> >> The same channel can be used to perform a signed or an unsigned
> >> conversion. Add a new infomask element to be able to select the type of
> >> conversion wanted: a raw one or a signed raw one.
> > 
> > If this is the difference between offset binary and two's complement then it
> > makes no sense to expose this at this level. Both are the same number just
> > in a different representation and converting between them is cheap. A few
> > magnitudes cheaper than reading the result over sysfs. So, if your device
> > supports both, just pick one.
> > 
> > For the buffered interface it may make sense to expose this, since the per
> > sample overhead is a lot lower. But still doing the conversion should be
> > cheap enough that it does not really matter. Before this is implemented I'd
> > like to see hard performance numbers that this actually makes a difference.
> > 
> > - Lars
> > 
> Definitely looking for more detail on this.  I'd missed we were talking simply
> about representation (which is also how I read 62.6.6 Conversion Results Format
> in the datasheet). Not entirely sure what I imagined the difference between
> signed and unsigned output would be!

You are both right, it is only about representation. I have asked hardware guys
why they add this feature. They told me it is for convenience and because some
librairies need signed results.

Regards

Ludovic

[PATCH v2 2/2] iio:adc:at91-sama5d2: add support for signed conversion

[Ludovic Desroches]

The at91-sama5d2 ADC controller can achieve unsigned and signed
conversions. For each channel, a signed and an unsigned variant are
created.
Sign mode is a global configuration, it is not tied to a specific
channel. For this reason, the controller has to be configured upon
conversion request.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/iio/adc/at91-sama5d2_adc.c | 125 ++++++++++++++++++++++++++++++-------
 1 file changed, 102 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 5bc038f..0370c4f 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -105,8 +105,26 @@
 #define AT91_SAMA5D2_LCCWR	0x38
 /* Overrun Status Register */
 #define AT91_SAMA5D2_OVER	0x3c
+
 /* Extended Mode Register */
 #define AT91_SAMA5D2_EMR	0x40
+/* Sign Mode */
+#define AT91_SAMA5D2_EMR_SIGNMODE(v)		((v) << 25)
+/*
+ * Single-Ended channels: Unsigned conversions.
+ * Differential channels: Signed conversions.
+ */
+#define AT91_SAMA5D2_EMR_SE_UNSG_DF_SIGN	0
+/*
+ * Single-Ended channels: Signed conversions.
+ * Differential channels: Unsigned conversions.
+ */
+#define AT91_SAMA5D2_EMR_SE_SIGN_DF_UNSG	1
+/* All channels: Unsigned conversions */
+#define AT91_SAMA5D2_EMR_ALL_UNSIGNED		2
+/* All channels: Signed conversions */
+#define AT91_SAMA5D2_EMR_ALL_SIGNED		3
+
 /* Compare Window Register */
 #define AT91_SAMA5D2_CWR	0x44
 /* Channel Gain Register */
@@ -140,22 +158,28 @@
 /* Version Register */
 #define AT91_SAMA5D2_VERSION	0xfc
 
-#define AT91_SAMA5D2_CHAN(num, addr)					\
+#define AT91_SAMA5D2_VOLTAGE_CHANNEL(num, addr, sign_mode)		\
 	{								\
 		.type = IIO_VOLTAGE,					\
 		.channel = num,						\
 		.address = addr,					\
 		.scan_type = {						\
-			.sign = 'u',					\
+			.sign = sign_mode,				\
 			.realbits = 12,					\
 		},							\
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_separate = (sign_mode == 's') ? BIT(IIO_CHAN_INFO_SIGNED) : BIT(IIO_CHAN_INFO_RAW),		\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
 		.datasheet_name = "CH"#num,				\
 		.indexed = 1,						\
 	}
 
+#define AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(num, addr)		\
+	AT91_SAMA5D2_VOLTAGE_CHANNEL(num, addr, 's')
+
+#define AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(num, addr)	\
+	AT91_SAMA5D2_VOLTAGE_CHANNEL(num, addr, 'u')
+
 #define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
 #define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
 
@@ -185,18 +209,30 @@ struct at91_adc_state {
 };
 
 static const struct iio_chan_spec at91_adc_channels[] = {
-	AT91_SAMA5D2_CHAN(0, 0x50),
-	AT91_SAMA5D2_CHAN(1, 0x54),
-	AT91_SAMA5D2_CHAN(2, 0x58),
-	AT91_SAMA5D2_CHAN(3, 0x5c),
-	AT91_SAMA5D2_CHAN(4, 0x60),
-	AT91_SAMA5D2_CHAN(5, 0x64),
-	AT91_SAMA5D2_CHAN(6, 0x68),
-	AT91_SAMA5D2_CHAN(7, 0x6c),
-	AT91_SAMA5D2_CHAN(8, 0x70),
-	AT91_SAMA5D2_CHAN(9, 0x74),
-	AT91_SAMA5D2_CHAN(10, 0x78),
-	AT91_SAMA5D2_CHAN(11, 0x7c),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(0, 0x50),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(1, 0x54),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(2, 0x58),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(3, 0x5c),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(4, 0x60),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(5, 0x64),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(6, 0x68),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(7, 0x6c),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(8, 0x70),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(9, 0x74),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(10, 0x78),
+	AT91_SAMA5D2_UNSIGNED_VOLTAGE_CHANNEL(11, 0x7c),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(0, 0x50),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(1, 0x54),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(2, 0x58),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(3, 0x5c),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(4, 0x60),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(5, 0x64),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(6, 0x68),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(7, 0x6c),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(8, 0x70),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(9, 0x74),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(10, 0x78),
+	AT91_SAMA5D2_SIGNED_VOLTAGE_CHANNEL(11, 0x7c),
 };
 
 static unsigned at91_adc_startup_time(unsigned startup_time_min,
@@ -273,6 +309,38 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
 	return IRQ_NONE;
 }
 
+static int at91_adc_read_conversion_value(struct at91_adc_state *st)
+{
+	u32 emr;
+	int ret;
+
+	/* Read EMR register and clear 'sign mode' field */
+	emr = at91_adc_readl(st, AT91_SAMA5D2_EMR)
+		& AT91_SAMA5D2_EMR_SIGNMODE(0);
+	/*
+	 * Check if the user requested a conversion on a signed or
+	 * unsigned channel.
+	 */
+	if (st->chan->scan_type.sign == 's')
+		emr |= AT91_SAMA5D2_EMR_SIGNMODE(AT91_SAMA5D2_EMR_ALL_SIGNED);
+	else
+		emr |= AT91_SAMA5D2_EMR_SIGNMODE(AT91_SAMA5D2_EMR_ALL_UNSIGNED);
+
+	at91_adc_writel(st, AT91_SAMA5D2_EMR, emr);
+	at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(st->chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(st->chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
+
+	ret = wait_event_interruptible_timeout(st->wq_data_available,
+					       st->conversion_done,
+					       msecs_to_jiffies(1000));
+
+	at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(st->chan->channel));
+	at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(st->chan->channel));
+
+	return ret;
+}
+
 static int at91_adc_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
@@ -286,13 +354,8 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 
 		st->chan = chan;
 
-		at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_IER, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
+		ret = at91_adc_read_conversion_value(st);
 
-		ret = wait_event_interruptible_timeout(st->wq_data_available,
-						       st->conversion_done,
-						       msecs_to_jiffies(1000));
 		if (ret == 0)
 			ret = -ETIMEDOUT;
 
@@ -302,8 +365,24 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
 			st->conversion_done = false;
 		}
 
-		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
-		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
+		mutex_unlock(&st->lock);
+		return ret;
+
+	case IIO_CHAN_INFO_SIGNED:
+		mutex_lock(&st->lock);
+
+		st->chan = chan;
+
+		ret = at91_adc_read_conversion_value(st);
+
+		if (ret == 0)
+			ret = -ETIMEDOUT;
+
+		if (ret > 0) {
+			*val = sign_extend32(st->conversion_value, 11);
+			ret = IIO_VAL_INT;
+			st->conversion_done = false;
+		}
 
 		mutex_unlock(&st->lock);
 		return ret;
-- 
2.5.0

[PATCH v2 2/2] iio:adc:at91-sama5d2: add support for signed conversion

[Lars-Peter Clausen]

On 03/07/2016 03:29 PM, Ludovic Desroches wrote:
> The at91-sama5d2 ADC controller can achieve unsigned and signed
> conversions. For each channel, a signed and an unsigned variant are
> created.
> Sign mode is a global configuration, it is not tied to a specific
> channel. For this reason, the controller has to be configured upon
> conversion request.

So, what's the difference between signed and unsigned mode?