[PATCH 0/4] iio: mxs-lradc: add support to optional divider_by_two

[PATCH 0/4] iio: mxs-lradc: add support to optional divider_by_two

[Hector Palacios]

Greetings,

This patchset adds support to the optional divider_by_two of
LRADC channels. The first patch changes the realbits to 12. The 
following add the scale read operation, scale_available read 
operation, and scale write operation.

This was tested on a custom i.MX28 platform.
Could someone please test on an i.MX23?

Thanks to Lars-Peter Clausen and Alexandre Belloni for helping me out 
through the IIO driver framework.
Man, it resulted a large amount of code for just enabling a flag.

Hector Palacios (4):
  iio: mxs-lradc: change the realbits to 12
  iio: mxs-lradc: add scale attribute to channels
  iio: mxs-lradc: add scale_available file to channels
  iio: mxs-lradc: add write_raw function to modify scale

 drivers/staging/iio/adc/mxs-lradc.c | 292 ++++++++++++++++++++++++++++++++----
 1 file changed, 261 insertions(+), 31 deletions(-)

[PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Hector Palacios]

The LRADC virtual channels have an 18 bit field to store the sum of up
to 2^5 accumulated samples. The read_raw function however only operates
over a single sample (12 bit resolution).
In order to use this field for scaling operations, we need it to be the
exact resolution value of the LRADC.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 163c638..d65a594 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -822,7 +822,7 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
 	.channel = (idx),					\
 	.scan_type = {						\
 		.sign = 'u',					\
-		.realbits = 18,					\
+		.realbits = 12,					\
 		.storagebits = 32,				\
 	},							\
 }

[PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Hector Palacios]

Some LRADC channels have a fixed pre-divider, and all can have enabled
an optional divisor by two which allows a maximum input voltage of
VDDIO - 50mV.

This patch
 - adds the scaling info flag to all channels
 - adds a lookup table with max reference voltage per channel
   (where the fixed pre-dividers apply)
 - allows to read the scaling attribute (computed from the Vref)

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 122 +++++++++++++++++++++++++++---------
 1 file changed, 93 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index d65a594..012c42e 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -107,19 +107,63 @@ static const char * const mx28_lradc_irq_names[] = {
 	"mxs-lradc-button1",
 };
 
+/*
+ * Max reference voltage (mV) for each channel
+ */
+static const int mx23_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
+	1850,		/* CH0 */
+	1850,		/* CH1 */
+	1850,		/* CH2 */
+	1850,		/* CH3 */
+	1850,		/* CH4 */
+	1850,		/* CH5 */
+	1850 * 2,	/* CH6 VDDIO */
+	1850 * 4,	/* CH7 VBATT */
+	1850,		/* CH8 Temp sense 0 */
+	1850,		/* CH9 Temp sense 1 */
+	1850,		/* CH10 */
+	1850,		/* CH11 */
+	1850,		/* CH12 USB_DP */
+	1850,		/* CH13 USB_DN */
+	1850,		/* CH14 VBG */
+	1850 * 4,	/* CH15 VDD5V */
+};
+
+static const int mx28_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
+	1850,		/* CH0 */
+	1850,		/* CH1 */
+	1850,		/* CH2 */
+	1850,		/* CH3 */
+	1850,		/* CH4 */
+	1850,		/* CH5 */
+	1850,		/* CH6 */
+	1850 * 4,	/* CH7 VBATT */
+	1850,		/* CH8 Temp sense 0 */
+	1850,		/* CH9 Temp sense 1 */
+	1850 * 2,	/* CH10 VDDIO */
+	1850,		/* CH11 VTH */
+	1850 * 2,	/* CH12 VDDA */
+	1850,		/* CH13 VDDD */
+	1850,		/* CH14 VBG */
+	1850 * 4,	/* CH15 VDD5V */
+};
+
 struct mxs_lradc_of_config {
 	const int		irq_count;
 	const char * const	*irq_name;
+	const int 		*vref_mv;
 };
 
 static const struct mxs_lradc_of_config mxs_lradc_of_config[] = {
 	[IMX23_LRADC] = {
 		.irq_count	= ARRAY_SIZE(mx23_lradc_irq_names),
 		.irq_name	= mx23_lradc_irq_names,
+		.vref_mv	= mx23_vref_mv,
 	},
 	[IMX28_LRADC] = {
 		.irq_count	= ARRAY_SIZE(mx28_lradc_irq_names),
 		.irq_name	= mx28_lradc_irq_names,
+		.vref_mv	= mx28_vref_mv,
 	},
 };
 
@@ -141,6 +185,8 @@ struct mxs_lradc {
 
 	struct completion	completion;
 
+	const int		*vref_mv;
+
 	/*
 	 * Touchscreen LRADC channels receives a private slot in the CTRL4
 	 * register, the slot #7. Therefore only 7 slots instead of 8 in the
@@ -225,39 +271,12 @@ struct mxs_lradc {
 #define	LRADC_CTRL4_LRADCSELECT_MASK(n)		(0xf << ((n) * 4))
 #define	LRADC_CTRL4_LRADCSELECT_OFFSET(n)	((n) * 4)
 
-/*
- * Raw I/O operations
- */
-static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
+static int mxs_lradc_read_single(struct iio_dev *iio_dev,
 			const struct iio_chan_spec *chan,
 			int *val, int *val2, long m)
 {
 	struct mxs_lradc *lradc = iio_priv(iio_dev);
 	int ret;
-	unsigned long mask;
-
-	if (m != IIO_CHAN_INFO_RAW)
-		return -EINVAL;
-
-	/* Check for invalid channel */
-	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
-		return -EINVAL;
-
-	/* Validate the channel if it doesn't intersect with reserved chans. */
-	bitmap_set(&mask, chan->channel, 1);
-	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
-	if (ret)
-		return -EINVAL;
-
-	/*
-	 * See if there is no buffered operation in progess. If there is, simply
-	 * bail out. This can be improved to support both buffered and raw IO at
-	 * the same time, yet the code becomes horribly complicated. Therefore I
-	 * applied KISS principle here.
-	 */
-	ret = mutex_trylock(&lradc->lock);
-	if (!ret)
-		return -EBUSY;
 
 	INIT_COMPLETION(lradc->completion);
 
@@ -297,6 +316,47 @@ err:
 	writel(LRADC_CTRL1_LRADC_IRQ_EN(0),
 		lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
 
+	return ret;
+}
+
+/*
+ * Raw I/O operations
+ */
+static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct mxs_lradc *lradc = iio_priv(iio_dev);
+	int ret;
+
+	/*
+	 * See if there is no buffered operation in progress. If there is, simply
+	 * bail out. This can be improved to support both buffered and raw IO at
+	 * the same time, yet the code becomes horribly complicated. Therefore I
+	 * applied KISS principle here.
+	 */
+	ret = mutex_trylock(&lradc->lock);
+	if (!ret)
+		return -EBUSY;
+
+	/* Check for invalid channel */
+	if (chan->channel > LRADC_MAX_TOTAL_CHANS)
+		ret = -EINVAL;
+
+	switch(m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = mxs_lradc_read_single(iio_dev, chan, val, val2, m);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		*val = lradc->vref_mv[chan->channel];
+		*val2 = chan->scan_type.realbits;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
 	mutex_unlock(&lradc->lock);
 
 	return ret;
@@ -818,7 +878,8 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
 	.type = (chan_type),					\
 	.indexed = 1,						\
 	.scan_index = (idx),					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			      BIT(IIO_CHAN_INFO_SCALE),		\
 	.channel = (idx),					\
 	.scan_type = {						\
 		.sign = 'u',					\
@@ -957,6 +1018,9 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 			goto err_addr;
 	}
 
+	/* Set pointer to Vref array */
+	lradc->vref_mv = of_cfg->vref_mv;
+
 	platform_set_drvdata(pdev, iio);
 
 	init_completion(&lradc->completion);

[PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Hector Palacios]

Adds in_voltageX_scale_available file for every channel to read
the different available scales.
There are two scales per channel:
 [0] = divider_by_two disabled (default)
 [1] = divider_by_two enabled

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 115 +++++++++++++++++++++++++++++++++++-
 1 file changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 012c42e..7f98c99 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -37,6 +37,7 @@
 #include <linux/input.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
@@ -186,6 +187,7 @@ struct mxs_lradc {
 	struct completion	completion;
 
 	const int		*vref_mv;
+	unsigned int		scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
 
 	/*
 	 * Touchscreen LRADC channels receives a private slot in the CTRL4
@@ -362,9 +364,99 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 	return ret;
 }
 
+static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
+		struct device_attribute *attr,
+		char *buf,
+		int ch)
+{
+	struct iio_dev *iio = dev_to_iio_dev(dev);
+	struct mxs_lradc *lradc = iio_priv(iio);
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
+		len += sprintf(buf + len, "%d.%09u ",
+			       lradc->scale_avail[ch][i][0],
+			       lradc->scale_avail[ch][i][1]);
+
+	len += sprintf(buf + len, "\n");
+
+	return len;
+}
+
+#define SHOW_SCALE_AVAILABLE_FUNC(ch) 					\
+static ssize_t mxs_lradc_show_scale_available##ch(struct device *dev,	\
+		struct device_attribute *attr, 				\
+		char *buf)						\
+{ 									\
+	return mxs_lradc_show_scale_available_ch(dev, attr, buf, ch);	\
+}
+
+SHOW_SCALE_AVAILABLE_FUNC(0)
+SHOW_SCALE_AVAILABLE_FUNC(1)
+SHOW_SCALE_AVAILABLE_FUNC(2)
+SHOW_SCALE_AVAILABLE_FUNC(3)
+SHOW_SCALE_AVAILABLE_FUNC(4)
+SHOW_SCALE_AVAILABLE_FUNC(5)
+SHOW_SCALE_AVAILABLE_FUNC(6)
+SHOW_SCALE_AVAILABLE_FUNC(7)
+SHOW_SCALE_AVAILABLE_FUNC(8)
+SHOW_SCALE_AVAILABLE_FUNC(9)
+SHOW_SCALE_AVAILABLE_FUNC(10)
+SHOW_SCALE_AVAILABLE_FUNC(11)
+SHOW_SCALE_AVAILABLE_FUNC(12)
+SHOW_SCALE_AVAILABLE_FUNC(13)
+SHOW_SCALE_AVAILABLE_FUNC(14)
+SHOW_SCALE_AVAILABLE_FUNC(15)
+
+#define SHOW_SCALE_AVAILABLE_ATTR(ch)					\
+static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO,	\
+		       mxs_lradc_show_scale_available##ch, NULL, 0)
+
+SHOW_SCALE_AVAILABLE_ATTR(0);
+SHOW_SCALE_AVAILABLE_ATTR(1);
+SHOW_SCALE_AVAILABLE_ATTR(2);
+SHOW_SCALE_AVAILABLE_ATTR(3);
+SHOW_SCALE_AVAILABLE_ATTR(4);
+SHOW_SCALE_AVAILABLE_ATTR(5);
+SHOW_SCALE_AVAILABLE_ATTR(6);
+SHOW_SCALE_AVAILABLE_ATTR(7);
+SHOW_SCALE_AVAILABLE_ATTR(8);
+SHOW_SCALE_AVAILABLE_ATTR(9);
+SHOW_SCALE_AVAILABLE_ATTR(10);
+SHOW_SCALE_AVAILABLE_ATTR(11);
+SHOW_SCALE_AVAILABLE_ATTR(12);
+SHOW_SCALE_AVAILABLE_ATTR(13);
+SHOW_SCALE_AVAILABLE_ATTR(14);
+SHOW_SCALE_AVAILABLE_ATTR(15);
+
+static struct attribute *mxs_lradc_attributes[] = {
+	&iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group mxs_lradc_attribute_group = {
+	.attrs = mxs_lradc_attributes,
+};
+
 static const struct iio_info mxs_lradc_iio_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= mxs_lradc_read_raw,
+	.attrs			= &mxs_lradc_attribute_group,
 };
 
 /*
@@ -968,7 +1060,8 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 	struct resource *iores;
 	uint32_t ts_wires = 0;
 	int ret = 0;
-	int i;
+	int i, s;
+	unsigned int scale_uv;
 
 	/* Allocate the IIO device. */
 	iio = iio_device_alloc(sizeof(*lradc));
@@ -1043,6 +1136,26 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_trig;
 
+	/* Populate available ADC input ranges */
+	for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
+		for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
+			/*
+			 * [0] = optional divider by two disabled (default)
+			 * [1] = optional divider by two enabled
+			 *
+			 * The scale is calculated by doing:
+			 *   Vref >> (realbits - s)
+			 * which multiplies by two on the second component
+			 * of the array.
+			 */
+			scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
+				   (iio->channels[i].scan_type.realbits - s);
+			lradc->scale_avail[i][s][1] = do_div(scale_uv,
+							     100000000) * 10;
+			lradc->scale_avail[i][s][0] = scale_uv;
+		}
+	}
+
 	/* Configure the hardware. */
 	mxs_lradc_hw_init(lradc);
 

[PATCH 4/4] iio: mxs-lradc: add write_raw function to modify scale

[Hector Palacios]

Added write_raw function to manipulate the optional divider_by_two
through the scaling attribute out of the available scales.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 drivers/staging/iio/adc/mxs-lradc.c | 55 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 7f98c99..acd05d6 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -188,6 +188,7 @@ struct mxs_lradc {
 
 	const int		*vref_mv;
 	unsigned int		scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
+	bool			is_divided[LRADC_MAX_TOTAL_CHANS];
 
 	/*
 	 * Touchscreen LRADC channels receives a private slot in the CTRL4
@@ -246,6 +247,7 @@ struct mxs_lradc {
 #define	LRADC_CTRL1_LRADC_IRQ_OFFSET		0
 
 #define	LRADC_CTRL2				0x20
+#define LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
 #define	LRADC_CTRL2_TEMPSENSE_PWD		(1 << 15)
 
 #define	LRADC_STATUS				0x40
@@ -351,7 +353,8 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		*val = lradc->vref_mv[chan->channel];
-		*val2 = chan->scan_type.realbits;
+		*val2 = chan->scan_type.realbits -
+			lradc->is_divided[chan->channel];
 		ret = IIO_VAL_FRACTIONAL_LOG2;
 		break;
 	default:
@@ -364,6 +367,54 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
 	return ret;
 }
 
+static int mxs_lradc_write_raw(struct iio_dev *iio_dev,
+			       const struct iio_chan_spec *chan,
+			       int val, int val2, long m)
+{
+	struct mxs_lradc *lradc = iio_priv(iio_dev);
+	int ret;
+
+	ret = mutex_trylock(&lradc->lock);
+	if (!ret)
+		return -EBUSY;
+
+	switch(m) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = -EINVAL;
+		if (val == lradc->scale_avail[chan->channel][0][0] &&
+		    val2 == lradc->scale_avail[chan->channel][0][1]) {
+			/* [0] -> divider by two disabled */
+			writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
+			       lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_CLR);
+			lradc->is_divided[chan->channel] = 0;
+			ret = 0;
+		} else if (val == lradc->scale_avail[chan->channel][1][0] &&
+			   val2 == lradc->scale_avail[chan->channel][1][1]) {
+			/* [1] -> divider by two enabled */
+			writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
+			       lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_SET);
+			lradc->is_divided[chan->channel] = 1;
+			ret = 0;
+		}
+
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&lradc->lock);
+
+	return ret;
+}
+
+static int mxs_lradc_write_raw_get_fmt(struct iio_dev *iio_dev,
+				       const struct iio_chan_spec *chan,
+				       long m)
+{
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
 static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
 		struct device_attribute *attr,
 		char *buf,
@@ -456,6 +507,8 @@ static const struct attribute_group mxs_lradc_attribute_group = {
 static const struct iio_info mxs_lradc_iio_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= mxs_lradc_read_raw,
+	.write_raw		= mxs_lradc_write_raw,
+	.write_raw_get_fmt 	= &mxs_lradc_write_raw_get_fmt,
 	.attrs			= &mxs_lradc_attribute_group,
 };
 

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Lars-Peter Clausen]

