[PATCH 0/4] iio: input: ti_am335x_adc: Add continuous sampling support round 4

[PATCH 0/4] iio: input: ti_am335x_adc: Add continuous sampling support round 4

[Zubair Lutfullah]

Note: These apply to the fixes-togreg branch in IIO because of
a fix on the adc side in there.

The first few are for input which tweak the TSC driver to
allow ADC in continuous mode with irqs to work nicely together.

The last one is for iio which adds continuous mode to the adc side.

Received feedback on previous series.
1. What if IRQs occur together? 
This is handled now. Even if both assert.
They both work.

2. IIO error handling wrong. 
Fixed now.

3. Headers wierd in IIO. 
Fixed.

Apart from that, found a few bugs in continuous mode
here and there. And squashed them into the same patch.

Thanks
Zubair Lutfullah

Zubair Lutfullah (4):
  input: ti_am335x_tsc: correct step mask update after IRQ
  input: ti_am335x_tsc: Increase sequencer delay time
  input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow
    checks
  iio: ti_am335x_adc: Add continuous sampling and trigger support

 drivers/iio/adc/ti_am335x_adc.c           |  353 ++++++++++++++++++++++++-----
 drivers/input/touchscreen/ti_am335x_tsc.c |   45 +++-
 include/linux/mfd/ti_am335x_tscadc.h      |   15 +-
 3 files changed, 342 insertions(+), 71 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] input: ti_am335x_tsc: correct step mask update after IRQ

[Zubair Lutfullah]

TSC steps should be enabled again after IRQ routine.
This fix ensures they are updated correctly every time.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..e165fcb 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
 	u32			config_inp[4];
 	u32			bit_xp, bit_xn, bit_yp, bit_yn;
 	u32			inp_xp, inp_xn, inp_yp, inp_yn;
+	u32			step_mask;
 };
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	/* The steps1 … end and bit 0 for TS_Charge */
 	stepenable = (1 << (end_step + 2)) - 1;
-	am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
+	ts_dev->step_mask = stepenable;
+	am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -316,7 +318,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	if (irqclr) {
 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
-		am335x_tsc_se_update(ts_dev->mfd_tscadc);
+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 		return IRQ_HANDLED;
 	}
 	return IRQ_NONE;
-- 
1.7.9.5

[PATCH 2/4] input: ti_am335x_tsc: Increase sequencer delay time

[Zubair Lutfullah]

Before checking PEN UP event, IRQ delays for this amount
of time to let FSM stabilize. Previously, with only the
TSC driver in IRQ mode, this delay was sufficient.

The delay is increased slightly to accomodate if the ADC
is also being used simultaneously in continuous mode.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e165fcb..766bc7e 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -31,7 +31,7 @@
 #include <linux/mfd/ti_am335x_tscadc.h>
 
 #define ADCFSM_STEPID		0x10
