[PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[Zubair Lutfullah]

Patches now give correct authorship.

and checkpatch.pl issues are checked for each patch.

I hope the actual code bashing can begin now.

A series of patches that add continuous sampling support
for the adc drivers for the am335x.

These apply on top of mfd-next after the recent set of patches
on this driver by Sebastian Andrzej Siewior

Tested on the Beaglebone Black running 3.11

Patil, Rachna (5):
  MFD: ti_tscadc: disable TSC config registers in adc mode
  iio: ti_am335x_adc: Fix wrong samples received on 1st read
  input: ti_tsc: Enable shared IRQ for TSC
  iio: mfd: input: ti_am335x_adc:Add support for continuous mode
  MFD: ti_tscadc: ADC Clock check not required

Russ Dill (10):
  iio: ti_am335x_adc: Handle set to clear IRQENABLE
  iio: ti_am335x_adc: Handle set to clear IRQSTATUS
  iio: ti_am335x_adc: Handle overrun before threshold event
  iio: ti_am335x_adc: Avoid double threshold event
  iio: ti_am335x_adc: Also clear threshold event when clearing overrun
    event
  iio: ti_am335x_adc: Print error and handle short FIFO events
  iio: ti_am335x_adc: Fix allocation count of FIFO buffer.
  iio: ti_am335x_adc: Fix capture operation during resume
  iio: ti_am335x_adc: Reset and clear overrun status before capture
  iio: ti_am335x_adc: Properly handle out of memory situation.

 drivers/iio/adc/ti_am335x_adc.c           |  354 +++++++++++++++++++++++++----
 drivers/input/touchscreen/ti_am335x_tsc.c |   17 +-
 drivers/mfd/ti_am335x_tscadc.c            |   30 ++-
 include/linux/mfd/ti_am335x_tscadc.h      |   27 ++-
 4 files changed, 366 insertions(+), 62 deletions(-)

-- 
1.7.9.5

[PATCH 01/15] 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

Re: [PATCH 01/15] MFD: ti_tscadc: disable TSC config registers in adc mode

[Greg KH]

On Thu, Jul 18, 2013 at 11:21:12PM +0100, 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>

Did Rachna really sign off on this, and the other, patches?  Or did they
just author them and place them somewhere on the web?  You can't add
other people's signed-off-by lines, that's not allowed for obvious
reasons (read what Signed-off-by: means in the
Documentation/SubmittingPatches file if you are curious.)

thanks,

greg k-h

Re: [PATCH 01/15] MFD: ti_tscadc: disable TSC config registers in adc mode

[Zubair Lutfullah :]

On Thu, Jul 18, 2013 at 03:45:55PM -0700, Greg KH wrote:

> Did Rachna really sign off on this, and the other, patches?  Or did they
Authored and signed off on it in their TI tree. based on 3.2.

I brought them forward to 3.11.

I guess I'll remove their sign-offs. Can the code be reviewed as is
or another set of mails?

thanks
ZubairLK

Re: [PATCH 01/15] MFD: ti_tscadc: disable TSC config registers in adc mode

[Greg KH]

On Fri, Jul 19, 2013 at 09:47:16PM +0100, Zubair Lutfullah : wrote:
> On Thu, Jul 18, 2013 at 03:45:55PM -0700, Greg KH wrote:
> 
> > Did Rachna really sign off on this, and the other, patches?  Or did they
> Authored and signed off on it in their TI tree. based on 3.2.

Ok, as long as they did that, it's fine to have those lines in the
patch.  As you didn't have them in the previous version, I had to ask.

thanks,

greg k-h

[PATCH 02/15] 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>
---
 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

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

[Jonathan Cameron]

On 07/18/2013 11:21 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>

Hi,

I have no problem with this patch, but as it is a fix it should have been part
of a separate series from the new stuff.

Fixes go into mainline normally within the current cycle whereas the other stuff
will wait for the next merge window.

If you are rerolling the series, at the very least I would like this and any
other fixes at the beginning and clearly marked as such.

Thanks,

Jonathan
> ---
>  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 {
> 

[PATCH 03/15] input: ti_tsc: Enable shared IRQ for TSC

[Zubair Lutfullah]

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

Touchscreen and ADC share the same IRQ line from parent MFD core.
Previously only Touchscreen was interrupt based.
With continuous mode support added in ADC driver, driver requires
interrupt to process the ADC samples, so enable shared IRQ flag bit for
touchscreen.

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/input/touchscreen/ti_am335x_tsc.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 0e9f02a..edc3d03 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -260,8 +260,16 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
-	if (status & IRQENB_FIFO0THRES) {
 
+	/*
+	* ADC and touchscreen share the IRQ line.
+	* FIFO1 threshold interrupt is used by ADC,
+	* hence return from touchscreen IRQ handler if FIFO1
+	* threshold interrupt occurred.
+	*/
+	if (status & IRQENB_FIFO1THRES)
+		return IRQ_NONE;
+	else if (status & IRQENB_FIFO0THRES) {
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
 
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
@@ -315,7 +323,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	}
 
 	if (irqclr) {
-		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
+		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
 		am335x_tsc_se_update(ts_dev->mfd_tscadc);
 		return IRQ_HANDLED;
 	}
@@ -389,7 +397,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

Re: [PATCH 03/15] input: ti_tsc: Enable shared IRQ for TSC

[Jonathan Cameron]

On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
> From: "Patil, Rachna" <rachna@ti.com>
> 
> Touchscreen and ADC share the same IRQ line from parent MFD core.
> Previously only Touchscreen was interrupt based.
> With continuous mode support added in ADC driver, driver requires
> interrupt to process the ADC samples, so enable shared IRQ flag bit for
> touchscreen.
> 
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
cc'd Dmitry and Linux-input.

If this goes through IIO as it is required for the rest of the series to
work then I will need an Ack from Dmitry.

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 0e9f02a..edc3d03 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -260,8 +260,16 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	unsigned int fsm;
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> -	if (status & IRQENB_FIFO0THRES) {
>  
> +	/*
> +	* ADC and touchscreen share the IRQ line.
> +	* FIFO1 threshold interrupt is used by ADC,
> +	* hence return from touchscreen IRQ handler if FIFO1
> +	* threshold interrupt occurred.
> +	*/
> +	if (status & IRQENB_FIFO1THRES)
> +		return IRQ_NONE;
> +	else if (status & IRQENB_FIFO0THRES) {
>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>  
>  		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
> @@ -315,7 +323,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	}
>  
>  	if (irqclr) {
> -		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
> +		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>  		am335x_tsc_se_update(ts_dev->mfd_tscadc);
>  		return IRQ_HANDLED;
>  	}
> @@ -389,7 +397,7 @@ static int titsc_probe(struct platform_device *pdev)
>  	}
>  
>  	err = request_irq(ts_dev->irq, titsc_irq,
> -			  0, pdev->dev.driver->name, ts_dev);
> +			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to allocate irq.\n");
>  		goto err_free_mem;
> 

Re: [PATCH 03/15] input: ti_tsc: Enable shared IRQ for TSC

[Jonathan Cameron]

On 07/20/2013 11:57 AM, Jonathan Cameron wrote:
> On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
>> From: "Patil, Rachna" <rachna@ti.com>
>>
>> Touchscreen and ADC share the same IRQ line from parent MFD core.
>> Previously only Touchscreen was interrupt based.
>> With continuous mode support added in ADC driver, driver requires
>> interrupt to process the ADC samples, so enable shared IRQ flag bit for
>> touchscreen.
>>
>> Signed-off-by: Patil, Rachna <rachna@ti.com>
>> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> cc'd Dmitry and Linux-input.
> 
> If this goes through IIO as it is required for the rest of the series to
> work then I will need an Ack from Dmitry.
Sorry, finger memory meant I cc'd linux-iio again, instead of linux-input.

> 
>> ---
>>  drivers/input/touchscreen/ti_am335x_tsc.c |   14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>> index 0e9f02a..edc3d03 100644
>> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
>> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>> @@ -260,8 +260,16 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>>  	unsigned int fsm;
>>  
>>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>> -	if (status & IRQENB_FIFO0THRES) {
>>  
>> +	/*
>> +	* ADC and touchscreen share the IRQ line.
>> +	* FIFO1 threshold interrupt is used by ADC,
>> +	* hence return from touchscreen IRQ handler if FIFO1
>> +	* threshold interrupt occurred.
>> +	*/
>> +	if (status & IRQENB_FIFO1THRES)
>> +		return IRQ_NONE;
>> +	else if (status & IRQENB_FIFO0THRES) {
>>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
>>  
>>  		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
>> @@ -315,7 +323,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>>  	}
>>  
>>  	if (irqclr) {
>> -		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
>> +		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>>  		am335x_tsc_se_update(ts_dev->mfd_tscadc);
>>  		return IRQ_HANDLED;
>>  	}
>> @@ -389,7 +397,7 @@ static int titsc_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	err = request_irq(ts_dev->irq, titsc_irq,
>> -			  0, pdev->dev.driver->name, ts_dev);
>> +			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
>>  	if (err) {
>>  		dev_err(&pdev->dev, "failed to allocate irq.\n");
>>  		goto err_free_mem;
>>

[PATCH 04/15] iio: mfd: input: ti_am335x_adc:Add support for continuous mode

[Zubair Lutfullah]

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

Current ADC driver supports only one shot mode.
Now ADC driver is enhanced to capture data continuously
from /dev/iio:deviceX interface.

ADC is now IRQ based, on interrupt ADC copies data onto
a software buffer, which is exposed to user.

Signed-off-by: Patil, Rachna <rachna@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c           |  350 +++++++++++++++++++++++++----
 drivers/input/touchscreen/ti_am335x_tsc.c |    9 +-
 drivers/mfd/ti_am335x_tscadc.c            |   12 +-
 include/linux/mfd/ti_am335x_tscadc.h      |   10 +
 4 files changed, 322 insertions(+), 59 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index f73fee1..c1b051c 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -26,14 +26,24 @@
 #include <linux/of_device.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
-
 #include <linux/mfd/ti_am335x_tscadc.h>
+#include <linux/stat.h>
+#include <linux/sched.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
 
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
 	int channels;
 	u8 channel_line[8];
 	u8 channel_step[8];
+	struct work_struct	poll_work;
+	wait_queue_head_t	wq_data_avail;
+	int	irq;
+	bool	is_continuous_mode;
+	u16	*buffer;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,7 +66,7 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
 	return step_en;
 }
 
-static void tiadc_step_config(struct tiadc_device *adc_dev)
+static void tiadc_step_config(struct tiadc_device *adc_dev, bool mode)
 {
 	unsigned int stepconfig;
 	int i, steps;
@@ -72,7 +82,11 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 	 */
 
 	steps = TOTAL_STEPS - adc_dev->channels;
-	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
+	if (mode == false)
+		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
+	else
+		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
+				| STEPCONFIG_MODE_SWCNT;
 
 	for (i = 0; i < adc_dev->channels; i++) {
 		int chan;
@@ -88,6 +102,213 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 
 }
 
+static ssize_t tiadc_show_mode(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int tmp;
+
+	tmp = tiadc_readl(adc_dev, REG_STEPCONFIG(TOTAL_STEPS));
+	tmp &= STEPCONFIG_MODE(1);
+
+	if (tmp == 0x00)
+		return sprintf(buf, "oneshot\n");
+	else if (tmp == 0x01)
+		return sprintf(buf, "continuous\n");
+	else
+		return sprintf(buf, "Operation mode unknown\n");
+}
+
+static ssize_t tiadc_set_mode(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	unsigned config;
+
+	config = tiadc_readl(adc_dev, REG_CTRL);
+	config &= ~(CNTRLREG_TSCSSENB);
+	tiadc_writel(adc_dev, REG_CTRL, config);
+
+	if (!strncmp(buf, "oneshot", 7))
+		adc_dev->is_continuous_mode = false;
+	else if (!strncmp(buf, "continuous", 10))
+		adc_dev->is_continuous_mode = true;
+	else {
+		dev_err(dev, "Operational mode unknown\n");
+		return -EINVAL;
+	}
+
+	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
+
+	config = tiadc_readl(adc_dev, REG_CTRL);
+	tiadc_writel(adc_dev, REG_CTRL,
+			(config | CNTRLREG_TSCSSENB));
+	return count;
+}
+
+static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, tiadc_show_mode,
+		tiadc_set_mode, 0);
+
+static struct attribute *tiadc_attributes[] = {
+	&iio_dev_attr_mode.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group tiadc_attribute_group = {
+	.attrs = tiadc_attributes,
+};
+
+static irqreturn_t tiadc_irq(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct tiadc_device *adc_dev = iio_priv(idev);
+	unsigned int status, config;
+
+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+	if (status & IRQENB_FIFO1THRES) {
+		tiadc_writel(adc_dev, REG_IRQCLR,
+				IRQENB_FIFO1THRES);
+
+		if (iio_buffer_enabled(idev)) {
+			if (!work_pending(&adc_dev->poll_work))
+				schedule_work(&adc_dev->poll_work);
+		} else {
+			wake_up_interruptible(&adc_dev->wq_data_avail);
+		}
+		tiadc_writel(adc_dev, REG_IRQSTATUS,
+				(status | IRQENB_FIFO1THRES));
+		return IRQ_HANDLED;
+	} else if ((status &  IRQENB_FIFO1OVRRUN) ||
+			(status &  IRQENB_FIFO1UNDRFLW)) {
+		config = tiadc_readl(adc_dev,  REG_CTRL);
+		config &= ~(CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev,  REG_CTRL, config);
+
+		if (status &  IRQENB_FIFO1UNDRFLW)
+			tiadc_writel(adc_dev,  REG_IRQSTATUS,
+			(status |  IRQENB_FIFO1UNDRFLW));
+		else
+			tiadc_writel(adc_dev,  REG_IRQSTATUS,
+				(status |  IRQENB_FIFO1OVRRUN));
+
+		tiadc_writel(adc_dev,  REG_CTRL,
+			(config |  CNTRLREG_TSCSSENB));
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
+}
+
+static void tiadc_poll_handler(struct work_struct *work_s)
+{
+	struct tiadc_device *adc_dev =
+		container_of(work_s, struct tiadc_device, poll_work);
+	struct iio_dev *idev = iio_priv_to_dev(adc_dev);
+	struct iio_buffer *buffer = idev->buffer;
+	unsigned int fifo1count, readx1, status;
+	int i;
+	u32 *inputbuffer;
+
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	inputbuffer = kmalloc((fifo1count + 1) * sizeof(u32), GFP_KERNEL);
+	if (inputbuffer == NULL)
+		return;
+
+	for (i = 0; i < fifo1count; i++) {
+		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
+		readx1 &= FIFOREAD_DATA_MASK;
+		inputbuffer[i] = readx1;
+	}
+
+	buffer->access->store_to(buffer, (u8 *) inputbuffer);
+	status = tiadc_readl(adc_dev, REG_IRQENABLE);
+	tiadc_writel(adc_dev, REG_IRQENABLE,
+			(status | IRQENB_FIFO1THRES));
+
+	kfree(inputbuffer);
+}
+
+static int tiadc_buffer_preenable(struct iio_dev *idev)
+{
+	struct iio_buffer *buffer = idev->buffer;
+
+	buffer->access->set_bytes_per_datum(buffer, 16);
+	return 0;
+}
+
+static int tiadc_buffer_postenable(struct iio_dev *idev)
+{
+	struct tiadc_device *adc_dev = iio_priv(idev);
+	struct iio_buffer *buffer = idev->buffer;
+	unsigned int enb, status, fifo1count;
+	int stepnum, i;
+	u8 bit;
+
+	if (!adc_dev->is_continuous_mode) {
+		pr_info("Data cannot be read continuously in one shot mode\n");
+		return -EINVAL;
+	} else {
+		status = tiadc_readl(adc_dev, REG_IRQENABLE);
+		tiadc_writel(adc_dev, REG_IRQENABLE,
+				(status | IRQENB_FIFO1THRES)|
+				 IRQENB_FIFO1OVRRUN |
+				 IRQENB_FIFO1UNDRFLW);
+
+		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+		for (i = 0; i < fifo1count; i++)
+			tiadc_readl(adc_dev, REG_FIFO1);
+
+		tiadc_writel(adc_dev, REG_SE, 0x00);
+		for_each_set_bit(bit, buffer->scan_mask,
+				adc_dev->channels) {
+			struct iio_chan_spec const *chan = idev->channels + bit;
+			/*
+			 * There are a total of 16 steps available
+			 * that are shared between ADC and touchscreen.
+			 * We start configuring from step 16 to 0 incase of
+			 * ADC. Hence the relation between input channel
+			 * and step for ADC would be as below.
+			 */
+			stepnum = chan->channel + 9;
+			enb = tiadc_readl(adc_dev, REG_SE);
+			enb |= (1 << stepnum);
+			tiadc_writel(adc_dev, REG_SE, enb);
+		}
+		return 0;
+	}
+}
+
+static int tiadc_buffer_postdisable(struct iio_dev *idev)
+{
+	struct tiadc_device *adc_dev = iio_priv(idev);
+
+	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+				IRQENB_FIFO1OVRRUN |
+				IRQENB_FIFO1UNDRFLW));
+	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB_TC);
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
+	.preenable = &tiadc_buffer_preenable,
+	.postenable = &tiadc_buffer_postenable,
+	.postdisable = &tiadc_buffer_postdisable,
+};
+
+static int tiadc_config_buffer(struct iio_dev *idev)
+{
+	struct tiadc_device *adc_dev = iio_priv(idev);
+	idev->buffer = iio_kfifo_allocate(idev);
+	if (!idev->buffer)
+		return -ENOMEM;
+	idev->setup_ops = &tiadc_buffer_setup_ops;
+	INIT_WORK(&adc_dev->poll_work, &tiadc_poll_handler);
+	idev->modes |= INDIO_BUFFER_HARDWARE;
+	return 0;
+}
+
 static const char * const chan_name_ain[] = {
 	"AIN0",
 	"AIN1",
@@ -120,6 +341,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 		chan->channel = adc_dev->channel_line[i];
 		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 		chan->datasheet_name = chan_name_ain[chan->channel];
+		chan->scan_index = i;
 		chan->scan_type.sign = 'u';
 		chan->scan_type.realbits = 12;
 		chan->scan_type.storagebits = 32;
@@ -147,56 +369,62 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	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,
-	 * the sequencer will always start with the
-	 * lowest step (1) and continue until step (16).
-	 * For ex: If we have enabled 4 ADC channels and
-	 * currently use only 1 out of them, the
-	 * sequencer still configures all the 4 steps,
-	 * leading to 3 unwanted data.
-	 * 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))
+	if (adc_dev->is_continuous_mode) {
+		pr_info("One shot mode not enabled\n");
 		return -EINVAL;
-
-	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
-	for (i = 0; i < fifo1count; i++) {
-		read = tiadc_readl(adc_dev, REG_FIFO1);
-		stepid = read & FIFOREAD_CHNLID_MASK;
-		stepid = stepid >> 0x10;
-
-		if (stepid == map_val) {
-			read = read & FIFOREAD_DATA_MASK;
-			found = true;
-			*val = read;
+	} else {
+		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,
+		 * the sequencer will always start with the
+		 * lowest step (1) and continue until step (16).
+		 * For ex: If we have enabled 4 ADC channels and
+		 * currently use only 1 out of them, the
+		 * sequencer still configures all the 4 steps,
+		 * leading to 3 unwanted data.
+		 * 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++) {
+			read = tiadc_readl(adc_dev, REG_FIFO1);
+			stepid = read & FIFOREAD_CHNLID_MASK;
+			stepid = stepid >> 0x10;
+
+			if (stepid == map_val) {
+				read = read & FIFOREAD_DATA_MASK;
+				found = true;
+				*val = read;
+			}
+		}
+		if (found == false)
+			return -EBUSY;
+		return IIO_VAL_INT;
 	}
-
-	if (found == false)
-		return -EBUSY;
-	return IIO_VAL_INT;
 }
 
 static const struct iio_info tiadc_info = {
 	.read_raw = &tiadc_read_raw,
+	.attrs = &tiadc_attribute_group,
 };
 
 static int tiadc_probe(struct platform_device *pdev)
@@ -230,18 +458,38 @@ static int tiadc_probe(struct platform_device *pdev)
 		channels++;
 	}
 	adc_dev->channels = channels;
+	adc_dev->irq = adc_dev->mfd_tscadc->irq;
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &tiadc_info;
 
-	tiadc_step_config(adc_dev);
+	/* by default driver comes up with oneshot mode */
+	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
+	/* program FIFO threshold to value minus 1 */
+	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
 
 	err = tiadc_channel_init(indio_dev, adc_dev->channels);
 	if (err < 0)
 		goto err_free_device;
 
+	init_waitqueue_head(&adc_dev->wq_data_avail);
+
+	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
+		indio_dev->name, indio_dev);
+	if (err)
+		goto err_free_irq;
+
+	err = tiadc_config_buffer(indio_dev);
+	if (err < 0)
+		goto err_unregister;
+
+	err = iio_buffer_register(indio_dev,
+		indio_dev->channels, indio_dev->num_channels);
+	if (err < 0)
+		goto err_unregister;
+
 	err = iio_device_register(indio_dev);
 	if (err)
 		goto err_free_channels;