On 07/05/2013 10:30 AM, Hector Palacios wrote:
> Some LRADC channels have a fixed pre-divider, and all can have enabled
> an optional divisor by two which allows a maximum input voltage of
> VDDIO - 50mV.
> 
> This patch
>  - adds the scaling info flag to all channels
>  - adds a lookup table with max reference voltage per channel
>    (where the fixed pre-dividers apply)
>  - allows to read the scaling attribute (computed from the Vref)
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>

Looks good, one minor issue.

[...]
> -
> -	/* Validate the channel if it doesn't intersect with reserved chans. */
> -	bitmap_set(&mask, chan->channel, 1);
> -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> -	if (ret)
> -		return -EINVAL;

This will conflict with Marek's "iio: mxs-lradc: Remove useless check in
read_raw", can you apply Marek's patch to your tree and rebase this series
on-top of it?

> +
> +/*
> + * Raw I/O operations
> + */
> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> +			const struct iio_chan_spec *chan,
> +			int *val, int *val2, long m)
> +{
> +	struct mxs_lradc *lradc = iio_priv(iio_dev);
> +	int ret;
> +
> +	/*
> +	 * See if there is no buffered operation in progress. If there is, simply
> +	 * bail out. This can be improved to support both buffered and raw IO at
> +	 * the same time, yet the code becomes horribly complicated. Therefore I
> +	 * applied KISS principle here.
> +	 */
> +	ret = mutex_trylock(&lradc->lock);
> +	if (!ret)
> +		return -EBUSY;

I think the locking in this driver needs some re-work (in a separate patch).
There is really no reason why you shouldn't be able to query the scale while
the device is running in buffered mode. The common idiom to protect against
concurrent buffer mode and raw access is to take the indio_dev->mlock in
read_raw, check iio_buffer_enabled(), if it returns true, unlock the lock
and return -EBUSY. Otherwise continue to read the raw value.

> +
> +	/* Check for invalid channel */
> +	if (chan->channel > LRADC_MAX_TOTAL_CHANS
> +		ret = -EINVAL;

This looks wrong. The code will still continue to read and value or the
scale and overwrite 'ret'. The original code did check this condition before
taking the lock and did return an error. But on the other hand the condition
will never be true anyway since you have no channels where the chan value is
larger than MAX_TOTAL_CHANS, so maybe just drop this completely.

> +
> +	switch(m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = mxs_lradc_read_single(iio_dev, chan, val, val2, m);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = lradc->vref_mv[chan->channel];
> +		*val2 = chan->scan_type.realbits;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
>  	mutex_unlock(&lradc->lock);
>  
>  	return ret;

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Lars-Peter Clausen]

On 07/05/2013 10:30 AM, Hector Palacios wrote:
[...]
> +#define SHOW_SCALE_AVAILABLE_FUNC(ch) 					\
> +static ssize_t mxs_lradc_show_scale_available##ch(struct device *dev,	\
> +		struct device_attribute *attr, 				\
> +		char *buf)						\
> +{ 									\
> +	return c ch);	\
> +}

No need for a separate function for each channel. Use the address attribute
of the iio_dev_attr. E.g.

struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);

return mxs_lradc_show_scale_available_ch(dev, attr, buf, iio_attr->address);

The last parameter to IIO_DEVICE_ATTR initializes this field.

But I think you can need less boilerplate code if you use the
iio_chan_spec_ext_info feature. Take a look at IIO_ENUM_AVAILABLE and
friends to see how it is done.

- Lars

Re: [PATCH 4/4] iio: mxs-lradc: add write_raw function to modify scale

[Lars-Peter Clausen]

On 07/05/2013 10:30 AM, Hector Palacios wrote:
> Added write_raw function to manipulate the optional divider_by_two
> through the scaling attribute out of the available scales.
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 55 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 7f98c99..acd05d6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -188,6 +188,7 @@ struct mxs_lradc {
>  
>  	const int		*vref_mv;
>  	unsigned int		scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
> +	bool			is_divided[LRADC_MAX_TOTAL_CHANS];

unsigned int, you do math with it and initialize it to either 0 or 1.
Otherwise the patch looks good.

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Marek Vasut]

Dear Hector Palacios,

> The LRADC virtual channels have an 18 bit field to store the sum of up
> to 2^5 accumulated samples. The read_raw function however only operates
> over a single sample (12 bit resolution).
> In order to use this field for scaling operations, we need it to be the
> exact resolution value of the LRADC.

How would this work once the accumulation is supported?

> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index 163c638..d65a594 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -822,7 +822,7 @@ static const struct iio_buffer_setup_ops
> mxs_lradc_buffer_ops = { .channel = (idx),					
\
>  	.scan_type = {						\
>  		.sign = 'u',					\
> -		.realbits = 18,					\
> +		.realbits = 12,					\
>  		.storagebits = 32,				\
>  	},							\
>  }

Best regards,
Marek Vasut

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Marek Vasut]

Dear Hector Palacios,