-#define SEQ_SETTLE		275
+#define SEQ_SETTLE		350
 #define MAX_12BIT		((1 << 12) - 1)
 
 static const int config_pins[] = {
-- 
1.7.9.5

[PATCH 3/4] input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow checks

[Zubair Lutfullah]

Enable shared IRQ to allow ADC to share IRQ line from
parent MFD core. Only FIFO0 IRQs are for TSC and handled
on the TSC side.

Patch also adds overrun and underflow irq handlers.

Russ Dill (TI) worked on overrun and underflow handlers.
Rachna Patil (TI) laid ground work for shared IRQ.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   37 +++++++++++++++++++++++++----
 include/linux/mfd/ti_am335x_tscadc.h      |    2 ++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 766bc7e..9c114b2 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -256,14 +256,39 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 {
 	struct titsc *ts_dev = dev;
 	struct input_dev *input_dev = ts_dev->input;
-	unsigned int status, irqclr = 0;
+	unsigned int status, irqclr = 0, config = 0;
 	unsigned int x = 0, y = 0;
 	unsigned int z1, z2, z;
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
-	if (status & IRQENB_FIFO0THRES) {
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
+	 * interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 * and check if any IRQs left in case both fifos interrupt.
+	 * If any irq left, return none, else return handled.
+	 */
+	if ((status & IRQENB_FIFO0OVRRUN) ||
+			(status & IRQENB_FIFO0UNDRFLW)) {
+
+		config = titsc_readl(ts_dev, REG_CTRL);
+		config &= ~(CNTRLREG_TSCSSENB);
+		titsc_writel(ts_dev, REG_CTRL, config);
+
+		if (status & IRQENB_FIFO0UNDRFLW) {
+			titsc_writel(ts_dev, REG_IRQSTATUS,
+				(status | IRQENB_FIFO0UNDRFLW));
+			irqclr |= IRQENB_FIFO0UNDRFLW;
+		} else {
+			titsc_writel(ts_dev, REG_IRQSTATUS,
+				(status | IRQENB_FIFO0OVRRUN));
+			irqclr |= IRQENB_FIFO0OVRRUN;
+		}
 
+		titsc_writel(ts_dev, REG_CTRL,
+			(config | CNTRLREG_TSCSSENB));
+	} else if (status & IRQENB_FIFO0THRES) {
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
 
 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
@@ -317,9 +342,11 @@ 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_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
-		return IRQ_HANDLED;
+		status = titsc_readl(ts_dev, REG_IRQSTATUS);
+		if (status == false)
+			return IRQ_HANDLED;
 	}
 	return IRQ_NONE;
 }
@@ -391,7 +418,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;
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..e2db978 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -50,6 +50,8 @@
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
 #define IRQENB_FIFO0THRES	BIT(2)
+#define IRQENB_FIFO0OVRRUN	BIT(3)
+#define IRQENB_FIFO0UNDRFLW	BIT(4)
 #define IRQENB_FIFO1THRES	BIT(5)
 #define IRQENB_PENUP		BIT(9)
 
-- 
1.7.9.5

[PATCH 4/4] iio: ti_am335x_adc: Add continuous sampling and trigger support

[Zubair Lutfullah]

Previously the driver had only one-shot reading functionality.
This patch adds triggered buffer support to the driver.
A buffer of samples can now be read via /dev/iio.
Any IIO trigger can be used to start acquisition.

Patil Rachna (TI) laid the ground work for ADC HW register access.
Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.

I fixed channel scanning so multiple ADC channels can be read
simultaneously and pushed to userspace. Restructured the driver
to fit IIO ABI. And added trigger support.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Russ Dill <Russ.Dill@ti.com>
---
 drivers/iio/adc/ti_am335x_adc.c      |  353 ++++++++++++++++++++++++++++------
 include/linux/mfd/ti_am335x_tscadc.h |   13 +-
 2 files changed, 303 insertions(+), 63 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 3ceac3e..0d7e313 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -24,16 +24,28 @@
 #include <linux/iio/iio.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.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;
+	bool data_avail;
+	u32 *inputbuffer;
+	int sample_count;
+	int irq;
+	int buffer_en_ch_steps;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,27 +68,28 @@ 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 iio_dev *indio_dev)
 {
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	unsigned int stepconfig;
-	int i, steps;
+	int i, steps, chan;
 
 	/*
 	 * There are 16 configurable steps and 8 analog input
 	 * lines available which are shared between Touchscreen and ADC.
-	 *
 	 * Steps backwards i.e. from 16 towards 0 are used by ADC
 	 * depending on number of input lines needed.
 	 * Channel would represent which analog input
 	 * needs to be given to ADC to digitalize data.
 	 */
-
 	steps = TOTAL_STEPS - adc_dev->channels;
-	stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
+	if (iio_buffer_enabled(indio_dev))
+		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
+					| STEPCONFIG_MODE_SWCNT;
+	else
+		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
 
 	for (i = 0; i < adc_dev->channels; i++) {
-		int chan;
-
 		chan = adc_dev->channel_line[i];
 		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
 				stepconfig | STEPCONFIG_INP(chan));
@@ -85,7 +98,203 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 		adc_dev->channel_step[i] = steps;
 		steps++;
 	}
+}
+
+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);
+
+	/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
+	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 | IRQENB_FIFO1THRES);
+		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+	} else if (status & IRQENB_FIFO1THRES) {
+		/* Wake adc_work that pushes FIFO data to iio buffer */
+		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
+		adc_dev->data_avail = 1;
+		wake_up_interruptible(&adc_dev->wq_data_avail);
+	} else
+		return IRQ_NONE;
+
+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+	if (status == false)
+			return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
+static irqreturn_t tiadc_trigger_h(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int config;
+
+	schedule_work(&adc_dev->poll_work);
+	am335x_tsc_se_set(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
+	if (adc_dev->mfd_tscadc->tsc_cell == -1) {
+		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);
+
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
+{
+	return iio_sw_buffer_preenable(indio_dev);
+}
+
+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
+	unsigned int enb = 0, stepnum;
+	u8 bit;
+
+	tiadc_step_config(indio_dev);
+	for_each_set_bit(bit, buffer->scan_mask,
+			adc_dev->channels) {
+		struct iio_chan_spec const *chan = indio_dev->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 |= (1 << stepnum);
+	}
+	adc_dev->buffer_en_ch_steps = enb;
+
+	return iio_triggered_buffer_postenable(indio_dev);
+}
+
+static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	int fifo1count, i, read, config;
+
+	if (adc_dev->mfd_tscadc->tsc_cell == -1) {
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		config &= ~(CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev, REG_CTRL, config);
+	} else
+		tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB_TC);
+
+	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
+
+	/* Flush FIFO of any leftover data */
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (i = 0; i < fifo1count; i++)
+		read = tiadc_readl(adc_dev, REG_FIFO1);
+
+	return iio_triggered_buffer_predisable(indio_dev);
+}
+
+static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	int config;
+
+	tiadc_step_config(indio_dev);
+	if (adc_dev->mfd_tscadc->tsc_cell == -1) {
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+	}
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
+	.preenable = &tiadc_buffer_preenable,
+	.postenable = &tiadc_buffer_postenable,
+	.predisable = &tiadc_buffer_predisable,
+	.postdisable = &tiadc_buffer_postdisable,
+};
+
+static void tiadc_adc_work(struct work_struct *work_s)
+{
+	struct tiadc_device *adc_dev =
+		container_of(work_s, struct tiadc_device, poll_work);
+	struct iio_dev *indio_dev = iio_priv_to_dev(adc_dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
+	int i, j, k, fifo1count, read;
+	unsigned int config;
+	int size_to_acquire = buffer->access->get_length(buffer);
+	int sample_count = 0;
+	u32 *data;
+
+	adc_dev->data_avail = 0;
+	data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	if (data == NULL)
+		goto out;
+
+	while (sample_count < size_to_acquire) {
+		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
+		tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
+
+		wait_event_interruptible(adc_dev->wq_data_avail,
+					(adc_dev->data_avail == 1));
+		adc_dev->data_avail = 0;
+
+		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+		if (fifo1count * sizeof(u32) <
+				buffer->access->get_bytes_per_datum(buffer))
+			continue;
+
+		sample_count = sample_count + fifo1count;
+		for (k = 0; k < fifo1count; k = k + i) {
+			for (i = 0, j = 0; i < (indio_dev->scan_bytes)/4; i++) {
+				read = tiadc_readl(adc_dev, REG_FIFO1);
+				data[i] = read & FIFOREAD_DATA_MASK;
+			}
+			iio_push_to_buffers(indio_dev, (u8 *) data);
+		}
+	}
+out:
+	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
+	am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
+	if (adc_dev->mfd_tscadc->tsc_cell == -1) {
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		tiadc_writel(adc_dev, REG_CTRL,	config & ~CNTRLREG_TSCSSENB);
+	}
+}
+
+irqreturn_t tiadc_iio_pollfunc(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	int i, fifo1count, read;
+
+	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
+				IRQENB_FIFO1OVRRUN |
+				IRQENB_FIFO1UNDRFLW));
+
+	/* Flush FIFO before trigger */
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (i = 0; i < fifo1count; i++)
+		read = tiadc_readl(adc_dev, REG_FIFO1);
 
+	return IRQ_WAKE_THREAD;
 }
 
 static const char * const chan_name_ain[] = {
@@ -120,13 +329,13 @@ 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;
 	}
 
 	indio_dev->channels = chan_array;
-
 	return 0;
 }
 
@@ -141,58 +350,51 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	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,
-	 * 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;
+	unsigned int fifo1count, read, stepid, step_en;
 
-		if (stepid == map_val) {
-			read = read & FIFOREAD_DATA_MASK;
-			found = true;
-			*val = read;
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+	else {
+		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.
+		 */
+
+		*val = -1;
+		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;
+				*val = read;
+			}
 		}
+		if (*val != -1)
+			return IIO_VAL_INT;
+		else
+			return -EAGAIN;
 	}
-
-	if (found == false)
-		return -EBUSY;
-	return IIO_VAL_INT;
 }
 
 static const struct iio_info tiadc_info = {
@@ -231,26 +433,45 @@ 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);
+	tiadc_step_config(indio_dev);
+	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
 
 	err = tiadc_channel_init(indio_dev, adc_dev->channels);
 	if (err < 0)
 		goto err_free_device;
 
-	err = iio_device_register(indio_dev);
+	INIT_WORK(&adc_dev->poll_work, &tiadc_adc_work);
+	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_channels;
 
