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

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

[Zubair Lutfullah]

Round 5 updates. Fixed the define order in the header as guided by Lee.

Round 4 updates below.

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_am335x_tsc: Enable shared IRQ for TSC, add overrun and
    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      |   14 ++
 3 files changed, 342 insertions(+), 70 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

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

[PATCH 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_am335x_tsc: Enable shared IRQ for TSC, add overrun and 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@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;
+		}
 
+		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 3/4] input: ti_am335x_tsc: Enable shared IRQ for TSC, add overrun and 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>
Acked-by: Lee Jones <lee.jones@linaro.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;
+		}
 
+		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 |   12 ++
 2 files changed, 303 insertions(+), 62 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..14f75a4 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)
@@ -53,12 +56,15 @@
 #define IRQENB_FIFO0OVRRUN	BIT(3)
 #define IRQENB_FIFO0UNDRFLW	BIT(4)
 #define IRQENB_FIFO1THRES	BIT(5)
+#define IRQENB_FIFO1OVRRUN	BIT(6)
+#define IRQENB_FIFO1UNDRFLW	BIT(7)
 #define IRQENB_PENUP		BIT(9)
 
 /* 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,6 +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
@@ -155,6 +163,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

[Jonathan Cameron]

On 08/13/13 21:05, 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>

Hi,

Whilst the below review is based on what we have here, I tried to apply it
to the latest iio.git togreg branch (which for this driver should be
much the same as staging-next and hence linux-next).  It's not even
close and I can't immediately spot where some of the changes came
from.  Have I missed a precursor patch or is this based on an
older version of this driver?

Note I'd also like a much more detailed description in the patch header.
This is an unusual driver in that it is pushing an entire fifo on a single
trigger into the iio buffers.  Normally it is one trigger / one scan. I have
no particular problem with hybrid software / hardware buffer approach but
it should certainly be clearly documented!

I would also expect an option for the trigger to be supplied from the
chip itself based on fifo watershead interrupts.  Thus the adc could be
operated at full rate without losing data.  As you described in a previous
email this is much more similar to a one shot osciloscope trigger where it
grabs the next set of readings.  Now this is an interesting option, but
it isn't the standard one for IIO.  I'd be interested to see a proposal
for adding this functionality to the core in a more general fashion.
(I actually like this idea a lot!)

A quick thought on interface would be to have

iio:device0/buffer/oneshot_length
(need not actually be the same as the buffer_length - longer and it would
 require userspace to have grabbed the data out in the meantime, shorter and
 we could grab a series before userspace read any of them).
iio:device0/buffer/buffer_enable_oneshot
(capture would then occur until oneshot_length had been acquired - then stop
 - if the trigger that is driving capture wants to be gated by another signal
   that would not be that hard to do either).

Could be neffarouious and abuse one of the poll variants to indicate when
our oneshot sample is done (this was discussed before as we had a watershed
interrupt on a ring buffer at one point).

For now I'd be much happier if this driver conformed to more or less the
standard form with the one exception being that the 'trigger' is based on
the fifo threshold and so it might appear to grab multiple scans of the
enabled channels for each trigger (which is fine). Even if we provide the
functionality sketched out above, it would still be as core functionality, not
in the driver as you have done here.

Sorry I can't really take the patch as is because we would end up with one
driver with a substantially different ABI to all others and that would make
thinks very fiddly for userspace.

Jonathan
.

> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  353 ++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |   12 ++
>  2 files changed, 303 insertions(+), 62 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) {

I'd normally expect this interrupt to drive a trigger that in turn results in
the data being dumped into the software buffers.

> +		/* 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);
Are you actually done?  What happens if another trigger turns up in the
meantime?  I'd expect this to occur only after the results of the trigger
have been handled.
> +	return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
Don't have pointless wrappers like this.
> +	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;
So normally we'd just fill with one sample hence for this case
I'd expect to push out whatever samples are in the fifo right now
then return.  The next trigger would grab the next lot etc.

> +	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..14f75a4 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)
> @@ -53,12 +56,15 @@
>  #define IRQENB_FIFO0OVRRUN	BIT(3)
>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
>  #define IRQENB_FIFO1THRES	BIT(5)
> +#define IRQENB_FIFO1OVRRUN	BIT(6)
> +#define IRQENB_FIFO1UNDRFLW	BIT(7)
>  #define IRQENB_PENUP		BIT(9)
>
>  /* 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,6 +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
> @@ -155,6 +163,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)
>

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

[Jonathan Cameron]

On 08/13/13 21:05, 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>

Hi,

Whilst the below review is based on what we have here, I tried to apply it
to the latest iio.git togreg branch (which for this driver should be
much the same as staging-next and hence linux-next).  It's not even
close and I can't immediately spot where some of the changes came
from.  Have I missed a precursor patch or is this based on an
older version of this driver?

Note I'd also like a much more detailed description in the patch header.
This is an unusual driver in that it is pushing an entire fifo on a single
trigger into the iio buffers.  Normally it is one trigger / one scan. I have
no particular problem with hybrid software / hardware buffer approach but
it should certainly be clearly documented!

I would also expect an option for the trigger to be supplied from the
chip itself based on fifo watershead interrupts.  Thus the adc could be
operated at full rate without losing data.  As you described in a previous
email this is much more similar to a one shot osciloscope trigger where it
grabs the next set of readings.  Now this is an interesting option, but
it isn't the standard one for IIO.  I'd be interested to see a proposal
for adding this functionality to the core in a more general fashion.
(I actually like this idea a lot!)

A quick thought on interface would be to have

iio:device0/buffer/oneshot_length
(need not actually be the same as the buffer_length - longer and it would
 require userspace to have grabbed the data out in the meantime, shorter and
 we could grab a series before userspace read any of them).
iio:device0/buffer/buffer_enable_oneshot
(capture would then occur until oneshot_length had been acquired - then stop
 - if the trigger that is driving capture wants to be gated by another signal
   that would not be that hard to do either).

Could be neffarouious and abuse one of the poll variants to indicate when
our oneshot sample is done (this was discussed before as we had a watershed
interrupt on a ring buffer at one point).

For now I'd be much happier if this driver conformed to more or less the
standard form with the one exception being that the 'trigger' is based on
the fifo threshold and so it might appear to grab multiple scans of the
enabled channels for each trigger (which is fine). Even if we provide the
functionality sketched out above, it would still be as core functionality, not
in the driver as you have done here.

Sorry I can't really take the patch as is because we would end up with one
driver with a substantially different ABI to all others and that would make
thinks very fiddly for userspace.

Jonathan
.

> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  353 ++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |   12 ++
>  2 files changed, 303 insertions(+), 62 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) {

I'd normally expect this interrupt to drive a trigger that in turn results in
the data being dumped into the software buffers.

> +		/* 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);
Are you actually done?  What happens if another trigger turns up in the
meantime?  I'd expect this to occur only after the results of the trigger
have been handled.
> +	return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> +{
Don't have pointless wrappers like this.
> +	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;
So normally we'd just fill with one sample hence for this case
I'd expect to push out whatever samples are in the fifo right now
then return.  The next trigger would grab the next lot etc.

> +	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..14f75a4 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)
> @@ -53,12 +56,15 @@
>  #define IRQENB_FIFO0OVRRUN	BIT(3)
>  #define IRQENB_FIFO0UNDRFLW	BIT(4)
>  #define IRQENB_FIFO1THRES	BIT(5)
> +#define IRQENB_FIFO1OVRRUN	BIT(6)
> +#define IRQENB_FIFO1UNDRFLW	BIT(7)
>  #define IRQENB_PENUP		BIT(9)
>
>  /* 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,6 +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
> @@ -155,6 +163,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)
>

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

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:

>Hi,
Hi Jonathan,

>Whilst the below review is based on what we have here, I tried to apply it
>to the latest iio.git togreg branch (which for this driver should be
>much the same as staging-next and hence linux-next).  It's not even
>close and I can't immediately spot where some of the changes came
>from.  Have I missed a precursor patch or is this based on an
>older version of this driver?

I took the four patches, checkout iio/fixes-togreg and then

|git am -3 iio.mbox
|Applying: input: ti_am335x_tsc: correct step mask update after IRQ
|Applying: input: ti_am335x_tsc: Increase sequencer delay time
|Applying: input: ti_am335x_tsc: Enable shared IRQ for TSC, add overrun and underflow checks
|Applying: iio: ti_am335x_adc: Add continuous sampling and trigger support

It did apply with no trouble here.
Let me look at your comments…

Sebastian

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

[Sebastian Andrzej Siewior]

* Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:

>Hi,
Hi Jonathan,

>Whilst the below review is based on what we have here, I tried to apply it
>to the latest iio.git togreg branch (which for this driver should be
>much the same as staging-next and hence linux-next).  It's not even
>close and I can't immediately spot where some of the changes came
>from.  Have I missed a precursor patch or is this based on an
>older version of this driver?

I took the four patches, checkout iio/fixes-togreg and then

|git am -3 iio.mbox
|Applying: input: ti_am335x_tsc: correct step mask update after IRQ
|Applying: input: ti_am335x_tsc: Increase sequencer delay time
|Applying: input: ti_am335x_tsc: Enable shared IRQ for TSC, add overrun and underflow checks
|Applying: iio: ti_am335x_adc: Add continuous sampling and trigger support

It did apply with no trouble here.
Let me look at your comments…

Sebastian

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

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2013-08-16 12:07:02 [+0200]:

>* Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:
>
>It did apply with no trouble here.

and with

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 93129ec..fdd9810 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -156,6 +156,7 @@ config TI_ADC081C
 config TI_AM335X_ADC
 	tristate "TI's ADC driver"
 	depends on MFD_TI_AM335X_TSCADC
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Texas Instruments ADC
 	  driver which is also a MFD client.


it links :)

Sebastian

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

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2013-08-16 12:07:02 [+0200]:

>* Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:
>
>It did apply with no trouble here.

and with

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 93129ec..fdd9810 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -156,6 +156,7 @@ config TI_ADC081C
 config TI_AM335X_ADC
 	tristate "TI's ADC driver"
 	depends on MFD_TI_AM335X_TSCADC
+	select IIO_TRIGGERED_BUFFER
 	help
 	  Say yes here to build support for Texas Instruments ADC
 	  driver which is also a MFD client.


it links :)

Sebastian

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

[Jonathan Cameron]

On 08/16/13 11:07, Sebastian Andrzej Siewior wrote:
> * Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:
> 
>> Hi,
> Hi Jonathan,
> 
>> Whilst the below review is based on what we have here, I tried to apply it
>> to the latest iio.git togreg branch (which for this driver should be
>> much the same as staging-next and hence linux-next).  It's not even
>> close and I can't immediately spot where some of the changes came
>> from.  Have I missed a precursor patch or is this based on an
>> older version of this driver?
> 
> I took the four patches, checkout iio/fixes-togreg and then
> 
> |git am -3 iio.mbox
> |Applying: input: ti_am335x_tsc: correct step mask update after IRQ
> |Applying: input: ti_am335x_tsc: Increase sequencer delay time
> |Applying: input: ti_am335x_tsc: Enable shared IRQ for TSC, add overrun and underflow checks
> |Applying: iio: ti_am335x_adc: Add continuous sampling and trigger support
> 
> It did apply with no trouble here.
> Let me look at your comments…

Ah, fixes-togreg is for this cycle, whereas new stuff like this needs
to go on the togreg branch.  Hence please rebase on the togreg branch
instead.

That would explain it.

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

[Jonathan Cameron]

On 08/16/13 11:07, Sebastian Andrzej Siewior wrote:
> * Jonathan Cameron | 2013-08-15 12:43:02 [+0100]:
> 
>> Hi,
> Hi Jonathan,
> 
>> Whilst the below review is based on what we have here, I tried to apply it
>> to the latest iio.git togreg branch (which for this driver should be
>> much the same as staging-next and hence linux-next).  It's not even
>> close and I can't immediately spot where some of the changes came
>> from.  Have I missed a precursor patch or is this based on an
>> older version of this driver?
> 
> I took the four patches, checkout iio/fixes-togreg and then
> 
> |git am -3 iio.mbox
> |Applying: input: ti_am335x_tsc: correct step mask update after IRQ
> |Applying: input: ti_am335x_tsc: Increase sequencer delay time
> |Applying: input: ti_am335x_tsc: Enable shared IRQ for TSC, add overrun and underflow checks
> |Applying: iio: ti_am335x_adc: Add continuous sampling and trigger support
> 
> It did apply with no trouble here.
> Let me look at your comments…

Ah, fixes-togreg is for this cycle, whereas new stuff like this needs
to go on the togreg branch.  Hence please rebase on the togreg branch
instead.

That would explain it.

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

[Sebastian Andrzej Siewior]

On 08/16/2013 01:33 PM, Jonathan Cameron wrote:
> Ah, fixes-togreg is for this cycle, whereas new stuff like this needs
> to go on the togreg branch.  Hence please rebase on the togreg branch
> instead.

But he needs "iio: ti_am335x_adc: Fix wrong samples received on 1st
read" from that branch. How should the rebase be done?

> 
> That would explain it.

Sebastian

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

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 12:46:31PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/16/2013 01:33 PM, Jonathan Cameron wrote:
> > Ah, fixes-togreg is for this cycle, whereas new stuff like this needs
> > to go on the togreg branch.  Hence please rebase on the togreg branch
> > instead.
> 
> But he needs "iio: ti_am335x_adc: Fix wrong samples received on 1st
> read" from that branch. How should the rebase be done?
> 
> Sebastian
First line of cover letter had a note on this..

Should I send that particular fix along with the patch series 
so its applied cleanly in the togreg branch. 

And then during the merge window things magically happen to 
fix this situation?

Thanks
Zubair

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

[Zubair Lutfullah :]

On Thu, Aug 15, 2013 at 12:43:02PM +0100, Jonathan Cameron wrote:
> Note I'd also like a much more detailed description in the patch header.
> 
> I would also expect an option for the trigger to be supplied from the
> chip itself based on fifo watershead interrupts.  Thus the adc could be
> operated at full rate without losing data.  As you described in a previous
> email this is much more similar to a one shot osciloscope trigger where it
> grabs the next set of readings.  Now this is an interesting option, but
> it isn't the standard one for IIO.  I'd be interested to see a proposal
> for adding this functionality to the core in a more general fashion.

When I read trigger, this functionality is what comes to my mind.
Not the single scan current implementation in IIO. My background I guess.

Adding this feature to the core feels a bit above my level at the moment.
I'd like to get this driver sorted first.
> 
> For now I'd be much happier if this driver conformed to more or less the
> standard form with the one exception being that the 'trigger' is based on
> the fifo threshold and so it might appear to grab multiple scans of the
> enabled channels for each trigger (which is fine). Even if we provide the
> functionality sketched out above, it would still be as core functionality, not
> in the driver as you have done here.
> 
> Jonathan

Will that affect the way generic_buffer.c will read from the driver?

Rest comments below.

> > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> > +	/* 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) {
> 
> I'd normally expect this interrupt to drive a trigger that in turn results in
> the data being dumped into the software buffers.
> 
The work handler is sort of providing similar functionality..
But, I guess using the trigger ABI is more efficient.
> > +
> > +	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);
> Are you actually done?  What happens if another trigger turns up in the
> meantime?  I'd expect this to occur only after the results of the trigger
> have been handled.

Indeed Done is passed immediately while ADC is still sampling.
Because of the way the trigger was used and structure of the driver.

> > +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> Don't have pointless wrappers like this.

Noted
> > +	return iio_sw_buffer_preenable(indio_dev);
> > +}
> > +
> > +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;
> So normally we'd just fill with one sample hence for this case
> I'd expect to push out whatever samples are in the fifo right now
> then return.  The next trigger would grab the next lot etc.
>
Again. If I understand correctly, 

I restructure the driver so that
enabling the buffer via userspace starts sampling the ADC channels.
Preenable/postenable do all that hard work.

I need to use iio_trigger_register to create an IIO trigger inside the
driver.

The IRQ handler for FIFO Threshold causes a trigger event.

The trigger handler pushes the entire fifo to userspace.

I'd like a small ACK before I get to work.
Just to make sure I got things correctly.

Thanks for the input
Zubair

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

[Zubair Lutfullah :]

On Thu, Aug 15, 2013 at 12:43:02PM +0100, Jonathan Cameron wrote:
> Note I'd also like a much more detailed description in the patch header.
> 
> I would also expect an option for the trigger to be supplied from the
> chip itself based on fifo watershead interrupts.  Thus the adc could be
> operated at full rate without losing data.  As you described in a previous
> email this is much more similar to a one shot osciloscope trigger where it
> grabs the next set of readings.  Now this is an interesting option, but
> it isn't the standard one for IIO.  I'd be interested to see a proposal
> for adding this functionality to the core in a more general fashion.

When I read trigger, this functionality is what comes to my mind.
Not the single scan current implementation in IIO. My background I guess.

Adding this feature to the core feels a bit above my level at the moment.
I'd like to get this driver sorted first.
> 
> For now I'd be much happier if this driver conformed to more or less the
> standard form with the one exception being that the 'trigger' is based on
> the fifo threshold and so it might appear to grab multiple scans of the
> enabled channels for each trigger (which is fine). Even if we provide the
> functionality sketched out above, it would still be as core functionality, not
> in the driver as you have done here.
> 
> Jonathan

Will that affect the way generic_buffer.c will read from the driver?

Rest comments below.

> > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> > +	/* 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) {
> 
> I'd normally expect this interrupt to drive a trigger that in turn results in
> the data being dumped into the software buffers.
> 
The work handler is sort of providing similar functionality..
But, I guess using the trigger ABI is more efficient.
> > +
> > +	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);
> Are you actually done?  What happens if another trigger turns up in the
> meantime?  I'd expect this to occur only after the results of the trigger
> have been handled.

Indeed Done is passed immediately while ADC is still sampling.
Because of the way the trigger was used and structure of the driver.

> > +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> Don't have pointless wrappers like this.

Noted
> > +	return iio_sw_buffer_preenable(indio_dev);
> > +}
> > +
> > +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;
> So normally we'd just fill with one sample hence for this case
> I'd expect to push out whatever samples are in the fifo right now
> then return.  The next trigger would grab the next lot etc.
>
Again. If I understand correctly, 

I restructure the driver so that
enabling the buffer via userspace starts sampling the ADC channels.
Preenable/postenable do all that hard work.

I need to use iio_trigger_register to create an IIO trigger inside the
driver.

The IRQ handler for FIFO Threshold causes a trigger event.

The trigger handler pushes the entire fifo to userspace.

I'd like a small ACK before I get to work.
Just to make sure I got things correctly.

Thanks for the input
Zubair

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

[Zubair Lutfullah :]

On Sat, Aug 17, 2013 at 04:15:33PM +0100, Jonathan Cameron wrote:
> > I restructure the driver so that
> > enabling the buffer via userspace starts sampling the ADC channels.
> > Preenable/postenable do all that hard work.
> > 
> > I need to use iio_trigger_register to create an IIO trigger inside the
> > driver.
> > 
> > The IRQ handler for FIFO Threshold causes a trigger event.
> > 
> > The trigger handler pushes the entire fifo to userspace.
> > 
> > I'd like a small ACK before I get to work.
> > Just to make sure I got things correctly.
> Sounds right.
> 
> As I said above, in this case it would be perfectly acceptable to
> not expose the trigger at all (e.g. drive the buffer filling directly)
> as we can't use it for any other purposes anyway and using any other
> trigger to drive this part doesn't make much sense.
>

Not exposing the trigger would require workarounds for generic_buffer.c
and I'd like to avoid that. Its pretty harmless to have it anyways.

Thanks for the ACK. I'll send another series in a while after all this.

Zubair

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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 @@
…
>+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));

I have no idea how other handle this but shouldn't you somehow inform
*anyone* that you lost some samples due to this overrun?

>+	} 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;

Sebastian

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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 @@
…
>+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));

I have no idea how other handle this but shouldn't you somehow inform
*anyone* that you lost some samples due to this overrun?

>+	} 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;

Sebastian

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

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 02:53:40PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:
> 
> >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 @@
> …
> >+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));
> 
> I have no idea how other handle this but shouldn't you somehow inform
> *anyone* that you lost some samples due to this overrun?
> 
> Sebastian

The ref manual states that TSCADC module doesn't recover 
from overrun interrupts. Thats why the disable enable of
the module here to handle the situation.
And of course, clearing interrupt flags.
The data is lost.

Informing someone about lost samples is important indeed.
But a print statement in this area causes more overruns.

I'm open to some input on how to inform userspace of such 
a situation.

Thanks
Zubair

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
>@@ -141,58 +350,51 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> {
…
>+	if (iio_buffer_enabled(indio_dev))
>+		return -EBUSY;
>+	else {

You can drop else so you lose one ident level.

>+		unsigned long timeout = jiffies + usecs_to_jiffies
>+					(IDLE_TIMEOUT * adc_dev->channels);

What computing this once? ->channels is assigned at probe time.

>+		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;

You should check the condition after the timeout occured once again. It
is possible that the task performing the read has been pushed away by a
higher prio task (or preempted incase this sounds like a bully) and
after it got back on the cpu the timeout occured but the condition is
valid and no reason for -EAGAIN.

>+			}
and the bracket needs to left by one tab.

Sebastian

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
>@@ -141,58 +350,51 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> {
…
>+	if (iio_buffer_enabled(indio_dev))
>+		return -EBUSY;
>+	else {

You can drop else so you lose one ident level.

>+		unsigned long timeout = jiffies + usecs_to_jiffies
>+					(IDLE_TIMEOUT * adc_dev->channels);

What computing this once? ->channels is assigned at probe time.

>+		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;

You should check the condition after the timeout occured once again. It
is possible that the task performing the read has been pushed away by a
higher prio task (or preempted incase this sounds like a bully) and
after it got back on the cpu the timeout occured but the condition is
valid and no reason for -EAGAIN.

>+			}
and the bracket needs to left by one tab.

Sebastian

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

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 03:25:49PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:
> 
> >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
> >@@ -141,58 +350,51 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> > {
> …
> >+	if (iio_buffer_enabled(indio_dev))
> >+		return -EBUSY;
> >+	else {
> 
> You can drop else so you lose one ident level.
> 
Noted.

> >+		unsigned long timeout = jiffies + usecs_to_jiffies
> >+					(IDLE_TIMEOUT * adc_dev->channels);
> 
> What computing this once? ->channels is assigned at probe time.
> 
The timeout depends on number of SW enabled channels.
Hence the calculation. In this read_raw, one channel is to be
read. 
However, all channels are sampled. And the one that the 
userspace requires is pushed to user.

> >+		/* Wait for ADC sequencer to complete sampling */
> >+		while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
> >+			if (time_after(jiffies, timeout))
> >+				return -EAGAIN;
> 
> You should check the condition after the timeout occured once again. It
> is possible that the task performing the read has been pushed away by a
> higher prio task (or preempted incase this sounds like a bully) and
> after it got back on the cpu the timeout occured but the condition is
> valid and no reason for -EAGAIN.
> 
> Sebastian

Interesting catch. I'll look into it.

Thanks for pointing it out.

Zubair

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
…

>+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));
>+	}

This kind of check is bad. The tsc cell may have been created but the
driver not enabled or loaded. Further you should document why you need
to enable / disable the ADC in this places and only if the TSC part is
not active.

Sebastian
--
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 4/4] iio: ti_am335x_adc: Add continuous sampling and trigger support

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
…

>+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));
>+	}

This kind of check is bad. The tsc cell may have been created but the
driver not enabled or loaded. Further you should document why you need
to enable / disable the ADC in this places and only if the TSC part is
not active.

Sebastian

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

[Zubair Lutfullah :]

On Fri, Aug 16, 2013 at 04:59:49PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:
> 
> >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
> …
> 
> >+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));
> >+	}
> 
> This kind of check is bad. The tsc cell may have been created but the
> driver not enabled or loaded. Further you should document why you need
> to enable / disable the ADC in this places and only if the TSC part is
> not active.
> 
> Sebastian

Noted. I'll look into it.

Thanks for pointing it out.

Zubair

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

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
…
>+static irqreturn_t tiadc_irq(int irq, void *private)
>+{
…
>+		wake_up_interruptible(&adc_dev->wq_data_avail);
…
>+}
…
>+static irqreturn_t tiadc_trigger_h(int irq, void *p)
>+{
…
>+	schedule_work(&adc_dev->poll_work);
…
>+}
…
>+static void tiadc_adc_work(struct work_struct *work_s)
>+{
…
>+		wait_event_interruptible(adc_dev->wq_data_avail,
>+					(adc_dev->data_avail == 1));
…
>+}

This is not very nice. The problem is that you might sleep in a
workqueue and so the other jobs will wait until you are done. I'm think
it is better if you create your own worker here instead using the
system's.
I'm looking into DMA support for this so once your code is working it
should be possible to switch to DMA instead reading byte wise from the
fifo.
How did you test the whole thing? Do you have a test program which
selects a few sources and reads them in continuous mode?

Sebastian
--
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 4/4] iio: ti_am335x_adc: Add continuous sampling and trigger support

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:

>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
…
>+static irqreturn_t tiadc_irq(int irq, void *private)
>+{
…
>+		wake_up_interruptible(&adc_dev->wq_data_avail);
…
>+}
…
>+static irqreturn_t tiadc_trigger_h(int irq, void *p)
>+{
…
>+	schedule_work(&adc_dev->poll_work);
…
>+}
…
>+static void tiadc_adc_work(struct work_struct *work_s)
>+{
…
>+		wait_event_interruptible(adc_dev->wq_data_avail,
>+					(adc_dev->data_avail == 1));
…
>+}

This is not very nice. The problem is that you might sleep in a
workqueue and so the other jobs will wait until you are done. I'm think
it is better if you create your own worker here instead using the
system's.
I'm looking into DMA support for this so once your code is working it
should be possible to switch to DMA instead reading byte wise from the
fifo.
How did you test the whole thing? Do you have a test program which
selects a few sources and reads them in continuous mode?

Sebastian

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

[Zubair Lutfullah :]

On Mon, Aug 19, 2013 at 07:12:38PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:
> 
> >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
> …
> >+static irqreturn_t tiadc_irq(int irq, void *private)
> >+{
> …
> >+		wake_up_interruptible(&adc_dev->wq_data_avail);
> …
> >+}
> …
> >+static irqreturn_t tiadc_trigger_h(int irq, void *p)
> >+{
> …
> >+	schedule_work(&adc_dev->poll_work);
> …
> >+}
> …
> >+static void tiadc_adc_work(struct work_struct *work_s)
> >+{
> …
> >+		wait_event_interruptible(adc_dev->wq_data_avail,
> >+					(adc_dev->data_avail == 1));
> …
> >+}
> 
> This is not very nice. The problem is that you might sleep in a
> workqueue and so the other jobs will wait until you are done. I'm think
This will change to a different style.

> I'm looking into DMA support for this so once your code is working it
> should be possible to switch to DMA instead reading byte wise from the
> fifo.
Great.

> How did you test the whole thing? Do you have a test program which
> selects a few sources and reads them in continuous mode?
> 
generic_buffer.c runs for this for sysfs trigger.. 
However. If you follow the other thread. 
This is obsolete and trigger style is changing to a driver trigger.

I'll update when I'm done.

Thanks
ZubairLK

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

[Zubair Lutfullah :]

On Mon, Aug 19, 2013 at 07:12:38PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-13 21:05:03 [+0100]:
> 
> >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
> …
> >+static irqreturn_t tiadc_irq(int irq, void *private)
> >+{
> …
> >+		wake_up_interruptible(&adc_dev->wq_data_avail);
> …
> >+}
> …
> >+static irqreturn_t tiadc_trigger_h(int irq, void *p)
> >+{
> …
> >+	schedule_work(&adc_dev->poll_work);
> …
> >+}
> …
> >+static void tiadc_adc_work(struct work_struct *work_s)
> >+{
> …
> >+		wait_event_interruptible(adc_dev->wq_data_avail,
> >+					(adc_dev->data_avail == 1));
> …
> >+}
> 
> This is not very nice. The problem is that you might sleep in a
> workqueue and so the other jobs will wait until you are done. I'm think
This will change to a different style.

> I'm looking into DMA support for this so once your code is working it
> should be possible to switch to DMA instead reading byte wise from the
> fifo.
Great.

> How did you test the whole thing? Do you have a test program which
> selects a few sources and reads them in continuous mode?
> 
generic_buffer.c runs for this for sysfs trigger.. 
However. If you follow the other thread. 
This is obsolete and trigger style is changing to a driver trigger.

I'll update when I'm done.

Thanks
ZubairLK

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

[Jonathan Cameron]

I'll actually look at these later, but please do put a version number
in your cover letter patch subject.
[PATCH V5 0/4] is pretty much the convention to reduce the chance of anyone
looking at the wrong version.

Thanks,

Jonathan

On 08/13/13 21:04, Zubair Lutfullah wrote:
> Round 5 updates. Fixed the define order in the header as guided by Lee.
> 
> Round 4 updates below.
> 
> 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_am335x_tsc: Enable shared IRQ for TSC, add overrun and
>     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      |   14 ++
>  3 files changed, 342 insertions(+), 70 deletions(-)
> 

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

[Sebastian Andrzej Siewior]

On 08/14/2013 08:57 PM, Jonathan Cameron wrote:
> I'll actually look at these later, but please do put a version number
me, too.

> Jonathan

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 14, 2013 at 07:57:12PM +0100, Jonathan Cameron wrote:
> I'll actually look at these later, but please do put a version number
> in your cover letter patch subject.
> [PATCH V5 0/4] is pretty much the convention to reduce the chance of anyone
> looking at the wrong version.
Noted. 

I actually had round 4/5 at the end of the subjects..

Thanks
Zubair
> 
> Thanks,
> 
> Jonathan
> 
> On 08/13/13 21:04, Zubair Lutfullah wrote:
> > Round 5 updates. Fixed the define order in the header as guided by Lee.
> > 
> > Round 4 updates below.
> > 
> > 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_am335x_tsc: Enable shared IRQ for TSC, add overrun and
> >     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      |   14 ++
> >  3 files changed, 342 insertions(+), 70 deletions(-)
> >