> Some LRADC channels have a fixed pre-divider, and all can have enabled
> an optional divisor by two which allows a maximum input voltage of
> VDDIO - 50mV.
> 
> This patch
>  - adds the scaling info flag to all channels
>  - adds a lookup table with max reference voltage per channel
>    (where the fixed pre-dividers apply)
>  - allows to read the scaling attribute (computed from the Vref)
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 122
> +++++++++++++++++++++++++++--------- 1 file changed, 93 insertions(+), 29
> deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index d65a594..012c42e 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -107,19 +107,63 @@ static const char * const mx28_lradc_irq_names[] = {
>  	"mxs-lradc-button1",
>  };
> 
> +/*
> + * Max reference voltage (mV) for each channel
> + */
> +static const int mx23_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
> +	1850,		/* CH0 */
> +	1850,		/* CH1 */
> +	1850,		/* CH2 */
> +	1850,		/* CH3 */
> +	1850,		/* CH4 */
> +	1850,		/* CH5 */
> +	1850 * 2,	/* CH6 VDDIO */
> +	1850 * 4,	/* CH7 VBATT */
> +	1850,		/* CH8 Temp sense 0 */
> +	1850,		/* CH9 Temp sense 1 */
> +	1850,		/* CH10 */
> +	1850,		/* CH11 */
> +	1850,		/* CH12 USB_DP */
> +	1850,		/* CH13 USB_DN */
> +	1850,		/* CH14 VBG */
> +	1850 * 4,	/* CH15 VDD5V */
> +};
> +
> +static const int mx28_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
> +	1850,		/* CH0 */
> +	1850,		/* CH1 */
> +	1850,		/* CH2 */
> +	1850,		/* CH3 */
> +	1850,		/* CH4 */
> +	1850,		/* CH5 */
> +	1850,		/* CH6 */
> +	1850 * 4,	/* CH7 VBATT */
> +	1850,		/* CH8 Temp sense 0 */
> +	1850,		/* CH9 Temp sense 1 */
> +	1850 * 2,	/* CH10 VDDIO */
> +	1850,		/* CH11 VTH */
> +	1850 * 2,	/* CH12 VDDA */
> +	1850,		/* CH13 VDDD */
> +	1850,		/* CH14 VBG */
> +	1850 * 4,	/* CH15 VDD5V */
> +};

Should the above not be in DT ?

>  struct mxs_lradc_of_config {
>  	const int		irq_count;
>  	const char * const	*irq_name;
> +	const int 		*vref_mv;
>  };
> 
>  static const struct mxs_lradc_of_config mxs_lradc_of_config[] = {
>  	[IMX23_LRADC] = {
>  		.irq_count	= ARRAY_SIZE(mx23_lradc_irq_names),
>  		.irq_name	= mx23_lradc_irq_names,

Idea for future patch, maybe we should remove these "irq names" here and just 
call the IRQs "lradc-irq-n"...

[...]

Best regards,
Marek Vasut

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Marek Vasut]

Dear Hector Palacios,

> Adds in_voltageX_scale_available file for every channel to read
> the different available scales.
> There are two scales per channel:
>  [0] = divider_by_two disabled (default)
>  [1] = divider_by_two enabled
> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 115
> +++++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 1
> deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index 012c42e..7f98c99 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -37,6 +37,7 @@
>  #include <linux/input.h>
> 
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
> @@ -186,6 +187,7 @@ struct mxs_lradc {
>  	struct completion	completion;
> 
>  	const int		*vref_mv;
> +	unsigned int		scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
> 
>  	/*
>  	 * Touchscreen LRADC channels receives a private slot in the CTRL4
> @@ -362,9 +364,99 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>  	return ret;
>  }
> 
> +static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf,
> +		int ch)
> +{
> +	struct iio_dev *iio = dev_to_iio_dev(dev);
> +	struct mxs_lradc *lradc = iio_priv(iio);
> +	int i, len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
> +		len += sprintf(buf + len, "%d.%09u ",
> +			       lradc->scale_avail[ch][i][0],
> +			       lradc->scale_avail[ch][i][1]);
> +
> +	len += sprintf(buf + len, "\n");
> +
> +	return len;
> +}
> +
> +#define SHOW_SCALE_AVAILABLE_FUNC(ch) 					
\
> +static ssize_t mxs_lradc_show_scale_available##ch(struct device *dev,	
\
> +		struct device_attribute *attr, 				\
> +		char *buf)						\
> +{ 									\
> +	return mxs_lradc_show_scale_available_ch(dev, attr, buf, ch);	\
> +}
> +
> +SHOW_SCALE_AVAILABLE_FUNC(0)
> +SHOW_SCALE_AVAILABLE_FUNC(1)
> +SHOW_SCALE_AVAILABLE_FUNC(2)
> +SHOW_SCALE_AVAILABLE_FUNC(3)
> +SHOW_SCALE_AVAILABLE_FUNC(4)
> +SHOW_SCALE_AVAILABLE_FUNC(5)
> +SHOW_SCALE_AVAILABLE_FUNC(6)
> +SHOW_SCALE_AVAILABLE_FUNC(7)
> +SHOW_SCALE_AVAILABLE_FUNC(8)
> +SHOW_SCALE_AVAILABLE_FUNC(9)
> +SHOW_SCALE_AVAILABLE_FUNC(10)
> +SHOW_SCALE_AVAILABLE_FUNC(11)
> +SHOW_SCALE_AVAILABLE_FUNC(12)
> +SHOW_SCALE_AVAILABLE_FUNC(13)
> +SHOW_SCALE_AVAILABLE_FUNC(14)
> +SHOW_SCALE_AVAILABLE_FUNC(15)
> +
> +#define SHOW_SCALE_AVAILABLE_ATTR(ch)					
\
> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO,	\
> +		       mxs_lradc_show_scale_available##ch, NULL, 0)
> +
> +SHOW_SCALE_AVAILABLE_ATTR(0);
> +SHOW_SCALE_AVAILABLE_ATTR(1);
> +SHOW_SCALE_AVAILABLE_ATTR(2);
> +SHOW_SCALE_AVAILABLE_ATTR(3);
> +SHOW_SCALE_AVAILABLE_ATTR(4);
> +SHOW_SCALE_AVAILABLE_ATTR(5);
> +SHOW_SCALE_AVAILABLE_ATTR(6);
> +SHOW_SCALE_AVAILABLE_ATTR(7);
> +SHOW_SCALE_AVAILABLE_ATTR(8);
> +SHOW_SCALE_AVAILABLE_ATTR(9);
> +SHOW_SCALE_AVAILABLE_ATTR(10);
> +SHOW_SCALE_AVAILABLE_ATTR(11);
> +SHOW_SCALE_AVAILABLE_ATTR(12);
> +SHOW_SCALE_AVAILABLE_ATTR(13);
> +SHOW_SCALE_AVAILABLE_ATTR(14);
> +SHOW_SCALE_AVAILABLE_ATTR(15);
> +
> +static struct attribute *mxs_lradc_attributes[] = {
> +	&iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group mxs_lradc_attribute_group = {
> +	.attrs = mxs_lradc_attributes,
> +};
> +
>  static const struct iio_info mxs_lradc_iio_info = {
>  	.driver_module		= THIS_MODULE,
>  	.read_raw		= mxs_lradc_read_raw,
> +	.attrs			= &mxs_lradc_attribute_group,
>  };
> 
>  /*
> @@ -968,7 +1060,8 @@ static int mxs_lradc_probe(struct platform_device
> *pdev) struct resource *iores;
>  	uint32_t ts_wires = 0;
>  	int ret = 0;
> -	int i;
> +	int i, s;
> +	unsigned int scale_uv;
> 
>  	/* Allocate the IIO device. */
>  	iio = iio_device_alloc(sizeof(*lradc));
> @@ -1043,6 +1136,26 @@ static int mxs_lradc_probe(struct platform_device
> *pdev) if (ret)
>  		goto err_trig;
> 
> +	/* Populate available ADC input ranges */
> +	for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
> +		for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
> +			/*
> +			 * [0] = optional divider by two disabled (default)
> +			 * [1] = optional divider by two enabled
> +			 *
> +			 * The scale is calculated by doing:
> +			 *   Vref >> (realbits - s)
> +			 * which multiplies by two on the second component
> +			 * of the array.
> +			 */
> +			scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
> +				   (iio->channels[i].scan_type.realbits - s);

Given that you do have a table of values already, can this table not be computed 
at compile-time as well?

> +			lradc->scale_avail[i][s][1] = do_div(scale_uv,
> +							     100000000) * 10;
> +			lradc->scale_avail[i][s][0] = scale_uv;

Is this correct? Why is one set to "scale_uv" and the other set to scale_uv / 
100000000 ? Maybe I just don't understand what each of the fields in the array 
stand for.

> +		}
> +	}
> +
>  	/* Configure the hardware. */
>  	mxs_lradc_hw_init(lradc);

Best regards,
Marek Vasut

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Hector Palacios]

Dear Marek,

On 07/05/2013 01:37 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> The LRADC virtual channels have an 18 bit field to store the sum of up
>> to 2^5 accumulated samples. The read_raw function however only operates
>> over a single sample (12 bit resolution).
>> In order to use this field for scaling operations, we need it to be the
>> exact resolution value of the LRADC.
>
> How would this work once the accumulation is supported?

As I see it, when you read a channel the driver should give you the 12-bit value 
either of one single sample or of N samples. If you are measuring an accumulation of N 
samples, the driver should divide the sum (the 18bit value) by N before returning the 
value to userland.

>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>   drivers/staging/iio/adc/mxs-lradc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>> b/drivers/staging/iio/adc/mxs-lradc.c index 163c638..d65a594 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -822,7 +822,7 @@ static const struct iio_buffer_setup_ops
>> mxs_lradc_buffer_ops = { .channel = (idx),					
> \
>>   	.scan_type = {						\
>>   		.sign = 'u',					\
>> -		.realbits = 18,					\
>> +		.realbits = 12,					\
>>   		.storagebits = 32,				\
>>   	},							\
>>   }
>
> Best regards,
> Marek Vasut
>


Best regards,
--
Hector Palacios

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Marek Vasut]

Dear Hector Palacios,

> Dear Marek,
> 
> On 07/05/2013 01:37 PM, Marek Vasut wrote:
> > Dear Hector Palacios,
> > 
> >> The LRADC virtual channels have an 18 bit field to store the sum of up
> >> to 2^5 accumulated samples. The read_raw function however only operates
> >> over a single sample (12 bit resolution).
> >> In order to use this field for scaling operations, we need it to be the
> >> exact resolution value of the LRADC.
> > 
> > How would this work once the accumulation is supported?
> 
> As I see it, when you read a channel the driver should give you the 12-bit
> value either of one single sample or of N samples.

The hardware will always give you 18 bit value, let's call it A of N accumulated 
samples, each 12 bit long. N is in range of 1 to 32 .

The driver currently supports N = 1.

Do I understand it correctly that if we want to support N > 1, we have to do the 
division of A / N in the driver and therefore we will again report only a 12-bit 
value to the userland ?

If so,

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Hector Palacios]

On 07/05/2013 03:10 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Dear Marek,
>>
>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> The LRADC virtual channels have an 18 bit field to store the sum of up
>>>> to 2^5 accumulated samples. The read_raw function however only operates
>>>> over a single sample (12 bit resolution).
>>>> In order to use this field for scaling operations, we need it to be the
>>>> exact resolution value of the LRADC.
>>>
>>> How would this work once the accumulation is supported?
>>
>> As I see it, when you read a channel the driver should give you the 12-bit
>> value either of one single sample or of N samples.
>
> The hardware will always give you 18 bit value, let's call it A of N accumulated
> samples, each 12 bit long. N is in range of 1 to 32 .
>
> The driver currently supports N = 1.
>
> Do I understand it correctly that if we want to support N > 1, we have to do the
> division of A / N in the driver and therefore we will again report only a 12-bit
> value to the userland ?
>
> If so,
>
> Acked-by: Marek Vasut <marex@denx.de>