+	err = iio_triggered_buffer_setup(indio_dev, &tiadc_iio_pollfunc,
+			&tiadc_trigger_h, &tiadc_buffer_setup_ops);
+	if (err)
+		goto err_free_irq;
+
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto err_buffer_unregister;
+
 	platform_set_drvdata(pdev, indio_dev);
 
 	return 0;
 
+err_buffer_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:
@@ -265,7 +486,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);
@@ -303,10 +526,16 @@ static int tiadc_resume(struct device *dev)
 
 	/* 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);
 
-	tiadc_step_config(adc_dev);
+	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
+	tiadc_step_config(indio_dev);
+
+	/* Make sure ADC is powered up */
+	restore &= ~(CNTRLREG_POWERDOWN);
+	restore |= CNTRLREG_TSCSSENB;
+	tiadc_writel(adc_dev, REG_CTRL, restore);
 
 	return 0;
 }
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e2db978..a1d22de 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,6 +46,9 @@
 /* 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)
@@ -54,11 +57,14 @@
 #define IRQENB_FIFO0UNDRFLW	BIT(4)
 #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)
@@ -126,7 +132,8 @@
 #define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
-
+#define FIFO1_THRESHOLD		19
+#define FIFO_SIZE			64
 /*
 * ADC runs at 3MHz, and it takes
 * 15 cycles to latch one data output.
@@ -155,6 +162,10 @@ 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 4/4] iio: ti_am335x_adc: Add continuous sampling and trigger support

[Lee Jones]

On Tue, 13 Aug 2013, Zubair Lutfullah wrote:

> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
> A buffer of samples can now be read via /dev/iio.
> Any IIO trigger can be used to start acquisition.
> 
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
> 
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace. Restructured the driver
> to fit IIO ABI. And added trigger support.
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Russ Dill <Russ.Dill@ti.com>
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  353 ++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |   13 +-
>  2 files changed, 303 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c

<snip>

> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index e2db978..a1d22de 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,6 +46,9 @@
>  /* 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)
> @@ -54,11 +57,14 @@
>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
>  #define IRQENB_FIFO1THRES	BIT(5)
>  #define IRQENB_PENUP		BIT(9)
> +#define IRQENB_FIFO1OVRRUN	BIT(6)
> +#define IRQENB_FIFO1UNDRFLW	BIT(7)

Nit: Can we keep these in numerical order?

>  /* 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)
> @@ -126,7 +132,8 @@
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
> -
> +#define FIFO1_THRESHOLD		19
> +#define FIFO_SIZE			64

Nit: Keep the line space between the defines and the multi-line comment.

>  /*
>  * ADC runs at 3MHz, and it takes
>  * 15 cycles to latch one data output.
> @@ -155,6 +162,10 @@ 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)

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

Re: [PATCH 4/4] iio: ti_am335x_adc: Add continuous sampling and trigger support

[Lee Jones]

On Tue, 13 Aug 2013, Zubair Lutfullah wrote:

> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
> A buffer of samples can now be read via /dev/iio.
> Any IIO trigger can be used to start acquisition.
> 
> Patil Rachna (TI) laid the ground work for ADC HW register access.
> Russ Dill (TI) fixed bugs in the driver relevant to FIFOs and IRQs.
> 
> I fixed channel scanning so multiple ADC channels can be read
> simultaneously and pushed to userspace. Restructured the driver
> to fit IIO ABI. And added trigger support.
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Russ Dill <Russ.Dill@ti.com>
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  353 ++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |   13 +-
>  2 files changed, 303 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c

<snip>

> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index e2db978..a1d22de 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,6 +46,9 @@
>  /* 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)
> @@ -54,11 +57,14 @@
>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
>  #define IRQENB_FIFO1THRES	BIT(5)
>  #define IRQENB_PENUP		BIT(9)
> +#define IRQENB_FIFO1OVRRUN	BIT(6)
> +#define IRQENB_FIFO1UNDRFLW	BIT(7)

Nit: Can we keep these in numerical order?

>  /* 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)
> @@ -126,7 +132,8 @@
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
> -
> +#define FIFO1_THRESHOLD		19
> +#define FIFO_SIZE			64

Nit: Keep the line space between the defines and the multi-line comment.

>  /*
>  * ADC runs at 3MHz, and it takes
>  * 15 cycles to latch one data output.
> @@ -155,6 +162,10 @@ 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)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow checks

[Lee Jones]

On Tue, 13 Aug 2013, Zubair Lutfullah wrote:

> Enable shared IRQ to allow ADC to share IRQ line from
> parent MFD core. Only FIFO0 IRQs are for TSC and handled
> on the TSC side.
> 
> Patch also adds overrun and underflow irq handlers.
> 
> Russ Dill (TI) worked on overrun and underflow handlers.
> Rachna Patil (TI) laid ground work for shared IRQ.
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   37 +++++++++++++++++++++++++----
>  include/linux/mfd/ti_am335x_tscadc.h      |    2 ++
>  2 files changed, 34 insertions(+), 5 deletions(-)

For the MFD part when the remainder of the entries have been placed in
order:

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

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

Re: [PATCH 1/4] input: ti_am335x_tsc: correct step mask update after IRQ

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 17:48:16 [+0100]:

>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index e1c5300..e165fcb 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -52,6 +52,7 @@ struct titsc {
> 	u32			config_inp[4];
> 	u32			bit_xp, bit_xn, bit_yp, bit_yn;
> 	u32			inp_xp, inp_xn, inp_yp, inp_yn;
>+	u32			step_mask;
> };
> 
> static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
>@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
> 
> 	/* The steps1 … end and bit 0 for TS_Charge */
> 	stepenable = (1 << (end_step + 2)) - 1;
>-	am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
>+	ts_dev->step_mask = stepenable;
>+	am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> }
> 
> static void titsc_read_coordinates(struct titsc *ts_dev,
>@@ -316,7 +318,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> 
> 	if (irqclr) {
> 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
>-		am335x_tsc_se_update(ts_dev->mfd_tscadc);
>+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> 		return IRQ_HANDLED;
> 	}
> 	return IRQ_NONE;

titsc_step_config() computes the mask once since it does not change. It
is then assigned via am335x_tsc_se_set() to ->reg_se_cache() and later
always udpated via am335x_tsc_se_update(). This should ensure that ADC's
and TSC's bits are in sync and clear each other out.
Now you call am335x_tsc_se_set() in every irq which adds the TSC's mask
to ->reg_se_cache but why? It was never removed.

Sebastian

Re: [PATCH 2/4] input: ti_am335x_tsc: Increase sequencer delay time

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 17:48:17 [+0100]:

>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index e165fcb..766bc7e 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -31,7 +31,7 @@
> #include <linux/mfd/ti_am335x_tscadc.h>
> 
> #define ADCFSM_STEPID		0x10
>-#define SEQ_SETTLE		275
>+#define SEQ_SETTLE		350
> #define MAX_12BIT		((1 << 12) - 1)

Okay, if this is required then it is. But in the long turn we should
make this interrupt threaded since this delay in interrupt context is
way too long.

Sebastian

Re: [PATCH 3/4] input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow checks

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 17:48:18 [+0100]:

>Enable shared IRQ to allow ADC to share IRQ line from
>parent MFD core. Only FIFO0 IRQs are for TSC and handled
>on the TSC side.
>
>Patch also adds overrun and underflow irq handlers.
>
>Russ Dill (TI) worked on overrun and underflow handlers.
>Rachna Patil (TI) laid ground work for shared IRQ.
>
>Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
>---
> drivers/input/touchscreen/ti_am335x_tsc.c |   37 +++++++++++++++++++++++++----
> include/linux/mfd/ti_am335x_tscadc.h      |    2 ++
> 2 files changed, 34 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index 766bc7e..9c114b2 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -256,14 +256,39 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> {
> 	struct titsc *ts_dev = dev;
> 	struct input_dev *input_dev = ts_dev->input;
>-	unsigned int status, irqclr = 0;
>+	unsigned int status, irqclr = 0, config = 0;
> 	unsigned int x = 0, y = 0;
> 	unsigned int z1, z2, z;
> 	unsigned int fsm;
> 
> 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>-	if (status & IRQENB_FIFO0THRES) {
>+	/*
>+	 * ADC and touchscreen share the IRQ line.
>+	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
>+	 * interrupts are used by ADC. Handle FIFO0 IRQs here only
>+	 * and check if any IRQs left in case both fifos interrupt.
>+	 * If any irq left, return none, else return handled.
>+	 */
>+	if ((status & IRQENB_FIFO0OVRRUN) ||
>+			(status & IRQENB_FIFO0UNDRFLW)) {
>+
>+		config = titsc_readl(ts_dev, REG_CTRL);
>+		config &= ~(CNTRLREG_TSCSSENB);
>+		titsc_writel(ts_dev, REG_CTRL, config);
>+
>+		if (status & IRQENB_FIFO0UNDRFLW) {
>+			titsc_writel(ts_dev, REG_IRQSTATUS,
>+				(status | IRQENB_FIFO0UNDRFLW));
>+			irqclr |= IRQENB_FIFO0UNDRFLW;
>+		} else {
>+			titsc_writel(ts_dev, REG_IRQSTATUS,
>+				(status | IRQENB_FIFO0OVRRUN));
>+			irqclr |= IRQENB_FIFO0OVRRUN;
>+		}

You don't do anything on overflow / underflow. Is this due to the fact
once enabled for FIFO1 it also triggers for FIFO0?

>+		titsc_writel(ts_dev, REG_CTRL,
>+			(config | CNTRLREG_TSCSSENB));
>+	} else if (status & IRQENB_FIFO0THRES) {
> 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> 
> 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
>@@ -317,9 +342,11 @@ 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));

Shouldn't FIFO1UNDRFLW & OVRRUN be handled by the adc driver? Why do you
or the unhandled bits as well here?

> 		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
>-		return IRQ_HANDLED;
>+		status = titsc_readl(ts_dev, REG_IRQSTATUS);
>+		if (status == false)
>+			return IRQ_HANDLED;

And why this? If you something you handled it, if you didn't you return
NONE. Why does it depend on REG_IRQSTATUS?

> 	}
> 	return IRQ_NONE;
> }