@@ -250,6 +498,10 @@ static int tiadc_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_unregister:
+	iio_buffer_unregister(indio_dev);
+err_free_irq:
+	free_irq(adc_dev->irq, indio_dev);
 err_free_channels:
 	tiadc_channels_remove(indio_dev);
 err_free_device:
@@ -264,7 +516,9 @@ static int tiadc_remove(struct platform_device *pdev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	u32 step_en;
 
+	free_irq(adc_dev->irq, indio_dev);
 	iio_device_unregister(indio_dev);
+	iio_buffer_unregister(indio_dev);
 	tiadc_channels_remove(indio_dev);
 
 	step_en = get_adc_step_mask(adc_dev);
@@ -300,13 +554,13 @@ static int tiadc_resume(struct device *dev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	unsigned int restore;
 
+	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
+	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
 	/* Make sure ADC is powered up */
 	restore = tiadc_readl(adc_dev, REG_CTRL);
 	restore &= ~(CNTRLREG_POWERDOWN);
 	tiadc_writel(adc_dev, REG_CTRL, restore);
 
-	tiadc_step_config(adc_dev);
-
 	return 0;
 }
 
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index edc3d03..ba7abfe 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -263,11 +263,14 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	/*
 	* ADC and touchscreen share the IRQ line.
-	* FIFO1 threshold interrupt is used by ADC,
+	* FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
+	* interrupts are used by ADC,
 	* hence return from touchscreen IRQ handler if FIFO1
-	* threshold interrupt occurred.
+	* related interrupts occurred.
 	*/
-	if (status & IRQENB_FIFO1THRES)
+	if ((status & IRQENB_FIFO1THRES) ||
+			(status & IRQENB_FIFO1OVRRUN) ||
+			(status & IRQENB_FIFO1UNDRFLW))
 		return IRQ_NONE;
 	else if (status & IRQENB_FIFO0THRES) {
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index d72001c..9f5c326 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -282,6 +282,8 @@ static int tscadc_suspend(struct device *dev)
 	struct ti_tscadc_dev	*tscadc_dev = dev_get_drvdata(dev);
 
 	tscadc_writel(tscadc_dev, REG_SE, 0x00);
+	tscadc_dev->irqstat = tscadc_readl(tscadc_dev, REG_IRQENABLE);
+	tscadc_dev->ctrl = tscadc_readl(tscadc_dev, REG_CTRL);
 	pm_runtime_put_sync(dev);
 
 	return 0;
@@ -290,22 +292,16 @@ static int tscadc_suspend(struct device *dev)
 static int tscadc_resume(struct device *dev)
 {
 	struct ti_tscadc_dev	*tscadc_dev = dev_get_drvdata(dev);
-	unsigned int restore, ctrl;
 
 	pm_runtime_get_sync(dev);
 
 	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	if (tscadc_dev->tsc_cell != -1)
-		ctrl |= CNTRLREG_TSCENB | CNTRLREG_4WIRE;
-	tscadc_writel(tscadc_dev, REG_CTRL, ctrl);
+	tscadc_writel(tscadc_dev, REG_IRQENABLE, tscadc_dev->irqstat);
 
 	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,
-			(restore | CNTRLREG_TSCSSENB));
+	tscadc_writel(tscadc_dev, REG_CTRL, tscadc_dev->ctrl);
 
 	return 0;
 }
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..3058aef 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,17 +46,23 @@
 /* Step Enable */
 #define STEPENB_MASK		(0x1FFFF << 0)
 #define STEPENB(val)		((val) << 0)
+#define ENB(val)			(1 << (val))
+#define STPENB_STEPENB		STEPENB(0x1FFFF)
+#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
 
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
 #define IRQENB_FIFO0THRES	BIT(2)
 #define IRQENB_FIFO1THRES	BIT(5)
 #define IRQENB_PENUP		BIT(9)
+#define IRQENB_FIFO1OVRRUN	BIT(6)
+#define IRQENB_FIFO1UNDRFLW	BIT(7)
 
 /* Step Configuration */
 #define STEPCONFIG_MODE_MASK	(3 << 0)
 #define STEPCONFIG_MODE(val)	((val) << 0)
 #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
+#define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
 #define STEPCONFIG_AVG_MASK	(7 << 2)
 #define STEPCONFIG_AVG(val)	((val) << 2)
 #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
@@ -124,6 +130,7 @@
 #define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
+#define FIFO1_THRESHOLD		19
 
 /*
 * ADC runs at 3MHz, and it takes
@@ -153,6 +160,9 @@ struct ti_tscadc_dev {
 
 	/* adc device */
 	struct adc_device *adc;
+	/* Context save */
+	unsigned int irqstat;
+	unsigned int ctrl;
 };
 
 static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
-- 
1.7.9.5

Re: [PATCH 04/15] iio: mfd: input: ti_am335x_adc:Add support for continuous mode

[Jonathan Cameron]

On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
> From: "Patil, Rachna" <rachna@ti.com>
>
> Current ADC driver supports only one shot mode.
> Now ADC driver is enhanced to capture data continuously
> from /dev/iio:deviceX interface.
>
> ADC is now IRQ based, on interrupt ADC copies data onto
> a software buffer, which is exposed to user.
>
> Signed-off-by: Patil, Rachna <rachna@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>

Couple of bits inline.

One reasonably big point though:

What is the mode attribute for?  The buffer enabled should handle
that fine within the standard abi.

> ---
>  drivers/iio/adc/ti_am335x_adc.c           |  350 +++++++++++++++++++++++++----
>  drivers/input/touchscreen/ti_am335x_tsc.c |    9 +-
>  drivers/mfd/ti_am335x_tscadc.c            |   12 +-
>  include/linux/mfd/ti_am335x_tscadc.h      |   10 +
>  4 files changed, 322 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index f73fee1..c1b051c 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -26,14 +26,24 @@
>  #include <linux/of_device.h>
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
> -
>  #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/stat.h>
> +#include <linux/sched.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
>
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
>  	int channels;
>  	u8 channel_line[8];
>  	u8 channel_step[8];
> +	struct work_struct	poll_work;
> +	wait_queue_head_t	wq_data_avail;
> +	int	irq;
> +	bool	is_continuous_mode;
> +	u16	*buffer;
>  };
>
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,7 +66,7 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
>  	return step_en;
>  }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static void tiadc_step_config(struct tiadc_device *adc_dev, bool mode)
>  {
>  	unsigned int stepconfig;
>  	int i, steps;
> @@ -72,7 +82,11 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	 */
>
>  	steps = TOTAL_STEPS - adc_dev->channels;
> -	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> +	if (mode == false)
> +		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> +	else
> +		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> +				| STEPCONFIG_MODE_SWCNT;
>
>  	for (i = 0; i < adc_dev->channels; i++) {
>  		int chan;
> @@ -88,6 +102,213 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>
>  }
>
> +static ssize_t tiadc_show_mode(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int tmp;
> +
> +	tmp = tiadc_readl(adc_dev, REG_STEPCONFIG(TOTAL_STEPS));
> +	tmp &= STEPCONFIG_MODE(1);
> +
> +	if (tmp == 0x00)
> +		return sprintf(buf, "oneshot\n");
> +	else if (tmp == 0x01)
> +		return sprintf(buf, "continuous\n");
> +	else
> +		return sprintf(buf, "Operation mode unknown\n");
> +}
> +
> +static ssize_t tiadc_set_mode(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned config;
> +
> +	config = tiadc_readl(adc_dev, REG_CTRL);
> +	config &= ~(CNTRLREG_TSCSSENB);
> +	tiadc_writel(adc_dev, REG_CTRL, config);
> +
> +	if (!strncmp(buf, "oneshot", 7))
> +		adc_dev->is_continuous_mode = false;
> +	else if (!strncmp(buf, "continuous", 10))
> +		adc_dev->is_continuous_mode = true;
> +	else {
> +		dev_err(dev, "Operational mode unknown\n");
> +		return -EINVAL;
> +	}
> +
> +	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
> +
> +	config = tiadc_readl(adc_dev, REG_CTRL);
> +	tiadc_writel(adc_dev, REG_CTRL,
> +			(config | CNTRLREG_TSCSSENB));
> +	return count;
> +}
> +
This should be controlled by whether the buffer is enabled or not.
If it is not, then we are in oneshot mode, if it is then we are
in continuous mode.  No additional control is needed.

> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, tiadc_show_mode,
> +		tiadc_set_mode, 0);
> +
> +static struct attribute *tiadc_attributes[] = {
> +	&iio_dev_attr_mode.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group tiadc_attribute_group = {
> +	.attrs = tiadc_attributes,
> +};
> +
> +static irqreturn_t tiadc_irq(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct tiadc_device *adc_dev = iio_priv(idev);
> +	unsigned int status, config;
> +
> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
Some brief comments in here might be of use to people
like me reading this without enough coffee..
What are the various interrupts and what are you doing in
response to them?

> +	if (status & IRQENB_FIFO1THRES) {
> +		tiadc_writel(adc_dev, REG_IRQCLR,
> +				IRQENB_FIFO1THRES);
> +
> +		if (iio_buffer_enabled(idev)) {
> +			if (!work_pending(&adc_dev->poll_work))
> +				schedule_work(&adc_dev->poll_work);
> +		} else {
> +			wake_up_interruptible(&adc_dev->wq_data_avail);
> +		}
> +		tiadc_writel(adc_dev, REG_IRQSTATUS,
> +				(status | IRQENB_FIFO1THRES));
> +		return IRQ_HANDLED;
> +	} else if ((status &  IRQENB_FIFO1OVRRUN) ||
> +			(status &  IRQENB_FIFO1UNDRFLW)) {
> +		config = tiadc_readl(adc_dev,  REG_CTRL);
> +		config &= ~(CNTRLREG_TSCSSENB);
> +		tiadc_writel(adc_dev,  REG_CTRL, config);
> +
> +		if (status &  IRQENB_FIFO1UNDRFLW)
> +			tiadc_writel(adc_dev,  REG_IRQSTATUS,
> +			(status |  IRQENB_FIFO1UNDRFLW));
> +		else
> +			tiadc_writel(adc_dev,  REG_IRQSTATUS,
> +				(status |  IRQENB_FIFO1OVRRUN));
> +
> +		tiadc_writel(adc_dev,  REG_CTRL,
> +			(config |  CNTRLREG_TSCSSENB));
> +		return IRQ_HANDLED;
> +	} else {
> +		return IRQ_NONE;
> +	}
> +}
> +
> +static void tiadc_poll_handler(struct work_struct *work_s)
> +{
> +	struct tiadc_device *adc_dev =
> +		container_of(work_s, struct tiadc_device, poll_work);
> +	struct iio_dev *idev = iio_priv_to_dev(adc_dev);
> +	struct iio_buffer *buffer = idev->buffer;
> +	unsigned int fifo1count, readx1, status;
> +	int i;
> +	u32 *inputbuffer;
> +
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	inputbuffer = kmalloc((fifo1count + 1) * sizeof(u32), GFP_KERNEL);
Worth keeping a buffer of sufficient size around to avoid the allocation
here?
> +	if (inputbuffer == NULL)
> +		return;
> +
> +	for (i = 0; i < fifo1count; i++) {
> +		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
> +		readx1 &= FIFOREAD_DATA_MASK;
> +		inputbuffer[i] = readx1;
> +	}
> +
I'm slightly confused... Can the fifo contain more than one
'scan' of readings?  If so you are only pushing the first into the buffer
here.
> +	buffer->access->store_to(buffer, (u8 *) inputbuffer);
> +	status = tiadc_readl(adc_dev, REG_IRQENABLE);
> +	tiadc_writel(adc_dev, REG_IRQENABLE,
> +			(status | IRQENB_FIFO1THRES));
> +
> +	kfree(inputbuffer);
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *idev)
> +{
> +	struct iio_buffer *buffer = idev->buffer;
> +
> +	buffer->access->set_bytes_per_datum(buffer, 16);
blank line here
> +	return 0;
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *idev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(idev);
> +	struct iio_buffer *buffer = idev->buffer;
> +	unsigned int enb, status, fifo1count;
> +	int stepnum, i;
> +	u8 bit;
> +
> +	if (!adc_dev->is_continuous_mode) {
> +		pr_info("Data cannot be read continuously in one shot mode\n");
This is probably a hint that you are doing it wrong.
The buffer enable should be the thing that switches to continuous mode.

> +		return -EINVAL;
> +	} else {
> +		status = tiadc_readl(adc_dev, REG_IRQENABLE);
> +		tiadc_writel(adc_dev, REG_IRQENABLE,
> +				(status | IRQENB_FIFO1THRES)|
> +				 IRQENB_FIFO1OVRRUN |
> +				 IRQENB_FIFO1UNDRFLW);
> +
> +		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +		for (i = 0; i < fifo1count; i++)
> +			tiadc_readl(adc_dev, REG_FIFO1);
> +
> +		tiadc_writel(adc_dev, REG_SE, 0x00);
> +		for_each_set_bit(bit, buffer->scan_mask,
> +				adc_dev->channels) {
> +			struct iio_chan_spec const *chan = idev->channels + bit;
> +			/*
> +			 * There are a total of 16 steps available
> +			 * that are shared between ADC and touchscreen.
> +			 * We start configuring from step 16 to 0 incase of
> +			 * ADC. Hence the relation between input channel
> +			 * and step for ADC would be as below.
> +			 */
> +			stepnum = chan->channel + 9;
> +			enb = tiadc_readl(adc_dev, REG_SE);
> +			enb |= (1 << stepnum);
> +			tiadc_writel(adc_dev, REG_SE, enb);
> +		}
> +		return 0;
> +	}
> +}
> +

For symmetry I'd expect to see this in predisable. Otherwise,
you might get such an interrupt after the buffer has been ripped
down and fun might ensue.

> +static int tiadc_buffer_postdisable(struct iio_dev *idev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(idev);
> +
> +	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
> +				IRQENB_FIFO1OVRRUN |
> +				IRQENB_FIFO1UNDRFLW));
> +	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB_TC);

blank line here.
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
> +	.preenable = &tiadc_buffer_preenable,
> +	.postenable = &tiadc_buffer_postenable,
> +	.postdisable = &tiadc_buffer_postdisable,
> +};
> +
> +static int tiadc_config_buffer(struct iio_dev *idev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(idev);
nitpick time ;) Blank line here.
> +	idev->buffer = iio_kfifo_allocate(idev);
> +	if (!idev->buffer)
> +		return -ENOMEM;
> +	idev->setup_ops = &tiadc_buffer_setup_ops;
> +	INIT_WORK(&adc_dev->poll_work, &tiadc_poll_handler);
> +	idev->modes |= INDIO_BUFFER_HARDWARE;
and here.
> +	return 0;
> +}
> +
>  static const char * const chan_name_ain[] = {
>  	"AIN0",
>  	"AIN1",
> @@ -120,6 +341,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  		chan->channel = adc_dev->channel_line[i];
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan->datasheet_name = chan_name_ain[chan->channel];
> +		chan->scan_index = i;
>  		chan->scan_type.sign = 'u';
>  		chan->scan_type.realbits = 12;
>  		chan->scan_type.storagebits = 32;
> @@ -147,56 +369,62 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	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,
> -	 * the sequencer will always start with the
> -	 * lowest step (1) and continue until step (16).
> -	 * For ex: If we have enabled 4 ADC channels and
> -	 * currently use only 1 out of them, the
> -	 * sequencer still configures all the 4 steps,
> -	 * leading to 3 unwanted data.
> -	 * 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))
> +	if (adc_dev->is_continuous_mode) {
> +		pr_info("One shot mode not enabled\n");
>  		return -EINVAL;
-EBUSY probably makes more sense.
> -
> -	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> -	for (i = 0; i < fifo1count; i++) {
> -		read = tiadc_readl(adc_dev, REG_FIFO1);
> -		stepid = read & FIFOREAD_CHNLID_MASK;
> -		stepid = stepid >> 0x10;
> -
> -		if (stepid == map_val) {
> -			read = read & FIFOREAD_DATA_MASK;
> -			found = true;
> -			*val = read;
> +	} else {
> +		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,
> +		 * the sequencer will always start with the
> +		 * lowest step (1) and continue until step (16).
> +		 * For ex: If we have enabled 4 ADC channels and
> +		 * currently use only 1 out of them, the
> +		 * sequencer still configures all the 4 steps,
> +		 * leading to 3 unwanted data.
> +		 * 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++) {
> +			read = tiadc_readl(adc_dev, REG_FIFO1);
> +			stepid = read & FIFOREAD_CHNLID_MASK;
> +			stepid = stepid >> 0x10;
> +
> +			if (stepid == map_val) {
> +				read = read & FIFOREAD_DATA_MASK;
> +				found = true;
> +				*val = read;
> +			}
> +		}
> +		if (found == false)
> +			return -EBUSY;
> +		return IIO_VAL_INT;
>  	}
> -
> -	if (found == false)
> -		return -EBUSY;
> -	return IIO_VAL_INT;
>  }
>
>  static const struct iio_info tiadc_info = {
>  	.read_raw = &tiadc_read_raw,
> +	.attrs = &tiadc_attribute_group,
>  };
>
>  static int tiadc_probe(struct platform_device *pdev)
> @@ -230,18 +458,38 @@ static int tiadc_probe(struct platform_device *pdev)
>  		channels++;
>  	}
>  	adc_dev->channels = channels;
> +	adc_dev->irq = adc_dev->mfd_tscadc->irq;
>
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &tiadc_info;
>
> -	tiadc_step_config(adc_dev);
> +	/* by default driver comes up with oneshot mode */
> +	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
> +	/* program FIFO threshold to value minus 1 */
> +	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
>
>  	err = tiadc_channel_init(indio_dev, adc_dev->channels);
>  	if (err < 0)
>  		goto err_free_device;
>
> +	init_waitqueue_head(&adc_dev->wq_data_avail);
> +
> +	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> +		indio_dev->name, indio_dev);
> +	if (err)
> +		goto err_free_irq;
> +
> +	err = tiadc_config_buffer(indio_dev);
> +	if (err < 0)
> +		goto err_unregister;
> +
> +	err = iio_buffer_register(indio_dev,
> +		indio_dev->channels, indio_dev->num_channels);
> +	if (err < 0)
> +		goto err_unregister;
> +
>  	err = iio_device_register(indio_dev);
>  	if (err)
>  		goto err_free_channels;
> @@ -250,6 +498,10 @@ static int tiadc_probe(struct platform_device *pdev)
>
>  	return 0;
>
> +err_unregister:
> +	iio_buffer_unregister(indio_dev);
> +err_free_irq:
> +	free_irq(adc_dev->irq, indio_dev);
>  err_free_channels:
>  	tiadc_channels_remove(indio_dev);
>  err_free_device:
> @@ -264,7 +516,9 @@ static int tiadc_remove(struct platform_device *pdev)
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	u32 step_en;
>
> +	free_irq(adc_dev->irq, indio_dev);
>  	iio_device_unregister(indio_dev);
> +	iio_buffer_unregister(indio_dev);
>  	tiadc_channels_remove(indio_dev);
>
>  	step_en = get_adc_step_mask(adc_dev);
> @@ -300,13 +554,13 @@ static int tiadc_resume(struct device *dev)
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	unsigned int restore;
>
> +	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
> +	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
>  	/* Make sure ADC is powered up */
>  	restore = tiadc_readl(adc_dev, REG_CTRL);
>  	restore &= ~(CNTRLREG_POWERDOWN);
>  	tiadc_writel(adc_dev, REG_CTRL, restore);
>
> -	tiadc_step_config(adc_dev);
> -
>  	return 0;
>  }
>
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index edc3d03..ba7abfe 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -263,11 +263,14 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>
>  	/*
>  	* ADC and touchscreen share the IRQ line.
> -	* FIFO1 threshold interrupt is used by ADC,
> +	* FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
> +	* interrupts are used by ADC,
>  	* hence return from touchscreen IRQ handler if FIFO1
> -	* threshold interrupt occurred.
> +	* related interrupts occurred.
>  	*/
> -	if (status & IRQENB_FIFO1THRES)
> +	if ((status & IRQENB_FIFO1THRES) ||
> +			(status & IRQENB_FIFO1OVRRUN) ||
> +			(status & IRQENB_FIFO1UNDRFLW))
>  		return IRQ_NONE;
Can this not be safely merged into the previous patch touching this
bit of input?

>  	else if (status & IRQENB_FIFO0THRES) {
>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index d72001c..9f5c326 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -282,6 +282,8 @@ static int tscadc_suspend(struct device *dev)
>  	struct ti_tscadc_dev	*tscadc_dev = dev_get_drvdata(dev);
>
>  	tscadc_writel(tscadc_dev, REG_SE, 0x00);
> +	tscadc_dev->irqstat = tscadc_readl(tscadc_dev, REG_IRQENABLE);
> +	tscadc_dev->ctrl = tscadc_readl(tscadc_dev, REG_CTRL);
>  	pm_runtime_put_sync(dev);
>
>  	return 0;
> @@ -290,22 +292,16 @@ static int tscadc_suspend(struct device *dev)
>  static int tscadc_resume(struct device *dev)
>  {
>  	struct ti_tscadc_dev	*tscadc_dev = dev_get_drvdata(dev);
> -	unsigned int restore, ctrl;
>
>  	pm_runtime_get_sync(dev);
>
>  	/* context restore */
> -	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
> -	if (tscadc_dev->tsc_cell != -1)
> -		ctrl |= CNTRLREG_TSCENB | CNTRLREG_4WIRE;
> -	tscadc_writel(tscadc_dev, REG_CTRL, ctrl);
> +	tscadc_writel(tscadc_dev, REG_IRQENABLE, tscadc_dev->irqstat);
>
>  	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,
> -			(restore | CNTRLREG_TSCSSENB));
> +	tscadc_writel(tscadc_dev, REG_CTRL, tscadc_dev->ctrl);
>
>  	return 0;
>  }
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index db1791b..3058aef 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,17 +46,23 @@
>  /* Step Enable */
>  #define STEPENB_MASK		(0x1FFFF << 0)
>  #define STEPENB(val)		((val) << 0)
> +#define ENB(val)			(1 << (val))
> +#define STPENB_STEPENB		STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
>
>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
>  #define IRQENB_FIFO0THRES	BIT(2)
>  #define IRQENB_FIFO1THRES	BIT(5)
>  #define IRQENB_PENUP		BIT(9)
> +#define IRQENB_FIFO1OVRRUN	BIT(6)
> +#define IRQENB_FIFO1UNDRFLW	BIT(7)
>
>  /* Step Configuration */
>  #define STEPCONFIG_MODE_MASK	(3 << 0)
>  #define STEPCONFIG_MODE(val)	((val) << 0)
>  #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
> +#define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
>  #define STEPCONFIG_AVG_MASK	(7 << 2)
>  #define STEPCONFIG_AVG(val)	((val) << 2)
>  #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
> @@ -124,6 +130,7 @@
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
> +#define FIFO1_THRESHOLD		19
>
>  /*
>  * ADC runs at 3MHz, and it takes
> @@ -153,6 +160,9 @@ struct ti_tscadc_dev {
>
>  	/* adc device */
>  	struct adc_device *adc;
> +	/* Context save */
> +	unsigned int irqstat;
> +	unsigned int ctrl;
>  };
>
>  static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>

