[PATCH V8 0/2] iio: input: ti_am335x_tscadc: Add continuous sampling support to adc

[PATCH V8 0/2] iio: input: ti_am335x_tscadc: Add continuous sampling support to adc

[Zubair Lutfullah]

Round 8 updates
Rebased to togreg branch of IIO..

Round 7 updates
- Fixes to error handling path for trigger register

- Optimization of step enable calculations based
on input from Sebastian

- trigger unregister added to remove section. was missing

- missing entry in Kconfig for triggered buffer support. added

- TSC/ADC IRQ handling was based on wierd ack system. not required. removed.
each irq handler deals with its own stuff *only*

Thanks for all the input

Regards
Zubair Lutfullah Kakakhel

Zubair Lutfullah (2):
  input: ti_am335x_tsc: Enable shared IRQ for TSC
  iio: ti_am335x_adc: Add continuous sampling support

 drivers/iio/adc/Kconfig                   |    1 +
 drivers/iio/adc/ti_am335x_adc.c           |  243 ++++++++++++++++++++++++++---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +-
 include/linux/mfd/ti_am335x_tscadc.h      |   13 ++
 4 files changed, 246 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

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

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

[PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Zubair Lutfullah]

Previously the driver had only one-shot reading functionality.
This patch adds triggered buffer support to the driver.

Continuous sampling starts when buffer is enabled.
And samples are pushed to userpace by the trigger which
triggers automatically at every hardware interrupt
of FIFO1 filling with samples upto threshold value.

Userspace responsibility to stop sampling by writing zero
in the buffer enable file.

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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/Kconfig              |    1 +
 drivers/iio/adc/ti_am335x_adc.c      |  243 +++++++++++++++++++++++++++++++---
 include/linux/mfd/ti_am335x_tscadc.h |   13 ++
 3 files changed, 237 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 09371cb..cfa2039 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -167,6 +167,7 @@ config TI_ADC081C
 config TI_AM335X_ADC
 	tristate "TI's AM335X 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.
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a952538..1626e16 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -24,16 +24,23 @@
 #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/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];
+	int irq;
+	int buffer_en_ch_steps;
+	struct iio_trigger *trig;
+	u32 *data;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,10 +63,16 @@ 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 u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
 {
+	return 1 << adc_dev->channel_step[chan];
+}
+
+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
@@ -72,11 +85,13 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 	 */
 
 	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 +100,167 @@ 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 *indio_dev = private;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int status, config;
+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
+	 */
+	if (status & IRQENB_FIFO1OVRRUN) {
+		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
+		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));
+		return IRQ_HANDLED;
+	} else if (status & IRQENB_FIFO1THRES) {
+		/* Trigger to push FIFO data to iio buffer */
+		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
+		iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
+		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);
+	int i, k, fifo1count, read;
+	u32 *data = adc_dev->data;
+
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (k = 0; k < fifo1count; k = k + i) {
+		for (i = 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);
+	}
 
+	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
+	tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int tiadc_buffer_preenable(struct iio_dev *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 starting sampling */
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (i = 0; i < fifo1count; i++)
+		read = tiadc_readl(adc_dev, REG_FIFO1);
+
+	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;
+	u8 bit;
+
+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	if (adc_dev->data == NULL)
+		return -ENOMEM;
+
+	tiadc_step_config(indio_dev);
+	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
+	adc_dev->buffer_en_ch_steps = enb;
+
+	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
+
+	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN);
+
+	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;
+
+	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);
+
+	/* 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);
+
+	tiadc_step_config(indio_dev);
+	kfree(adc_dev->data);
+	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 const struct iio_trigger_ops tiadc_trigger_ops = {
+	.owner = THIS_MODULE,
+};
+
+static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct iio_trigger *trig = adc_dev->trig;
+	int ret;
+
+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return -ENOMEM;
+
+	trig->dev.parent = indio_dev->dev.parent;
+	trig->ops = &tiadc_trigger_ops;
+	iio_trigger_set_drvdata(trig, indio_dev);
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		goto err_free_trigger;
+
+	return 0;
+
+err_free_trigger:
+	iio_trigger_free(adc_dev->trig);
+
+	return ret;
 }
 
 static const char * const chan_name_ain[] = {
@@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 		chan->channel = adc_dev->channel_line[i];
 		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 		chan->datasheet_name = chan_name_ain[chan->channel];
+		chan->scan_index = i;
 		chan->scan_type.sign = 'u';
 		chan->scan_type.realbits = 12;
 		chan->scan_type.storagebits = 32;
@@ -142,11 +318,14 @@ 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);
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
 	step_en = get_adc_step_mask(adc_dev);
 	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
 
@@ -168,15 +347,6 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	 * Hence we need to flush out this data.
 	 */
 
-	for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
-		if (chan->channel == adc_dev->channel_line[i]) {
-			step = adc_dev->channel_step[i];
-			break;
-		}
-	}
-	if (WARN_ON_ONCE(step == UINT_MAX))
-		return -EINVAL;
-
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (i = 0; i < fifo1count; i++) {
 		read = tiadc_readl(adc_dev, REG_FIFO1);
@@ -231,26 +401,49 @@ 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)
 		return err;
 
-	err = iio_device_register(indio_dev);
+	err = tiadc_iio_allocate_trigger(indio_dev);
 	if (err)
 		goto err_free_channels;
 
+	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
+		indio_dev->name, indio_dev);
+	if (err)
+		goto err_unregister_trigger;
+
+	err = iio_triggered_buffer_setup(indio_dev, NULL,
+			&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_unregister_trigger:
+	iio_trigger_unregister(adc_dev->trig);
+	iio_trigger_free(adc_dev->trig);
 err_free_channels:
 	tiadc_channels_remove(indio_dev);
 	return err;
@@ -262,7 +455,11 @@ static int tiadc_remove(struct platform_device *pdev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	u32 step_en;
 
+	iio_trigger_unregister(adc_dev->trig);
+	iio_trigger_free(adc_dev->trig);
+	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);
@@ -298,10 +495,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 db1791b..a372ebf 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,17 +46,25 @@
 /* Step Enable */
 #define STEPENB_MASK		(0x1FFFF << 0)
 #define STEPENB(val)		((val) << 0)
+#define ENB(val)			(1 << (val))
+#define STPENB_STEPENB		STEPENB(0x1FFFF)
+#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
 
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
 #define IRQENB_FIFO0THRES	BIT(2)
+#define IRQENB_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)
@@ -124,6 +132,7 @@
 #define	MAX_CLK_DIV		7
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
+#define FIFO1_THRESHOLD		19
 
 /*
 * ADC runs at 3MHz, and it takes
@@ -153,6 +162,10 @@ struct ti_tscadc_dev {
 
 	/* adc device */
 	struct adc_device *adc;
+
+	/* Context save */
+	unsigned int irqstat;
+	unsigned int ctrl;
 };
 
 static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
-- 
1.7.9.5

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Jonathan Cameron]

On 09/01/13 12:17, Zubair Lutfullah wrote:
> Enable shared IRQ to allow ADC to share IRQ line from
> parent MFD core. Only FIFO0 IRQs are for TSC and handled
> on the TSC side.
> 
> Step mask would be updated from cached variable only previously.
> In rare cases when both TSC and ADC are used, the cached
> variable gets mixed up.
> The step mask is written with the required mask every time.
> 
> Rachna Patil (TI) laid ground work for shared IRQ.
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Whilst I would have prefered an mfd under these drivers and elegant handling
of the interrupts, I guess if this works it is at least not terribly invasive.

However, this does need an Ack from Dmitry before I can take it (or for
Dmitry to take it himself?)

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index e1c5300..24e625c 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,
> @@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	unsigned int fsm;
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
> +	 */
>  	if (status & IRQENB_FIFO0THRES) {
>  
>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> @@ -316,7 +322,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;
> @@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
>  	}
>  
>  	err = request_irq(ts_dev->irq, titsc_irq,
> -			  0, pdev->dev.driver->name, ts_dev);
> +			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to allocate irq.\n");
>  		goto err_free_mem;
> 

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Jonathan Cameron]

On 09/01/13 12:17, Zubair Lutfullah wrote:
> Enable shared IRQ to allow ADC to share IRQ line from
> parent MFD core. Only FIFO0 IRQs are for TSC and handled
> on the TSC side.
> 
> Step mask would be updated from cached variable only previously.
> In rare cases when both TSC and ADC are used, the cached
> variable gets mixed up.
> The step mask is written with the required mask every time.
> 
> Rachna Patil (TI) laid ground work for shared IRQ.
> 
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Whilst I would have prefered an mfd under these drivers and elegant handling
of the interrupts, I guess if this works it is at least not terribly invasive.

However, this does need an Ack from Dmitry before I can take it (or for
Dmitry to take it himself?)

> ---
>  drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index e1c5300..24e625c 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,
> @@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
>  	unsigned int fsm;
>  
>  	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
> +	 */
>  	if (status & IRQENB_FIFO0THRES) {
>  
>  		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
> @@ -316,7 +322,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;
> @@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
>  	}
>  
>  	err = request_irq(ts_dev->irq, titsc_irq,
> -			  0, pdev->dev.driver->name, ts_dev);
> +			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to allocate irq.\n");
>  		goto err_free_mem;
> 

Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Jonathan Cameron]

On 09/01/13 12:17, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> And samples are pushed to userpace by the trigger which
> triggers automatically at every hardware interrupt
> of FIFO1 filling with samples upto threshold value.
>
> Userspace responsibility to stop sampling by writing zero
> in the buffer enable file.
>
> 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>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Hi

I'm afraid I'm still unconvinced that we can use the trigger infrastructure
as done here.  This trigger from a fifo threshold does not agree with how it
happens in any other driver. Unfortunately as the first hardware buffered
device we have had in recent times (there is the sca3000 in staging, but that
does not have a front end kfifo) you get to break some new ground.

I've suggested below how I would handle this.  In short, drop the trigger
and drive the buffers directly, marking the mode as INDIO_BUFFER_HARDWARE to
avoid the core code complaining at the lack of trigger.  It will end up simpler
and not expose a trigger to userspace that is doing 'interesting' things.
This is the only way I can envision a hardware buffer equiped device like this
working.  It's certainly what I've been intending to do with the sca3000
driver when I finally get around to bringing it up to date. I don't think
we can take the current approach because it would give us horribly inconsistent
userspace interfaces we can't maintain in the long run.

Few other little bits and bobs inline to do with unused variables and
a few places where a small comment or two might make the operation easier
to follow when we have all forgotten how the hardware works ;)

Jonathan

> ---
>  drivers/iio/adc/Kconfig              |    1 +
>  drivers/iio/adc/ti_am335x_adc.c      |  243 +++++++++++++++++++++++++++++++---
>  include/linux/mfd/ti_am335x_tscadc.h |   13 ++
>  3 files changed, 237 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 09371cb..cfa2039 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -167,6 +167,7 @@ config TI_ADC081C
>  config TI_AM335X_ADC
>  	tristate "TI's AM335X 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.
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..1626e16 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -24,16 +24,23 @@
>  #include <linux/iio/iio.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
Ouch, how did that slip in here.  Dropping this should have been
in a separate patch as it doesn't really have anything to do with
the rest of this series.

> -#include <linux/iio/machine.h>
>  #include <linux/iio/driver.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];
> +	int irq;
> +	int buffer_en_ch_steps;
> +	struct iio_trigger *trig;
> +	u32 *data;
>  };
>
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,10 +63,16 @@ 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 u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>  {
> +	return 1 << adc_dev->channel_step[chan];
> +}
> +
> +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;
Whilst I agree that pulling chan up here is slightly cleaner, technically that
is a bit of unconnected cleanup and should have been in a precursor patch.
(though don't bother doing so now!)
>
>  	/*
>  	 * There are 16 configurable steps and 8 analog input
> @@ -72,11 +85,13 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	 */
>
>  	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 +100,167 @@ 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 *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int status, config;
> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> +	 */
> +	if (status & IRQENB_FIFO1OVRRUN) {
> +		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> +		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));
> +		return IRQ_HANDLED;
> +	} else if (status & IRQENB_FIFO1THRES) {
> +		/* Trigger to push FIFO data to iio buffer */
> +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);

So this is triggering at the threshold.  Thus it isn't acting as a conventional
trigger in the IIO sense at all.  It is of no real use to anything outside
this driver, so at the very least add the two callbacks to prevent this
driver using any other trigger and the trigger being used by any other device.
However, I'm really not happy with this solution.

Take a look at running with no trigger at all and calling the buffer
filling more directly.  The issue is that if we have a trigger now it will
become part of the userspace interface and become a pain to drop later.

It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
or iio_triggered_buffer_predisable.  Then make this a threaded interrupt and
put your tiadc_trigger_h as the thread.  Finally just return IRQ_WAKE_THREAD
in this case.  You'll have to pull a few bits out of
industrialio-triggered-buffer.c to create the kfifo etc, but no more than
a few lines given the pollfunc won't exist.

Actually, might be cleaner to move the flush case into the thread as well
and just have a single check in this top half of the handler. That bit
is up to you given it is really a question of how long it take and hence
if it is acceptable in interrupt context.

> +		iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> +		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);
> +	int i, k, fifo1count, read;
> +	u32 *data = adc_dev->data;
> +

Hmm.. This results in all data in the fifo being read from a single
trigger.  I think I would still be happier if in the case of hardware
buffers like this we didn't use a visible trigger at all.  Such
a trigger has a somewhat fuzzy meaning to say the least.

> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (k = 0; k < fifo1count; k = k + i) {
As mentioned below, why not use 16 bit storage in the kfifo?
> +		for (i = 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);
> +	}
>
> +	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> +	tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *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));
> +

Why would there be data in the fifo?  Seems to me that flushing
after disabling it and before enabling it should not both be
necessary? Is this just a case of playing safe?

> +	/* Flush FIFO before starting sampling */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	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;
> +	u8 bit;
> +
> +	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	if (adc_dev->data == NULL)
> +		return -ENOMEM;
> +
> +	tiadc_step_config(indio_dev);
> +	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> +		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> +	adc_dev->buffer_en_ch_steps = enb;
> +
> +	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> +	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> +	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN);
> +
> +	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;
> +
> +	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);
> +
> +	/* 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);
> +
> +	tiadc_step_config(indio_dev);
> +	kfree(adc_dev->data);
blank line here please.

> +	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 const struct iio_trigger_ops tiadc_trigger_ops = {
> +	.owner = THIS_MODULE,
> +};
> +

If you want a utility function for allocation, convention would
say have one for freeing as well so that the code is obviously
balanced and easy to check.  Also this registers the buffer as
well so the name should probably reflect that.

> +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct iio_trigger *trig = adc_dev->trig;
> +	int ret;
> +
devm_iio_trigger_alloc is now available and will simplify things a little.

> +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> +	if (trig == NULL)
> +		return -ENOMEM;
> +
> +	trig->dev.parent = indio_dev->dev.parent;
> +	trig->ops = &tiadc_trigger_ops;
> +	iio_trigger_set_drvdata(trig, indio_dev);
> +
> +	ret = iio_trigger_register(trig);
> +	if (ret)
> +		goto err_free_trigger;
> +
> +	return 0;
> +
> +err_free_trigger:
> +	iio_trigger_free(adc_dev->trig);
> +
> +	return ret;
>  }
>
>  static const char * const chan_name_ain[] = {
> @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  		chan->channel = adc_dev->channel_line[i];
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan->datasheet_name = chan_name_ain[chan->channel];
> +		chan->scan_index = i;
>  		chan->scan_type.sign = 'u';
>  		chan->scan_type.realbits = 12;
>  		chan->scan_type.storagebits = 32;
I never noticed this before but putting 12 bit values in 32 bit storage
does seem rather wasteful.  Far as I can see above there is no real
reason for doing so.  If there is, then please add a comment justifying this
choice!

> @@ -142,11 +318,14 @@ 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);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
>  	step_en = get_adc_step_mask(adc_dev);
>  	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>
> @@ -168,15 +347,6 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	 * Hence we need to flush out this data.
>  	 */
Is the comment above still valid?  I'd guess it referred to this code
which has now gone.
>
> -	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);
> @@ -231,26 +401,49 @@ static int tiadc_probe(struct platform_device *pdev)
>  		channels++;
>  	}
>  	adc_dev->channels = channels;

Use the devm interfaces and there is no need to keep a copy
of the irq number about.

> +	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)
>  		return err;
>
> -	err = iio_device_register(indio_dev);
> +	err = tiadc_iio_allocate_trigger(indio_dev);
>  	if (err)
>  		goto err_free_channels;
>
devm_request_irq will simplify error handling a little.

> +	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> +		indio_dev->name, indio_dev);
> +	if (err)
> +		goto err_unregister_trigger;
> +
> +	err = iio_triggered_buffer_setup(indio_dev, NULL,
> +			&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_unregister_trigger:
> +	iio_trigger_unregister(adc_dev->trig);
> +	iio_trigger_free(adc_dev->trig);
>  err_free_channels:
>  	tiadc_channels_remove(indio_dev);
>  	return err;
> @@ -262,7 +455,11 @@ static int tiadc_remove(struct platform_device *pdev)
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	u32 step_en;
>
> +	iio_trigger_unregister(adc_dev->trig);
> +	iio_trigger_free(adc_dev->trig);
There is a devm trigger allocation function now. Using that
would make things a little cleaner.

> +	free_irq(adc_dev->irq, indio_dev);
Best to use the devm interfaces for irq handling unless
there is a good reason to not do so (cleaner code ;)

>  	iio_device_unregister(indio_dev);

We would normally expect this function to run in exact
reverse order of the probe.  That is not the case here
so please reorder things so it is.
> +	iio_buffer_unregister(indio_dev);
>  	tiadc_channels_remove(indio_dev);
>
>  	step_en = get_adc_step_mask(adc_dev);
> @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
>
>  	/* Make sure ADC is powered up */
This comment doesn't seem to me to be true any more.
>  	restore = tiadc_readl(adc_dev, REG_CTRL);
> -	restore &= ~(CNTRLREG_POWERDOWN);
Some coments here would be good to explain the sequence that
is going on.

I'm guessing a bit, but looks like you turn off the touch screen
but leave everything else alone, then set the threshold then
reenable the touch screen before powering up the screen?

> +	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 db1791b..a372ebf 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,17 +46,25 @@
>  /* Step Enable */
>  #define STEPENB_MASK		(0x1FFFF << 0)
>  #define STEPENB(val)		((val) << 0)
> +#define ENB(val)			(1 << (val))
> +#define STPENB_STEPENB		STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
>
>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
>  #define IRQENB_FIFO0THRES	BIT(2)
> +#define IRQENB_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)
> @@ -124,6 +132,7 @@
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
> +#define FIFO1_THRESHOLD		19
>
>  /*
>  * ADC runs at 3MHz, and it takes
> @@ -153,6 +162,10 @@ struct ti_tscadc_dev {
>
>  	/* adc device */
>  	struct adc_device *adc;
> +
> +	/* Context save */
> +	unsigned int irqstat;
Where are these used?
> +	unsigned int ctrl;
>  };
>
>  static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>

Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Jonathan Cameron]

On 09/01/13 12:17, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds triggered buffer support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> And samples are pushed to userpace by the trigger which
> triggers automatically at every hardware interrupt
> of FIFO1 filling with samples upto threshold value.
>
> Userspace responsibility to stop sampling by writing zero
> in the buffer enable file.
>
> 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>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

Hi

I'm afraid I'm still unconvinced that we can use the trigger infrastructure
as done here.  This trigger from a fifo threshold does not agree with how it
happens in any other driver. Unfortunately as the first hardware buffered
device we have had in recent times (there is the sca3000 in staging, but that
does not have a front end kfifo) you get to break some new ground.

I've suggested below how I would handle this.  In short, drop the trigger
and drive the buffers directly, marking the mode as INDIO_BUFFER_HARDWARE to
avoid the core code complaining at the lack of trigger.  It will end up simpler
and not expose a trigger to userspace that is doing 'interesting' things.
This is the only way I can envision a hardware buffer equiped device like this
working.  It's certainly what I've been intending to do with the sca3000
driver when I finally get around to bringing it up to date. I don't think
we can take the current approach because it would give us horribly inconsistent
userspace interfaces we can't maintain in the long run.

Few other little bits and bobs inline to do with unused variables and
a few places where a small comment or two might make the operation easier
to follow when we have all forgotten how the hardware works ;)

Jonathan

> ---
>  drivers/iio/adc/Kconfig              |    1 +
>  drivers/iio/adc/ti_am335x_adc.c      |  243 +++++++++++++++++++++++++++++++---
>  include/linux/mfd/ti_am335x_tscadc.h |   13 ++
>  3 files changed, 237 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 09371cb..cfa2039 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -167,6 +167,7 @@ config TI_ADC081C
>  config TI_AM335X_ADC
>  	tristate "TI's AM335X 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.
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..1626e16 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -24,16 +24,23 @@
>  #include <linux/iio/iio.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
Ouch, how did that slip in here.  Dropping this should have been
in a separate patch as it doesn't really have anything to do with
the rest of this series.

> -#include <linux/iio/machine.h>
>  #include <linux/iio/driver.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];
> +	int irq;
> +	int buffer_en_ch_steps;
> +	struct iio_trigger *trig;
> +	u32 *data;
>  };
>
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,10 +63,16 @@ 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 u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>  {
> +	return 1 << adc_dev->channel_step[chan];
> +}
> +
> +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;
Whilst I agree that pulling chan up here is slightly cleaner, technically that
is a bit of unconnected cleanup and should have been in a precursor patch.
(though don't bother doing so now!)
>
>  	/*
>  	 * There are 16 configurable steps and 8 analog input
> @@ -72,11 +85,13 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	 */
>
>  	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 +100,167 @@ 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 *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int status, config;
> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> +	 */
> +	if (status & IRQENB_FIFO1OVRRUN) {
> +		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> +		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));
> +		return IRQ_HANDLED;
> +	} else if (status & IRQENB_FIFO1THRES) {
> +		/* Trigger to push FIFO data to iio buffer */
> +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);

So this is triggering at the threshold.  Thus it isn't acting as a conventional
trigger in the IIO sense at all.  It is of no real use to anything outside
this driver, so at the very least add the two callbacks to prevent this
driver using any other trigger and the trigger being used by any other device.
However, I'm really not happy with this solution.

Take a look at running with no trigger at all and calling the buffer
filling more directly.  The issue is that if we have a trigger now it will
become part of the userspace interface and become a pain to drop later.

It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
or iio_triggered_buffer_predisable.  Then make this a threaded interrupt and
put your tiadc_trigger_h as the thread.  Finally just return IRQ_WAKE_THREAD
in this case.  You'll have to pull a few bits out of
industrialio-triggered-buffer.c to create the kfifo etc, but no more than
a few lines given the pollfunc won't exist.

Actually, might be cleaner to move the flush case into the thread as well
and just have a single check in this top half of the handler. That bit
is up to you given it is really a question of how long it take and hence
if it is acceptable in interrupt context.

> +		iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> +		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);
> +	int i, k, fifo1count, read;
> +	u32 *data = adc_dev->data;
> +

Hmm.. This results in all data in the fifo being read from a single
trigger.  I think I would still be happier if in the case of hardware
buffers like this we didn't use a visible trigger at all.  Such
a trigger has a somewhat fuzzy meaning to say the least.

> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (k = 0; k < fifo1count; k = k + i) {
As mentioned below, why not use 16 bit storage in the kfifo?
> +		for (i = 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);
> +	}
>
> +	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> +	tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int tiadc_buffer_preenable(struct iio_dev *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));
> +

Why would there be data in the fifo?  Seems to me that flushing
after disabling it and before enabling it should not both be
necessary? Is this just a case of playing safe?

> +	/* Flush FIFO before starting sampling */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	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;
> +	u8 bit;
> +
> +	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	if (adc_dev->data == NULL)
> +		return -ENOMEM;
> +
> +	tiadc_step_config(indio_dev);
> +	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> +		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> +	adc_dev->buffer_en_ch_steps = enb;
> +
> +	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> +	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> +	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN);
> +
> +	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;
> +
> +	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);
> +
> +	/* 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);
> +
> +	tiadc_step_config(indio_dev);
> +	kfree(adc_dev->data);
blank line here please.

> +	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 const struct iio_trigger_ops tiadc_trigger_ops = {
> +	.owner = THIS_MODULE,
> +};
> +

If you want a utility function for allocation, convention would
say have one for freeing as well so that the code is obviously
balanced and easy to check.  Also this registers the buffer as
well so the name should probably reflect that.

> +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct iio_trigger *trig = adc_dev->trig;
> +	int ret;
> +
devm_iio_trigger_alloc is now available and will simplify things a little.

> +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> +	if (trig == NULL)
> +		return -ENOMEM;
> +
> +	trig->dev.parent = indio_dev->dev.parent;
> +	trig->ops = &tiadc_trigger_ops;
> +	iio_trigger_set_drvdata(trig, indio_dev);
> +
> +	ret = iio_trigger_register(trig);
> +	if (ret)
> +		goto err_free_trigger;
> +
> +	return 0;
> +
> +err_free_trigger:
> +	iio_trigger_free(adc_dev->trig);
> +
> +	return ret;
>  }
>
>  static const char * const chan_name_ain[] = {
> @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  		chan->channel = adc_dev->channel_line[i];
>  		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan->datasheet_name = chan_name_ain[chan->channel];
> +		chan->scan_index = i;
>  		chan->scan_type.sign = 'u';
>  		chan->scan_type.realbits = 12;
>  		chan->scan_type.storagebits = 32;
I never noticed this before but putting 12 bit values in 32 bit storage
does seem rather wasteful.  Far as I can see above there is no real
reason for doing so.  If there is, then please add a comment justifying this
choice!

> @@ -142,11 +318,14 @@ 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);
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
>  	step_en = get_adc_step_mask(adc_dev);
>  	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>
> @@ -168,15 +347,6 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	 * Hence we need to flush out this data.
>  	 */
Is the comment above still valid?  I'd guess it referred to this code
which has now gone.
>
> -	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);
> @@ -231,26 +401,49 @@ static int tiadc_probe(struct platform_device *pdev)
>  		channels++;
>  	}
>  	adc_dev->channels = channels;

Use the devm interfaces and there is no need to keep a copy
of the irq number about.

> +	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)
>  		return err;
>
> -	err = iio_device_register(indio_dev);
> +	err = tiadc_iio_allocate_trigger(indio_dev);
>  	if (err)
>  		goto err_free_channels;
>
devm_request_irq will simplify error handling a little.

> +	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
> +		indio_dev->name, indio_dev);
> +	if (err)
> +		goto err_unregister_trigger;
> +
> +	err = iio_triggered_buffer_setup(indio_dev, NULL,
> +			&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_unregister_trigger:
> +	iio_trigger_unregister(adc_dev->trig);
> +	iio_trigger_free(adc_dev->trig);
>  err_free_channels:
>  	tiadc_channels_remove(indio_dev);
>  	return err;
> @@ -262,7 +455,11 @@ static int tiadc_remove(struct platform_device *pdev)
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	u32 step_en;
>
> +	iio_trigger_unregister(adc_dev->trig);
> +	iio_trigger_free(adc_dev->trig);
There is a devm trigger allocation function now. Using that
would make things a little cleaner.

> +	free_irq(adc_dev->irq, indio_dev);
Best to use the devm interfaces for irq handling unless
there is a good reason to not do so (cleaner code ;)

>  	iio_device_unregister(indio_dev);

We would normally expect this function to run in exact
reverse order of the probe.  That is not the case here
so please reorder things so it is.
> +	iio_buffer_unregister(indio_dev);
>  	tiadc_channels_remove(indio_dev);
>
>  	step_en = get_adc_step_mask(adc_dev);
> @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
>
>  	/* Make sure ADC is powered up */
This comment doesn't seem to me to be true any more.
>  	restore = tiadc_readl(adc_dev, REG_CTRL);
> -	restore &= ~(CNTRLREG_POWERDOWN);
Some coments here would be good to explain the sequence that
is going on.

I'm guessing a bit, but looks like you turn off the touch screen
but leave everything else alone, then set the threshold then
reenable the touch screen before powering up the screen?

> +	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 db1791b..a372ebf 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -46,17 +46,25 @@
>  /* Step Enable */
>  #define STEPENB_MASK		(0x1FFFF << 0)
>  #define STEPENB(val)		((val) << 0)
> +#define ENB(val)			(1 << (val))
> +#define STPENB_STEPENB		STEPENB(0x1FFFF)
> +#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
>
>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
>  #define IRQENB_FIFO0THRES	BIT(2)
> +#define IRQENB_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)
> @@ -124,6 +132,7 @@
>  #define	MAX_CLK_DIV		7
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
> +#define FIFO1_THRESHOLD		19
>
>  /*
>  * ADC runs at 3MHz, and it takes
> @@ -153,6 +162,10 @@ struct ti_tscadc_dev {
>
>  	/* adc device */
>  	struct adc_device *adc;
> +
> +	/* Context save */
> +	unsigned int irqstat;
Where are these used?
> +	unsigned int ctrl;
>  };
>
>  static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Dmitry Torokhov]

On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:
> > Enable shared IRQ to allow ADC to share IRQ line from
> > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > on the TSC side.
> > 
> > Step mask would be updated from cached variable only previously.
> > In rare cases when both TSC and ADC are used, the cached
> > variable gets mixed up.
> > The step mask is written with the required mask every time.
> > 
> > Rachna Patil (TI) laid ground work for shared IRQ.
> > 
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Whilst I would have prefered an mfd under these drivers and elegant handling
> of the interrupts, I guess if this works it is at least not terribly invasive.
> 
> However, this does need an Ack from Dmitry before I can take it (or for
> Dmitry to take it himself?)

I completely agree with Jonathan, more elegant handling would be nice
but current one will do for now.

Since most of the work on the driver is going through IIO tree I think
it would make sense for this patch to go through it as well.

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

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Dmitry Torokhov]

On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:
> > Enable shared IRQ to allow ADC to share IRQ line from
> > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > on the TSC side.
> > 
> > Step mask would be updated from cached variable only previously.
> > In rare cases when both TSC and ADC are used, the cached
> > variable gets mixed up.
> > The step mask is written with the required mask every time.
> > 
> > Rachna Patil (TI) laid ground work for shared IRQ.
> > 
> > Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Whilst I would have prefered an mfd under these drivers and elegant handling
> of the interrupts, I guess if this works it is at least not terribly invasive.
> 
> However, this does need an Ack from Dmitry before I can take it (or for
> Dmitry to take it himself?)

I completely agree with Jonathan, more elegant handling would be nice
but current one will do for now.

Since most of the work on the driver is going through IIO tree I think
it would make sense for this patch to go through it as well.

Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Zubair Lutfullah :]

On Sun, Sep 08, 2013 at 02:42:51PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:

Thanks for the review. Fitting this driver in the usual trigger ABI
is a bad workaround indeed.

I am still a bit unclear regarding the implementation.
Questions and replies inline.

I hope I find the time to pursue the approach you suggest.

Thanks
ZubairLK


> >  #include <linux/of.h>
> Ouch, how did that slip in here.  Dropping this should have been
> in a separate patch as it doesn't really have anything to do with
> the rest of this series.
> 
> > -#include <linux/iio/machine.h>

Squashed it in.. I'll do a cleanup patch separately.

> >  #include <linux/iio/driver.h>
> > +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;
> Whilst I agree that pulling chan up here is slightly cleaner, technically that
> is a bit of unconnected cleanup and should have been in a precursor patch.
> (though don't bother doing so now!)

Again. Squashed in..

> > +
> > +static irqreturn_t tiadc_irq(int irq, void *private)
> > +{
...
> > +		return IRQ_HANDLED;
> > +	} else if (status & IRQENB_FIFO1THRES) {
> > +		/* Trigger to push FIFO data to iio buffer */
> > +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
> 
> So this is triggering at the threshold.  Thus it isn't acting as a conventional
> trigger in the IIO sense at all.  It is of no real use to anything outside
> this driver, so at the very least add the two callbacks to prevent this
> driver using any other trigger and the trigger being used by any other device.
> However, I'm really not happy with this solution.
> 
> Take a look at running with no trigger at all and calling the buffer
> filling more directly.  The issue is that if we have a trigger now it will
> become part of the userspace interface and become a pain to drop later.
> 
> It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
> the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
> or iio_triggered_buffer_predisable.  Then make this a threaded interrupt and
> put your tiadc_trigger_h as the thread.  Finally just return IRQ_WAKE_THREAD
> in this case.  You'll have to pull a few bits out of
> industrialio-triggered-buffer.c to create the kfifo etc, but no more than
> a few lines given the pollfunc won't exist.

If I understand correctly.
buffer setup ops for postenable and predisable have 

return iio_triggered_buffer_*(indio_dev)

calls at the end. I return 0 instead.

Insead of iio_triggered_buffer setup. I only use iio_kfifo_alloc
(what size fifo is made? :s )
And do 

indio_dev->modes |= INDIO_BUFFER_HARDWARE;

Then I use request_threaded_irq(*). The hardware IRQ line for the fifos
So the HW irq handler runs in interrupt context which wakes the threaded handler
which pushes samples to the iio buffer.

Shared IRQ line. ADC uses request_threaded_irq(). TSC uses request_irq()
ADC returns IRQ_WAKE_THREAD. But the TSC side returns NONE/HANDLED.
Does this change to a threaded irq affect the TSC side of things?
I just received an ack by Dmitry for that patch..

> 
> Actually, might be cleaner to move the flush case into the thread as well
> and just have a single check in this top half of the handler. That bit
> is up to you given it is really a question of how long it take and hence
> if it is acceptable in interrupt context.

Sorry. I don't understand this paragraph.. Could you please elaborate?
The flushing fifo before/after enabling/disabling the buffer part? 
I explained the reason behind the flushes

> 
> > +		iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> > +		return IRQ_HANDLED;
> > +	} else
> > +		return IRQ_NONE;
> > +
> > +}
> > +
> > +static irqreturn_t tiadc_trigger_h(int irq, void *p)
> > +{
...
> 
> > +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> > +	for (k = 0; k < fifo1count; k = k + i) {
> As mentioned below, why not use 16 bit storage in the kfifo?

32 bit storage is because the FIFO is 32 bit in hardware.
It returns 12 bits data. some reserve bits. And 4 bit channel id.
The channel id is used to separate which channel data which.
Need the full 32 bits..

I've added a comment. Not here. but when the storage_bits = 32 is 
set.

...
> > +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
...
> Why would there be data in the fifo?  Seems to me that flushing
> after disabling it and before enabling it should not both be
> necessary? Is this just a case of playing safe?
> 
> > +	/* Flush FIFO before starting sampling */
> > +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> > +	for (i = 0; i < fifo1count; i++)
> > +		read = tiadc_readl(adc_dev, REG_FIFO1);

Pre-flush was needed because TSC events would cause some
redundant ADC samples in the FIFO. Fixes have been sent to input.
Shouldn't happen now.

Post-flush is playing safe. There 'might' be a chance that 
another sample comes in the time it takes for fifocount to be read
And that number of samples read from fifo. and then adc disabled.

> > +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > +
> > +	tiadc_step_config(indio_dev);
> > +	kfree(adc_dev->data);
> blank line here please.

ok

> > +	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 const struct iio_trigger_ops tiadc_trigger_ops = {
> > +	.owner = THIS_MODULE,
> > +};
> > +
> 
> If you want a utility function for allocation, convention would
> say have one for freeing as well so that the code is obviously
> balanced and easy to check.  Also this registers the buffer as
> well so the name should probably reflect that.
> 

Noted.

> > +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> > +{
> > +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > +	struct iio_trigger *trig = adc_dev->trig;
> > +	int ret;
> > +
> devm_iio_trigger_alloc is now available and will simplify things a little.
> 

Trigger related stuff is redundant anyways as moving away from trigger..

> > +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);

> > @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> >  		chan->scan_type.realbits = 12;
> >  		chan->scan_type.storagebits = 32;
> I never noticed this before but putting 12 bit values in 32 bit storage
> does seem rather wasteful.  Far as I can see above there is no real
> reason for doing so.  If there is, then please add a comment justifying this
> choice!
>

Answered earlier. I added a comment here explaining. 

> > @@ -142,11 +318,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
...
> >  	 * Hence we need to flush out this data.
> >  	 */
> Is the comment above still valid?  I'd guess it referred to this code
> which has now gone.
> >

Its valid when the comment is read completely. All ADC channels
are sampled even if only one is read. The one needed is returned
to userspace. The rest of the fifo is flushed. 

Not the ideal way. But I'm focusing on continuous sampling mode at the moment

> >  	adc_dev->channels = channels;
> 
> Use the devm interfaces and there is no need to keep a copy
> of the irq number about.
>

Noted. 
...

> >
> devm_request_irq will simplify error handling a little.
> 

Indeed error handling gets confusing. I'll update.
...

> > +	iio_trigger_free(adc_dev->trig);
> There is a devm trigger allocation function now. Using that
> would make things a little cleaner.

Noted

> 
> > +	free_irq(adc_dev->irq, indio_dev);
> Best to use the devm interfaces for irq handling unless
> there is a good reason to not do so (cleaner code ;)
> 
> >  	iio_device_unregister(indio_dev);
> 

Noted.

> We would normally expect this function to run in exact
> reverse order of the probe.  That is not the case here
> so please reorder things so it is.
> > +	iio_buffer_unregister(indio_dev);

I'll look into it.

> >  	tiadc_channels_remove(indio_dev);
> >
> >  	step_en = get_adc_step_mask(adc_dev);
> > @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
> >
> >  	/* Make sure ADC is powered up */
> This comment doesn't seem to me to be true any more.
> >  	restore = tiadc_readl(adc_dev, REG_CTRL);
> > -	restore &= ~(CNTRLREG_POWERDOWN);
> Some coments here would be good to explain the sequence that
> is going on.
> 
> I'm guessing a bit, but looks like you turn off the touch screen
> but leave everything else alone, then set the threshold then
> reenable the touch screen before powering up the screen?
> 

To be honest. I don't even remember. 
It might have been a patch I squashed in.
does not seem to be related to the continuous sampling I guess..
I should remove this and send it separately.

> > +	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 db1791b..a372ebf 100644
> > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > @@ -46,17 +46,25 @@
> >  	/* adc device */
> >  	struct adc_device *adc;
> > +
> > +	/* Context save */
> > +	unsigned int irqstat;
> Where are these used?
> > +	unsigned int ctrl;
> >  };
Oops. Don't think they are used anywhere.
Thanks for pointing it out.
> >
> >  static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
> >

Thanks
ZubairLK

Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Zubair Lutfullah :]

On Sun, Sep 08, 2013 at 02:42:51PM +0100, Jonathan Cameron wrote:
> On 09/01/13 12:17, Zubair Lutfullah wrote:

Thanks for the review. Fitting this driver in the usual trigger ABI
is a bad workaround indeed.

I am still a bit unclear regarding the implementation.
Questions and replies inline.

I hope I find the time to pursue the approach you suggest.

Thanks
ZubairLK


> >  #include <linux/of.h>
> Ouch, how did that slip in here.  Dropping this should have been
> in a separate patch as it doesn't really have anything to do with
> the rest of this series.
> 
> > -#include <linux/iio/machine.h>

Squashed it in.. I'll do a cleanup patch separately.

> >  #include <linux/iio/driver.h>
> > +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;
> Whilst I agree that pulling chan up here is slightly cleaner, technically that
> is a bit of unconnected cleanup and should have been in a precursor patch.
> (though don't bother doing so now!)

Again. Squashed in..

> > +
> > +static irqreturn_t tiadc_irq(int irq, void *private)
> > +{
...
> > +		return IRQ_HANDLED;
> > +	} else if (status & IRQENB_FIFO1THRES) {
> > +		/* Trigger to push FIFO data to iio buffer */
> > +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
> 
> So this is triggering at the threshold.  Thus it isn't acting as a conventional
> trigger in the IIO sense at all.  It is of no real use to anything outside
> this driver, so at the very least add the two callbacks to prevent this
> driver using any other trigger and the trigger being used by any other device.
> However, I'm really not happy with this solution.
> 
> Take a look at running with no trigger at all and calling the buffer
> filling more directly.  The issue is that if we have a trigger now it will
> become part of the userspace interface and become a pain to drop later.
> 
> It is valid here I think to use the INDIO_BUFFER_HARDWARE mode (which skips
> the checks for a trigger) then just don't call iio_triggered_buffer_post_enable
> or iio_triggered_buffer_predisable.  Then make this a threaded interrupt and
> put your tiadc_trigger_h as the thread.  Finally just return IRQ_WAKE_THREAD
> in this case.  You'll have to pull a few bits out of
> industrialio-triggered-buffer.c to create the kfifo etc, but no more than
> a few lines given the pollfunc won't exist.

If I understand correctly.
buffer setup ops for postenable and predisable have 

return iio_triggered_buffer_*(indio_dev)

calls at the end. I return 0 instead.

Insead of iio_triggered_buffer setup. I only use iio_kfifo_alloc
(what size fifo is made? :s )
And do 

indio_dev->modes |= INDIO_BUFFER_HARDWARE;

Then I use request_threaded_irq(*). The hardware IRQ line for the fifos
So the HW irq handler runs in interrupt context which wakes the threaded handler
which pushes samples to the iio buffer.

Shared IRQ line. ADC uses request_threaded_irq(). TSC uses request_irq()
ADC returns IRQ_WAKE_THREAD. But the TSC side returns NONE/HANDLED.
Does this change to a threaded irq affect the TSC side of things?
I just received an ack by Dmitry for that patch..

> 
> Actually, might be cleaner to move the flush case into the thread as well
> and just have a single check in this top half of the handler. That bit
> is up to you given it is really a question of how long it take and hence
> if it is acceptable in interrupt context.

Sorry. I don't understand this paragraph.. Could you please elaborate?
The flushing fifo before/after enabling/disabling the buffer part? 
I explained the reason behind the flushes

> 
> > +		iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
> > +		return IRQ_HANDLED;
> > +	} else
> > +		return IRQ_NONE;
> > +
> > +}
> > +
> > +static irqreturn_t tiadc_trigger_h(int irq, void *p)
> > +{
...
> 
> > +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> > +	for (k = 0; k < fifo1count; k = k + i) {
> As mentioned below, why not use 16 bit storage in the kfifo?

32 bit storage is because the FIFO is 32 bit in hardware.
It returns 12 bits data. some reserve bits. And 4 bit channel id.
The channel id is used to separate which channel data which.
Need the full 32 bits..

I've added a comment. Not here. but when the storage_bits = 32 is 
set.

...
> > +static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
...
> Why would there be data in the fifo?  Seems to me that flushing
> after disabling it and before enabling it should not both be
> necessary? Is this just a case of playing safe?
> 
> > +	/* Flush FIFO before starting sampling */
> > +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> > +	for (i = 0; i < fifo1count; i++)
> > +		read = tiadc_readl(adc_dev, REG_FIFO1);

Pre-flush was needed because TSC events would cause some
redundant ADC samples in the FIFO. Fixes have been sent to input.
Shouldn't happen now.

Post-flush is playing safe. There 'might' be a chance that 
another sample comes in the time it takes for fifocount to be read
And that number of samples read from fifo. and then adc disabled.

> > +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > +
> > +	tiadc_step_config(indio_dev);
> > +	kfree(adc_dev->data);
> blank line here please.

ok

> > +	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 const struct iio_trigger_ops tiadc_trigger_ops = {
> > +	.owner = THIS_MODULE,
> > +};
> > +
> 
> If you want a utility function for allocation, convention would
> say have one for freeing as well so that the code is obviously
> balanced and easy to check.  Also this registers the buffer as
> well so the name should probably reflect that.
> 

Noted.

> > +static int tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> > +{
> > +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > +	struct iio_trigger *trig = adc_dev->trig;
> > +	int ret;
> > +
> devm_iio_trigger_alloc is now available and will simplify things a little.
> 

Trigger related stuff is redundant anyways as moving away from trigger..

> > +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);

> > @@ -120,6 +295,7 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> >  		chan->scan_type.realbits = 12;
> >  		chan->scan_type.storagebits = 32;
> I never noticed this before but putting 12 bit values in 32 bit storage
> does seem rather wasteful.  Far as I can see above there is no real
> reason for doing so.  If there is, then please add a comment justifying this
> choice!
>

Answered earlier. I added a comment here explaining. 

> > @@ -142,11 +318,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
...
> >  	 * Hence we need to flush out this data.
> >  	 */
> Is the comment above still valid?  I'd guess it referred to this code
> which has now gone.
> >

Its valid when the comment is read completely. All ADC channels
are sampled even if only one is read. The one needed is returned
to userspace. The rest of the fifo is flushed. 

Not the ideal way. But I'm focusing on continuous sampling mode at the moment

> >  	adc_dev->channels = channels;
> 
> Use the devm interfaces and there is no need to keep a copy
> of the irq number about.
>

Noted. 
...

> >
> devm_request_irq will simplify error handling a little.
> 

Indeed error handling gets confusing. I'll update.
...

> > +	iio_trigger_free(adc_dev->trig);
> There is a devm trigger allocation function now. Using that
> would make things a little cleaner.

Noted

> 
> > +	free_irq(adc_dev->irq, indio_dev);
> Best to use the devm interfaces for irq handling unless
> there is a good reason to not do so (cleaner code ;)
> 
> >  	iio_device_unregister(indio_dev);
> 

Noted.

> We would normally expect this function to run in exact
> reverse order of the probe.  That is not the case here
> so please reorder things so it is.
> > +	iio_buffer_unregister(indio_dev);

I'll look into it.

> >  	tiadc_channels_remove(indio_dev);
> >
> >  	step_en = get_adc_step_mask(adc_dev);
> > @@ -298,10 +495,16 @@ static int tiadc_resume(struct device *dev)
> >
> >  	/* Make sure ADC is powered up */
> This comment doesn't seem to me to be true any more.
> >  	restore = tiadc_readl(adc_dev, REG_CTRL);
> > -	restore &= ~(CNTRLREG_POWERDOWN);
> Some coments here would be good to explain the sequence that
> is going on.
> 
> I'm guessing a bit, but looks like you turn off the touch screen
> but leave everything else alone, then set the threshold then
> reenable the touch screen before powering up the screen?
> 

To be honest. I don't even remember. 
It might have been a patch I squashed in.
does not seem to be related to the continuous sampling I guess..
I should remove this and send it separately.

> > +	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 db1791b..a372ebf 100644
> > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > @@ -46,17 +46,25 @@
> >  	/* adc device */
> >  	struct adc_device *adc;
> > +
> > +	/* Context save */
> > +	unsigned int irqstat;
> Where are these used?
> > +	unsigned int ctrl;
> >  };
Oops. Don't think they are used anywhere.
Thanks for pointing it out.
> >
> >  static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
> >

Thanks
ZubairLK

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Zubair Lutfullah :]

On Mon, Sep 09, 2013 at 09:12:30AM -0700, Dmitry Torokhov wrote:
> On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> > On 09/01/13 12:17, Zubair Lutfullah wrote:
> > > Enable shared IRQ to allow ADC to share IRQ line from
> > > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > > on the TSC side.
> > > 
> > > Step mask would be updated from cached variable only previously.
> > > In rare cases when both TSC and ADC are used, the cached
> > > variable gets mixed up.
> > > The step mask is written with the required mask every time.
> > > 
> > > Rachna Patil (TI) laid ground work for shared IRQ.
> > > 
> > > Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> > Whilst I would have prefered an mfd under these drivers and elegant handling
> > of the interrupts, I guess if this works it is at least not terribly invasive.
> > 
> > However, this does need an Ack from Dmitry before I can take it (or for
> > Dmitry to take it himself?)
> 
> I completely agree with Jonathan, more elegant handling would be nice
> but current one will do for now.
> 
> Since most of the work on the driver is going through IIO tree I think
> it would make sense for this patch to go through it as well.
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thank-you.
ZubairLK

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Zubair Lutfullah :]

On Mon, Sep 09, 2013 at 09:12:30AM -0700, Dmitry Torokhov wrote:
> On Sun, Sep 08, 2013 at 12:29:26PM +0100, Jonathan Cameron wrote:
> > On 09/01/13 12:17, Zubair Lutfullah wrote:
> > > Enable shared IRQ to allow ADC to share IRQ line from
> > > parent MFD core. Only FIFO0 IRQs are for TSC and handled
> > > on the TSC side.
> > > 
> > > Step mask would be updated from cached variable only previously.
> > > In rare cases when both TSC and ADC are used, the cached
> > > variable gets mixed up.
> > > The step mask is written with the required mask every time.
> > > 
> > > Rachna Patil (TI) laid ground work for shared IRQ.
> > > 
> > > Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Whilst I would have prefered an mfd under these drivers and elegant handling
> > of the interrupts, I guess if this works it is at least not terribly invasive.
> > 
> > However, this does need an Ack from Dmitry before I can take it (or for
> > Dmitry to take it himself?)
> 
> I completely agree with Jonathan, more elegant handling would be nice
> but current one will do for now.
> 
> Since most of the work on the driver is going through IIO tree I think
> it would make sense for this patch to go through it as well.
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thank-you.
ZubairLK

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

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

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,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;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 12:42:11PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:
> 
> >diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> >index e1c5300..4124e580 100644
> >--- a/drivers/input/touchscreen/ti_am335x_tsc.c
> >+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> >@@ -315,11 +321,17 @@ static irqreturn_t titsc_irq(int irq, void *dev)
...
> >+	/* If any IRQ flags left, return none. So ADC can handle its IRQs */
> >+	status = titsc_readl(ts_dev, REG_IRQSTATUS);
> >+	if (status == false)
> >+		return IRQ_HANDLED;
> >+	else
> >+		return IRQ_NONE;
> 
> If I understand this correctly you return IRQ_NONE the TSC interrupt has
> been handled and no ADC interrupt is outstanding.

Its actually the opposite. TSC handler checks if there are any ADC IRQ flags
outstanding.

If there is no outstanding, then IRQ_HANDLED is returned.
If there is ADC IRQ outstanding, then IRQ_NONE is returned.

ZubairLK

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:

>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index e1c5300..4124e580 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -315,11 +321,17 @@ 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);
>-		return IRQ_HANDLED;
>+		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> 	}
>-	return IRQ_NONE;
>+
>+	/* If any IRQ flags left, return none. So ADC can handle its IRQs */
>+	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>+	if (status == false)
>+		return IRQ_HANDLED;
>+	else
>+		return IRQ_NONE;

If I understand this correctly you return IRQ_NONE the TSC interrupt has
been handled and no ADC interrupt is outstanding.
This is bad because if you received 1k _only_ TSC interrupts you return
always IRQ_NONE and the irq-core will disable the interrupt line. You
don't want this.

Now, if you handle an interrupt at the TSC level and return IRQ_HANDLED
then the second handler, the ADC in your case, is also invoked.
So you can drop this and return IRQ_HANDLED if you *did* something here
and NONE if didn't do anything.

Basides that, I'm fine with it.

>+
> }

Sebastian

Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[Sebastian Andrzej Siewior]

* Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:

>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index e1c5300..4124e580 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -315,11 +321,17 @@ 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);
>-		return IRQ_HANDLED;
>+		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> 	}
>-	return IRQ_NONE;
>+
>+	/* If any IRQ flags left, return none. So ADC can handle its IRQs */
>+	status = titsc_readl(ts_dev, REG_IRQSTATUS);
>+	if (status == false)
>+		return IRQ_HANDLED;
>+	else
>+		return IRQ_NONE;

If I understand this correctly you return IRQ_NONE the TSC interrupt has
been handled and no ADC interrupt is outstanding.
This is bad because if you received 1k _only_ TSC interrupts you return
always IRQ_NONE and the irq-core will disable the interrupt line. You
don't want this.

Now, if you handle an interrupt at the TSC level and return IRQ_HANDLED
then the second handler, the ADC in your case, is also invoked.
So you can drop this and return IRQ_HANDLED if you *did* something here
and NONE if didn't do anything.

Basides that, I'm fine with it.

>+
> }

Sebastian

[PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC

[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.

Step mask would be updated from cached variable only previously.
In rare cases when both TSC and ADC are used, the cached
variable gets mixed up.
The step mask is written with the required mask every time.

Rachna Patil (TI) laid ground work for shared IRQ.

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

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..4124e580 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,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -315,11 +321,17 @@ 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);
-		return IRQ_HANDLED;
+		titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 	}
-	return IRQ_NONE;
+
+	/* If any IRQ flags left, return none. So ADC can handle its IRQs */
+	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	if (status == false)
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+
 }
 
 static int titsc_parse_dt(struct platform_device *pdev,
@@ -389,7 +401,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5