Sebastian

Re: [PATCH 3/4] input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow checks

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 17:48:18 [+0100]:

>Enable shared IRQ to allow ADC to share IRQ line from
>parent MFD core. Only FIFO0 IRQs are for TSC and handled
>on the TSC side.
>
>Patch also adds overrun and underflow irq handlers.
>
>Russ Dill (TI) worked on overrun and underflow handlers.
>Rachna Patil (TI) laid ground work for shared IRQ.
>
>Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>---
> drivers/input/touchscreen/ti_am335x_tsc.c |   37 +++++++++++++++++++++++++----
> include/linux/mfd/ti_am335x_tscadc.h      |    2 ++
> 2 files changed, 34 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index 766bc7e..9c114b2 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -256,14 +256,39 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> {
> 	struct titsc *ts_dev = dev;
> 	struct input_dev *input_dev = ts_dev->input;
>-	unsigned int status, irqclr = 0;
>+	unsigned int status, irqclr = 0, config = 0;
> 	unsigned int x = 0, y = 0;
> 	unsigned int z1, z2, z;
> 	unsigned int fsm;
> 
> 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>-	if (status & IRQENB_FIFO0THRES) {
>+	/*
>+	 * ADC and touchscreen share the IRQ line.
>+	 * FIFO1 threshold, FIFO1 Overrun and FIFO1 underflow
>+	 * interrupts are used by ADC. Handle FIFO0 IRQs here only
>+	 * and check if any IRQs left in case both fifos interrupt.
>+	 * If any irq left, return none, else return handled.
>+	 */
>+	if ((status & IRQENB_FIFO0OVRRUN) ||
>+			(status & IRQENB_FIFO0UNDRFLW)) {
>+
>+		config = titsc_readl(ts_dev, REG_CTRL);
>+		config &= ~(CNTRLREG_TSCSSENB);
>+		titsc_writel(ts_dev, REG_CTRL, config);
>+
>+		if (status & IRQENB_FIFO0UNDRFLW) {
>+			titsc_writel(ts_dev, REG_IRQSTATUS,
>+				(status | IRQENB_FIFO0UNDRFLW));
>+			irqclr |= IRQENB_FIFO0UNDRFLW;
>+		} else {
>+			titsc_writel(ts_dev, REG_IRQSTATUS,
>+				(status | IRQENB_FIFO0OVRRUN));
>+			irqclr |= IRQENB_FIFO0OVRRUN;
>+		}

You don't do anything on overflow / underflow. Is this due to the fact
once enabled for FIFO1 it also triggers for FIFO0?

>+		titsc_writel(ts_dev, REG_CTRL,
>+			(config | CNTRLREG_TSCSSENB));
>+	} else if (status & IRQENB_FIFO0THRES) {
> 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> 
> 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
>@@ -317,9 +342,11 @@ 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));

Shouldn't FIFO1UNDRFLW & OVRRUN be handled by the adc driver? Why do you
or the unhandled bits as well here?

> 		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
>-		return IRQ_HANDLED;
>+		status = titsc_readl(ts_dev, REG_IRQSTATUS);
>+		if (status == false)
>+			return IRQ_HANDLED;

And why this? If you something you handled it, if you didn't you return
NONE. Why does it depend on REG_IRQSTATUS?

> 	}
> 	return IRQ_NONE;
> }

Sebastian

Re: [PATCH 3/4] input: ti_tsc: Enable shared IRQ for TSC and add overrun, underflow checks

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 11:14:09AM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 17:48:18 [+0100]:
> >+	if ((status & IRQENB_FIFO0OVRRUN) ||
> >+			(status & IRQENB_FIFO0UNDRFLW)) {
> >+
> >+		config = titsc_readl(ts_dev, REG_CTRL);
> >+		config &= ~(CNTRLREG_TSCSSENB);
> >+		titsc_writel(ts_dev, REG_CTRL, config);
> >+
> >+		if (status & IRQENB_FIFO0UNDRFLW) {
> >+			titsc_writel(ts_dev, REG_IRQSTATUS,
> >+				(status | IRQENB_FIFO0UNDRFLW));
> >+			irqclr |= IRQENB_FIFO0UNDRFLW;
> >+		} else {
> >+			titsc_writel(ts_dev, REG_IRQSTATUS,
> >+				(status | IRQENB_FIFO0OVRRUN));
> >+			irqclr |= IRQENB_FIFO0OVRRUN;
> >+		}
> 
> You don't do anything on overflow / underflow. Is this due to the fact
> once enabled for FIFO1 it also triggers for FIFO0?
> 
The TSCADC module doesn't recover from these interrupts.

> >+		titsc_writel(ts_dev, REG_CTRL,
> >+			(config | CNTRLREG_TSCSSENB));

The fix is to re-enable the module after disabling 
and clearing the interrupts.

That is what the handler is doing.
> >+	} else if (status & IRQENB_FIFO0THRES) {
> > 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> > 
> > 		if (ts_dev->pen_down && z1 != 0 && z2 != 0) {
> >@@ -317,9 +342,11 @@ 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));
> 
> Shouldn't FIFO1UNDRFLW & OVRRUN be handled by the adc driver? Why do you
> or the unhandled bits as well here?
FIFO1 is only used by TSC. ADC doesn't touch it.
> 
> > 		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> >-		return IRQ_HANDLED;
> >+		status = titsc_readl(ts_dev, REG_IRQSTATUS);
> >+		if (status == false)
> >+			return IRQ_HANDLED;
> 
> And why this? If you something you handled it, if you didn't you return
> NONE. Why does it depend on REG_IRQSTATUS?

These quirks are to handle the situation where both IRQs happen
simultaneously. Which can occur when someone is using the TSC
while continuously sampling using the ADC.

REG_IRQSTATUS has flags for FIFO0 used by ADC as well.

If there are still those IRQs to handle, then IRQ_NONE is returned.
Otherwise, all IRQ flags are clear so IRQ_HANDLED is returned.

Thanks
Zubair

Re: [PATCH 1/4] input: ti_am335x_tsc: correct step mask update after IRQ

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 10:53:50AM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 17:48:16 [+0100]:
> 
> >@@ -316,7 +318,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> > 
> > 	if (irqclr) {
> > 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
> >-		am335x_tsc_se_update(ts_dev->mfd_tscadc);
> >+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> > 		return IRQ_HANDLED;
> > 	}
> > 	return IRQ_NONE;
> 
> titsc_step_config() computes the mask once since it does not change. It
> is then assigned via am335x_tsc_se_set() to ->reg_se_cache() and later
> always udpated via am335x_tsc_se_update(). This should ensure that ADC's
> and TSC's bits are in sync and clear each other out.
> Now you call am335x_tsc_se_set() in every irq which adds the TSC's mask
> to ->reg_se_cache but why? It was never removed.
> 
> Sebastian

The problem is when ADC/TSC are used together.

