am335x adc & tsc, second batch

am335x adc & tsc, second batch

[Sebastian Andrzej Siewior]

Hi,

this basically fixes a few issues when TSC and ADC are used at the same
time. One issues was found in the TSC part where it used one wrong
coordinate while reading from the FIFO.

This depends on the series I sent the 19 patchset I sent earlier.
The complete series can be found at
 git://git.breakpoint.cc/bigeasy/linux.git am335x_tsc_adc

Sebastian

am335x adc & tsc, second batch

[Sebastian Andrzej Siewior]

Hi,

this basically fixes a few issues when TSC and ADC are used at the same
time. One issues was found in the TSC part where it used one wrong
coordinate while reading from the FIFO.

This depends on the series I sent the 19 patchset I sent earlier.
The complete series can be found at
 git://git.breakpoint.cc/bigeasy/linux.git am335x_tsc_adc

Sebastian

[PATCH 1/5] drivers/iio: am335x_adc: cleanup on missing DT nodes

[Sebastian Andrzej Siewior]

Currently we leak indio_dev() if the DT has no "adc" node in it.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index d0317fc..b2f27de 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -208,8 +208,10 @@ static int tiadc_probe(struct platform_device *pdev)
 	adc_dev->mfd_tscadc = tscadc_dev;
 
 	node = of_get_child_by_name(node, "adc");
-	if (!node)
-		return  -EINVAL;
+	if (!node) {
+		err = -EINVAL;
+		goto err_free_device;
+	}
 	err = of_property_read_u32(node,
 			"ti,adc-channels", &val32);
 	if (err < 0)
-- 
1.7.10.4

Re: [PATCH 1/5] drivers/iio: am335x_adc: cleanup on missing DT nodes

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> Currently we leak indio_dev() if the DT has no "adc" node in it.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Given you are going to end up rerolling the original series
(and it hasn't been taken by anyone yet) this wants to go
in a new version of that not as a separate patch.
> ---
>  drivers/iio/adc/ti_am335x_adc.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index d0317fc..b2f27de 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -208,8 +208,10 @@ static int tiadc_probe(struct platform_device *pdev)
>  	adc_dev->mfd_tscadc = tscadc_dev;
>  
>  	node = of_get_child_by_name(node, "adc");
> -	if (!node)
> -		return  -EINVAL;
> +	if (!node) {
> +		err = -EINVAL;
> +		goto err_free_device;
> +	}
>  	err = of_property_read_u32(node,
>  			"ti,adc-channels", &val32);
>  	if (err < 0)
> 

Re: [PATCH 1/5] drivers/iio: am335x_adc: cleanup on missing DT nodes

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> Currently we leak indio_dev() if the DT has no "adc" node in it.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Given you are going to end up rerolling the original series
(and it hasn't been taken by anyone yet) this wants to go
in a new version of that not as a separate patch.
> ---
>  drivers/iio/adc/ti_am335x_adc.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index d0317fc..b2f27de 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -208,8 +208,10 @@ static int tiadc_probe(struct platform_device *pdev)
>  	adc_dev->mfd_tscadc = tscadc_dev;
>  
>  	node = of_get_child_by_name(node, "adc");
> -	if (!node)
> -		return  -EINVAL;
> +	if (!node) {
> +		err = -EINVAL;
> +		goto err_free_device;
> +	}
>  	err = of_property_read_u32(node,
>  			"ti,adc-channels", &val32);
>  	if (err < 0)
> 

Re: [PATCH 1/5] drivers/iio: am335x_adc: cleanup on missing DT nodes

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:56:40 [+0100]:

>On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
>> Currently we leak indio_dev() if the DT has no "adc" node in it.
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>Given you are going to end up rerolling the original series
>(and it hasn't been taken by anyone yet) this wants to go
>in a new version of that not as a separate patch.

Yes, why not.

Sebastian

Re: [PATCH 1/5] drivers/iio: am335x_adc: cleanup on missing DT nodes

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 18:56:40 [+0100]:

>On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
>> Currently we leak indio_dev() if the DT has no "adc" node in it.
>> 
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>Given you are going to end up rerolling the original series
>(and it hasn't been taken by anyone yet) this wants to go
>in a new version of that not as a separate patch.

Yes, why not.

Sebastian

[PATCH 2/5] input/ti_am335x_adc: use only FIFO0 and clean up a little

[Sebastian Andrzej Siewior]

The driver programs a threshold of "steps-to-configure" say 5. The
REG_FIFO0THR registers says it should it be programmed to "threshold
minus one". The driver does not expect just 5 coordinates but 5 * 2 + 2.
Multiplied by two because 5 for X and 5 for Y. Plus 2 because we have
two Z.
The whole thing works because It reads the 5 coordinates for X and Y and
split over FIFO0 and FIFO1 and the last element in each FIFO is ignored
within the loop and read later.
Nothing guaranties that FIFO1 is ready by the time it is read. In fact I
could see that that FIFO1 reaturns for Y channels 8,9, 10, 12, 6 and for
Y channel 7 for Z. The problem is that channel 7 and channel 12 got
somehow mixed up. The variance filter here maybe tries to get rid of the
wrong Y values, dunno.
The other Problem is that FIFO1 is also used by the IIO part leading to
not wrong results if both (tsc & adc) are used.

The patch tries to clean up the whole thing a little:
- Name it "coordiante-readouts" and not "steps-to-configure" and
  document it properly. Point out that it the hardware does a total of
  "5 * 2 + 2" reads / steps and not just what is configured.

- Remove the +1 and -1 in REG_STEPCONFIG, REG_STEPDELAY and its counter
  part in the for loop. This is just confusing.

- Use only FIFO0 in TSC. The fifo has space for 64 entries so should be
  fine.

- Read the whole FIFO in one function and check the channel.

- in case we dawdle around, make sure we only read a multiple of our
  coordinate set. On the second interrupt we will cleanup the remaining
  enties.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |   15 ++--
 arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
 drivers/iio/adc/ti_am335x_adc.c                    |    2 +-
 drivers/input/touchscreen/ti_am335x_tsc.c          |   87 ++++++++++----------
 include/linux/mfd/ti_am335x_tscadc.h               |    4 +-
 5 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index e533e9d..80e04e3 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -6,10 +6,15 @@
 	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
 		  support on the platform.
 	ti,x-plate-resistance: X plate resistance
-	ti,steps-to-configure: The sequencer supports a total of 16
-			       programmable steps. A step configured to read a
-			       single co-ordinate value. Can be applied more
-			       number of times for better results.
+	ti,coordiante-readouts: The sequencer supports a total of 16
+				programmable steps each step is used to
+				read a single coordinate. A single
+                                readout is enough but multiple reads can
+				increase the quality.
+				A value of 5 means, 5 reads for X, 5 for
+				Y and 2 for Z (always). This utilises 12
+				of the 16 software steps available. The
+				remaining 4 can be used by the ADC.
 	ti,wire-config: Different boards could have a different order for
 			connecting wires on touchscreen. We need to provide an
 			8 bit number where in the 1st four bits represent the
@@ -28,7 +33,7 @@
 		tsc {
 			ti,wires = <4>;
 			ti,x-plate-resistance = <200>;
-			ti,steps-to-configure = <5>;
+			ti,coordiante-readouts = <5>;
 			ti,wire-config = <0x00 0x11 0x22 0x33>;
 		};
 
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index be6a5b2..26fea97 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -250,7 +250,7 @@
 	tsc {
 		ti,wires = <4>;
 		ti,x-plate-resistance = <200>;
-		ti,steps-to-configure = <5>;
+		ti,coordiante-readouts = <5>;
 		ti,wire-config = <0x00 0x11 0x22 0x33>;
 	};
 
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index b2f27de..2988840 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -72,7 +72,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 
 	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
 
-	for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
+	for (i = steps; i < TOTAL_STEPS; i++) {
 		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
 				stepconfig | STEPCONFIG_INP(channels));
 		tiadc_writel(adc_dev, REG_STEPDELAY(i),
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 2921163..7c97fc7 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -58,7 +58,7 @@ struct titsc {
 	unsigned int		bckup_x;
 	unsigned int		bckup_y;
 	bool			pen_down;
-	int			steps_to_configure;
+	int			coordiante_readouts;
 	u32			config_inp[4];
 	int			bit_xp, bit_xn, bit_yp, bit_yn;
 	int			inp_xp, inp_xn, inp_yp, inp_yn;
@@ -168,11 +168,8 @@ static int titsc_config_wires(struct titsc *ts_dev)
 static void titsc_step_config(struct titsc *ts_dev)
 {
 	unsigned int	config;
-	unsigned int	stepenable = 0;
-	int i, total_steps;
-
-	/* Configure the Step registers */
-	total_steps = 2 * ts_dev->steps_to_configure;
+	int i;
+	int end_step;
 
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_xp;
@@ -190,7 +187,9 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	for (i = 1; i <= ts_dev->steps_to_configure; i++) {
+	/* 1 … coordiante_readouts is for X */
+	end_step = ts_dev->coordiante_readouts;
+	for (i = 0; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
@@ -198,7 +197,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 	config = 0;
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_yn |
-			STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1;
+			STEPCONFIG_INM_ADCREFM;
 	switch (ts_dev->wires) {
 	case 4:
 		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
@@ -212,12 +211,13 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	for (i = (ts_dev->steps_to_configure + 1); i <= total_steps; i++) {
+	/* coordiante_readouts … coordiante_readouts * 2 is for Y */
+	end_step = ts_dev->coordiante_readouts * 2;
+	for (i = ts_dev->coordiante_readouts; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
 
-	config = 0;
 	/* Charge step configuration */
 	config = ts_dev->bit_xp | ts_dev->bit_yn |
 			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
@@ -226,37 +226,40 @@ static void titsc_step_config(struct titsc *ts_dev)
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
 	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
 
-	config = 0;
-	/* Configure to calculate pressure */
+	/* coordiante_readouts * 2 … coordiante_readouts * 2 + 2 is for Z */
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
 			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
 			STEPCONFIG_INP(ts_dev->inp_xp);
-	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config);
-	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1),
+	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
+	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
 			STEPCONFIG_OPENDLY);
 
-	config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1;
-	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config);
-	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
+	end_step++;
+	config |= STEPCONFIG_INP(ts_dev->inp_yn);
+	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
+	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
 			STEPCONFIG_OPENDLY);
 
-	for (i = 0; i <= (total_steps + 2); i++)
-		stepenable |= 1 << i;
-	ts_dev->enable_bits = stepenable;
+	/* The steps1 … end and bit 0 for TS_Charge */
+	ts_dev->enable_bits = (1 << (end_step + 2)) - 1;
 
 	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
-				    unsigned int *x, unsigned int *y)
+		u32 *x, u32 *y, u32 *z1, u32 *z2)
 {
 	unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT);
 	unsigned int prev_val_x = ~0, prev_val_y = ~0;
 	unsigned int prev_diff_x = ~0, prev_diff_y = ~0;
 	unsigned int read, diff;
 	unsigned int i, channel;
+	unsigned int creads = ts_dev->coordiante_readouts;
 
+	*z1 = *z2 = 0;
+	if (fifocount % (creads * 2 + 2))
+		fifocount -= fifocount % (creads * 2 + 2);
 	/*
 	 * Delta filter is used to remove large variations in sampled
 	 * values from ADC. The filter tries to predict where the next
@@ -265,32 +268,31 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
 	 * algorithm compares the difference with that of a present value,
 	 * if true the value is reported to the sub system.
 	 */
-	for (i = 0; i < fifocount - 1; i++) {
+	for (i = 0; i < fifocount; i++) {
 		read = titsc_readl(ts_dev, REG_FIFO0);
-		channel = read & 0xf0000;
-		channel = channel >> 0x10;
-		if ((channel >= 0) && (channel < ts_dev->steps_to_configure)) {
-			read &= 0xfff;
+		channel = (read & 0xf0000) >> 16;
+		read &= 0xfff;
+		if (channel < creads) {
 			diff = abs(read - prev_val_x);
 			if (diff < prev_diff_x) {
 				prev_diff_x = diff;
 				*x = read;
 			}
 			prev_val_x = read;
-		}
 
-		read = titsc_readl(ts_dev, REG_FIFO1);
-		channel = read & 0xf0000;
-		channel = channel >> 0x10;
-		if ((channel >= ts_dev->steps_to_configure) &&
-			(channel < (2 * ts_dev->steps_to_configure - 1))) {
-			read &= 0xfff;
+		} else if (channel < creads * 2) {
 			diff = abs(read - prev_val_y);
 			if (diff < prev_diff_y) {
 				prev_diff_y = diff;
 				*y = read;
 			}
 			prev_val_y = read;
+
+		} else if (channel < creads * 2 + 1) {
+			*z1 = read;
+
+		} else if (channel < creads * 2 + 2) {
+			*z2 = read;
 		}
 	}
 }
@@ -307,26 +309,24 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
 	if (status & IRQENB_FIFO0THRES) {
-		titsc_read_coordinates(ts_dev, &x, &y);
+
+		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
 
 		diffx = abs(x - (ts_dev->bckup_x));
 		diffy = abs(y - (ts_dev->bckup_y));
 		ts_dev->bckup_x = x;
 		ts_dev->bckup_y = y;
 
-		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
-		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
-
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
 			/*
 			 * Calculate pressure using formula
 			 * Resistance(touch) = x plate resistance *
 			 * x postion/4096 * ((z2 / z1) - 1)
 			 */
-			z = z2 - z1;
+			z = z1 - z2;
 			z *= x;
 			z *= ts_dev->x_plate_resistance;
-			z /= z1;
+			z /= z2;
 			z = (z + 2047) >> 12;
 
 			if ((diffx < TSCADC_DELTA_X) &&
@@ -399,8 +399,8 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
 	if (err < 0)
 		return err;
 
-	err = of_property_read_u32(node, "ti,steps-to-configure",
-			&ts_dev->steps_to_configure);
+	err = of_property_read_u32(node, "ti,coordiante-readouts",
+			&ts_dev->coordiante_readouts);
 	if (err < 0)
 		return err;
 
@@ -454,7 +454,8 @@ static int titsc_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 	titsc_step_config(ts_dev);
-	titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure);
+	titsc_writel(ts_dev, REG_FIFO0THR,
+			ts_dev->coordiante_readouts * 2 + 2 - 1);
 
 	input_dev->name = "ti-tsc";
 	input_dev->dev.parent = &pdev->dev;
@@ -526,7 +527,7 @@ static int titsc_resume(struct device *dev)
 	}
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
-			ts_dev->steps_to_configure);
+			ts_dev->coordiante_readouts * 2 + 2 - 1);
 	return 0;
 }
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index c985262..4ec52b0 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -30,8 +30,8 @@
 #define REG_IDLECONFIG		0x058
 #define REG_CHARGECONFIG	0x05C
 #define REG_CHARGEDELAY		0x060
-#define REG_STEPCONFIG(n)	(0x64 + ((n - 1) * 8))
-#define REG_STEPDELAY(n)	(0x68 + ((n - 1) * 8))
+#define REG_STEPCONFIG(n)	(0x64 + ((n) * 8))
+#define REG_STEPDELAY(n)	(0x68 + ((n) * 8))
 #define REG_FIFO0CNT		0xE4
 #define REG_FIFO0THR		0xE8
 #define REG_FIFO1CNT		0xF0
-- 
1.7.10.4

[PATCH 2/5] input/ti_am335x_adc: use only FIFO0 and clean up a little

[Sebastian Andrzej Siewior]

The driver programs a threshold of "steps-to-configure" say 5. The
REG_FIFO0THR registers says it should it be programmed to "threshold
minus one". The driver does not expect just 5 coordinates but 5 * 2 + 2.
Multiplied by two because 5 for X and 5 for Y. Plus 2 because we have
two Z.
The whole thing works because It reads the 5 coordinates for X and Y and
split over FIFO0 and FIFO1 and the last element in each FIFO is ignored
within the loop and read later.
Nothing guaranties that FIFO1 is ready by the time it is read. In fact I
could see that that FIFO1 reaturns for Y channels 8,9, 10, 12, 6 and for
Y channel 7 for Z. The problem is that channel 7 and channel 12 got
somehow mixed up. The variance filter here maybe tries to get rid of the
wrong Y values, dunno.
The other Problem is that FIFO1 is also used by the IIO part leading to
not wrong results if both (tsc & adc) are used.

The patch tries to clean up the whole thing a little:
- Name it "coordiante-readouts" and not "steps-to-configure" and
  document it properly. Point out that it the hardware does a total of
  "5 * 2 + 2" reads / steps and not just what is configured.

- Remove the +1 and -1 in REG_STEPCONFIG, REG_STEPDELAY and its counter
  part in the for loop. This is just confusing.

- Use only FIFO0 in TSC. The fifo has space for 64 entries so should be
  fine.

- Read the whole FIFO in one function and check the channel.

- in case we dawdle around, make sure we only read a multiple of our
  coordinate set. On the second interrupt we will cleanup the remaining
  enties.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |   15 ++--
 arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
 drivers/iio/adc/ti_am335x_adc.c                    |    2 +-
 drivers/input/touchscreen/ti_am335x_tsc.c          |   87 ++++++++++----------
 include/linux/mfd/ti_am335x_tscadc.h               |    4 +-
 5 files changed, 58 insertions(+), 52 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index e533e9d..80e04e3 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -6,10 +6,15 @@
 	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
 		  support on the platform.
 	ti,x-plate-resistance: X plate resistance
-	ti,steps-to-configure: The sequencer supports a total of 16
-			       programmable steps. A step configured to read a
-			       single co-ordinate value. Can be applied more
-			       number of times for better results.
+	ti,coordiante-readouts: The sequencer supports a total of 16
+				programmable steps each step is used to
+				read a single coordinate. A single
+                                readout is enough but multiple reads can
+				increase the quality.
+				A value of 5 means, 5 reads for X, 5 for
+				Y and 2 for Z (always). This utilises 12
+				of the 16 software steps available. The
+				remaining 4 can be used by the ADC.
 	ti,wire-config: Different boards could have a different order for
 			connecting wires on touchscreen. We need to provide an
 			8 bit number where in the 1st four bits represent the
@@ -28,7 +33,7 @@
 		tsc {
 			ti,wires = <4>;
 			ti,x-plate-resistance = <200>;
-			ti,steps-to-configure = <5>;
+			ti,coordiante-readouts = <5>;
 			ti,wire-config = <0x00 0x11 0x22 0x33>;
 		};
 
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index be6a5b2..26fea97 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -250,7 +250,7 @@
 	tsc {
 		ti,wires = <4>;
 		ti,x-plate-resistance = <200>;
-		ti,steps-to-configure = <5>;
+		ti,coordiante-readouts = <5>;
 		ti,wire-config = <0x00 0x11 0x22 0x33>;
 	};
 
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index b2f27de..2988840 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -72,7 +72,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 
 	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
 
-	for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
+	for (i = steps; i < TOTAL_STEPS; i++) {
 		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
 				stepconfig | STEPCONFIG_INP(channels));
 		tiadc_writel(adc_dev, REG_STEPDELAY(i),
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 2921163..7c97fc7 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -58,7 +58,7 @@ struct titsc {
 	unsigned int		bckup_x;
 	unsigned int		bckup_y;
 	bool			pen_down;
-	int			steps_to_configure;
+	int			coordiante_readouts;
 	u32			config_inp[4];
 	int			bit_xp, bit_xn, bit_yp, bit_yn;
 	int			inp_xp, inp_xn, inp_yp, inp_yn;
@@ -168,11 +168,8 @@ static int titsc_config_wires(struct titsc *ts_dev)
 static void titsc_step_config(struct titsc *ts_dev)
 {
 	unsigned int	config;
-	unsigned int	stepenable = 0;
-	int i, total_steps;
-
-	/* Configure the Step registers */
-	total_steps = 2 * ts_dev->steps_to_configure;
+	int i;
+	int end_step;
 
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_xp;
@@ -190,7 +187,9 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	for (i = 1; i <= ts_dev->steps_to_configure; i++) {
+	/* 1 … coordiante_readouts is for X */
+	end_step = ts_dev->coordiante_readouts;
+	for (i = 0; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
@@ -198,7 +197,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 	config = 0;
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_yn |
-			STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1;
+			STEPCONFIG_INM_ADCREFM;
 	switch (ts_dev->wires) {
 	case 4:
 		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
@@ -212,12 +211,13 @@ static void titsc_step_config(struct titsc *ts_dev)
 		break;
 	}
 
-	for (i = (ts_dev->steps_to_configure + 1); i <= total_steps; i++) {
+	/* coordiante_readouts … coordiante_readouts * 2 is for Y */
+	end_step = ts_dev->coordiante_readouts * 2;
+	for (i = ts_dev->coordiante_readouts; i < end_step; i++) {
 		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
 		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
 	}
 
-	config = 0;
 	/* Charge step configuration */
 	config = ts_dev->bit_xp | ts_dev->bit_yn |
 			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
@@ -226,37 +226,40 @@ static void titsc_step_config(struct titsc *ts_dev)
 	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
 	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
 
-	config = 0;
-	/* Configure to calculate pressure */
+	/* coordiante_readouts * 2 … coordiante_readouts * 2 + 2 is for Z */
 	config = STEPCONFIG_MODE_HWSYNC |
 			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
 			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
 			STEPCONFIG_INP(ts_dev->inp_xp);
-	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config);
-	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1),
+	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
+	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
 			STEPCONFIG_OPENDLY);
 
-	config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1;
-	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config);
-	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
+	end_step++;
+	config |= STEPCONFIG_INP(ts_dev->inp_yn);
+	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
+	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
 			STEPCONFIG_OPENDLY);
 
-	for (i = 0; i <= (total_steps + 2); i++)
-		stepenable |= 1 << i;
-	ts_dev->enable_bits = stepenable;
+	/* The steps1 … end and bit 0 for TS_Charge */
+	ts_dev->enable_bits = (1 << (end_step + 2)) - 1;
 
 	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
-				    unsigned int *x, unsigned int *y)
+		u32 *x, u32 *y, u32 *z1, u32 *z2)
 {
 	unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT);
 	unsigned int prev_val_x = ~0, prev_val_y = ~0;
 	unsigned int prev_diff_x = ~0, prev_diff_y = ~0;
 	unsigned int read, diff;
 	unsigned int i, channel;
+	unsigned int creads = ts_dev->coordiante_readouts;
 
+	*z1 = *z2 = 0;
+	if (fifocount % (creads * 2 + 2))
+		fifocount -= fifocount % (creads * 2 + 2);
 	/*
 	 * Delta filter is used to remove large variations in sampled
 	 * values from ADC. The filter tries to predict where the next
@@ -265,32 +268,31 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
 	 * algorithm compares the difference with that of a present value,
 	 * if true the value is reported to the sub system.
 	 */
-	for (i = 0; i < fifocount - 1; i++) {
+	for (i = 0; i < fifocount; i++) {
 		read = titsc_readl(ts_dev, REG_FIFO0);
-		channel = read & 0xf0000;
-		channel = channel >> 0x10;
-		if ((channel >= 0) && (channel < ts_dev->steps_to_configure)) {
-			read &= 0xfff;
+		channel = (read & 0xf0000) >> 16;
+		read &= 0xfff;
+		if (channel < creads) {
 			diff = abs(read - prev_val_x);
 			if (diff < prev_diff_x) {
 				prev_diff_x = diff;
 				*x = read;
 			}
 			prev_val_x = read;
-		}
 
-		read = titsc_readl(ts_dev, REG_FIFO1);
-		channel = read & 0xf0000;
-		channel = channel >> 0x10;
-		if ((channel >= ts_dev->steps_to_configure) &&
-			(channel < (2 * ts_dev->steps_to_configure - 1))) {
-			read &= 0xfff;
+		} else if (channel < creads * 2) {
 			diff = abs(read - prev_val_y);
 			if (diff < prev_diff_y) {
 				prev_diff_y = diff;
 				*y = read;
 			}
 			prev_val_y = read;
+
+		} else if (channel < creads * 2 + 1) {
+			*z1 = read;
+
+		} else if (channel < creads * 2 + 2) {
+			*z2 = read;
 		}
 	}
 }
@@ -307,26 +309,24 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
 	if (status & IRQENB_FIFO0THRES) {
-		titsc_read_coordinates(ts_dev, &x, &y);
+
+		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
 
 		diffx = abs(x - (ts_dev->bckup_x));
 		diffy = abs(y - (ts_dev->bckup_y));
 		ts_dev->bckup_x = x;
 		ts_dev->bckup_y = y;
 
-		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
-		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
-
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
 			/*
 			 * Calculate pressure using formula
 			 * Resistance(touch) = x plate resistance *
 			 * x postion/4096 * ((z2 / z1) - 1)
 			 */
-			z = z2 - z1;
+			z = z1 - z2;
 			z *= x;
 			z *= ts_dev->x_plate_resistance;
-			z /= z1;
+			z /= z2;
 			z = (z + 2047) >> 12;
 
 			if ((diffx < TSCADC_DELTA_X) &&
@@ -399,8 +399,8 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
 	if (err < 0)
 		return err;
 
-	err = of_property_read_u32(node, "ti,steps-to-configure",
-			&ts_dev->steps_to_configure);
+	err = of_property_read_u32(node, "ti,coordiante-readouts",
+			&ts_dev->coordiante_readouts);
 	if (err < 0)
 		return err;
 
@@ -454,7 +454,8 @@ static int titsc_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 	titsc_step_config(ts_dev);
-	titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure);
+	titsc_writel(ts_dev, REG_FIFO0THR,
+			ts_dev->coordiante_readouts * 2 + 2 - 1);
 
 	input_dev->name = "ti-tsc";
 	input_dev->dev.parent = &pdev->dev;
@@ -526,7 +527,7 @@ static int titsc_resume(struct device *dev)
 	}
 	titsc_step_config(ts_dev);
 	titsc_writel(ts_dev, REG_FIFO0THR,
-			ts_dev->steps_to_configure);
+			ts_dev->coordiante_readouts * 2 + 2 - 1);
 	return 0;
 }
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index c985262..4ec52b0 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -30,8 +30,8 @@
 #define REG_IDLECONFIG		0x058
 #define REG_CHARGECONFIG	0x05C
 #define REG_CHARGEDELAY		0x060
-#define REG_STEPCONFIG(n)	(0x64 + ((n - 1) * 8))
-#define REG_STEPDELAY(n)	(0x68 + ((n - 1) * 8))
+#define REG_STEPCONFIG(n)	(0x64 + ((n) * 8))
+#define REG_STEPDELAY(n)	(0x68 + ((n) * 8))
 #define REG_FIFO0CNT		0xE4
 #define REG_FIFO0THR		0xE8
 #define REG_FIFO1CNT		0xF0
-- 
1.7.10.4

Re: [PATCH 2/5] input/ti_am335x_adc: use only FIFO0 and clean up a little

[Dmitry Torokhov]

On Wed, May 29, 2013 at 07:09:53PM +0200, Sebastian Andrzej Siewior wrote:
> The driver programs a threshold of "steps-to-configure" say 5. The
> REG_FIFO0THR registers says it should it be programmed to "threshold
> minus one". The driver does not expect just 5 coordinates but 5 * 2 + 2.
> Multiplied by two because 5 for X and 5 for Y. Plus 2 because we have
> two Z.
> The whole thing works because It reads the 5 coordinates for X and Y and
> split over FIFO0 and FIFO1 and the last element in each FIFO is ignored
> within the loop and read later.
> Nothing guaranties that FIFO1 is ready by the time it is read. In fact I
> could see that that FIFO1 reaturns for Y channels 8,9, 10, 12, 6 and for
> Y channel 7 for Z. The problem is that channel 7 and channel 12 got
> somehow mixed up. The variance filter here maybe tries to get rid of the
> wrong Y values, dunno.
> The other Problem is that FIFO1 is also used by the IIO part leading to
> not wrong results if both (tsc & adc) are used.
> 
> The patch tries to clean up the whole thing a little:
> - Name it "coordiante-readouts" and not "steps-to-configure" and
>   document it properly. Point out that it the hardware does a total of
>   "5 * 2 + 2" reads / steps and not just what is configured.
> 
> - Remove the +1 and -1 in REG_STEPCONFIG, REG_STEPDELAY and its counter
>   part in the for loop. This is just confusing.
> 
> - Use only FIFO0 in TSC. The fifo has space for 64 entries so should be
>   fine.
> 
> - Read the whole FIFO in one function and check the channel.
> 
> - in case we dawdle around, make sure we only read a multiple of our
>   coordinate set. On the second interrupt we will cleanup the remaining
>   enties.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   15 ++--
>  arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
>  drivers/iio/adc/ti_am335x_adc.c                    |    2 +-
>  drivers/input/touchscreen/ti_am335x_tsc.c          |   87 ++++++++++----------
>  include/linux/mfd/ti_am335x_tscadc.h               |    4 +-
>  5 files changed, 58 insertions(+), 52 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index e533e9d..80e04e3 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -6,10 +6,15 @@
>  	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
>  		  support on the platform.
>  	ti,x-plate-resistance: X plate resistance
> -	ti,steps-to-configure: The sequencer supports a total of 16
> -			       programmable steps. A step configured to read a
> -			       single co-ordinate value. Can be applied more
> -			       number of times for better results.
> +	ti,coordiante-readouts: The sequencer supports a total of 16
> +				programmable steps each step is used to
> +				read a single coordinate. A single
> +                                readout is enough but multiple reads can
> +				increase the quality.
> +				A value of 5 means, 5 reads for X, 5 for
> +				Y and 2 for Z (always). This utilises 12
> +				of the 16 software steps available. The
> +				remaining 4 can be used by the ADC.
>  	ti,wire-config: Different boards could have a different order for
>  			connecting wires on touchscreen. We need to provide an
>  			8 bit number where in the 1st four bits represent the
> @@ -28,7 +33,7 @@
>  		tsc {
>  			ti,wires = <4>;
>  			ti,x-plate-resistance = <200>;
> -			ti,steps-to-configure = <5>;
> +			ti,coordiante-readouts = <5>;
>  			ti,wire-config = <0x00 0x11 0x22 0x33>;
>  		};
>  
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index be6a5b2..26fea97 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -250,7 +250,7 @@
>  	tsc {
>  		ti,wires = <4>;
>  		ti,x-plate-resistance = <200>;
> -		ti,steps-to-configure = <5>;
> +		ti,coordiante-readouts = <5>;
>  		ti,wire-config = <0x00 0x11 0x22 0x33>;
>  	};
>  
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index b2f27de..2988840 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -72,7 +72,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  
>  	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>  
> -	for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
> +	for (i = steps; i < TOTAL_STEPS; i++) {
>  		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
>  				stepconfig | STEPCONFIG_INP(channels));
>  		tiadc_writel(adc_dev, REG_STEPDELAY(i),
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 2921163..7c97fc7 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -58,7 +58,7 @@ struct titsc {
>  	unsigned int		bckup_x;
>  	unsigned int		bckup_y;
>  	bool			pen_down;
> -	int			steps_to_configure;
> +	int			coordiante_readouts;
>  	u32			config_inp[4];
>  	int			bit_xp, bit_xn, bit_yp, bit_yn;
>  	int			inp_xp, inp_xn, inp_yp, inp_yn;
> @@ -168,11 +168,8 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  static void titsc_step_config(struct titsc *ts_dev)
>  {
>  	unsigned int	config;
> -	unsigned int	stepenable = 0;
> -	int i, total_steps;
> -
> -	/* Configure the Step registers */
> -	total_steps = 2 * ts_dev->steps_to_configure;
> +	int i;
> +	int end_step;
>  
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_xp;
> @@ -190,7 +187,9 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	for (i = 1; i <= ts_dev->steps_to_configure; i++) {
> +	/* 1 … coordiante_readouts is for X */
> +	end_step = ts_dev->coordiante_readouts;
> +	for (i = 0; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
> @@ -198,7 +197,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	config = 0;
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_yn |
> -			STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1;
> +			STEPCONFIG_INM_ADCREFM;
>  	switch (ts_dev->wires) {
>  	case 4:
>  		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
> @@ -212,12 +211,13 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	for (i = (ts_dev->steps_to_configure + 1); i <= total_steps; i++) {
> +	/* coordiante_readouts … coordiante_readouts * 2 is for Y */
> +	end_step = ts_dev->coordiante_readouts * 2;
> +	for (i = ts_dev->coordiante_readouts; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
>  
> -	config = 0;
>  	/* Charge step configuration */
>  	config = ts_dev->bit_xp | ts_dev->bit_yn |
>  			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
> @@ -226,37 +226,40 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
>  	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
>  
> -	config = 0;
> -	/* Configure to calculate pressure */
> +	/* coordiante_readouts * 2 … coordiante_readouts * 2 + 2 is for Z */
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
>  			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
>  			STEPCONFIG_INP(ts_dev->inp_xp);
> -	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config);
> -	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1),
> +	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
> +	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>  			STEPCONFIG_OPENDLY);
>  
> -	config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1;
> -	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config);
> -	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
> +	end_step++;
> +	config |= STEPCONFIG_INP(ts_dev->inp_yn);
> +	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
> +	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>  			STEPCONFIG_OPENDLY);
>  
> -	for (i = 0; i <= (total_steps + 2); i++)
> -		stepenable |= 1 << i;
> -	ts_dev->enable_bits = stepenable;
> +	/* The steps1 … end and bit 0 for TS_Charge */
> +	ts_dev->enable_bits = (1 << (end_step + 2)) - 1;
>  
>  	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
>  }
>  
>  static void titsc_read_coordinates(struct titsc *ts_dev,
> -				    unsigned int *x, unsigned int *y)
> +		u32 *x, u32 *y, u32 *z1, u32 *z2)
>  {
>  	unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT);
>  	unsigned int prev_val_x = ~0, prev_val_y = ~0;
>  	unsigned int prev_diff_x = ~0, prev_diff_y = ~0;
>  	unsigned int read, diff;
>  	unsigned int i, channel;
> +	unsigned int creads = ts_dev->coordiante_readouts;
>  
> +	*z1 = *z2 = 0;
> +	if (fifocount % (creads * 2 + 2))
> +		fifocount -= fifocount % (creads * 2 + 2);
>  	/*
>  	 * Delta filter is used to remove large variations in sampled
>  	 * values from ADC. The filter tries to predict where the next
> @@ -265,32 +268,31 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  	 * algorithm compares the difference with that of a present value,
>  	 * if true the value is reported to the sub system.
>  	 */
> -	for (i = 0; i < fifocount - 1; i++) {
> +	for (i = 0; i < fifocount; i++) {
>  		read = titsc_readl(ts_dev, REG_FIFO0);
> -		channel = read & 0xf0000;
> -		channel = channel >> 0x10;
> -		if ((channel >= 0) && (channel < ts_dev->steps_to_configure)) {
> -			read &= 0xfff;
> +		channel = (read & 0xf0000) >> 16;
> +		read &= 0xfff;
> +		if (channel < creads) {
>  			diff = abs(read - prev_val_x);
>  			if (diff < prev_diff_x) {
>  				prev_diff_x = diff;
>  				*x = read;
>  			}
>  			prev_val_x = read;
> -		}
>  
> -		read = titsc_readl(ts_dev, REG_FIFO1);
> -		channel = read & 0xf0000;
> -		channel = channel >> 0x10;
> -		if ((channel >= ts_dev->steps_to_configure) &&
> -			(channel < (2 * ts_dev->steps_to_configure - 1))) {
> -			read &= 0xfff;
> +		} else if (channel < creads * 2) {
>  			diff = abs(read - prev_val_y);
>  			if (diff < prev_diff_y) {
>  				prev_diff_y = diff;
>  				*y = read;
>  			}
>  			prev_val_y = read;
> +
> +		} else if (channel < creads * 2 + 1) {
> +			*z1 = read;
> +
> +		} else if (channel < creads * 2 + 2) {
> +			*z2 = read;
>  		}
>  	}
>  }
> @@ -307,26 +309,24 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>  	if (status & IRQENB_FIFO0THRES) {
> -		titsc_read_coordinates(ts_dev, &x, &y);
> +
> +		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>  
>  		diffx = abs(x - (ts_dev->bckup_x));
>  		diffy = abs(y - (ts_dev->bckup_y));
>  		ts_dev->bckup_x = x;
>  		ts_dev->bckup_y = y;
>  
> -		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
> -		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
> -
>  		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
>  			/*
>  			 * Calculate pressure using formula
>  			 * Resistance(touch) = x plate resistance *
>  			 * x postion/4096 * ((z2 / z1) - 1)
>  			 */
> -			z = z2 - z1;
> +			z = z1 - z2;
>  			z *= x;
>  			z *= ts_dev->x_plate_resistance;
> -			z /= z1;
> +			z /= z2;
>  			z = (z + 2047) >> 12;
>  
>  			if ((diffx < TSCADC_DELTA_X) &&
> @@ -399,8 +399,8 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
>  	if (err < 0)
>  		return err;
>  
> -	err = of_property_read_u32(node, "ti,steps-to-configure",
> -			&ts_dev->steps_to_configure);
> +	err = of_property_read_u32(node, "ti,coordiante-readouts",
> +			&ts_dev->coordiante_readouts);
>  	if (err < 0)
>  		return err;
>  
> @@ -454,7 +454,8 @@ static int titsc_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
>  	titsc_step_config(ts_dev);
> -	titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure);
> +	titsc_writel(ts_dev, REG_FIFO0THR,
> +			ts_dev->coordiante_readouts * 2 + 2 - 1);
>  
>  	input_dev->name = "ti-tsc";
>  	input_dev->dev.parent = &pdev->dev;
> @@ -526,7 +527,7 @@ static int titsc_resume(struct device *dev)
>  	}
>  	titsc_step_config(ts_dev);
>  	titsc_writel(ts_dev, REG_FIFO0THR,
> -			ts_dev->steps_to_configure);
> +			ts_dev->coordiante_readouts * 2 + 2 - 1);
>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index c985262..4ec52b0 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -30,8 +30,8 @@
>  #define REG_IDLECONFIG		0x058
>  #define REG_CHARGECONFIG	0x05C
>  #define REG_CHARGEDELAY		0x060
> -#define REG_STEPCONFIG(n)	(0x64 + ((n - 1) * 8))
> -#define REG_STEPDELAY(n)	(0x68 + ((n - 1) * 8))
> +#define REG_STEPCONFIG(n)	(0x64 + ((n) * 8))
> +#define REG_STEPDELAY(n)	(0x68 + ((n) * 8))
>  #define REG_FIFO0CNT		0xE4
>  #define REG_FIFO0THR		0xE8
>  #define REG_FIFO1CNT		0xF0
> -- 
> 1.7.10.4
> 

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

Re: [PATCH 2/5] input/ti_am335x_adc: use only FIFO0 and clean up a little

[Dmitry Torokhov]

On Wed, May 29, 2013 at 07:09:53PM +0200, Sebastian Andrzej Siewior wrote:
> The driver programs a threshold of "steps-to-configure" say 5. The
> REG_FIFO0THR registers says it should it be programmed to "threshold
> minus one". The driver does not expect just 5 coordinates but 5 * 2 + 2.
> Multiplied by two because 5 for X and 5 for Y. Plus 2 because we have
> two Z.
> The whole thing works because It reads the 5 coordinates for X and Y and
> split over FIFO0 and FIFO1 and the last element in each FIFO is ignored
> within the loop and read later.
> Nothing guaranties that FIFO1 is ready by the time it is read. In fact I
> could see that that FIFO1 reaturns for Y channels 8,9, 10, 12, 6 and for
> Y channel 7 for Z. The problem is that channel 7 and channel 12 got
> somehow mixed up. The variance filter here maybe tries to get rid of the
> wrong Y values, dunno.
> The other Problem is that FIFO1 is also used by the IIO part leading to
> not wrong results if both (tsc & adc) are used.
> 
> The patch tries to clean up the whole thing a little:
> - Name it "coordiante-readouts" and not "steps-to-configure" and
>   document it properly. Point out that it the hardware does a total of
>   "5 * 2 + 2" reads / steps and not just what is configured.
> 
> - Remove the +1 and -1 in REG_STEPCONFIG, REG_STEPDELAY and its counter
>   part in the for loop. This is just confusing.
> 
> - Use only FIFO0 in TSC. The fifo has space for 64 entries so should be
>   fine.
> 
> - Read the whole FIFO in one function and check the channel.
> 
> - in case we dawdle around, make sure we only read a multiple of our
>   coordinate set. On the second interrupt we will cleanup the remaining
>   enties.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   15 ++--
>  arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
>  drivers/iio/adc/ti_am335x_adc.c                    |    2 +-
>  drivers/input/touchscreen/ti_am335x_tsc.c          |   87 ++++++++++----------
>  include/linux/mfd/ti_am335x_tscadc.h               |    4 +-
>  5 files changed, 58 insertions(+), 52 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index e533e9d..80e04e3 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -6,10 +6,15 @@
>  	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
>  		  support on the platform.
>  	ti,x-plate-resistance: X plate resistance
> -	ti,steps-to-configure: The sequencer supports a total of 16
> -			       programmable steps. A step configured to read a
> -			       single co-ordinate value. Can be applied more
> -			       number of times for better results.
> +	ti,coordiante-readouts: The sequencer supports a total of 16
> +				programmable steps each step is used to
> +				read a single coordinate. A single
> +                                readout is enough but multiple reads can
> +				increase the quality.
> +				A value of 5 means, 5 reads for X, 5 for
> +				Y and 2 for Z (always). This utilises 12
> +				of the 16 software steps available. The
> +				remaining 4 can be used by the ADC.
>  	ti,wire-config: Different boards could have a different order for
>  			connecting wires on touchscreen. We need to provide an
>  			8 bit number where in the 1st four bits represent the
> @@ -28,7 +33,7 @@
>  		tsc {
>  			ti,wires = <4>;
>  			ti,x-plate-resistance = <200>;
> -			ti,steps-to-configure = <5>;
> +			ti,coordiante-readouts = <5>;
>  			ti,wire-config = <0x00 0x11 0x22 0x33>;
>  		};
>  
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index be6a5b2..26fea97 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -250,7 +250,7 @@
>  	tsc {
>  		ti,wires = <4>;
>  		ti,x-plate-resistance = <200>;
> -		ti,steps-to-configure = <5>;
> +		ti,coordiante-readouts = <5>;
>  		ti,wire-config = <0x00 0x11 0x22 0x33>;
>  	};
>  
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index b2f27de..2988840 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -72,7 +72,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  
>  	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>  
> -	for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
> +	for (i = steps; i < TOTAL_STEPS; i++) {
>  		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
>  				stepconfig | STEPCONFIG_INP(channels));
>  		tiadc_writel(adc_dev, REG_STEPDELAY(i),
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 2921163..7c97fc7 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -58,7 +58,7 @@ struct titsc {
>  	unsigned int		bckup_x;
>  	unsigned int		bckup_y;
>  	bool			pen_down;
> -	int			steps_to_configure;
> +	int			coordiante_readouts;
>  	u32			config_inp[4];
>  	int			bit_xp, bit_xn, bit_yp, bit_yn;
>  	int			inp_xp, inp_xn, inp_yp, inp_yn;
> @@ -168,11 +168,8 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  static void titsc_step_config(struct titsc *ts_dev)
>  {
>  	unsigned int	config;
> -	unsigned int	stepenable = 0;
> -	int i, total_steps;
> -
> -	/* Configure the Step registers */
> -	total_steps = 2 * ts_dev->steps_to_configure;
> +	int i;
> +	int end_step;
>  
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_xp;
> @@ -190,7 +187,9 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	for (i = 1; i <= ts_dev->steps_to_configure; i++) {
> +	/* 1 … coordiante_readouts is for X */
> +	end_step = ts_dev->coordiante_readouts;
> +	for (i = 0; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
> @@ -198,7 +197,7 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	config = 0;
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_yn |
> -			STEPCONFIG_INM_ADCREFM | STEPCONFIG_FIFO1;
> +			STEPCONFIG_INM_ADCREFM;
>  	switch (ts_dev->wires) {
>  	case 4:
>  		config |= ts_dev->bit_yp | STEPCONFIG_INP(ts_dev->inp_xp);
> @@ -212,12 +211,13 @@ static void titsc_step_config(struct titsc *ts_dev)
>  		break;
>  	}
>  
> -	for (i = (ts_dev->steps_to_configure + 1); i <= total_steps; i++) {
> +	/* coordiante_readouts … coordiante_readouts * 2 is for Y */
> +	end_step = ts_dev->coordiante_readouts * 2;
> +	for (i = ts_dev->coordiante_readouts; i < end_step; i++) {
>  		titsc_writel(ts_dev, REG_STEPCONFIG(i), config);
>  		titsc_writel(ts_dev, REG_STEPDELAY(i), STEPCONFIG_OPENDLY);
>  	}
>  
> -	config = 0;
>  	/* Charge step configuration */
>  	config = ts_dev->bit_xp | ts_dev->bit_yn |
>  			STEPCHARGE_RFP_XPUL | STEPCHARGE_RFM_XNUR |
> @@ -226,37 +226,40 @@ static void titsc_step_config(struct titsc *ts_dev)
>  	titsc_writel(ts_dev, REG_CHARGECONFIG, config);
>  	titsc_writel(ts_dev, REG_CHARGEDELAY, CHARGEDLY_OPENDLY);
>  
> -	config = 0;
> -	/* Configure to calculate pressure */
> +	/* coordiante_readouts * 2 … coordiante_readouts * 2 + 2 is for Z */
>  	config = STEPCONFIG_MODE_HWSYNC |
>  			STEPCONFIG_AVG_16 | ts_dev->bit_yp |
>  			ts_dev->bit_xn | STEPCONFIG_INM_ADCREFM |
>  			STEPCONFIG_INP(ts_dev->inp_xp);
> -	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 1), config);
> -	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 1),
> +	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
> +	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>  			STEPCONFIG_OPENDLY);
>  
> -	config |= STEPCONFIG_INP(ts_dev->inp_yn) | STEPCONFIG_FIFO1;
> -	titsc_writel(ts_dev, REG_STEPCONFIG(total_steps + 2), config);
> -	titsc_writel(ts_dev, REG_STEPDELAY(total_steps + 2),
> +	end_step++;
> +	config |= STEPCONFIG_INP(ts_dev->inp_yn);
> +	titsc_writel(ts_dev, REG_STEPCONFIG(end_step), config);
> +	titsc_writel(ts_dev, REG_STEPDELAY(end_step),
>  			STEPCONFIG_OPENDLY);
>  
> -	for (i = 0; i <= (total_steps + 2); i++)
> -		stepenable |= 1 << i;
> -	ts_dev->enable_bits = stepenable;
> +	/* The steps1 … end and bit 0 for TS_Charge */
> +	ts_dev->enable_bits = (1 << (end_step + 2)) - 1;
>  
>  	titsc_writel(ts_dev, REG_SE, ts_dev->enable_bits);
>  }
>  
>  static void titsc_read_coordinates(struct titsc *ts_dev,
> -				    unsigned int *x, unsigned int *y)
> +		u32 *x, u32 *y, u32 *z1, u32 *z2)
>  {
>  	unsigned int fifocount = titsc_readl(ts_dev, REG_FIFO0CNT);
>  	unsigned int prev_val_x = ~0, prev_val_y = ~0;
>  	unsigned int prev_diff_x = ~0, prev_diff_y = ~0;
>  	unsigned int read, diff;
>  	unsigned int i, channel;
> +	unsigned int creads = ts_dev->coordiante_readouts;
>  
> +	*z1 = *z2 = 0;
> +	if (fifocount % (creads * 2 + 2))
> +		fifocount -= fifocount % (creads * 2 + 2);
>  	/*
>  	 * Delta filter is used to remove large variations in sampled
>  	 * values from ADC. The filter tries to predict where the next
> @@ -265,32 +268,31 @@ static void titsc_read_coordinates(struct titsc *ts_dev,
>  	 * algorithm compares the difference with that of a present value,
>  	 * if true the value is reported to the sub system.
>  	 */
> -	for (i = 0; i < fifocount - 1; i++) {
> +	for (i = 0; i < fifocount; i++) {
>  		read = titsc_readl(ts_dev, REG_FIFO0);
> -		channel = read & 0xf0000;
> -		channel = channel >> 0x10;
> -		if ((channel >= 0) && (channel < ts_dev->steps_to_configure)) {
> -			read &= 0xfff;
> +		channel = (read & 0xf0000) >> 16;
> +		read &= 0xfff;
> +		if (channel < creads) {
>  			diff = abs(read - prev_val_x);
>  			if (diff < prev_diff_x) {
>  				prev_diff_x = diff;
>  				*x = read;
>  			}
>  			prev_val_x = read;
> -		}
>  
> -		read = titsc_readl(ts_dev, REG_FIFO1);
> -		channel = read & 0xf0000;
> -		channel = channel >> 0x10;
> -		if ((channel >= ts_dev->steps_to_configure) &&
> -			(channel < (2 * ts_dev->steps_to_configure - 1))) {
> -			read &= 0xfff;
> +		} else if (channel < creads * 2) {
>  			diff = abs(read - prev_val_y);
>  			if (diff < prev_diff_y) {
>  				prev_diff_y = diff;
>  				*y = read;
>  			}
>  			prev_val_y = read;
> +
> +		} else if (channel < creads * 2 + 1) {
> +			*z1 = read;
> +
> +		} else if (channel < creads * 2 + 2) {
> +			*z2 = read;
>  		}
>  	}
>  }
> @@ -307,26 +309,24 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>  	if (status & IRQENB_FIFO0THRES) {
> -		titsc_read_coordinates(ts_dev, &x, &y);
> +
> +		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>  
>  		diffx = abs(x - (ts_dev->bckup_x));
>  		diffy = abs(y - (ts_dev->bckup_y));
>  		ts_dev->bckup_x = x;
>  		ts_dev->bckup_y = y;
>  
> -		z1 = titsc_readl(ts_dev, REG_FIFO0) & 0xfff;
> -		z2 = titsc_readl(ts_dev, REG_FIFO1) & 0xfff;
> -
>  		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
>  			/*
>  			 * Calculate pressure using formula
>  			 * Resistance(touch) = x plate resistance *
>  			 * x postion/4096 * ((z2 / z1) - 1)
>  			 */
> -			z = z2 - z1;
> +			z = z1 - z2;
>  			z *= x;
>  			z *= ts_dev->x_plate_resistance;
> -			z /= z1;
> +			z /= z2;
>  			z = (z + 2047) >> 12;
>  
>  			if ((diffx < TSCADC_DELTA_X) &&
> @@ -399,8 +399,8 @@ static int titsc_parse_dt(struct ti_tscadc_dev *tscadc_dev,
>  	if (err < 0)
>  		return err;
>  
> -	err = of_property_read_u32(node, "ti,steps-to-configure",
> -			&ts_dev->steps_to_configure);
> +	err = of_property_read_u32(node, "ti,coordiante-readouts",
> +			&ts_dev->coordiante_readouts);
>  	if (err < 0)
>  		return err;
>  
> @@ -454,7 +454,8 @@ static int titsc_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
>  	titsc_step_config(ts_dev);
> -	titsc_writel(ts_dev, REG_FIFO0THR, ts_dev->steps_to_configure);
> +	titsc_writel(ts_dev, REG_FIFO0THR,
> +			ts_dev->coordiante_readouts * 2 + 2 - 1);
>  
>  	input_dev->name = "ti-tsc";
>  	input_dev->dev.parent = &pdev->dev;
> @@ -526,7 +527,7 @@ static int titsc_resume(struct device *dev)
>  	}
>  	titsc_step_config(ts_dev);
>  	titsc_writel(ts_dev, REG_FIFO0THR,
> -			ts_dev->steps_to_configure);
> +			ts_dev->coordiante_readouts * 2 + 2 - 1);
>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index c985262..4ec52b0 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -30,8 +30,8 @@
>  #define REG_IDLECONFIG		0x058
>  #define REG_CHARGECONFIG	0x05C
>  #define REG_CHARGEDELAY		0x060
> -#define REG_STEPCONFIG(n)	(0x64 + ((n - 1) * 8))
> -#define REG_STEPDELAY(n)	(0x68 + ((n - 1) * 8))
> +#define REG_STEPCONFIG(n)	(0x64 + ((n) * 8))
> +#define REG_STEPDELAY(n)	(0x68 + ((n) * 8))
>  #define REG_FIFO0CNT		0xE4
>  #define REG_FIFO0THR		0xE8
>  #define REG_FIFO1CNT		0xF0
> -- 
> 1.7.10.4
> 

-- 
Dmitry

[PATCH 5/5] iio/ti_am335x_adc: check if we found the value

[Sebastian Andrzej Siewior]

Usually we get all the values we wanted but it is possible, that te ADC
unit is busy performing the conversation for the HW events. In that case
-EBUSY is returned and the user may re-call the function.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 drivers/iio/adc/ti_am335x_adc.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index df2de4c..6a00874 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -161,6 +161,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	int i;
 	unsigned int fifo1count, read;
 	u32 step = UINT_MAX;
+	bool found = false;
 
 	/*
 	 * When the sub-system is first enabled,
@@ -185,10 +186,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (i = 0; i < fifo1count; i++) {
 		read = tiadc_readl(adc_dev, REG_FIFO1);
-		if (read >> 16 == step)
+		if (read >> 16 == step) {
 			*val = read & 0xfff;
+			found = true;
+		}
 	}
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
+	if (found == false)
+		return -EBUSY;
 
 	return IIO_VAL_INT;
 }
-- 
1.7.10.4

[PATCH 5/5] iio/ti_am335x_adc: check if we found the value

[Sebastian Andrzej Siewior]

Usually we get all the values we wanted but it is possible, that te ADC
unit is busy performing the conversation for the HW events. In that case
-EBUSY is returned and the user may re-call the function.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index df2de4c..6a00874 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -161,6 +161,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	int i;
 	unsigned int fifo1count, read;
 	u32 step = UINT_MAX;
+	bool found = false;
 
 	/*
 	 * When the sub-system is first enabled,
@@ -185,10 +186,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (i = 0; i < fifo1count; i++) {
 		read = tiadc_readl(adc_dev, REG_FIFO1);
-		if (read >> 16 == step)
+		if (read >> 16 == step) {
 			*val = read & 0xfff;
+			found = true;
+		}
 	}
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
+	if (found == false)
+		return -EBUSY;
 
 	return IIO_VAL_INT;
 }
-- 
1.7.10.4

Re: [PATCH 5/5] iio/ti_am335x_adc: check if we found the value

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> Usually we get all the values we wanted but it is possible, that te ADC
> unit is busy performing the conversation for the HW events. In that case
> -EBUSY is returned and the user may re-call the function.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/iio/adc/ti_am335x_adc.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index df2de4c..6a00874 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -161,6 +161,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	int i;
>  	unsigned int fifo1count, read;
>  	u32 step = UINT_MAX;
> +	bool found = false;
>  
>  	/*
>  	 * When the sub-system is first enabled,
> @@ -185,10 +186,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>  	for (i = 0; i < fifo1count; i++) {
>  		read = tiadc_readl(adc_dev, REG_FIFO1);
> -		if (read >> 16 == step)
> +		if (read >> 16 == step) {
>  			*val = read & 0xfff;
> +			found = true;
> +		}
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
> +	if (found == false)
> +		return -EBUSY;
>  
>  	return IIO_VAL_INT;
>  }
> 

Re: [PATCH 5/5] iio/ti_am335x_adc: check if we found the value

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> Usually we get all the values we wanted but it is possible, that te ADC
> unit is busy performing the conversation for the HW events. In that case
> -EBUSY is returned and the user may re-call the function.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/iio/adc/ti_am335x_adc.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index df2de4c..6a00874 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -161,6 +161,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	int i;
>  	unsigned int fifo1count, read;
>  	u32 step = UINT_MAX;
> +	bool found = false;
>  
>  	/*
>  	 * When the sub-system is first enabled,
> @@ -185,10 +186,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>  	for (i = 0; i < fifo1count; i++) {
>  		read = tiadc_readl(adc_dev, REG_FIFO1);
> -		if (read >> 16 == step)
> +		if (read >> 16 == step) {
>  			*val = read & 0xfff;
> +			found = true;
> +		}
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
> +	if (found == false)
> +		return -EBUSY;
>  
>  	return IIO_VAL_INT;
>  }
> 

[PATCH 3/5] input/ti_am335x_tsc: fold regbit_map() and simplfy

[Sebastian Andrzej Siewior]

If we put the values which are looked up by regbit_map() directly in the
config array then we can remove the function.
And now when I look at it I don't understand why the array has to have
two dimensions. One does it, too. And while at it, the description says
that AIN0 … AIN7 can be used so allow this.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   76 ++++++++---------------------
 1 file changed, 19 insertions(+), 57 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 7c97fc7..63cee57 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -37,15 +37,11 @@
 #define TSCADC_DELTA_X		15
 #define TSCADC_DELTA_Y		15
 
-/*
- * Refer to function regbit_map() to
- * map the values in the matrix.
- */
-static int config[4][4] = {
-		{1,	0,	1,	0},
-		{2,	3,	2,	3},
-		{4,	5,	4,	5},
-		{0,	6,	0,	6}
+static const int config_pins[] = {
+		XPP,
+		XNN,
+		YPP,
+		YNN,
 };
 
 struct titsc {
@@ -79,45 +75,11 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
 	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
-/*
- * Each of the analog lines are mapped
- * with one or two register bits,
- * which can be either pulled high/low
- * depending on the value to be read.
- */
-static int regbit_map(int val)
-{
-	int map_bits = 0;
-
-	switch (val) {
-	case 1:
-		map_bits = XPP;
-		break;
-	case 2:
-		map_bits = XNP;
-		break;
-	case 3:
-		map_bits = XNN;
-		break;
-	case 4:
-		map_bits = YPP;
-		break;
-	case 5:
-		map_bits = YPN;
-		break;
-	case 6:
-		map_bits = YNN;
-		break;
-	}
-
-	return map_bits;
-}
-
 static int titsc_config_wires(struct titsc *ts_dev)
 {
 	u32 analog_line[4];
 	u32 wire_order[4];
-	int i, temp_bits;
+	int i, bit_cfg;
 
 	for (i = 0; i < 4; i++) {
 		/*
@@ -126,9 +88,9 @@ static int titsc_config_wires(struct titsc *ts_dev)
 		 */
 		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
 		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
-		if (WARN_ON(analog_line[i] > 4))
+		if (WARN_ON(analog_line[i] > 7))
 				return -EINVAL;
-		if (WARN_ON(wire_order[i] > 4))
+		if (WARN_ON(wire_order[i] > ARRAY_SIZE(config_pins)))
 				return -EINVAL;
 	}
 
@@ -138,27 +100,27 @@ static int titsc_config_wires(struct titsc *ts_dev)
 
 		an_line = analog_line[i];
 		wi_order = wire_order[i];
-		temp_bits = config[an_line][wi_order];
-		if (temp_bits == 0)
+		bit_cfg = config_pins[wi_order];
+		if (bit_cfg == 0)
 			return -EINVAL;
-		switch (wire_order[i]) {
+		switch (wi_order) {
 		case 0:
-			ts_dev->bit_xp = regbit_map(temp_bits);
-			ts_dev->inp_xp = analog_line[i];
+			ts_dev->bit_xp = bit_cfg;
+			ts_dev->inp_xp = an_line;
 			break;
 
 		case 1:
-			ts_dev->bit_xn = regbit_map(temp_bits);
-			ts_dev->inp_xn = analog_line[i];
+			ts_dev->bit_xn = bit_cfg;
+			ts_dev->inp_xn = an_line;
 			break;
 
 		case 2:
-			ts_dev->bit_yp = regbit_map(temp_bits);
-			ts_dev->inp_yp = analog_line[i];
+			ts_dev->bit_yp = bit_cfg;
+			ts_dev->inp_yp = an_line;
 			break;
 		case 3:
-			ts_dev->bit_yn = regbit_map(temp_bits);
-			ts_dev->inp_yn = analog_line[i];
+			ts_dev->bit_yn = bit_cfg;
+			ts_dev->inp_yn = an_line;
 			break;
 		}
 	}
-- 
1.7.10.4

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

[PATCH 3/5] input/ti_am335x_tsc: fold regbit_map() and simplfy

[Sebastian Andrzej Siewior]

If we put the values which are looked up by regbit_map() directly in the
config array then we can remove the function.
And now when I look at it I don't understand why the array has to have
two dimensions. One does it, too. And while at it, the description says
that AIN0 … AIN7 can be used so allow this.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   76 ++++++++---------------------
 1 file changed, 19 insertions(+), 57 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 7c97fc7..63cee57 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -37,15 +37,11 @@
 #define TSCADC_DELTA_X		15
 #define TSCADC_DELTA_Y		15
 
-/*
- * Refer to function regbit_map() to
- * map the values in the matrix.
- */
-static int config[4][4] = {
-		{1,	0,	1,	0},
-		{2,	3,	2,	3},
-		{4,	5,	4,	5},
-		{0,	6,	0,	6}
+static const int config_pins[] = {
+		XPP,
+		XNN,
+		YPP,
+		YNN,
 };
 
 struct titsc {
@@ -79,45 +75,11 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
 	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
-/*
- * Each of the analog lines are mapped
- * with one or two register bits,
- * which can be either pulled high/low
- * depending on the value to be read.
- */
-static int regbit_map(int val)
-{
-	int map_bits = 0;
-
-	switch (val) {
-	case 1:
-		map_bits = XPP;
-		break;
-	case 2:
-		map_bits = XNP;
-		break;
-	case 3:
-		map_bits = XNN;
-		break;
-	case 4:
-		map_bits = YPP;
-		break;
-	case 5:
-		map_bits = YPN;
-		break;
-	case 6:
-		map_bits = YNN;
-		break;
-	}
-
-	return map_bits;
-}
-
 static int titsc_config_wires(struct titsc *ts_dev)
 {
 	u32 analog_line[4];
 	u32 wire_order[4];
-	int i, temp_bits;
+	int i, bit_cfg;
 
 	for (i = 0; i < 4; i++) {
 		/*
@@ -126,9 +88,9 @@ static int titsc_config_wires(struct titsc *ts_dev)
 		 */
 		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
 		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
-		if (WARN_ON(analog_line[i] > 4))
+		if (WARN_ON(analog_line[i] > 7))
 				return -EINVAL;
-		if (WARN_ON(wire_order[i] > 4))
+		if (WARN_ON(wire_order[i] > ARRAY_SIZE(config_pins)))
 				return -EINVAL;
 	}
 
@@ -138,27 +100,27 @@ static int titsc_config_wires(struct titsc *ts_dev)
 
 		an_line = analog_line[i];
 		wi_order = wire_order[i];
-		temp_bits = config[an_line][wi_order];
-		if (temp_bits == 0)
+		bit_cfg = config_pins[wi_order];
+		if (bit_cfg == 0)
 			return -EINVAL;
-		switch (wire_order[i]) {
+		switch (wi_order) {
 		case 0:
-			ts_dev->bit_xp = regbit_map(temp_bits);
-			ts_dev->inp_xp = analog_line[i];
+			ts_dev->bit_xp = bit_cfg;
+			ts_dev->inp_xp = an_line;
 			break;
 
 		case 1:
-			ts_dev->bit_xn = regbit_map(temp_bits);
-			ts_dev->inp_xn = analog_line[i];
+			ts_dev->bit_xn = bit_cfg;
+			ts_dev->inp_xn = an_line;
 			break;
 
 		case 2:
-			ts_dev->bit_yp = regbit_map(temp_bits);
-			ts_dev->inp_yp = analog_line[i];
+			ts_dev->bit_yp = bit_cfg;
+			ts_dev->inp_yp = an_line;
 			break;
 		case 3:
-			ts_dev->bit_yn = regbit_map(temp_bits);
-			ts_dev->inp_yn = analog_line[i];
+			ts_dev->bit_yn = bit_cfg;
+			ts_dev->inp_yn = an_line;
 			break;
 		}
 	}
-- 
1.7.10.4

Re: [PATCH 3/5] input/ti_am335x_tsc: fold regbit_map() and simplfy

[Dmitry Torokhov]

On Wed, May 29, 2013 at 07:09:54PM +0200, Sebastian Andrzej Siewior wrote:
> If we put the values which are looked up by regbit_map() directly in the
> config array then we can remove the function.
> And now when I look at it I don't understand why the array has to have
> two dimensions. One does it, too. And while at it, the description says
> that AIN0 … AIN7 can be used so allow this.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   76 ++++++++---------------------
>  1 file changed, 19 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 7c97fc7..63cee57 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -37,15 +37,11 @@
>  #define TSCADC_DELTA_X		15
>  #define TSCADC_DELTA_Y		15
>  
> -/*
> - * Refer to function regbit_map() to
> - * map the values in the matrix.
> - */
> -static int config[4][4] = {
> -		{1,	0,	1,	0},
> -		{2,	3,	2,	3},
> -		{4,	5,	4,	5},
> -		{0,	6,	0,	6}
> +static const int config_pins[] = {
> +		XPP,
> +		XNN,
> +		YPP,
> +		YNN,
>  };
>  
>  struct titsc {
> @@ -79,45 +75,11 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
>  	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
> -/*
> - * Each of the analog lines are mapped
> - * with one or two register bits,
> - * which can be either pulled high/low
> - * depending on the value to be read.
> - */
> -static int regbit_map(int val)
> -{
> -	int map_bits = 0;
> -
> -	switch (val) {
> -	case 1:
> -		map_bits = XPP;
> -		break;
> -	case 2:
> -		map_bits = XNP;
> -		break;
> -	case 3:
> -		map_bits = XNN;
> -		break;
> -	case 4:
> -		map_bits = YPP;
> -		break;
> -	case 5:
> -		map_bits = YPN;
> -		break;
> -	case 6:
> -		map_bits = YNN;
> -		break;
> -	}
> -
> -	return map_bits;
> -}
> -
>  static int titsc_config_wires(struct titsc *ts_dev)
>  {
>  	u32 analog_line[4];
>  	u32 wire_order[4];
> -	int i, temp_bits;
> +	int i, bit_cfg;
>  
>  	for (i = 0; i < 4; i++) {
>  		/*
> @@ -126,9 +88,9 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  		 */
>  		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
>  		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
> -		if (WARN_ON(analog_line[i] > 4))
> +		if (WARN_ON(analog_line[i] > 7))
>  				return -EINVAL;
> -		if (WARN_ON(wire_order[i] > 4))
> +		if (WARN_ON(wire_order[i] > ARRAY_SIZE(config_pins)))
>  				return -EINVAL;
>  	}
>  
> @@ -138,27 +100,27 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  
>  		an_line = analog_line[i];
>  		wi_order = wire_order[i];
> -		temp_bits = config[an_line][wi_order];
> -		if (temp_bits == 0)
> +		bit_cfg = config_pins[wi_order];
> +		if (bit_cfg == 0)
>  			return -EINVAL;
> -		switch (wire_order[i]) {
> +		switch (wi_order) {
>  		case 0:
> -			ts_dev->bit_xp = regbit_map(temp_bits);
> -			ts_dev->inp_xp = analog_line[i];
> +			ts_dev->bit_xp = bit_cfg;
> +			ts_dev->inp_xp = an_line;
>  			break;
>  
>  		case 1:
> -			ts_dev->bit_xn = regbit_map(temp_bits);
> -			ts_dev->inp_xn = analog_line[i];
> +			ts_dev->bit_xn = bit_cfg;
> +			ts_dev->inp_xn = an_line;
>  			break;
>  
>  		case 2:
> -			ts_dev->bit_yp = regbit_map(temp_bits);
> -			ts_dev->inp_yp = analog_line[i];
> +			ts_dev->bit_yp = bit_cfg;
> +			ts_dev->inp_yp = an_line;
>  			break;
>  		case 3:
> -			ts_dev->bit_yn = regbit_map(temp_bits);
> -			ts_dev->inp_yn = analog_line[i];
> +			ts_dev->bit_yn = bit_cfg;
> +			ts_dev->inp_yn = an_line;
>  			break;
>  		}
>  	}
> -- 
> 1.7.10.4
> 

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

Re: [PATCH 3/5] input/ti_am335x_tsc: fold regbit_map() and simplfy

[Dmitry Torokhov]

On Wed, May 29, 2013 at 07:09:54PM +0200, Sebastian Andrzej Siewior wrote:
> If we put the values which are looked up by regbit_map() directly in the
> config array then we can remove the function.
> And now when I look at it I don't understand why the array has to have
> two dimensions. One does it, too. And while at it, the description says
> that AIN0 … AIN7 can be used so allow this.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   76 ++++++++---------------------
>  1 file changed, 19 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 7c97fc7..63cee57 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -37,15 +37,11 @@
>  #define TSCADC_DELTA_X		15
>  #define TSCADC_DELTA_Y		15
>  
> -/*
> - * Refer to function regbit_map() to
> - * map the values in the matrix.
> - */
> -static int config[4][4] = {
> -		{1,	0,	1,	0},
> -		{2,	3,	2,	3},
> -		{4,	5,	4,	5},
> -		{0,	6,	0,	6}
> +static const int config_pins[] = {
> +		XPP,
> +		XNN,
> +		YPP,
> +		YNN,
>  };
>  
>  struct titsc {
> @@ -79,45 +75,11 @@ static void titsc_writel(struct titsc *tsc, unsigned int reg,
>  	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
> -/*
> - * Each of the analog lines are mapped
> - * with one or two register bits,
> - * which can be either pulled high/low
> - * depending on the value to be read.
> - */
> -static int regbit_map(int val)
> -{
> -	int map_bits = 0;
> -
> -	switch (val) {
> -	case 1:
> -		map_bits = XPP;
> -		break;
> -	case 2:
> -		map_bits = XNP;
> -		break;
> -	case 3:
> -		map_bits = XNN;
> -		break;
> -	case 4:
> -		map_bits = YPP;
> -		break;
> -	case 5:
> -		map_bits = YPN;
> -		break;
> -	case 6:
> -		map_bits = YNN;
> -		break;
> -	}
> -
> -	return map_bits;
> -}
> -
>  static int titsc_config_wires(struct titsc *ts_dev)
>  {
>  	u32 analog_line[4];
>  	u32 wire_order[4];
> -	int i, temp_bits;
> +	int i, bit_cfg;
>  
>  	for (i = 0; i < 4; i++) {
>  		/*
> @@ -126,9 +88,9 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  		 */
>  		analog_line[i] = (ts_dev->config_inp[i] & 0xF0) >> 4;
>  		wire_order[i] = ts_dev->config_inp[i] & 0x0F;
> -		if (WARN_ON(analog_line[i] > 4))
> +		if (WARN_ON(analog_line[i] > 7))
>  				return -EINVAL;
> -		if (WARN_ON(wire_order[i] > 4))
> +		if (WARN_ON(wire_order[i] > ARRAY_SIZE(config_pins)))
>  				return -EINVAL;
>  	}
>  
> @@ -138,27 +100,27 @@ static int titsc_config_wires(struct titsc *ts_dev)
>  
>  		an_line = analog_line[i];
>  		wi_order = wire_order[i];
> -		temp_bits = config[an_line][wi_order];
> -		if (temp_bits == 0)
> +		bit_cfg = config_pins[wi_order];
> +		if (bit_cfg == 0)
>  			return -EINVAL;
> -		switch (wire_order[i]) {
> +		switch (wi_order) {
>  		case 0:
> -			ts_dev->bit_xp = regbit_map(temp_bits);
> -			ts_dev->inp_xp = analog_line[i];
> +			ts_dev->bit_xp = bit_cfg;
> +			ts_dev->inp_xp = an_line;
>  			break;
>  
>  		case 1:
> -			ts_dev->bit_xn = regbit_map(temp_bits);
> -			ts_dev->inp_xn = analog_line[i];
> +			ts_dev->bit_xn = bit_cfg;
> +			ts_dev->inp_xn = an_line;
>  			break;
>  
>  		case 2:
> -			ts_dev->bit_yp = regbit_map(temp_bits);
> -			ts_dev->inp_yp = analog_line[i];
> +			ts_dev->bit_yp = bit_cfg;
> +			ts_dev->inp_yp = an_line;
>  			break;
>  		case 3:
> -			ts_dev->bit_yn = regbit_map(temp_bits);
> -			ts_dev->inp_yn = analog_line[i];
> +			ts_dev->bit_yn = bit_cfg;
> +			ts_dev->inp_yn = an_line;
>  			break;
>  		}
>  	}
> -- 
> 1.7.10.4
> 

-- 
Dmitry

[PATCH 4/5] iio/ti_am335x_adc: Allow to specify input line

[Sebastian Andrzej Siewior]

The TSC part allows to specify the input lines. The IIO part assumes
that it usues always the last few, that means if IIO has adc-channels
set to 2 it will use channel 6 and 7. However it might make sense to use
only 6.
This patch changes the device property (which was introduced recently
and was never in an official release) in a way that the user can specify
which of the AIN lines should be used. In Addition to this, the name is
now AINx where x is the channel number i.e. for AIN6 we would have 6.
Prior this, it always started counting at 0 which is confusing. In
addition to this, it also checks for correct step number during reading
and does not rely on proper FIFO depth.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |    5 +-
 arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
 drivers/iio/adc/ti_am335x_adc.c                    |   59 +++++++++++++-------
 drivers/mfd/ti_am335x_tscadc.c                     |   20 ++++++-
 4 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 80e04e3..204c9cc 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -25,7 +25,8 @@
 			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
 			XP  = 0, XN = 1, YP = 2, YN = 3.
 - child "adc"
-	ti,adc-channels: Number of analog inputs available for ADC
+	ti,adc-channels: List of analog inputs available for ADC.
+			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
 
 Example:
 	tscadc: tscadc@44e0d000 {
@@ -38,6 +39,6 @@
 		};
 
 		adc {
-			ti,adc-channels = <4>;
+			ti,adc-channels = <4 5 6 7>;
 		};
 	}
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 26fea97..0fa4c7f 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -255,6 +255,6 @@
 	};
 
 	adc {
-		ti,adc-channels = <4>;
+		ti,adc-channels = <4 5 6 7>;
 	};
 };
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 2988840..df2de4c 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -33,6 +33,8 @@
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
 	int channels;
+	u8 channel_line[8];
+	u8 channel_step[8];
 	char *buf;
 	struct iio_map *map;
 };
@@ -55,7 +57,7 @@ static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
 static void tiadc_step_config(struct tiadc_device *adc_dev)
 {
 	unsigned int stepconfig;
-	int i, channels = 0, steps;
+	int i, steps;
 
 	/*
 	 * There are 16 configurable steps and 8 analog input
@@ -68,16 +70,18 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 	 */
 
 	steps = TOTAL_STEPS - adc_dev->channels;
-	channels = TOTAL_CHANNELS - adc_dev->channels;
-
 	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
 
-	for (i = steps; i < TOTAL_STEPS; i++) {
-		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
-				stepconfig | STEPCONFIG_INP(channels));
-		tiadc_writel(adc_dev, REG_STEPDELAY(i),
+	for (i = 0; i < adc_dev->channels; i++) {
+		int chan;
+
+		chan = adc_dev->channel_line[i];
+		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
+				stepconfig | STEPCONFIG_INP(chan));
+		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
 				STEPCONFIG_OPENDLY);
-		channels++;
+		adc_dev->channel_step[i] = steps;
+		steps++;
 	}
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
 }
@@ -101,11 +105,11 @@ static int tiadc_channel_init(struct iio_dev *indio_dev,
 	chan = chan_array;
 	for (i = 0; i < channels; i++, chan++, s += len + 1) {
 
-		len = sprintf(s, "AIN%d", i);
+		len = sprintf(s, "AIN%d", adc_dev->channel_line[i]);
 
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
-		chan->channel = i;
+		chan->channel = adc_dev->channel_line[i];
 		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 		chan->datasheet_name = s;
 		chan->scan_type.sign = 'u';
@@ -155,7 +159,8 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	int i;
-	unsigned int fifo1count, readx1;
+	unsigned int fifo1count, read;
+	u32 step = UINT_MAX;
 
 	/*
 	 * When the sub-system is first enabled,
@@ -168,11 +173,20 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	 * Hence we need to flush out this data.
 	 */
 
+	for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
+		if (chan->channel == adc_dev->channel_line[i]) {
+			step = adc_dev->channel_step[i];
+			break;
+		}
+	}
+	if (WARN_ON_ONCE(step == UINT_MAX))
+		return -EINVAL;
+
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (i = 0; i < fifo1count; i++) {
-		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
-		if (i == chan->channel)
-			*val = readx1 & 0xfff;
+		read = tiadc_readl(adc_dev, REG_FIFO1);
+		if (read >> 16 == step)
+			*val = read & 0xfff;
 	}
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
 
@@ -189,8 +203,11 @@ static int tiadc_probe(struct platform_device *pdev)
 	struct tiadc_device	*adc_dev;
 	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
 	struct device_node	*node = tscadc_dev->dev->of_node;
+	struct property		*prop;
+	const __be32		*cur;
 	int			err;
-	u32			val32;
+	u32			val;
+	int channels		= 0;
 
 	if (!node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
@@ -212,11 +229,13 @@ static int tiadc_probe(struct platform_device *pdev)
 		err = -EINVAL;
 		goto err_free_device;
 	}
-	err = of_property_read_u32(node,
-			"ti,adc-channels", &val32);
-	if (err < 0)
-		goto err_free_device;
-	adc_dev->channels = val32;
+
+	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
+
+		adc_dev->channel_line[channels] = val;
+		channels++;
+	}
+	adc_dev->channels = channels;
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5e4076f..b228c41 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -66,9 +66,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct clk		*clk;
 	struct device_node	*node = pdev->dev.of_node;
 	struct mfd_cell		*cell;
+	struct property         *prop;
+	const __be32            *cur;
+	u32			val;
 	int			err, ctrl;
 	int			clk_value, clock_rate;
 	int			tsc_wires = 0, adc_channels = 0, total_channels;
+	int			readouts = 0;
 
 	if (!pdev->dev.of_node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
@@ -77,16 +81,26 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
 	of_property_read_u32(node, "ti,wires", &tsc_wires);
+	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
-	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
-
+	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
+		adc_channels++;
+		if (val > 7) {
+			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
+					val);
+			return -EINVAL;
+		}
+	}
 	total_channels = tsc_wires + adc_channels;
-
 	if (total_channels > 8) {
 		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
 		return -EINVAL;
 	}
+	if (readouts * 2 + 2 + adc_channels > 16) {
+		dev_err(&pdev->dev, "Too many step configurations requested\n");
+		return -EINVAL;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-- 
1.7.10.4

Re: [PATCH 4/5] iio/ti_am335x_adc: Allow to specify input line

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> The TSC part allows to specify the input lines. The IIO part assumes
> that it usues always the last few, that means if IIO has adc-channels
> set to 2 it will use channel 6 and 7. However it might make sense to use
> only 6.
> This patch changes the device property (which was introduced recently
> and was never in an official release) in a way that the user can specify
> which of the AIN lines should be used. In Addition to this, the name is
> now AINx where x is the channel number i.e. for AIN6 we would have 6.
> Prior this, it always started counting at 0 which is confusing. In
> addition to this, it also checks for correct step number during reading
> and does not rely on proper FIFO depth.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
One nit pick below.
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |    5 +-
>  arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
>  drivers/iio/adc/ti_am335x_adc.c                    |   59 +++++++++++++-------
>  drivers/mfd/ti_am335x_tscadc.c                     |   20 ++++++-
>  4 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index 80e04e3..204c9cc 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -25,7 +25,8 @@
>  			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>  			XP  = 0, XN = 1, YP = 2, YN = 3.
>  - child "adc"
> -	ti,adc-channels: Number of analog inputs available for ADC
> +	ti,adc-channels: List of analog inputs available for ADC.
> +			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>
>  Example:
>  	tscadc: tscadc@44e0d000 {
> @@ -38,6 +39,6 @@
>  		};
>
>  		adc {
> -			ti,adc-channels = <4>;
> +			ti,adc-channels = <4 5 6 7>;
>  		};
>  	}
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index 26fea97..0fa4c7f 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -255,6 +255,6 @@
>  	};
>
>  	adc {
> -		ti,adc-channels = <4>;
> +		ti,adc-channels = <4 5 6 7>;
>  	};
>  };
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 2988840..df2de4c 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -33,6 +33,8 @@
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
>  	int channels;
> +	u8 channel_line[8];
> +	u8 channel_step[8];
>  	char *buf;
>  	struct iio_map *map;
>  };
> @@ -55,7 +57,7 @@ static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
>  static void tiadc_step_config(struct tiadc_device *adc_dev)
>  {
>  	unsigned int stepconfig;
> -	int i, channels = 0, steps;
> +	int i, steps;
>
>  	/*
>  	 * There are 16 configurable steps and 8 analog input
> @@ -68,16 +70,18 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	 */
>
>  	steps = TOTAL_STEPS - adc_dev->channels;
> -	channels = TOTAL_CHANNELS - adc_dev->channels;
> -
>  	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> -	for (i = steps; i < TOTAL_STEPS; i++) {
> -		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
> -				stepconfig | STEPCONFIG_INP(channels));
> -		tiadc_writel(adc_dev, REG_STEPDELAY(i),
> +	for (i = 0; i < adc_dev->channels; i++) {
> +		int chan;
> +
> +		chan = adc_dev->channel_line[i];
> +		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
> +				stepconfig | STEPCONFIG_INP(chan));
> +		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
>  				STEPCONFIG_OPENDLY);
> -		channels++;
> +		adc_dev->channel_step[i] = steps;
> +		steps++;
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>  }
> @@ -101,11 +105,11 @@ static int tiadc_channel_init(struct iio_dev *indio_dev,
>  	chan = chan_array;
>  	for (i = 0; i < channels; i++, chan++, s += len + 1) {
>
> -		len = sprintf(s, "AIN%d", i);
> +		len = sprintf(s, "AIN%d", adc_dev->channel_line[i]);
>
>  		chan->type = IIO_VOLTAGE;
>  		chan->indexed = 1;
> -		chan->channel = i;
> +		chan->channel = adc_dev->channel_line[i];
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan->datasheet_name = s;
>  		chan->scan_type.sign = 'u';
> @@ -155,7 +159,8 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	int i;
> -	unsigned int fifo1count, readx1;
> +	unsigned int fifo1count, read;
> +	u32 step = UINT_MAX;
>
>  	/*
>  	 * When the sub-system is first enabled,
> @@ -168,11 +173,20 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	 * Hence we need to flush out this data.
>  	 */
>
> +	for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
> +		if (chan->channel == adc_dev->channel_line[i]) {
> +			step = adc_dev->channel_step[i];
> +			break;
> +		}
> +	}
> +	if (WARN_ON_ONCE(step == UINT_MAX))
> +		return -EINVAL;
> +
>  	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>  	for (i = 0; i < fifo1count; i++) {
> -		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
> -		if (i == chan->channel)
> -			*val = readx1 & 0xfff;
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +		if (read >> 16 == step)
> +			*val = read & 0xfff;
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>
> @@ -189,8 +203,11 @@ static int tiadc_probe(struct platform_device *pdev)
>  	struct tiadc_device	*adc_dev;
>  	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
>  	struct device_node	*node = tscadc_dev->dev->of_node;
> +	struct property		*prop;
> +	const __be32		*cur;
>  	int			err;
> -	u32			val32;
> +	u32			val;
> +	int channels		= 0;
>
>  	if (!node) {
>  		dev_err(&pdev->dev, "Could not find valid DT data.\n");
> @@ -212,11 +229,13 @@ static int tiadc_probe(struct platform_device *pdev)
>  		err = -EINVAL;
>  		goto err_free_device;
>  	}
> -	err = of_property_read_u32(node,
> -			"ti,adc-channels", &val32);
> -	if (err < 0)
> -		goto err_free_device;
> -	adc_dev->channels = val32;
> +
> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> +
> +		adc_dev->channel_line[channels] = val;
> +		channels++;
> +	}
> +	adc_dev->channels = channels;
>
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 5e4076f..b228c41 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -66,9 +66,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	struct clk		*clk;
>  	struct device_node	*node = pdev->dev.of_node;
>  	struct mfd_cell		*cell;
> +	struct property         *prop;
> +	const __be32            *cur;
> +	u32			val;
>  	int			err, ctrl;
>  	int			clk_value, clock_rate;
>  	int			tsc_wires = 0, adc_channels = 0, total_channels;
> +	int			readouts = 0;
>
>  	if (!pdev->dev.of_node) {
>  		dev_err(&pdev->dev, "Could not find valid DT data.\n");
> @@ -77,16 +81,26 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>
>  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>  	of_property_read_u32(node, "ti,wires", &tsc_wires);
> +	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
>
>  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
> -	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
> -
> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> +		adc_channels++;
> +		if (val > 7) {
> +			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
> +					val);
> +			return -EINVAL;
> +		}
> +	}
>  	total_channels = tsc_wires + adc_channels;
> -
Should not change this whitespace in this sort of patch.
>  	if (total_channels > 8) {
>  		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
>  		return -EINVAL;
>  	}
> +	if (readouts * 2 + 2 + adc_channels > 16) {
> +		dev_err(&pdev->dev, "Too many step configurations requested\n");
> +		return -EINVAL;
> +	}
>
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>

Re: [PATCH 4/5] iio/ti_am335x_adc: Allow to specify input line

[Jonathan Cameron]

On 05/29/2013 06:09 PM, Sebastian Andrzej Siewior wrote:
> The TSC part allows to specify the input lines. The IIO part assumes
> that it usues always the last few, that means if IIO has adc-channels
> set to 2 it will use channel 6 and 7. However it might make sense to use
> only 6.
> This patch changes the device property (which was introduced recently
> and was never in an official release) in a way that the user can specify
> which of the AIN lines should be used. In Addition to this, the name is
> now AINx where x is the channel number i.e. for AIN6 we would have 6.
> Prior this, it always started counting at 0 which is confusing. In
> addition to this, it also checks for correct step number during reading
> and does not rely on proper FIFO depth.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
One nit pick below.
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |    5 +-
>  arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
>  drivers/iio/adc/ti_am335x_adc.c                    |   59 +++++++++++++-------
>  drivers/mfd/ti_am335x_tscadc.c                     |   20 ++++++-
>  4 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index 80e04e3..204c9cc 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -25,7 +25,8 @@
>  			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>  			XP  = 0, XN = 1, YP = 2, YN = 3.
>  - child "adc"
> -	ti,adc-channels: Number of analog inputs available for ADC
> +	ti,adc-channels: List of analog inputs available for ADC.
> +			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>
>  Example:
>  	tscadc: tscadc@44e0d000 {
> @@ -38,6 +39,6 @@
>  		};
>
>  		adc {
> -			ti,adc-channels = <4>;
> +			ti,adc-channels = <4 5 6 7>;
>  		};
>  	}
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index 26fea97..0fa4c7f 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -255,6 +255,6 @@
>  	};
>
>  	adc {
> -		ti,adc-channels = <4>;
> +		ti,adc-channels = <4 5 6 7>;
>  	};
>  };
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 2988840..df2de4c 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -33,6 +33,8 @@
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
>  	int channels;
> +	u8 channel_line[8];
> +	u8 channel_step[8];
>  	char *buf;
>  	struct iio_map *map;
>  };
> @@ -55,7 +57,7 @@ static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
>  static void tiadc_step_config(struct tiadc_device *adc_dev)
>  {
>  	unsigned int stepconfig;
> -	int i, channels = 0, steps;
> +	int i, steps;
>
>  	/*
>  	 * There are 16 configurable steps and 8 analog input
> @@ -68,16 +70,18 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	 */
>
>  	steps = TOTAL_STEPS - adc_dev->channels;
> -	channels = TOTAL_CHANNELS - adc_dev->channels;
> -
>  	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>
> -	for (i = steps; i < TOTAL_STEPS; i++) {
> -		tiadc_writel(adc_dev, REG_STEPCONFIG(i),
> -				stepconfig | STEPCONFIG_INP(channels));
> -		tiadc_writel(adc_dev, REG_STEPDELAY(i),
> +	for (i = 0; i < adc_dev->channels; i++) {
> +		int chan;
> +
> +		chan = adc_dev->channel_line[i];
> +		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
> +				stepconfig | STEPCONFIG_INP(chan));
> +		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
>  				STEPCONFIG_OPENDLY);
> -		channels++;
> +		adc_dev->channel_step[i] = steps;
> +		steps++;
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>  }
> @@ -101,11 +105,11 @@ static int tiadc_channel_init(struct iio_dev *indio_dev,
>  	chan = chan_array;
>  	for (i = 0; i < channels; i++, chan++, s += len + 1) {
>
> -		len = sprintf(s, "AIN%d", i);
> +		len = sprintf(s, "AIN%d", adc_dev->channel_line[i]);
>
>  		chan->type = IIO_VOLTAGE;
>  		chan->indexed = 1;
> -		chan->channel = i;
> +		chan->channel = adc_dev->channel_line[i];
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan->datasheet_name = s;
>  		chan->scan_type.sign = 'u';
> @@ -155,7 +159,8 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	int i;
> -	unsigned int fifo1count, readx1;
> +	unsigned int fifo1count, read;
> +	u32 step = UINT_MAX;
>
>  	/*
>  	 * When the sub-system is first enabled,
> @@ -168,11 +173,20 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	 * Hence we need to flush out this data.
>  	 */
>
> +	for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
> +		if (chan->channel == adc_dev->channel_line[i]) {
> +			step = adc_dev->channel_step[i];
> +			break;
> +		}
> +	}
> +	if (WARN_ON_ONCE(step == UINT_MAX))
> +		return -EINVAL;
> +
>  	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>  	for (i = 0; i < fifo1count; i++) {
> -		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
> -		if (i == chan->channel)
> -			*val = readx1 & 0xfff;
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +		if (read >> 16 == step)
> +			*val = read & 0xfff;
>  	}
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>
> @@ -189,8 +203,11 @@ static int tiadc_probe(struct platform_device *pdev)
>  	struct tiadc_device	*adc_dev;
>  	struct ti_tscadc_dev	*tscadc_dev = pdev->dev.platform_data;
>  	struct device_node	*node = tscadc_dev->dev->of_node;
> +	struct property		*prop;
> +	const __be32		*cur;
>  	int			err;
> -	u32			val32;
> +	u32			val;
> +	int channels		= 0;
>
>  	if (!node) {
>  		dev_err(&pdev->dev, "Could not find valid DT data.\n");
> @@ -212,11 +229,13 @@ static int tiadc_probe(struct platform_device *pdev)
>  		err = -EINVAL;
>  		goto err_free_device;
>  	}
> -	err = of_property_read_u32(node,
> -			"ti,adc-channels", &val32);
> -	if (err < 0)
> -		goto err_free_device;
> -	adc_dev->channels = val32;
> +
> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> +
> +		adc_dev->channel_line[channels] = val;
> +		channels++;
> +	}
> +	adc_dev->channels = channels;
>
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 5e4076f..b228c41 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -66,9 +66,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	struct clk		*clk;
>  	struct device_node	*node = pdev->dev.of_node;
>  	struct mfd_cell		*cell;
> +	struct property         *prop;
> +	const __be32            *cur;
> +	u32			val;
>  	int			err, ctrl;
>  	int			clk_value, clock_rate;
>  	int			tsc_wires = 0, adc_channels = 0, total_channels;
> +	int			readouts = 0;
>
>  	if (!pdev->dev.of_node) {
>  		dev_err(&pdev->dev, "Could not find valid DT data.\n");
> @@ -77,16 +81,26 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>
>  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>  	of_property_read_u32(node, "ti,wires", &tsc_wires);
> +	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
>
>  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
> -	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
> -
> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> +		adc_channels++;
> +		if (val > 7) {
> +			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
> +					val);
> +			return -EINVAL;
> +		}
> +	}
>  	total_channels = tsc_wires + adc_channels;
> -
Should not change this whitespace in this sort of patch.
>  	if (total_channels > 8) {
>  		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
>  		return -EINVAL;
>  	}
> +	if (readouts * 2 + 2 + adc_channels > 16) {
> +		dev_err(&pdev->dev, "Too many step configurations requested\n");
> +		return -EINVAL;
> +	}
>
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
>

Re: [PATCH 4/5] iio/ti_am335x_adc: Allow to specify input line

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 19:02:38 [+0100]:

>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>> index 5e4076f..b228c41 100644
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -77,16 +81,26 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>>
>>  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>>  	of_property_read_u32(node, "ti,wires", &tsc_wires);
>> +	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
>>
>>  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
>> -	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
>> -
>> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
>> +		adc_channels++;
>> +		if (val > 7) {
>> +			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
>> +					val);
>> +			return -EINVAL;
>> +		}
>> +	}
>>  	total_channels = tsc_wires + adc_channels;
>> -
>Should not change this whitespace in this sort of patch.

Okay.

Sebastian

Re: [PATCH 4/5] iio/ti_am335x_adc: Allow to specify input line

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-06-02 19:02:38 [+0100]:

>> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
>> index 5e4076f..b228c41 100644
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -77,16 +81,26 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>>
>>  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>>  	of_property_read_u32(node, "ti,wires", &tsc_wires);
>> +	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
>>
>>  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
>> -	of_property_read_u32(node, "ti,adc-channels", &adc_channels);
>> -
>> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
>> +		adc_channels++;
>> +		if (val > 7) {
>> +			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
>> +					val);
>> +			return -EINVAL;
>> +		}
>> +	}
>>  	total_channels = tsc_wires + adc_channels;
>> -
>Should not change this whitespace in this sort of patch.

Okay.

Sebastian