That's what I would expect. I mean, what is A worth for? It's just a sum, it tells 
nothing. The value that really carries information is A / N, which is the average value.

@Lars: is there any driver that allows to read N samples? Does the IIO subsystem 
supply such interface (i.e. a file called n_samples that you can program from userland 
to trigger a read of that N samples in order to get the average value when you read 
that channel)?

Best regards,
--
Hector Palacios

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Hector Palacios]

Dear Lars,

On 07/05/2013 12:32 PM, Lars-Peter Clausen wrote:
> On 07/05/2013 10:30 AM, Hector Palacios wrote:
>> Some LRADC channels have a fixed pre-divider, and all can have enabled
>> an optional divisor by two which allows a maximum input voltage of
>> VDDIO - 50mV.
>>
>> This patch
>>   - adds the scaling info flag to all channels
>>   - adds a lookup table with max reference voltage per channel
>>     (where the fixed pre-dividers apply)
>>   - allows to read the scaling attribute (computed from the Vref)
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>
> Looks good, one minor issue.
>
> [...]
>> -
>> -	/* Validate the channel if it doesn't intersect with reserved chans. */
>> -	bitmap_set(&mask, chan->channel, 1);
>> -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
>> -	if (ret)
>> -		return -EINVAL;
>
> This will conflict with Marek's "iio: mxs-lradc: Remove useless check in
> read_raw", can you apply Marek's patch to your tree and rebase this series
> on-top of it?

Yes, sorry, I missed Marek's patch when preparing this series.

>> +
>> +/*
>> + * Raw I/O operations
>> + */
>> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>> +			const struct iio_chan_spec *chan,
>> +			int *val, int *val2, long m)
>> +{
>> +	struct mxs_lradc *lradc = iio_priv(iio_dev);
>> +	int ret;
>> +
>> +	/*
>> +	 * See if there is no buffered operation in progress. If there is, simply
>> +	 * bail out. This can be improved to support both buffered and raw IO at
>> +	 * the same time, yet the code becomes horribly complicated. Therefore I
>> +	 * applied KISS principle here.
>> +	 */
>> +	ret = mutex_trylock(&lradc->lock);
>> +	if (!ret)
>> +		return -EBUSY;
>
> I think the locking in this driver needs some re-work (in a separate patch).
> There is really no reason why you shouldn't be able to query the scale while
> the device is running in buffered mode. The common idiom to protect against
> concurrent buffer mode and raw access is to take the indio_dev->mlock in
> read_raw, check iio_buffer_enabled(), if it returns true, unlock the lock
> and return -EBUSY. Otherwise continue to read the raw value.

Actually I didn't touch this code. It's the original code by Marek.
It appears as difference because I moved part of the code into a function called 
mxs_lradc_read_single() for clarity.
@Marek, will you look at this suggestion?

>> +
>> +	/* Check for invalid channel */
>> +	if (chan->channel > LRADC_MAX_TOTAL_CHANS
>> +		ret = -EINVAL;
>
> This looks wrong. The code will still continue to read and value or the
> scale and overwrite 'ret'. The original code did check this condition before
> taking the lock and did return an error. But on the other hand the condition
> will never be true anyway since you have no channels where the chan value is
> larger than MAX_TOTAL_CHANS, so maybe just drop this completely.

Same here. This is Marek's original code that was moved. I agree that the check is 
redundant.
@Marek, will you take care of it as well?

Best regards,
--
Hector Palacios

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Hector Palacios]

Dear Marek,

On 07/05/2013 01:41 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Some LRADC channels have a fixed pre-divider, and all can have enabled
>> an optional divisor by two which allows a maximum input voltage of
>> VDDIO - 50mV.
>>
>> This patch
>>   - adds the scaling info flag to all channels
>>   - adds a lookup table with max reference voltage per channel
>>     (where the fixed pre-dividers apply)
>>   - allows to read the scaling attribute (computed from the Vref)
>>
>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>> ---
>>   drivers/staging/iio/adc/mxs-lradc.c | 122
>> +++++++++++++++++++++++++++--------- 1 file changed, 93 insertions(+), 29
>> deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>> b/drivers/staging/iio/adc/mxs-lradc.c index d65a594..012c42e 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -107,19 +107,63 @@ static const char * const mx28_lradc_irq_names[] = {
>>   	"mxs-lradc-button1",
>>   };
>>
>> +/*
>> + * Max reference voltage (mV) for each channel
>> + */
>> +static const int mx23_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
>> +	1850,		/* CH0 */
>> +	1850,		/* CH1 */
>> +	1850,		/* CH2 */
>> +	1850,		/* CH3 */
>> +	1850,		/* CH4 */
>> +	1850,		/* CH5 */
>> +	1850 * 2,	/* CH6 VDDIO */
>> +	1850 * 4,	/* CH7 VBATT */
>> +	1850,		/* CH8 Temp sense 0 */
>> +	1850,		/* CH9 Temp sense 1 */
>> +	1850,		/* CH10 */
>> +	1850,		/* CH11 */
>> +	1850,		/* CH12 USB_DP */
>> +	1850,		/* CH13 USB_DN */
>> +	1850,		/* CH14 VBG */
>> +	1850 * 4,	/* CH15 VDD5V */
>> +};
>> +
>> +static const int mx28_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
>> +	1850,		/* CH0 */
>> +	1850,		/* CH1 */
>> +	1850,		/* CH2 */
>> +	1850,		/* CH3 */
>> +	1850,		/* CH4 */
>> +	1850,		/* CH5 */
>> +	1850,		/* CH6 */
>> +	1850 * 4,	/* CH7 VBATT */
>> +	1850,		/* CH8 Temp sense 0 */
>> +	1850,		/* CH9 Temp sense 1 */
>> +	1850 * 2,	/* CH10 VDDIO */
>> +	1850,		/* CH11 VTH */
>> +	1850 * 2,	/* CH12 VDDA */
>> +	1850,		/* CH13 VDDD */
>> +	1850,		/* CH14 VBG */
>> +	1850 * 4,	/* CH15 VDD5V */
>> +};
>
> Should the above not be in DT ?

Do you mean for example:

	lradc@80050000 {
		compatible = "fsl,imx28-lradc";
		reg = <0x80050000 0x2000>;
		interrupts = <10 14 15 16 17 18 19
				20 21 22 23 24 25>;
		vref = <1850 1850 1850 1850
			1850 1850 1850 7400
			1850 1850 3700 1850
			3700 1850 1850 7400>
	};

I'm ok with it, but are there other examples of driver using a similar approach?
I see for example the spear-adc which reads:

	Optional properties:
	- vref-external: External voltage reference in milli-volts. If omitted
	  the internal voltage reference will be used.

which makes me believe the 'internal voltage reference' is hardcoded in the driver.
More opinions?

Best regards,
--
Hector Palacios

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Marek Vasut]

Hi Hector,

> Dear Lars,
> 
> On 07/05/2013 12:32 PM, Lars-Peter Clausen wrote:
> > On 07/05/2013 10:30 AM, Hector Palacios wrote:
> >> Some LRADC channels have a fixed pre-divider, and all can have enabled
> >> an optional divisor by two which allows a maximum input voltage of
> >> VDDIO - 50mV.
> >> 
> >> This patch
> >> 
> >>   - adds the scaling info flag to all channels
> >>   - adds a lookup table with max reference voltage per channel
> >>   
> >>     (where the fixed pre-dividers apply)
> >>   
> >>   - allows to read the scaling attribute (computed from the Vref)
> >> 
> >> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> > 
> > Looks good, one minor issue.
> > 
> > [...]
> > 
> >> -
> >> -	/* Validate the channel if it doesn't intersect with reserved chans.
> >> */ -	bitmap_set(&mask, chan->channel, 1);
> >> -	ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> >> -	if (ret)
> >> -		return -EINVAL;
> > 
> > This will conflict with Marek's "iio: mxs-lradc: Remove useless check in
> > read_raw", can you apply Marek's patch to your tree and rebase this
> > series on-top of it?
> 
> Yes, sorry, I missed Marek's patch when preparing this series.
> 
> >> +
> >> +/*
> >> + * Raw I/O operations
> >> + */
> >> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
> >> +			const struct iio_chan_spec *chan,
> >> +			int *val, int *val2, long m)
> >> +{
> >> +	struct mxs_lradc *lradc = iio_priv(iio_dev);
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * See if there is no buffered operation in progress. If there is,
> >> simply +	 * bail out. This can be improved to support both buffered and
> >> raw IO at +	 * the same time, yet the code becomes horribly
> >> complicated. Therefore I +	 * applied KISS principle here.
> >> +	 */
> >> +	ret = mutex_trylock(&lradc->lock);
> >> +	if (!ret)
> >> +		return -EBUSY;
> > 
> > I think the locking in this driver needs some re-work (in a separate
> > patch). There is really no reason why you shouldn't be able to query the
> > scale while the device is running in buffered mode. The common idiom to
> > protect against concurrent buffer mode and raw access is to take the
> > indio_dev->mlock in read_raw, check iio_buffer_enabled(), if it returns
> > true, unlock the lock and return -EBUSY. Otherwise continue to read the
> > raw value.
> 
> Actually I didn't touch this code. It's the original code by Marek.
> It appears as difference because I moved part of the code into a function
> called mxs_lradc_read_single() for clarity.
> @Marek, will you look at this suggestion?

Feel free to implement it, I fully agree with the comment. I will gladly review 
it.

> >> +
> >> +	/* Check for invalid channel */
> >> +	if (chan->channel > LRADC_MAX_TOTAL_CHANS
> >> +		ret = -EINVAL;
> > 
> > This looks wrong. The code will still continue to read and value or the
> > scale and overwrite 'ret'. The original code did check this condition
> > before taking the lock and did return an error. But on the other hand
> > the condition will never be true anyway since you have no channels where
> > the chan value is larger than MAX_TOTAL_CHANS, so maybe just drop this
> > completely.
> 
> Same here. This is Marek's original code that was moved. I agree that the
> check is redundant.
> @Marek, will you take care of it as well?

The patch should be rather easy, so feel free to take care of it.

Best regards,
Marek Vasut

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Marek Vasut]

Dear Hector Palacios,

> Dear Marek,
> 
> On 07/05/2013 01:41 PM, Marek Vasut wrote:
> > Dear Hector Palacios,
> > 
> >> Some LRADC channels have a fixed pre-divider, and all can have enabled
> >> an optional divisor by two which allows a maximum input voltage of
> >> VDDIO - 50mV.
> >> 
> >> This patch
> >> 
> >>   - adds the scaling info flag to all channels
> >>   - adds a lookup table with max reference voltage per channel
> >>   
> >>     (where the fixed pre-dividers apply)
> >>   
> >>   - allows to read the scaling attribute (computed from the Vref)
> >> 
> >> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> >> ---
> >> 
> >>   drivers/staging/iio/adc/mxs-lradc.c | 122
> >> 
> >> +++++++++++++++++++++++++++--------- 1 file changed, 93 insertions(+),
> >> 29 deletions(-)
> >> 
> >> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> >> b/drivers/staging/iio/adc/mxs-lradc.c index d65a594..012c42e 100644
> >> --- a/drivers/staging/iio/adc/mxs-lradc.c
> >> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> >> @@ -107,19 +107,63 @@ static const char * const mx28_lradc_irq_names[] =
> >> {
> >> 
> >>   	"mxs-lradc-button1",
> >>   
> >>   };
> >> 
> >> +/*
> >> + * Max reference voltage (mV) for each channel
> >> + */
> >> +static const int mx23_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
> >> +	1850,		/* CH0 */
> >> +	1850,		/* CH1 */
> >> +	1850,		/* CH2 */
> >> +	1850,		/* CH3 */
> >> +	1850,		/* CH4 */
> >> +	1850,		/* CH5 */
> >> +	1850 * 2,	/* CH6 VDDIO */
> >> +	1850 * 4,	/* CH7 VBATT */
> >> +	1850,		/* CH8 Temp sense 0 */
> >> +	1850,		/* CH9 Temp sense 1 */
> >> +	1850,		/* CH10 */
> >> +	1850,		/* CH11 */
> >> +	1850,		/* CH12 USB_DP */
> >> +	1850,		/* CH13 USB_DN */
> >> +	1850,		/* CH14 VBG */
> >> +	1850 * 4,	/* CH15 VDD5V */
> >> +};
> >> +
> >> +static const int mx28_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
> >> +	1850,		/* CH0 */
> >> +	1850,		/* CH1 */
> >> +	1850,		/* CH2 */
> >> +	1850,		/* CH3 */
> >> +	1850,		/* CH4 */
> >> +	1850,		/* CH5 */
> >> +	1850,		/* CH6 */
> >> +	1850 * 4,	/* CH7 VBATT */
> >> +	1850,		/* CH8 Temp sense 0 */
> >> +	1850,		/* CH9 Temp sense 1 */
> >> +	1850 * 2,	/* CH10 VDDIO */
> >> +	1850,		/* CH11 VTH */
> >> +	1850 * 2,	/* CH12 VDDA */
> >> +	1850,		/* CH13 VDDD */
> >> +	1850,		/* CH14 VBG */
> >> +	1850 * 4,	/* CH15 VDD5V */
> >> +};
> > 
> > Should the above not be in DT ?
> 
> Do you mean for example:
> 
> 	lradc@80050000 {
> 		compatible = "fsl,imx28-lradc";
> 		reg = <0x80050000 0x2000>;
> 		interrupts = <10 14 15 16 17 18 19
> 				20 21 22 23 24 25>;
> 		vref = <1850 1850 1850 1850
> 			1850 1850 1850 7400
> 			1850 1850 3700 1850
> 			3700 1850 1850 7400>
> 	};
> 
> I'm ok with it, but are there other examples of driver using a similar
> approach? I see for example the spear-adc which reads:
> 
> 	Optional properties:
> 	- vref-external: External voltage reference in milli-volts. If omitted
> 	  the internal voltage reference will be used.
> 
> which makes me believe the 'internal voltage reference' is hardcoded in the
> driver. More opinions?

I'd duke this one out on devicetree-discuss@ list. In general, I'd use something 
like fsl,vref prop, but this really better be discussed in a separate thread.

On the other hand, I'd hate to keep these patches here waiting , so maybe we 
should apply them as-is and remove this part from the driver in a subsequent 
patch? What do you say, Lars ?

Best regards,
Marek Vasut

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Lars-Peter Clausen]

On 07/05/2013 06:59 PM, Marek Vasut wrote:
[...]
>>> Should the above not be in DT ?
>>
>> Do you mean for example:
>>
>> 	lradc@80050000 {
>> 		compatible = "fsl,imx28-lradc";
>> 		reg = <0x80050000 0x2000>;
>> 		interrupts = <10 14 15 16 17 18 19
>> 				20 21 22 23 24 25>;
>> 		vref = <1850 1850 1850 1850
>> 			1850 1850 1850 7400
>> 			1850 1850 3700 1850
>> 			3700 1850 1850 7400>
>> 	};
>>
>> I'm ok with it, but are there other examples of driver using a similar
>> approach? I see for example the spear-adc which reads:
>>
>> 	Optional properties:
>> 	- vref-external: External voltage reference in milli-volts. If omitted
>> 	  the internal voltage reference will be used.
>>
>> which makes me believe the 'internal voltage reference' is hardcoded in the
>> driver. More opinions?
> 
> I'd duke this one out on devicetree-discuss@ list. In general, I'd use something 
> like fsl,vref prop, but this really better be discussed in a separate thread.
> 
> On the other hand, I'd hate to keep these patches here waiting , so maybe we 
> should apply them as-is and remove this part from the driver in a subsequent 
> patch? What do you say, Lars ?

I'd leave it as it is. I don't see much point in putting them into the
devicetree, it would have made sense if the scales were board specific.

- Lars

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Lars-Peter Clausen]

On 07/05/2013 04:35 PM, Hector Palacios wrote:
> On 07/05/2013 03:10 PM, Marek Vasut wrote:
>> Dear Hector Palacios,
>>
>>> Dear Marek,
>>>
>>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
>>>> Dear Hector Palacios,
>>>>
>>>>> The LRADC virtual channels have an 18 bit field to store the sum of up
>>>>> to 2^5 accumulated samples. The read_raw function however only operates
>>>>> over a single sample (12 bit resolution).
>>>>> In order to use this field for scaling operations, we need it to be the
>>>>> exact resolution value of the LRADC.
>>>>
>>>> How would this work once the accumulation is supported?
>>>
>>> As I see it, when you read a channel the driver should give you the 12-bit
>>> value either of one single sample or of N samples.
>>
>> The hardware will always give you 18 bit value, let's call it A of N accumulated
>> samples, each 12 bit long. N is in range of 1 to 32 .
>>
>> The driver currently supports N = 1.
>>
>> Do I understand it correctly that if we want to support N > 1, we have to do the
>> division of A / N in the driver and therefore we will again report only a 12-bit
>> value to the userland ?
>>
>> If so,
>>
>> Acked-by: Marek Vasut <marex@denx.de>
> 
> That's what I would expect. I mean, what is A worth for? It's just a sum, it
> tells nothing. The value that really carries information is A / N, which is the
> average value.
> 
> @Lars: is there any driver that allows to read N samples? Does the IIO
> subsystem supply such interface (i.e. a file called n_samples that you can
> program from userland to trigger a read of that N samples in order to get the
> average value when you read that channel)?

The ad7606 has the 'oversampling_ratio' attribute. On the other hand the ad7606
is not the best example either and this is a custom API. But well that's what
it is and since it's not the only device that supports oversampling we should
try and standardize a property name for this. The ad7606 does the averaging in
hardware though.

- Lars

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Marek Vasut]

Dear Lars-Peter Clausen,

> On 07/05/2013 06:59 PM, Marek Vasut wrote:
> [...]
> 
> >>> Should the above not be in DT ?
> >> 
> >> Do you mean for example:
> >> 	lradc@80050000 {
> >> 	
> >> 		compatible = "fsl,imx28-lradc";
> >> 		reg = <0x80050000 0x2000>;
> >> 		interrupts = <10 14 15 16 17 18 19
> >> 		
> >> 				20 21 22 23 24 25>;
> >> 		
> >> 		vref = <1850 1850 1850 1850
> >> 		
> >> 			1850 1850 1850 7400
> >> 			1850 1850 3700 1850
> >> 			3700 1850 1850 7400>
> >> 	
> >> 	};
> >> 
> >> I'm ok with it, but are there other examples of driver using a similar
> >> 
> >> approach? I see for example the spear-adc which reads:
> >> 	Optional properties:
> >> 	- vref-external: External voltage reference in milli-volts. If omitted
> >> 	
> >> 	  the internal voltage reference will be used.
> >> 
> >> which makes me believe the 'internal voltage reference' is hardcoded in
> >> the driver. More opinions?
> > 
> > I'd duke this one out on devicetree-discuss@ list. In general, I'd use
> > something like fsl,vref prop, but this really better be discussed in a
> > separate thread.
> > 
> > On the other hand, I'd hate to keep these patches here waiting , so maybe
> > we should apply them as-is and remove this part from the driver in a
> > subsequent patch? What do you say, Lars ?
> 
> I'd leave it as it is. I don't see much point in putting them into the
> devicetree, it would have made sense if the scales were board specific.

They are CPU specific, therefore if we have a new CPU with different scale setup 
(again), we will need to add another table here. That's why I'd vouch for having 
them in DT. As we already saw in the past, FSL did merge some MX23/28 components 
into MX53 and MX6Q.

Best regards,
Marek Vasut

Re: [PATCH 2/4] iio: mxs-lradc: add scale attribute to channels

[Jonathan Cameron]

On 07/05/2013 05:59 PM, Marek Vasut wrote:
> Dear Hector Palacios,
> 
>> Dear Marek,
>>
>> On 07/05/2013 01:41 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> Some LRADC channels have a fixed pre-divider, and all can have enabled
>>>> an optional divisor by two which allows a maximum input voltage of
>>>> VDDIO - 50mV.
>>>>
>>>> This patch
>>>>
>>>>   - adds the scaling info flag to all channels
>>>>   - adds a lookup table with max reference voltage per channel
>>>>   
>>>>     (where the fixed pre-dividers apply)
>>>>   
>>>>   - allows to read the scaling attribute (computed from the Vref)
>>>>
>>>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>>>> ---
>>>>
>>>>   drivers/staging/iio/adc/mxs-lradc.c | 122
>>>>
>>>> +++++++++++++++++++++++++++--------- 1 file changed, 93 insertions(+),
>>>> 29 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>>>> b/drivers/staging/iio/adc/mxs-lradc.c index d65a594..012c42e 100644
>>>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>>>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>>>> @@ -107,19 +107,63 @@ static const char * const mx28_lradc_irq_names[] =
>>>> {
>>>>
>>>>   	"mxs-lradc-button1",
>>>>   
>>>>   };
>>>>
>>>> +/*
>>>> + * Max reference voltage (mV) for each channel
>>>> + */
>>>> +static const int mx23_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
>>>> +	1850,		/* CH0 */
>>>> +	1850,		/* CH1 */
>>>> +	1850,		/* CH2 */
>>>> +	1850,		/* CH3 */
>>>> +	1850,		/* CH4 */
>>>> +	1850,		/* CH5 */
>>>> +	1850 * 2,	/* CH6 VDDIO */
>>>> +	1850 * 4,	/* CH7 VBATT */
>>>> +	1850,		/* CH8 Temp sense 0 */
>>>> +	1850,		/* CH9 Temp sense 1 */
>>>> +	1850,		/* CH10 */
>>>> +	1850,		/* CH11 */
>>>> +	1850,		/* CH12 USB_DP */
>>>> +	1850,		/* CH13 USB_DN */
>>>> +	1850,		/* CH14 VBG */
>>>> +	1850 * 4,	/* CH15 VDD5V */
>>>> +};
>>>> +
>>>> +static const int mx28_vref_mv[LRADC_MAX_TOTAL_CHANS] = {
>>>> +	1850,		/* CH0 */
>>>> +	1850,		/* CH1 */
>>>> +	1850,		/* CH2 */
>>>> +	1850,		/* CH3 */
>>>> +	1850,		/* CH4 */
>>>> +	1850,		/* CH5 */
>>>> +	1850,		/* CH6 */
>>>> +	1850 * 4,	/* CH7 VBATT */
>>>> +	1850,		/* CH8 Temp sense 0 */
>>>> +	1850,		/* CH9 Temp sense 1 */
>>>> +	1850 * 2,	/* CH10 VDDIO */
>>>> +	1850,		/* CH11 VTH */
>>>> +	1850 * 2,	/* CH12 VDDA */
>>>> +	1850,		/* CH13 VDDD */
>>>> +	1850,		/* CH14 VBG */
>>>> +	1850 * 4,	/* CH15 VDD5V */
>>>> +};
>>>
>>> Should the above not be in DT ?
>>
>> Do you mean for example:
>>
>> 	lradc@80050000 {
>> 		compatible = "fsl,imx28-lradc";
>> 		reg = <0x80050000 0x2000>;
>> 		interrupts = <10 14 15 16 17 18 19
>> 				20 21 22 23 24 25>;
>> 		vref = <1850 1850 1850 1850
>> 			1850 1850 1850 7400
>> 			1850 1850 3700 1850
>> 			3700 1850 1850 7400>
>> 	};
>>
>> I'm ok with it, but are there other examples of driver using a similar
>> approach? I see for example the spear-adc which reads:
>>
>> 	Optional properties:
>> 	- vref-external: External voltage reference in milli-volts. If omitted
>> 	  the internal voltage reference will be used.
>>
>> which makes me believe the 'internal voltage reference' is hardcoded in the
>> driver. More opinions?
> 
> I'd duke this one out on devicetree-discuss@ list. In general, I'd use something 
> like fsl,vref prop, but this really better be discussed in a separate thread.
Given timing wrt the merge window we have a couple of months to pin this down
without effecting when it hits mainline so I would go ahead and open a discussion
on devicetree-discuss.
> 
> On the other hand, I'd hate to keep these patches here waiting , so maybe we 
> should apply them as-is and remove this part from the driver in a subsequent 
> patch? What do you say, Lars ?
Could do so but as I said, no particular rush unless people have lots of stuff
they want to do on top of these?

> 
> Best regards,
> Marek Vasut
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Jonathan Cameron]

On 07/05/2013 06:17 PM, Lars-Peter Clausen wrote:
> On 07/05/2013 04:35 PM, Hector Palacios wrote:
>> On 07/05/2013 03:10 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> Dear Marek,
>>>>
>>>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
>>>>> Dear Hector Palacios,
>>>>>
>>>>>> The LRADC virtual channels have an 18 bit field to store the sum of up
>>>>>> to 2^5 accumulated samples. The read_raw function however only operates
>>>>>> over a single sample (12 bit resolution).
>>>>>> In order to use this field for scaling operations, we need it to be the
>>>>>> exact resolution value of the LRADC.
>>>>>
>>>>> How would this work once the accumulation is supported?
>>>>
>>>> As I see it, when you read a channel the driver should give you the 12-bit
>>>> value either of one single sample or of N samples.
>>>
>>> The hardware will always give you 18 bit value, let's call it A of N accumulated
>>> samples, each 12 bit long. N is in range of 1 to 32 .
>>>
>>> The driver currently supports N = 1.
>>>
>>> Do I understand it correctly that if we want to support N > 1, we have to do the
>>> division of A / N in the driver and therefore we will again report only a 12-bit
>>> value to the userland ?
>>>
>>> If so,
>>>
>>> Acked-by: Marek Vasut <marex@denx.de>
>>
>> That's what I would expect. I mean, what is A worth for? It's just a sum, it
>> tells nothing. The value that really carries information is A / N, which is the
>> average value.
>>
>> @Lars: is there any driver that allows to read N samples? Does the IIO
>> subsystem supply such interface (i.e. a file called n_samples that you can
>> program from userland to trigger a read of that N samples in order to get the
>> average value when you read that channel)?
> 
> The ad7606 has the 'oversampling_ratio' attribute. On the other hand the ad7606
> is not the best example either and this is a custom API. But well that's what
> it is and since it's not the only device that supports oversampling we should
> try and standardize a property name for this. The ad7606 does the averaging in
> hardware though.

There is some 'filtering' abi defined and arguably this is just a mean filter with
a particular width window, perhaps treating it like that is the cleanest abi wise.
The intent was always to extend this filtering ABI to describe common filter types
but it hasn't happened yet :)  (that oversampling_ratio is horrible and doesn't
generalize nicely at all).

Propose an ABI addition for what you need....
Just to start things off, my gut feeling would be somethign along the lines of

in_voltageX_filter_mean_width

with appropriate additions to info_mask as there are plenty of devices that do this
(though usually with on board division as Lars suggested - though often for
short widths they just fill the last few bits with 0's).

On this note, one of the more 'interesting' uses of the buffering infrastructure
is that you can do software implementations of simple filters to cut down on the data
flow to userspace. If only there was more time in the day ;)

Jonathan


> 
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Marek Vasut]

Hi Jonathan,

> On 07/05/2013 06:17 PM, Lars-Peter Clausen wrote:
> > On 07/05/2013 04:35 PM, Hector Palacios wrote:
> >> On 07/05/2013 03:10 PM, Marek Vasut wrote:
> >>> Dear Hector Palacios,
> >>> 
> >>>> Dear Marek,
> >>>> 
> >>>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
> >>>>> Dear Hector Palacios,
> >>>>> 
> >>>>>> The LRADC virtual channels have an 18 bit field to store the sum of
> >>>>>> up to 2^5 accumulated samples. The read_raw function however only
> >>>>>> operates over a single sample (12 bit resolution).
> >>>>>> In order to use this field for scaling operations, we need it to be
> >>>>>> the exact resolution value of the LRADC.
> >>>>> 
> >>>>> How would this work once the accumulation is supported?
> >>>> 
> >>>> As I see it, when you read a channel the driver should give you the
> >>>> 12-bit value either of one single sample or of N samples.
> >>> 
> >>> The hardware will always give you 18 bit value, let's call it A of N
> >>> accumulated samples, each 12 bit long. N is in range of 1 to 32 .
> >>> 
> >>> The driver currently supports N = 1.
> >>> 
> >>> Do I understand it correctly that if we want to support N > 1, we have
> >>> to do the division of A / N in the driver and therefore we will again
> >>> report only a 12-bit value to the userland ?
> >>> 
> >>> If so,
> >>> 
> >>> Acked-by: Marek Vasut <marex@denx.de>
> >> 
> >> That's what I would expect. I mean, what is A worth for? It's just a
> >> sum, it tells nothing. The value that really carries information is A /
> >> N, which is the average value.
> >> 
> >> @Lars: is there any driver that allows to read N samples? Does the IIO
> >> subsystem supply such interface (i.e. a file called n_samples that you
> >> can program from userland to trigger a read of that N samples in order
> >> to get the average value when you read that channel)?
> > 
> > The ad7606 has the 'oversampling_ratio' attribute. On the other hand the
> > ad7606 is not the best example either and this is a custom API. But well
> > that's what it is and since it's not the only device that supports
> > oversampling we should try and standardize a property name for this. The
> > ad7606 does the averaging in hardware though.
> 
> There is some 'filtering' abi defined and arguably this is just a mean
> filter with a particular width window, perhaps treating it like that is
> the cleanest abi wise. The intent was always to extend this filtering ABI
> to describe common filter types but it hasn't happened yet :)  (that
> oversampling_ratio is horrible and doesn't generalize nicely at all).
> 
> Propose an ABI addition for what you need....
> Just to start things off, my gut feeling would be somethign along the lines
> of
> 
> in_voltageX_filter_mean_width
> 
> with appropriate additions to info_mask as there are plenty of devices that
> do this (though usually with on board division as Lars suggested - though
> often for short widths they just fill the last few bits with 0's).
> 
> On this note, one of the more 'interesting' uses of the buffering
> infrastructure is that you can do software implementations of simple
> filters to cut down on the data flow to userspace. If only there was more
> time in the day ;)

... and a stronger coffee.

Best regards,
Marek Vasut

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Hector Palacios]

Dear Lars,

On 07/05/2013 12:40 PM, Lars-Peter Clausen wrote:
> On 07/05/2013 10:30 AM, Hector Palacios wrote:
> [...]
>> +#define SHOW_SCALE_AVAILABLE_FUNC(ch) 					\
>> +static ssize_t mxs_lradc_show_scale_available##ch(struct device *dev,	\
>> +		struct device_attribute *attr, 				\
>> +		char *buf)						\
>> +{ 									\
>> +	return c ch);	\
>> +}
>
> No need for a separate function for each channel. Use the address attribute
> of the iio_dev_attr. E.g.
>
> struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>
> return mxs_lradc_show_scale_available_ch(dev, attr, buf, iio_attr->address);
>
> The last parameter to IIO_DEVICE_ATTR initializes this field.

Nice, I knew there had to be a better way.

> But I think you can need less boilerplate code if you use the
> iio_chan_spec_ext_info feature. Take a look at IIO_ENUM_AVAILABLE and
> friends to see how it is done.

I just took a look but don't know how I can use this framework with scales, as a scale 
is an array composed of two numbers (integer and fractional parts), not simple strings 
which is what iio_enum.items expects.

Best regards,
--
Hector Palacios

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Lars-Peter Clausen]

On 07/08/2013 10:27 AM, Hector Palacios wrote:
> Dear Lars,
> 
> On 07/05/2013 12:40 PM, Lars-Peter Clausen wrote:
>> On 07/05/2013 10:30 AM, Hector Palacios wrote:
>> [...]
>>> +#define SHOW_SCALE_AVAILABLE_FUNC(ch)                     \
>>> +static ssize_t mxs_lradc_show_scale_available##ch(struct device *dev,    \
>>> +        struct device_attribute *attr,                 \
>>> +        char *buf)                        \
>>> +{                                     \
>>> +    return c ch);    \
>>> +}
>>
>> No need for a separate function for each channel. Use the address attribute
>> of the iio_dev_attr. E.g.
>>
>> struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>>
>> return mxs_lradc_show_scale_available_ch(dev, attr, buf, iio_attr->address);
>>
>> The last parameter to IIO_DEVICE_ATTR initializes this field.
> 
> Nice, I knew there had to be a better way.
> 
>> But I think you can need less boilerplate code if you use the
>> iio_chan_spec_ext_info feature. Take a look at IIO_ENUM_AVAILABLE and
>> friends to see how it is done.
> 
> I just took a look but don't know how I can use this framework with scales,
> as a scale is an array composed of two numbers (integer and fractional
> parts), not simple strings which is what iio_enum.items expects.

Yes, using iio_enum won't work in this case, but you can use it as an
inspiration for your own solution.

- Lars

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Hector Palacios]

Dear Marek,

On 07/05/2013 01:46 PM, Marek Vasut wrote:
[...]
>> +	/* Populate available ADC input ranges */
>> +	for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
>> +		for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
>> +			/*
>> +			 * [0] = optional divider by two disabled (default)
>> +			 * [1] = optional divider by two enabled
>> +			 *
>> +			 * The scale is calculated by doing:
>> +			 *   Vref >> (realbits - s)
>> +			 * which multiplies by two on the second component
>> +			 * of the array.
>> +			 */
>> +			scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
>> +				   (iio->channels[i].scan_type.realbits - s);
>
> Given that you do have a table of values already, can this table not be computed
> at compile-time as well?

Yes and no. On one hand, it would be a little redundant and ugly to have two tables 
(one computed out of the other). Considering there are two CPUs, it would force you to 
maintain four tables.
On the other hand the formula uses the 'realbits' (yeah, well it won't change, but still).
I copied the code from ad7192.c. The operation is only done during probe() so I don't 
think the time penalty is worth the effort of having another table.

>> +			lradc->scale_avail[i][s][1] = do_div(scale_uv,
>> +							     100000000) * 10;
>> +			lradc->scale_avail[i][s][0] = scale_uv;
>
> Is this correct? Why is one set to "scale_uv" and the other set to scale_uv /
> 100000000 ? Maybe I just don't understand what each of the fields in the array
> stand for.

The [0] component is the integer (volts) part of the scale. The [1] component is the 
nanoV part.
To be honest, after some unsuccessful tries of doing my own math here, I just copied 
the one from ad7192.c. It is a bit unintelligible because it is doing 64bit math 
operations, but it works. Remember do_div() modifies the first parameter (scale_uv) 
during the operation, apart from returning a value. That may have fooled you.

Best regards,
--
Hector Palacios

Re: [PATCH 3/4] iio: mxs-lradc: add scale_available file to channels

[Marek Vasut]

Dear Hector Palacios,

> Dear Marek,
> 
> On 07/05/2013 01:46 PM, Marek Vasut wrote:
> [...]
> 
> >> +	/* Populate available ADC input ranges */
> >> +	for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
> >> +		for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
> >> +			/*
> >> +			 * [0] = optional divider by two disabled (default)
> >> +			 * [1] = optional divider by two enabled
> >> +			 *
> >> +			 * The scale is calculated by doing:
> >> +			 *   Vref >> (realbits - s)
> >> +			 * which multiplies by two on the second component
> >> +			 * of the array.
> >> +			 */
> >> +			scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
> >> +				   (iio->channels[i].scan_type.realbits - s);
> > 
> > Given that you do have a table of values already, can this table not be
> > computed at compile-time as well?
> 
> Yes and no. On one hand, it would be a little redundant and ugly to have
> two tables (one computed out of the other). Considering there are two
> CPUs, it would force you to maintain four tables.
> On the other hand the formula uses the 'realbits' (yeah, well it won't
> change, but still). I copied the code from ad7192.c. The operation is only
> done during probe() so I don't think the time penalty is worth the effort
> of having another table.

Ok. I suspect we should wait how the discussion in DT-discuss turns out too.

> >> +			lradc->scale_avail[i][s][1] = do_div(scale_uv,
> >> +							     100000000) * 10;
> >> +			lradc->scale_avail[i][s][0] = scale_uv;
> > 
> > Is this correct? Why is one set to "scale_uv" and the other set to
> > scale_uv / 100000000 ? Maybe I just don't understand what each of the
> > fields in the array stand for.
> 
> The [0] component is the integer (volts) part of the scale. The [1]
> component is the nanoV part.
> To be honest, after some unsuccessful tries of doing my own math here, I
> just copied the one from ad7192.c. It is a bit unintelligible because it
> is doing 64bit math operations, but it works. Remember do_div() modifies
> the first parameter (scale_uv) during the operation, apart from returning
> a value. That may have fooled you.

I'd vouch for cleaning this up so it's readable ;-)

Best regards,
Marek Vasut

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Hector Palacios]

Hello,

On 07/05/2013 03:10 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Dear Marek,
>>
>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> The LRADC virtual channels have an 18 bit field to store the sum of up
>>>> to 2^5 accumulated samples. The read_raw function however only operates
>>>> over a single sample (12 bit resolution).
>>>> In order to use this field for scaling operations, we need it to be the
>>>> exact resolution value of the LRADC.
>>>
>>> How would this work once the accumulation is supported?
>>
>> As I see it, when you read a channel the driver should give you the 12-bit
>> value either of one single sample or of N samples.
>
> The hardware will always give you 18 bit value, let's call it A of N accumulated
> samples, each 12 bit long. N is in range of 1 to 32 .
>
> The driver currently supports N = 1.
>
> Do I understand it correctly that if we want to support N > 1, we have to do the
> division of A / N in the driver and therefore we will again report only a 12-bit
> value to the userland ?
>
> If so,
>
> Acked-by: Marek Vasut <marex@denx.de>

Coming back to this patch, I just noticed that it is not enough to just change the 
realbits from 18 to 12. When using this driver as touchscreen for Yocto's SATO graphic 
rootfs I noticed that the driver is using LRADC_CH_VALUE_MASK for reporting the 
coordinates.

	input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
	input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
	input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);

which is defined as an 18bit mask:

	#define	LRADC_CH_VALUE_MASK			0x3ffff

The result is that the touch calibration range in Xorg is expecting values between 0 
and 262143 (0x3ffff), which causes trouble (at least I had problems to calibrate it).

root@computer:~# xinput --list mxs-lradc
mxs-lradc                                       id=6    [slave  pointer  (2)]
         Reporting 4 classes:
                 Class originated from: 6. Type: XIButtonClass
                 Buttons supported: 5
                 Button labels: "Button Unknown" "Button Unknown" "Button Unknown" 
"Button Wheel Up" "Button Wheel Down"
                 Button state:
                 Class originated from: 6. Type: XIValuatorClass
                 Detail for Valuator 0:
                   Label: Abs X
                   Range: 0.000000 - 262143.000000
                   Resolution: 0 units/m
                   Mode: absolute
                   Current value: 2512.000000
                 Class originated from: 6. Type: XIValuatorClass
                 Detail for Valuator 1:
                   Label: Abs Y
                   Range: 0.000000 - 262143.000000
                   Resolution: 0 units/m
                   Mode: absolute
                   Current value: 1688.000000
                 Class originated from: 6. Type: XIValuatorClass
                 Detail for Valuator 2:
                   Label: Abs Pressure
                   Range: 0.000000 - 262143.000000
                   Resolution: 0 units/m
                   Mode: absolute
                   Current value: 0.000000

So probably we should complete this patch with the following:

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 407a124..d2a0a83 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -275,6 +275,9 @@ struct mxs_lradc {
  #define        LRADC_CTRL4_LRADCSELECT_MASK(n)         (0xf << ((n) * 4))
  #define        LRADC_CTRL4_LRADCSELECT_OFFSET(n)       ((n) * 4)

+#define LRADC_RESOLUTION                       12
+#define LRADC_SINGLE_SAMPLE_MASK               ((1 << LRADC_RESOLUTION) - 1)
+
  static int mxs_lradc_read_single(struct iio_dev *iio_dev,
                         const struct iio_chan_spec *chan,
                         int *val, int *val2, long m)
@@ -737,9 +740,10 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
         __set_bit(EV_ABS, input->evbit);
         __set_bit(EV_KEY, input->evbit);
         __set_bit(BTN_TOUCH, input->keybit);
-       input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
-       input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
-       input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);
+       input_set_abs_params(input, ABS_X, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
+       input_set_abs_params(input, ABS_Y, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
+       input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_SINGLE_SAMPLE_MASK,
+                            0, 0);

         lradc->ts_input = input;
         input_set_drvdata(input, lradc);
@@ -1014,7 +1018,7 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
         .address = (idx),                                       \
         .scan_type = {                                          \
                 .sign = 'u',                                    \
-               .realbits = 18,                                 \
+               .realbits = LRADC_RESOLUTION,                   \
                 .storagebits = 32,                              \
         },                                                      \
  }

Notice that I leave the existing LRADC_CH_VALUE_MASK 18bit mask in the rest of the 
driver, to support accumulated samples.
Curiously, ts_lib works fine without this (maybe the calibration fixes this?), but 
nevertheless the information passed by the driver is incorrect, I guess.
Is anybody out there working with the touch? Under which graphic system?

Best regards,
--
Hector Palacios

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Marek Vasut]

Hi Hector,

> Hello,
> 
> On 07/05/2013 03:10 PM, Marek Vasut wrote:
> > Dear Hector Palacios,
> > 
> >> Dear Marek,
> >> 
> >> On 07/05/2013 01:37 PM, Marek Vasut wrote:
> >>> Dear Hector Palacios,
> >>> 
> >>>> The LRADC virtual channels have an 18 bit field to store the sum of up
> >>>> to 2^5 accumulated samples. The read_raw function however only
> >>>> operates over a single sample (12 bit resolution).
> >>>> In order to use this field for scaling operations, we need it to be
> >>>> the exact resolution value of the LRADC.
> >>> 
> >>> How would this work once the accumulation is supported?
> >> 
> >> As I see it, when you read a channel the driver should give you the
> >> 12-bit value either of one single sample or of N samples.
> > 
> > The hardware will always give you 18 bit value, let's call it A of N
> > accumulated samples, each 12 bit long. N is in range of 1 to 32 .
> > 
> > The driver currently supports N = 1.
> > 
> > Do I understand it correctly that if we want to support N > 1, we have to
> > do the division of A / N in the driver and therefore we will again
> > report only a 12-bit value to the userland ?
> > 
> > If so,
> > 
> > Acked-by: Marek Vasut <marex@denx.de>
> 
> Coming back to this patch, I just noticed that it is not enough to just
> change the realbits from 18 to 12. When using this driver as touchscreen
> for Yocto's SATO graphic rootfs I noticed that the driver is using
> LRADC_CH_VALUE_MASK for reporting the coordinates.
> 
> 	input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
> 	input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
> 	input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);
> 
> which is defined as an 18bit mask:
> 
> 	#define	LRADC_CH_VALUE_MASK			0x3ffff
> 
> The result is that the touch calibration range in Xorg is expecting values
> between 0 and 262143 (0x3ffff), which causes trouble (at least I had
> problems to calibrate it).

What kind of trouble does it cause?

[...]

> Notice that I leave the existing LRADC_CH_VALUE_MASK 18bit mask in the rest
> of the driver, to support accumulated samples.
> Curiously, ts_lib works fine without this (maybe the calibration fixes
> this?), but nevertheless the information passed by the driver is
> incorrect, I guess. Is anybody out there working with the touch? Under
> which graphic system?

Both QtE and Sato work for me, that's why I'm curious about your issues.

Best regards,
Marek Vasut

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Hector Palacios]

Dear Marek,

On 07/10/2013 01:49 PM, Marek Vasut wrote:
> Hi Hector,
>
>> Hello,
>>
>> On 07/05/2013 03:10 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> Dear Marek,
>>>>
>>>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
>>>>> Dear Hector Palacios,
>>>>>
>>>>>> The LRADC virtual channels have an 18 bit field to store the sum of up
>>>>>> to 2^5 accumulated samples. The read_raw function however only
>>>>>> operates over a single sample (12 bit resolution).
>>>>>> In order to use this field for scaling operations, we need it to be
>>>>>> the exact resolution value of the LRADC.
>>>>>
>>>>> How would this work once the accumulation is supported?
>>>>
>>>> As I see it, when you read a channel the driver should give you the
>>>> 12-bit value either of one single sample or of N samples.
>>>
>>> The hardware will always give you 18 bit value, let's call it A of N
>>> accumulated samples, each 12 bit long. N is in range of 1 to 32 .
>>>
>>> The driver currently supports N = 1.
>>>
>>> Do I understand it correctly that if we want to support N > 1, we have to
>>> do the division of A / N in the driver and therefore we will again
>>> report only a 12-bit value to the userland ?
>>>
>>> If so,
>>>
>>> Acked-by: Marek Vasut <marex@denx.de>
>>
>> Coming back to this patch, I just noticed that it is not enough to just
>> change the realbits from 18 to 12. When using this driver as touchscreen
>> for Yocto's SATO graphic rootfs I noticed that the driver is using
>> LRADC_CH_VALUE_MASK for reporting the coordinates.
>>
>> 	input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
>> 	input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
>> 	input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);
>>
>> which is defined as an 18bit mask:
>>
>> 	#define	LRADC_CH_VALUE_MASK			0x3ffff
>>
>> The result is that the touch calibration range in Xorg is expecting values
>> between 0 and 262143 (0x3ffff), which causes trouble (at least I had
>> problems to calibrate it).
>
> What kind of trouble does it cause?

Sorry, I should have said.
First of all, I'm using a resistive touch that needs calibration.

There something I definitely don't understand about calibrating the touch in Sato.
Initially I don't have any file /etc/pointercal.xinput. When Sato launches the touch 
is not calibrated so I run the xinput_calibrator. When I run this, the console displays:

Calibrating EVDEV driver for "mxs-lradc" id=6
         current calibration values (from XInput): min_x=0, max_x=262143 and min_y=0, 
max_y=262143

Notice the max_x and max_y are showing 0x3ffff which is the value sent by the driver.

This calibration program shows a target at each corner. At this point, it will only 
accept me clicking on the first target (upper left corner). When I click the second it 
doesn't get it. It eventually gets it if I click several times somewhere else in the 
screen, but yet it won't get the third target (lower left corner), no matter where I 
press. So I can't calibrate the touch. I believe the reason might be the calibration 
tool is expecting much higher values (around the max value) and maybe discarding 
values that are too close (in relation to the range) to the min coordinate (it's just 
my guess).

After doing the above patch, when launching the vcalibration tool, I get this on the 
console:

Calibrating EVDEV driver for "mxs-lradc" id=6
         current calibration values (from XInput): min_x=0, max_x=4095 and min_y=0, 
max_y=4095

and the application lets me click on every target. The application showed the 
following output:

Doing dynamic recalibration:
         Swapping X and Y axis...
         Setting calibration data: 1405, 1367, 3693, 3733
         --> Making the calibration permanent <--
   copy the snippet below into '/etc/X11/xorg.conf.d/99-calibration.conf'
Section "InputClass"
         Identifier      "calibration"
         MatchProduct    "mxs-lradc"
         Option  "Calibration"   "1405 1367 3693 3733"
         Option  "SwapAxes"      "1"
EndSection

I created the file /etc/X11/xorg.conf.d/99-calibration.conf and wrote that 
information, but the touch still didn't work correctly. :o(

I manually removed the "Calibration" line from this file and launched the calibration 
tool again. This time I was given the following data:

Section "InputClass"
         Identifier      "calibration"
         MatchProduct    "mxs-lradc"
         Option  "Calibration"   "4207 320 4063 326"
         Option  "SwapAxes"      "1"
EndSection

and finally the touch works correctly calibrated.

So, ok, maybe the driver is not responsible for all these problems, but reporting a 
max value of 0x3ffff seems definitely incorrect, and it was preventing me from 
completing the calibration.

> [...]
>
>> Notice that I leave the existing LRADC_CH_VALUE_MASK 18bit mask in the rest
>> of the driver, to support accumulated samples.
>> Curiously, ts_lib works fine without this (maybe the calibration fixes
>> this?), but nevertheless the information passed by the driver is
>> incorrect, I guess. Is anybody out there working with the touch? Under
>> which graphic system?
>
> Both QtE and Sato work for me, that's why I'm curious about your issues.

ts_lib application also worked for me without the patch, but I thought maybe the 
ts_calibrate was doing his job regardless the ranges reported by the driver and the 
Sato worked differently.
Are you also using a resistive touch? How did you do the calibration?

Best regards,
--
Hector Palacios

Re: [PATCH 1/4] iio: mxs-lradc: change the realbits to 12

[Marek Vasut]

Hi Hector,

> Dear Marek,
> 
> On 07/10/2013 01:49 PM, Marek Vasut wrote:
> > Hi Hector,
> > 
> >> Hello,
> >> 
> >> On 07/05/2013 03:10 PM, Marek Vasut wrote:
> >>> Dear Hector Palacios,
> >>> 
> >>>> Dear Marek,
> >>>> 
> >>>> On 07/05/2013 01:37 PM, Marek Vasut wrote:
> >>>>> Dear Hector Palacios,
> >>>>> 
> >>>>>> The LRADC virtual channels have an 18 bit field to store the sum of
> >>>>>> up to 2^5 accumulated samples. The read_raw function however only
> >>>>>> operates over a single sample (12 bit resolution).
> >>>>>> In order to use this field for scaling operations, we need it to be
> >>>>>> the exact resolution value of the LRADC.
> >>>>> 
> >>>>> How would this work once the accumulation is supported?
> >>>> 
> >>>> As I see it, when you read a channel the driver should give you the
> >>>> 12-bit value either of one single sample or of N samples.
> >>> 
> >>> The hardware will always give you 18 bit value, let's call it A of N
> >>> accumulated samples, each 12 bit long. N is in range of 1 to 32 .
> >>> 
> >>> The driver currently supports N = 1.
> >>> 
> >>> Do I understand it correctly that if we want to support N > 1, we have
> >>> to do the division of A / N in the driver and therefore we will again
> >>> report only a 12-bit value to the userland ?
> >>> 
> >>> If so,
> >>> 
> >>> Acked-by: Marek Vasut <marex@denx.de>
> >> 
> >> Coming back to this patch, I just noticed that it is not enough to just
> >> change the realbits from 18 to 12. When using this driver as touchscreen
> >> for Yocto's SATO graphic rootfs I noticed that the driver is using
> >> LRADC_CH_VALUE_MASK for reporting the coordinates.
> >> 
> >> 	input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
> >> 	input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
> >> 	input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0,
> >> 	0);
> >> 
> >> which is defined as an 18bit mask:
> >> 	#define	LRADC_CH_VALUE_MASK			0x3ffff
> >> 
> >> The result is that the touch calibration range in Xorg is expecting
> >> values between 0 and 262143 (0x3ffff), which causes trouble (at least I
> >> had problems to calibrate it).
> > 
> > What kind of trouble does it cause?
> 
> Sorry, I should have said.
> First of all, I'm using a resistive touch that needs calibration.
> 
> There something I definitely don't understand about calibrating the touch
> in Sato. Initially I don't have any file /etc/pointercal.xinput. When Sato
> launches the touch is not calibrated so I run the xinput_calibrator. When
> I run this, the console displays:
> 
> Calibrating EVDEV driver for "mxs-lradc" id=6
>          current calibration values (from XInput): min_x=0, max_x=262143
> and min_y=0, max_y=262143
> 
> Notice the max_x and max_y are showing 0x3ffff which is the value sent by
> the driver.
> 
> This calibration program shows a target at each corner. At this point, it
> will only accept me clicking on the first target (upper left corner). When
> I click the second it doesn't get it. It eventually gets it if I click
> several times somewhere else in the screen, but yet it won't get the third
> target (lower left corner), no matter where I press. So I can't calibrate
> the touch. I believe the reason might be the calibration tool is expecting
> much higher values (around the max value) and maybe discarding values that
> are too close (in relation to the range) to the min coordinate (it's just
> my guess).
> 
> After doing the above patch, when launching the vcalibration tool, I get
> this on the console:
> 
> Calibrating EVDEV driver for "mxs-lradc" id=6
>          current calibration values (from XInput): min_x=0, max_x=4095 and
> min_y=0, max_y=4095
> 
> and the application lets me click on every target. The application showed
> the following output:
> 
> Doing dynamic recalibration:
>          Swapping X and Y axis...
>          Setting calibration data: 1405, 1367, 3693, 3733
>          --> Making the calibration permanent <--
>    copy the snippet below into '/etc/X11/xorg.conf.d/99-calibration.conf'
> Section "InputClass"
>          Identifier      "calibration"
>          MatchProduct    "mxs-lradc"
>          Option  "Calibration"   "1405 1367 3693 3733"
>          Option  "SwapAxes"      "1"
> EndSection
> 
> I created the file /etc/X11/xorg.conf.d/99-calibration.conf and wrote that
> information, but the touch still didn't work correctly. :o(
> 
> I manually removed the "Calibration" line from this file and launched the
> calibration tool again. This time I was given the following data:
> 
> Section "InputClass"
>          Identifier      "calibration"
>          MatchProduct    "mxs-lradc"
>          Option  "Calibration"   "4207 320 4063 326"
>          Option  "SwapAxes"      "1"
> EndSection
> 
> and finally the touch works correctly calibrated.
> 
> So, ok, maybe the driver is not responsible for all these problems, but
> reporting a max value of 0x3ffff seems definitely incorrect, and it was
> preventing me from completing the calibration.

Can you try with the ts_calibrate tool in qte image? The X11 calibrator is 
horrible piece of ... you know. The tslib ts_calibrate one always worked much 
better for me. So maybe you should also peek into the X11 calibrator and fix it.

On the other hand, the patch is correct, it should report only 12-bit values.

These are my X11 calibration values:

Section "InputClass"
        Identifier      "calibration"
        MatchProduct    "mxs-lradc"
        Option          "Calibration"   "98 4035 4007 198"
EndSection

> > [...]
> > 
> >> Notice that I leave the existing LRADC_CH_VALUE_MASK 18bit mask in the
> >> rest of the driver, to support accumulated samples.
> >> Curiously, ts_lib works fine without this (maybe the calibration fixes
> >> this?), but nevertheless the information passed by the driver is
> >> incorrect, I guess. Is anybody out there working with the touch? Under
> >> which graphic system?
> > 
> > Both QtE and Sato work for me, that's why I'm curious about your issues.
> 
> ts_lib application also worked for me without the patch, but I thought
> maybe the ts_calibrate was doing his job regardless the ranges reported by
> the driver and the Sato worked differently.
> Are you also using a resistive touch? How did you do the calibration?

Yes, I'm using resistive touch. Maybe it's due to a different brand.

Best regards,
Marek Vasut