[PATCH 0/4] mfd: iio: ti_am335x_tscadc: fixes adc sampling, TSC config and a typo

[PATCH 0/4] mfd: iio: ti_am335x_tscadc: fixes adc sampling, TSC config and a typo

[Zubair Lutfullah]

Hi,

This short series has 4 patches that apply on the mfd-next tree that fix/cleanup the am335x_tsadc driver.

1- Fix. ADC driver would read data from fifo before sampling completes. Fixed by waiting for sequencer to complete and then read data
Acked by IIO.

2- Fix. TSC registers would be configured even in ADC only mode. Fixed so that they are configured only when TSC is enabled.

3- Cleanup. Removes a check on the ADC clock which isn't needed anymore.

4- Typo. Coordinate is spelled Coordiante. Corrected in all files in one patch.

Note: The typo patch has been sent before on all relevant lists too. No response..

Patil, Rachna (3):
  iio: ti_am335x_adc: Fix wrong samples received on 1st read
  MFD: ti_tscadc: disable TSC config registers in adc mode
  MFD: ti_tscadc: ADC Clock check not required

Zubair Lutfullah (1):
  mfd: input: arm: dts: doc: ti_am335x: typo fixes (coordiante ->
    coordinate)

 .../bindings/input/touchscreen/ti-tsc-adc.txt      |    4 +--
 arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
 drivers/iio/adc/ti_am335x_adc.c                    |   30 ++++++++++++++------
 drivers/input/touchscreen/ti_am335x_tsc.c          |    2 +-
 drivers/mfd/ti_am335x_tscadc.c                     |   26 ++++++++---------
 include/linux/mfd/ti_am335x_tscadc.h               |   17 ++++++++++-
 6 files changed, 55 insertions(+), 26 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] iio: ti_am335x_adc: Fix wrong samples received on 1st read

[Zubair Lutfullah]

From: "Patil, Rachna" <rachna@ti.com>

Previously we tried to read data form ADC even before ADC sequencer
finished sampling. This led to wrong samples.
We now wait on ADC status register idle bit to be set.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/iio/adc/ti_am335x_adc.c      |   30 ++++++++++++++++++++++--------
 include/linux/mfd/ti_am335x_tscadc.h |   16 ++++++++++++++++
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 4427e8e..f73fee1 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -60,7 +60,6 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 {
 	unsigned int stepconfig;
 	int i, steps;
-	u32 step_en;
 
 	/*
 	 * There are 16 configurable steps and 8 analog input
@@ -86,8 +85,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 		adc_dev->channel_step[i] = steps;
 		steps++;
 	}
-	step_en = get_adc_step_mask(adc_dev);
-	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
+
 }
 
 static const char * const chan_name_ain[] = {
@@ -142,10 +140,22 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 		int *val, int *val2, long mask)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-	int i;
-	unsigned int fifo1count, read;
+	int i, map_val;
+	unsigned int fifo1count, read, stepid;
 	u32 step = UINT_MAX;
 	bool found = false;
+	u32 step_en;
+	unsigned long timeout = jiffies + usecs_to_jiffies
+				(IDLE_TIMEOUT * adc_dev->channels);
+	step_en = get_adc_step_mask(adc_dev);
+	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
+
+	/* Wait for ADC sequencer to complete sampling */
+	while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
+		if (time_after(jiffies, timeout))
+			return -EAGAIN;
+		}
+	map_val = chan->channel + TOTAL_CHANNELS;
 
 	/*
 	 * When the sub-system is first enabled,
@@ -170,12 +180,16 @@ 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) {
-			*val = read & 0xfff;
+		stepid = read & FIFOREAD_CHNLID_MASK;
+		stepid = stepid >> 0x10;
+
+		if (stepid == map_val) {
+			read = read & FIFOREAD_DATA_MASK;
 			found = true;
+			*val = read;
 		}
 	}
-	am335x_tsc_se_update(adc_dev->mfd_tscadc);
+
 	if (found == false)
 		return -EBUSY;
 	return IIO_VAL_INT;
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 8d73fe2..db1791b 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -113,11 +113,27 @@
 #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
 #define CNTRLREG_TSCENB		BIT(7)
 
+/* FIFO READ Register */
+#define FIFOREAD_DATA_MASK (0xfff << 0)
+#define FIFOREAD_CHNLID_MASK (0xf << 16)
+
+/* Sequencer Status */
+#define SEQ_STATUS BIT(5)
+
 #define ADC_CLK			3000000
 #define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 