reg_se_cache would get updated with masks in the se_set function in MFD core.

>From TSC driver, the TSC steps would be set in the reg_se_cache variable.
>From ADC driver, the ADC steps would be set in the reg_se_cache variable.

But the ADC masks weren't being cleared from the reg_se_cache variable
anywhere.

After a while of using ADC/TSC together, the reg_se_cache 
variable would have FFFF always. Resulting in redundant sampling 
and data in ADC FIFO0.

MFD has received fixes to update the reg_se_cache upon 
every call to the se_set functions.

Thus, the step_mask must be set every time to ensure
that it is updated correctly every time.

Hope that clears the confusion.
This TSC/ADC sharing can be pretty confusing.

Thanks
Zubair

[PATCH 2/4] input: ti_am335x_tsc: Increase sequencer delay time

[Zubair Lutfullah]

Before checking PEN UP event, IRQ delays for this amount
of time to let FSM stabilize. Previously, with only the
TSC driver in IRQ mode, this delay was sufficient.

The delay is increased slightly to accomodate if the ADC
is also being used simultaneously in continuous mode.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e165fcb..766bc7e 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -31,7 +31,7 @@
 #include <linux/mfd/ti_am335x_tscadc.h>
 
 #define ADCFSM_STEPID		0x10
-#define SEQ_SETTLE		275
+#define SEQ_SETTLE		350
 #define MAX_12BIT		((1 << 12) - 1)
 
 static const int config_pins[] = {
-- 
1.7.9.5