[PATCH 05/15] 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 9f5c326..b8d7dfb 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 3058aef..136463b 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -127,7 +127,6 @@
 #define SEQ_STATUS BIT(5)
 
 #define ADC_CLK			3000000
-#define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 #define FIFO1_THRESHOLD		19
-- 
1.7.9.5

Re: [PATCH 05/15] MFD: ti_tscadc: ADC Clock check not required

[Jonathan Cameron]

On 07/18/2013 11:21 PM, 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.
A I read this comment, this is a fairly stand alone patch and everything
else in the series will work whether or not it is there?

If so, should not really be part of this series and I'm certainly
going to leave taking it up to the mfd maintainer.
> 
> 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 9f5c326..b8d7dfb 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 3058aef..136463b 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -127,7 +127,6 @@
>  #define SEQ_STATUS BIT(5)
>  
>  #define ADC_CLK			3000000
> -#define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
>  #define FIFO1_THRESHOLD		19
> 

[PATCH 06/15] iio: ti_am335x_adc: Handle set to clear IRQENABLE

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

The driver is currently mishandling the IRQENABLE register. The driver
should write a 1 for bits it wishes to set, and a zero for bits it does not
wish to change. The read of the current register contents is not
necessary.

Write 0 = No action.
Read 0 = Interrupt disabled (masked).
Read 1 = Interrupt enabled.
Write 1 = Enable interrupt.

The current read/update/write method is currently not causing any
problems, but could cause confusion in the future.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index c1b051c..b566e6a 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -207,7 +207,7 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 		container_of(work_s, struct tiadc_device, poll_work);
 	struct iio_dev *idev = iio_priv_to_dev(adc_dev);
 	struct iio_buffer *buffer = idev->buffer;
-	unsigned int fifo1count, readx1, status;
+	unsigned int fifo1count, readx1;
 	int i;
 	u32 *inputbuffer;
 
@@ -223,9 +223,8 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 	}
 
 	buffer->access->store_to(buffer, (u8 *) inputbuffer);
-	status = tiadc_readl(adc_dev, REG_IRQENABLE);
 	tiadc_writel(adc_dev, REG_IRQENABLE,
-			(status | IRQENB_FIFO1THRES));
+			IRQENB_FIFO1THRES);
 
 	kfree(inputbuffer);
 }
@@ -242,7 +241,7 @@ static int tiadc_buffer_postenable(struct iio_dev *idev)
 {
 	struct tiadc_device *adc_dev = iio_priv(idev);
 	struct iio_buffer *buffer = idev->buffer;
-	unsigned int enb, status, fifo1count;
+	unsigned int enb, fifo1count;
 	int stepnum, i;
 	u8 bit;
 
@@ -250,11 +249,10 @@ static int tiadc_buffer_postenable(struct iio_dev *idev)
 		pr_info("Data cannot be read continuously in one shot mode\n");
 		return -EINVAL;
 	} else {
-		status = tiadc_readl(adc_dev, REG_IRQENABLE);
 		tiadc_writel(adc_dev, REG_IRQENABLE,
-				(status | IRQENB_FIFO1THRES)|
-				 IRQENB_FIFO1OVRRUN |
-				 IRQENB_FIFO1UNDRFLW);
+				(IRQENB_FIFO1THRES |
+				IRQENB_FIFO1OVRRUN |
+				IRQENB_FIFO1UNDRFLW));
 
 		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 		for (i = 0; i < fifo1count; i++)
-- 
1.7.9.5

Re: [PATCH 06/15] iio: ti_am335x_adc: Handle set to clear IRQENABLE

[Jonathan Cameron]

On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> The driver is currently mishandling the IRQENABLE register. The driver
> should write a 1 for bits it wishes to set, and a zero for bits it does not
> wish to change. The read of the current register contents is not
> necessary.
> 
> Write 0 = No action.
> Read 0 = Interrupt disabled (masked).
> Read 1 = Interrupt enabled.
> Write 1 = Enable interrupt.
> 
> The current read/update/write method is currently not causing any
> problems, but could cause confusion in the future.
> 
> Signed-off-by: Russ Dill <Russ.Dill@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>

I guess this makes sense as a separate patch to maintain history, but
I'd personally have preferred it rolled into the write places and
appropriate credit given as comments.

I don't like the fact that the code will be broken in a known fashion
at somepoints in this series...

Jonathan
> ---
>  drivers/iio/adc/ti_am335x_adc.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index c1b051c..b566e6a 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -207,7 +207,7 @@ static void tiadc_poll_handler(struct work_struct *work_s)
>  		container_of(work_s, struct tiadc_device, poll_work);
>  	struct iio_dev *idev = iio_priv_to_dev(adc_dev);
>  	struct iio_buffer *buffer = idev->buffer;
> -	unsigned int fifo1count, readx1, status;
> +	unsigned int fifo1count, readx1;
>  	int i;
>  	u32 *inputbuffer;
>  
> @@ -223,9 +223,8 @@ static void tiadc_poll_handler(struct work_struct *work_s)
>  	}
>  
>  	buffer->access->store_to(buffer, (u8 *) inputbuffer);
> -	status = tiadc_readl(adc_dev, REG_IRQENABLE);
>  	tiadc_writel(adc_dev, REG_IRQENABLE,
> -			(status | IRQENB_FIFO1THRES));
> +			IRQENB_FIFO1THRES);
>  
>  	kfree(inputbuffer);
>  }
> @@ -242,7 +241,7 @@ static int tiadc_buffer_postenable(struct iio_dev *idev)
>  {
>  	struct tiadc_device *adc_dev = iio_priv(idev);
>  	struct iio_buffer *buffer = idev->buffer;
> -	unsigned int enb, status, fifo1count;
> +	unsigned int enb, fifo1count;
>  	int stepnum, i;
>  	u8 bit;
>  
> @@ -250,11 +249,10 @@ static int tiadc_buffer_postenable(struct iio_dev *idev)
>  		pr_info("Data cannot be read continuously in one shot mode\n");
>  		return -EINVAL;
>  	} else {
> -		status = tiadc_readl(adc_dev, REG_IRQENABLE);
>  		tiadc_writel(adc_dev, REG_IRQENABLE,
> -				(status | IRQENB_FIFO1THRES)|
> -				 IRQENB_FIFO1OVRRUN |
> -				 IRQENB_FIFO1UNDRFLW);
> +				(IRQENB_FIFO1THRES |
> +				IRQENB_FIFO1OVRRUN |
> +				IRQENB_FIFO1UNDRFLW));
>  
>  		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
>  		for (i = 0; i < fifo1count; i++)
> 

[PATCH 07/15] iio: ti_am335x_adc: Handle set to clear IRQSTATUS

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

The driver is currently mishandling the IRQSTATUS register by peforming
a read/update/write cycle. The actual functionality of the register is as follows:

Write 0 = No action.
Read 0 = No (enabled) event pending.
Read 1 = Event pending.
Write 1 = Clear (raw) event.

By reading the status and writing it back, the driver is clearing
all pending events, not just the one indicated in the bitmask.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index b566e6a..1c47818 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -178,7 +178,7 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 			wake_up_interruptible(&adc_dev->wq_data_avail);
 		}
 		tiadc_writel(adc_dev, REG_IRQSTATUS,
-				(status | IRQENB_FIFO1THRES));
+					IRQENB_FIFO1THRES);
 		return IRQ_HANDLED;
 	} else if ((status &  IRQENB_FIFO1OVRRUN) ||
 			(status &  IRQENB_FIFO1UNDRFLW)) {
@@ -186,12 +186,9 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 		config &= ~(CNTRLREG_TSCSSENB);
 		tiadc_writel(adc_dev,  REG_CTRL, config);
 
-		if (status &  IRQENB_FIFO1UNDRFLW)
-			tiadc_writel(adc_dev,  REG_IRQSTATUS,
-			(status |  IRQENB_FIFO1UNDRFLW));
-		else
-			tiadc_writel(adc_dev,  REG_IRQSTATUS,
-				(status |  IRQENB_FIFO1OVRRUN));
+		tiadc_writel(adc_dev, REG_IRQSTATUS,
+				IRQENB_FIFO1OVRRUN |
+				IRQENB_FIFO1UNDRFLW);
 
 		tiadc_writel(adc_dev,  REG_CTRL,
 			(config |  CNTRLREG_TSCSSENB));
-- 
1.7.9.5

[PATCH 08/15] iio: ti_am335x_adc: Handle overrun before threshold event

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

If an overrun occurs, the threshold event is meaningless, handle
the overrun event first.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 1c47818..7ac28a9 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -167,7 +167,16 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 	unsigned int status, config;
 
 	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
-	if (status & IRQENB_FIFO1THRES) {
+	if (status & IRQENB_FIFO1OVRRUN) {
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		config &= ~(CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev, REG_CTRL, config);
+		tiadc_writel(adc_dev, REG_IRQSTATUS,
+				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+		tiadc_writel(adc_dev, REG_CTRL,
+				(config | CNTRLREG_TSCSSENB));
+		return IRQ_HANDLED;
+	} else if (status & IRQENB_FIFO1THRES) {
 		tiadc_writel(adc_dev, REG_IRQCLR,
 				IRQENB_FIFO1THRES);
 
@@ -180,19 +189,6 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 		tiadc_writel(adc_dev, REG_IRQSTATUS,
 					IRQENB_FIFO1THRES);
 		return IRQ_HANDLED;
-	} else if ((status &  IRQENB_FIFO1OVRRUN) ||
-			(status &  IRQENB_FIFO1UNDRFLW)) {
-		config = tiadc_readl(adc_dev,  REG_CTRL);
-		config &= ~(CNTRLREG_TSCSSENB);
-		tiadc_writel(adc_dev,  REG_CTRL, config);
-
-		tiadc_writel(adc_dev, REG_IRQSTATUS,
-				IRQENB_FIFO1OVRRUN |
-				IRQENB_FIFO1UNDRFLW);
-
-		tiadc_writel(adc_dev,  REG_CTRL,
-			(config |  CNTRLREG_TSCSSENB));
-		return IRQ_HANDLED;
 	} else {
 		return IRQ_NONE;
 	}
-- 
1.7.9.5

Re: [PATCH 08/15] iio: ti_am335x_adc: Handle overrun before threshold event

[Jonathan Cameron]

On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
> From: Russ Dill <Russ.Dill@ti.com>
> 
> If an overrun occurs, the threshold event is meaningless, handle
> the overrun event first.
> 
> Signed-off-by: Russ Dill <Russ.Dill@ti.com>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>

This is getting a little silly.  Would Russ mind if these get
rolled up into the original patches?  Perhaps with his sign-off to
reflect his various contributions?

> ---
>  drivers/iio/adc/ti_am335x_adc.c |   24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 1c47818..7ac28a9 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -167,7 +167,16 @@ static irqreturn_t tiadc_irq(int irq, void *private)
>  	unsigned int status, config;
>  
>  	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> -	if (status & IRQENB_FIFO1THRES) {
> +	if (status & IRQENB_FIFO1OVRRUN) {
> +		config = tiadc_readl(adc_dev, REG_CTRL);
> +		config &= ~(CNTRLREG_TSCSSENB);
> +		tiadc_writel(adc_dev, REG_CTRL, config);
> +		tiadc_writel(adc_dev, REG_IRQSTATUS,
> +				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> +		tiadc_writel(adc_dev, REG_CTRL,
> +				(config | CNTRLREG_TSCSSENB));
> +		return IRQ_HANDLED;
> +	} else if (status & IRQENB_FIFO1THRES) {
>  		tiadc_writel(adc_dev, REG_IRQCLR,
>  				IRQENB_FIFO1THRES);
>  
> @@ -180,19 +189,6 @@ static irqreturn_t tiadc_irq(int irq, void *private)
>  		tiadc_writel(adc_dev, REG_IRQSTATUS,
>  					IRQENB_FIFO1THRES);
>  		return IRQ_HANDLED;
> -	} else if ((status &  IRQENB_FIFO1OVRRUN) ||
> -			(status &  IRQENB_FIFO1UNDRFLW)) {
> -		config = tiadc_readl(adc_dev,  REG_CTRL);
> -		config &= ~(CNTRLREG_TSCSSENB);
> -		tiadc_writel(adc_dev,  REG_CTRL, config);
> -
> -		tiadc_writel(adc_dev, REG_IRQSTATUS,
> -				IRQENB_FIFO1OVRRUN |
> -				IRQENB_FIFO1UNDRFLW);
> -
> -		tiadc_writel(adc_dev,  REG_CTRL,
> -			(config |  CNTRLREG_TSCSSENB));
> -		return IRQ_HANDLED;
>  	} else {
>  		return IRQ_NONE;
>  	}
> 

[PATCH 09/15] iio: ti_am335x_adc: Avoid double threshold event

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

The threshold event handler is being called before the FIFO has
actually reached the threshold.
The current code receives a FIFO threshold event, masks the interrupt,
clears the event, and schedules a workqueue. The workqueue is run, it
empties the FIFO, and unmasks the interrupt.
In the above sequence, after the event is cleared, it immediately
retriggers since the FIFO remains beyond the threshold. When the IRQ is
unmasked, this triggered event generates another IRQ. However, as the
FIFO has just been emptied, it is likely to not contain enough samples.
The waits to clear the event until the FIFO has actually been emptied,
in the workqueue. The unmasking and masking of the interrupt remains
unchanged.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 7ac28a9..8dc070d 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -183,11 +183,9 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 		if (iio_buffer_enabled(idev)) {
 			if (!work_pending(&adc_dev->poll_work))
 				schedule_work(&adc_dev->poll_work);
-		} else {
+		} else
 			wake_up_interruptible(&adc_dev->wq_data_avail);
-		}
-		tiadc_writel(adc_dev, REG_IRQSTATUS,
-					IRQENB_FIFO1THRES);
+
 		return IRQ_HANDLED;
 	} else {
 		return IRQ_NONE;
@@ -216,6 +214,8 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 	}
 
 	buffer->access->store_to(buffer, (u8 *) inputbuffer);
+	tiadc_writel(adc_dev, REG_IRQSTATUS,
+			IRQENB_FIFO1THRES);
 	tiadc_writel(adc_dev, REG_IRQENABLE,
 			IRQENB_FIFO1THRES);
 
-- 
1.7.9.5

[PATCH 10/15] iio: ti_am335x_adc: Also clear threshold event when clearing overrun event

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

When an overrun occurs, the FIFO is cleared. If a FIFO threshold event
was pending, the data is now gone. Clear the threshold event when
handling an overrun (or underflow).

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 8dc070d..6957011 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -171,8 +171,8 @@ static irqreturn_t tiadc_irq(int irq, void *private)
 		config = tiadc_readl(adc_dev, REG_CTRL);
 		config &= ~(CNTRLREG_TSCSSENB);
 		tiadc_writel(adc_dev, REG_CTRL, config);
-		tiadc_writel(adc_dev, REG_IRQSTATUS,
-				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
 		tiadc_writel(adc_dev, REG_CTRL,
 				(config | CNTRLREG_TSCSSENB));
 		return IRQ_HANDLED;
-- 
1.7.9.5

[PATCH 11/15] iio: ti_am335x_adc: Print error and handle short FIFO events

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

In the case that the FIFO threshold handler gets called when the
FIFO has not actually reached the threshold, the driver will pass
uninitialized memory to the IIO subsystem.
In the past, this would occur due to bugs in the driver, those bugs
have been fixed. However, it is still a good idea to close this just
in case additional bugs in hardware or software exist.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 6957011..1b0af2a 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -203,6 +203,13 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 	u32 *inputbuffer;
 
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	if (fifo1count * sizeof(u32) <
+				buffer->access->get_bytes_per_datum(buffer)) {
+		dev_err(adc_dev->mfd_tscadc->dev, "%s: Short FIFO event\n",
+								__func__);
+		goto out;
+	}
+
 	inputbuffer = kmalloc((fifo1count + 1) * sizeof(u32), GFP_KERNEL);
 	if (inputbuffer == NULL)
 		return;
-- 
1.7.9.5

[PATCH 12/15] iio: ti_am335x_adc: Fix allocation count of FIFO buffer.

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

Allocating an extra byte is not necessary here. The driver will check
that the allocation is large enough to satisfy the IIO subsystem.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 1b0af2a..44b7181 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -210,7 +210,7 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 		goto out;
 	}
 
-	inputbuffer = kmalloc((fifo1count + 1) * sizeof(u32), GFP_KERNEL);
+	inputbuffer = kmalloc((fifo1count) * sizeof(u32), GFP_KERNEL);
 	if (inputbuffer == NULL)
 		return;
 
-- 
1.7.9.5

[PATCH 13/15] iio: ti_am335x_adc: Fix capture operation during resume

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

The ADC needs to go through a proper initialization sequence after
resuming from suspend.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 44b7181..265ed27 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -552,11 +552,14 @@ static int tiadc_resume(struct device *dev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	unsigned int restore;
 
+	restore = tiadc_readl(adc_dev, REG_CTRL);
+	restore &= ~(CNTRLREG_TSCSSENB);
+	tiadc_writel(adc_dev, REG_CTRL, restore);
 	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
 	tiadc_step_config(adc_dev, adc_dev->is_continuous_mode);
 	/* Make sure ADC is powered up */
-	restore = tiadc_readl(adc_dev, REG_CTRL);
 	restore &= ~(CNTRLREG_POWERDOWN);
+	restore |= CNTRLREG_TSCSSENB;
 	tiadc_writel(adc_dev, REG_CTRL, restore);
 
 	return 0;
-- 
1.7.9.5

[PATCH 14/15] iio: ti_am335x_adc: Reset and clear overrun status before capture

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

While not pulling out samples, the FIFO will fill up causing an
overrun event. Before starting up another continuous sample, clear that
event.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/iio/adc/ti_am335x_adc.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 265ed27..00fdb22 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -241,22 +241,23 @@ static int tiadc_buffer_postenable(struct iio_dev *idev)
 {
 	struct tiadc_device *adc_dev = iio_priv(idev);
 	struct iio_buffer *buffer = idev->buffer;
-	unsigned int enb, fifo1count;
-	int stepnum, i;
+	unsigned int enb, config;
+	int stepnum;
 	u8 bit;
 
 	if (!adc_dev->is_continuous_mode) {
 		pr_info("Data cannot be read continuously in one shot mode\n");
 		return -EINVAL;
 	} else {
-		tiadc_writel(adc_dev, REG_IRQENABLE,
-				(IRQENB_FIFO1THRES |
-				IRQENB_FIFO1OVRRUN |
-				IRQENB_FIFO1UNDRFLW));
-
-		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
-		for (i = 0; i < fifo1count; i++)
-			tiadc_readl(adc_dev, REG_FIFO1);
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		tiadc_writel(adc_dev, REG_CTRL,
+					config & ~CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev, REG_CTRL,
+					config |  CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+		tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN);
 
 		tiadc_writel(adc_dev, REG_SE, 0x00);
 		for_each_set_bit(bit, buffer->scan_mask,
-- 
1.7.9.5

[PATCH 15/15] iio: ti_am335x_adc: Properly handle out of memory situation.

[Zubair Lutfullah]

From: Russ Dill <Russ.Dill@ti.com>

If we fail to allocate a buffer, unmask the interrupt to allow a retry.
The interrupt handler will be re-run, and our workqueue rescheduled.
If we are able to allocate memory next time around, everything will
continue as normal, otherwise, we will eventually get an underrun.
Before this patch, the driver would stop capturing without any
indication of error to the IIO subsystem or the user.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 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 00fdb22..5b0efbe 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -212,7 +212,7 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 
 	inputbuffer = kmalloc((fifo1count) * sizeof(u32), GFP_KERNEL);
 	if (inputbuffer == NULL)
-		return;
+		goto out;
 
 	for (i = 0; i < fifo1count; i++) {
 		readx1 = tiadc_readl(adc_dev, REG_FIFO1);
@@ -221,12 +221,14 @@ static void tiadc_poll_handler(struct work_struct *work_s)
 	}
 
 	buffer->access->store_to(buffer, (u8 *) inputbuffer);
+	kfree(inputbuffer);
+
+out:
 	tiadc_writel(adc_dev, REG_IRQSTATUS,
 			IRQENB_FIFO1THRES);
 	tiadc_writel(adc_dev, REG_IRQENABLE,
 			IRQENB_FIFO1THRES);
 
-	kfree(inputbuffer);
 }
 
 static int tiadc_buffer_preenable(struct iio_dev *idev)
-- 
1.7.9.5

Re: [PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[Jonathan Cameron]

On 07/18/2013 11:21 PM, Zubair Lutfullah wrote:
> Patches now give correct authorship.
> 
> and checkpatch.pl issues are checked for each patch.
> 
> I hope the actual code bashing can begin now.
> 
> A series of patches that add continuous sampling support
> for the adc drivers for the am335x.
> 
> These apply on top of mfd-next after the recent set of patches
> on this driver by Sebastian Andrzej Siewior
> 
> Tested on the Beaglebone Black running 3.11
> 
> Patil, Rachna (5):
>   MFD: ti_tscadc: disable TSC config registers in adc mode
>   iio: ti_am335x_adc: Fix wrong samples received on 1st read
>   input: ti_tsc: Enable shared IRQ for TSC
>   iio: mfd: input: ti_am335x_adc:Add support for continuous mode
>   MFD: ti_tscadc: ADC Clock check not required
> 
> Russ Dill (10):
>   iio: ti_am335x_adc: Handle set to clear IRQENABLE
>   iio: ti_am335x_adc: Handle set to clear IRQSTATUS
>   iio: ti_am335x_adc: Handle overrun before threshold event
>   iio: ti_am335x_adc: Avoid double threshold event
>   iio: ti_am335x_adc: Also clear threshold event when clearing overrun
>     event
>   iio: ti_am335x_adc: Print error and handle short FIFO events
>   iio: ti_am335x_adc: Fix allocation count of FIFO buffer.
>   iio: ti_am335x_adc: Fix capture operation during resume
>   iio: ti_am335x_adc: Reset and clear overrun status before capture
>   iio: ti_am335x_adc: Properly handle out of memory situation

I am a little irritated by the mess we have here.  Russ has clearly done
a lot of fine work cleaning up the earlier patches.  As a result
we have a series of initial buggy patches and then a series of patches
fixing them again.

As you are submitting these for mainline I would really like it all merged
down into a clean series of clear patches.

Part 1.  Any fixes that are unconnected to the rest of the series.  These will
  then get sent upstream within this cycle.

Part 2. Feature add patch. Here that is basically a single patch adding the
  continous mode support.

It is nice to maintain history and all but I would much rather have something
that is easy to review, with appropriate comments or if people will give
them, sign offs to reflect the various contributions.

Jonathan


> 
>  drivers/iio/adc/ti_am335x_adc.c           |  354 +++++++++++++++++++++++++----
>  drivers/input/touchscreen/ti_am335x_tsc.c |   17 +-
>  drivers/mfd/ti_am335x_tscadc.c            |   30 ++-
>  include/linux/mfd/ti_am335x_tscadc.h      |   27 ++-
>  4 files changed, 366 insertions(+), 62 deletions(-)
> 

Re: [PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[Zubair Lutfullah :]

On Sat, Jul 20, 2013 at 12:25:48PM +0100, Jonathan Cameron wrote:
> I am a little irritated by the mess we have here.  Russ has clearly done
> a lot of fine work cleaning up the earlier patches.  As a result
> we have a series of initial buggy patches and then a series of patches
> fixing them again.

Indeed.

I had already merged a lot of minor bug fixes from Rachil into the main
patch that adds continuous mode.

But I kept Russ's ones separate because of authorship and history.
 

> Part 1.  Any fixes that are unconnected to the rest of the series.  These will
>   then get sent upstream within this cycle.

Understood. I'll send a small series that is disjoint and contains fixes for the existing driver.

> Part 2. Feature add patch. Here that is basically a single patch adding the
>   continous mode support.
>
> It is nice to maintain history and all but I would much rather have something
> that is easy to review, with appropriate comments or if people will give
> them, sign offs to reflect the various contributions.
> 
> Jonathan

And then one patch for continuous mode with appropriate comments/sign-offs.

Thank-you
Zubair Lutfullah

Re: [PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[Lars-Peter Clausen]

On 07/20/2013 02:47 PM, Zubair Lutfullah : wrote:
> On Sat, Jul 20, 2013 at 12:25:48PM +0100, Jonathan Cameron wrote:
>> I am a little irritated by the mess we have here.  Russ has clearly done
>> a lot of fine work cleaning up the earlier patches.  As a result
>> we have a series of initial buggy patches and then a series of patches
>> fixing them again.
>
> Indeed.
>
> I had already merged a lot of minor bug fixes from Rachil into the main
> patch that adds continuous mode.
>
> But I kept Russ's ones separate because of authorship and history.

It's ok to merge the patches if you add a note to the commit messages saying 
what aspects you fixed. There is no need to add patches to the git tree that we 
know of that they are broken.

- Lars

Re: [PATCH 00/15] iio: ti_am335x_adc: Add continuous mode take 2

[Jonathan Cameron]

"Zubair Lutfullah :" <zubair.lutfullah@gmail.com> wrote:
>On Sat, Jul 20, 2013 at 12:25:48PM +0100, Jonathan Cameron wrote:
>> I am a little irritated by the mess we have here.  Russ has clearly
>done
>> a lot of fine work cleaning up the earlier patches.  As a result
>> we have a series of initial buggy patches and then a series of
>patches
>> fixing them again.
>
>Indeed.
>
>I had already merged a lot of minor bug fixes from Rachil into the main
>patch that adds continuous mode.
>
>But I kept Russ's ones separate because of authorship and history.
> 
>
>> Part 1.  Any fixes that are unconnected to the rest of the series. 
>These will
>>   then get sent upstream within this cycle.
>
>Understood. I'll send a small series that is disjoint and contains
>fixes for the existing driver.
>
>> Part 2. Feature add patch. Here that is basically a single patch
>adding the
>>   continous mode support.
>>
>> It is nice to maintain history and all but I would much rather have
>something
>> that is easy to review, with appropriate comments or if people will
>give
>> them, sign offs to reflect the various contributions.
>> 
>> Jonathan
>
>And then one patch for continuous mode with appropriate
>comments/sign-offs.
>
Excellent. Thanks for sorting these out.
>Thank-you
>Zubair Lutfullah
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.