+/*
+* ADC runs at 3MHz, and it takes
+* 15 cycles to latch one data output.
+* Hence the idle time for ADC to
+* process one sample data would be
+* around 5 micro seconds.
+*/
+#define IDLE_TIMEOUT 5 /* microsec */
+
 #define TSCADC_CELLS		2
 
 struct ti_tscadc_dev {
-- 
1.7.9.5

[PATCH 2/4] MFD: ti_tscadc: disable TSC config registers in adc mode

[Zubair Lutfullah]

From: "Patil, Rachna" <rachna@ti.com>

AFE Pen Ctrl and TouchScreen transistors enabling is not
required when only ADC mode is being used, so check for availability of
TSC driver before accessing control register.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/mfd/ti_am335x_tscadc.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index b003a16..d72001c 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -208,13 +208,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	/* Set the control register bits */
 	ctrl = CNTRLREG_STEPCONFIGWRT |
-			CNTRLREG_TSCENB |
-			CNTRLREG_STEPID |
-			CNTRLREG_4WIRE;
+			CNTRLREG_STEPID;
+	if (tsc_wires > 0)
+		ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
 	tscadc_writel(tscadc, REG_CTRL, ctrl);
 
 	/* Set register bits for Idle Config Mode */
-	tscadc_idle_config(tscadc);
+	if (tsc_wires > 0)
+		tscadc_idle_config(tscadc);
 
 	/* Enable the TSC module enable bit */
 	ctrl = tscadc_readl(tscadc, REG_CTRL);
@@ -294,10 +295,13 @@ static int tscadc_resume(struct device *dev)
 	pm_runtime_get_sync(dev);
 
 	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_TSCENB |
-			CNTRLREG_STEPID | CNTRLREG_4WIRE;
+	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
+	if (tscadc_dev->tsc_cell != -1)
+		ctrl |= CNTRLREG_TSCENB | CNTRLREG_4WIRE;
 	tscadc_writel(tscadc_dev, REG_CTRL, ctrl);
-	tscadc_idle_config(tscadc_dev);
+
+	if (tscadc_dev->tsc_cell != -1)
+		tscadc_idle_config(tscadc_dev);
 	am335x_tsc_se_update(tscadc_dev);
 	restore = tscadc_readl(tscadc_dev, REG_CTRL);
 	tscadc_writel(tscadc_dev, REG_CTRL,
-- 
1.7.9.5

[PATCH 3/4] MFD: ti_tscadc: ADC Clock check not required

[Zubair Lutfullah]

From: "Patil, Rachna" <rachna@ti.com>

ADC is ideally expected to work at a frequency of 3MHz.
The present code had a check, which returned error if the frequency
went below the threshold  value. But since AM335x supports various
working frequencies, this check is not required.
Now the code just uses the internal ADC clock divider to set the ADC
frequency w.r.t the sys clock.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/mfd/ti_am335x_tscadc.c       |    6 +-----
 include/linux/mfd/ti_am335x_tscadc.h |    1 -
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index d72001c..cd74d59 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -197,11 +197,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	clock_rate = clk_get_rate(clk);
 	clk_put(clk);
 	clk_value = clock_rate / ADC_CLK;
-	if (clk_value < MAX_CLK_DIV) {
-		dev_err(&pdev->dev, "clock input less than min clock requirement\n");
-		err = -EINVAL;
-		goto err_disable_clk;
-	}
+
 	/* TSCADC_CLKDIV needs to be configured to the value minus 1 */
 	clk_value = clk_value - 1;
 	tscadc_writel(tscadc, REG_CLKDIV, clk_value);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..25f2c61 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -121,7 +121,6 @@
 #define SEQ_STATUS BIT(5)
 
 #define ADC_CLK			3000000
-#define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 
-- 
1.7.9.5

[PATCH 4/4] mfd: input: arm: dts: doc: ti_am335x: typo fixes (coordiante -> coordinate)

[Zubair Lutfullah]

Did a grep for 'coordiante' and replaced them all with 'coordinate'
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |    4 ++--
 arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
 drivers/input/touchscreen/ti_am335x_tsc.c          |    2 +-
 drivers/mfd/ti_am335x_tscadc.c                     |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 491c97b..3e22aec 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -6,7 +6,7 @@ Required properties:
 	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,coordiante-readouts: The sequencer supports a total of 16
+	ti,coordinate-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
@@ -34,7 +34,7 @@ Example:
 		tsc {
 			ti,wires = <4>;
 			ti,x-plate-resistance = <200>;
-			ti,coordiante-readouts = <5>;
+			ti,coordinate-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 0fa4c7f..e50b781 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,coordiante-readouts = <5>;
+		ti,coordinate-readouts = <5>;
 		ti,wire-config = <0x00 0x11 0x22 0x33>;
 	};
 
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 0e9f02a..6422f65 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -348,7 +348,7 @@ static int titsc_parse_dt(struct platform_device *pdev,
 	if (err < 0)
 		return err;
 
-	err = of_property_read_u32(node, "ti,coordiante-readouts",
+	err = of_property_read_u32(node, "ti,coordinate-readouts",
 			&ts_dev->coordinate_readouts);
 	if (err < 0)
 		return err;
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index cd74d59..1686ae6 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -106,7 +106,7 @@ 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);
+	of_property_read_u32(node, "ti,coordinate-readouts", &readouts);
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
-- 
1.7.9.5

Re: [PATCH 1/4] iio: ti_am335x_adc: Fix wrong samples received on 1st read

[Jonathan Cameron]

On 07/20/2013 05:27 PM, Zubair Lutfullah wrote:
> From: "Patil, Rachna" <rachna@ti.com>
> 
> Previously we tried to read data form ADC even before ADC sequencer
> finished sampling. This led to wrong samples.
> We now wait on ADC status register idle bit to be set.
> 
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Jonathan Cameron <jic23@kernel.org>

Applied this one to the fixes-togreg branch of iio.git.

I'd imagine the others will go through mfd?
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |   30 ++++++++++++++++++++++--------
>  include/linux/mfd/ti_am335x_tscadc.h |   16 ++++++++++++++++
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 4427e8e..f73fee1 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -60,7 +60,6 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  {
>  	unsigned int stepconfig;
>  	int i, steps;
> -	u32 step_en;
>  
>  	/*
>  	 * There are 16 configurable steps and 8 analog input
> @@ -86,8 +85,7 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  		adc_dev->channel_step[i] = steps;
>  		steps++;
>  	}
> -	step_en = get_adc_step_mask(adc_dev);
> -	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
> +
>  }
>  
>  static const char * const chan_name_ain[] = {
> @@ -142,10 +140,22 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  		int *val, int *val2, long mask)
>  {
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> -	int i;
> -	unsigned int fifo1count, read;
> +	int i, map_val;
> +	unsigned int fifo1count, read, stepid;
>  	u32 step = UINT_MAX;
>  	bool found = false;
> +	u32 step_en;
> +	unsigned long timeout = jiffies + usecs_to_jiffies
> +				(IDLE_TIMEOUT * adc_dev->channels);
> +	step_en = get_adc_step_mask(adc_dev);
> +	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
> +
> +	/* Wait for ADC sequencer to complete sampling */
> +	while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
> +		if (time_after(jiffies, timeout))
> +			return -EAGAIN;
> +		}
> +	map_val = chan->channel + TOTAL_CHANNELS;
>  
>  	/*
>  	 * When the sub-system is first enabled,
> @@ -170,12 +180,16 @@ 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) {
> -			*val = read & 0xfff;
> +		stepid = read & FIFOREAD_CHNLID_MASK;
> +		stepid = stepid >> 0x10;
> +
> +		if (stepid == map_val) {
> +			read = read & FIFOREAD_DATA_MASK;
>  			found = true;
> +			*val = read;
>  		}
>  	}
> -	am335x_tsc_se_update(adc_dev->mfd_tscadc);
> +
>  	if (found == false)
>  		return -EBUSY;
>  	return IIO_VAL_INT;
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index 8d73fe2..db1791b 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -113,11 +113,27 @@
>  #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
>  #define CNTRLREG_TSCENB		BIT(7)
>  
> +/* FIFO READ Register */
> +#define FIFOREAD_DATA_MASK (0xfff << 0)
> +#define FIFOREAD_CHNLID_MASK (0xf << 16)
> +
> +/* Sequencer Status */
> +#define SEQ_STATUS BIT(5)
> +
>  #define ADC_CLK			3000000
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
>  
> +/*
> +* ADC runs at 3MHz, and it takes
> +* 15 cycles to latch one data output.
> +* Hence the idle time for ADC to
> +* process one sample data would be
> +* around 5 micro seconds.
> +*/
> +#define IDLE_TIMEOUT 5 /* microsec */
> +
>  #define TSCADC_CELLS		2
>  
>  struct ti_tscadc_dev {
> 

Re: [PATCH 4/4] mfd: input: arm: dts: doc: ti_am335x: typo fixes (coordiante -> coordinate)

[Lee Jones]

On Sat, 20 Jul 2013, Zubair Lutfullah wrote:

> Did a grep for 'coordiante' and replaced them all with 'coordinate'
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |    4 ++--
>  arch/arm/boot/dts/am335x-evm.dts                   |    2 +-
>  drivers/input/touchscreen/ti_am335x_tsc.c          |    2 +-
>  drivers/mfd/ti_am335x_tscadc.c                     |    2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)

Once these DT bindings move from a development state to something more
concrete, I guess misspellings such as these will just have to remain
incorrect. However, as we are still in a state of development you can
have my:

Acked-by: Lee Jones <lee.jones@linaro.org>

... for the MFD part. 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/4] MFD: ti_tscadc: ADC Clock check not required

[Lee Jones]

On Sat, 20 Jul 2013, Zubair Lutfullah wrote:

> From: "Patil, Rachna" <rachna@ti.com>
> 
> ADC is ideally expected to work at a frequency of 3MHz.
> The present code had a check, which returned error if the frequency
> went below the threshold  value. But since AM335x supports various
> working frequencies, this check is not required.
> Now the code just uses the internal ADC clock divider to set the ADC
> frequency w.r.t the sys clock.
> 
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c       |    6 +-----
>  include/linux/mfd/ti_am335x_tscadc.h |    1 -
>  2 files changed, 1 insertion(+), 6 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] MFD: ti_tscadc: disable TSC config registers in adc mode

[Lee Jones]

On Sat, 20 Jul 2013, Zubair Lutfullah wrote:

> From: "Patil, Rachna" <rachna@ti.com>
> 
> AFE Pen Ctrl and TouchScreen transistors enabling is not
> required when only ADC mode is being used, so check for availability of
> TSC driver before accessing control register.
> 
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c |   18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)

Applied, thanks.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog