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

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

[Zubair Lutfullah]

This applies to togreg branch in iio.

Round 6 updates
Fixed trigger the way iio list wanted. Driver has its own trigger.
Triggers at every FIFO Threshold IRQ and pushes samples to iio buffer.

Went through the driver and cleaned it up quite a bit.

Squashed patches together instead of having multiple tiny patches.

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

 drivers/iio/adc/ti_am335x_adc.c           |  254 ++++++++++++++++++++++++++---
 drivers/input/touchscreen/ti_am335x_tsc.c |   24 ++-
 include/linux/mfd/ti_am335x_tscadc.h      |   13 ++
 3 files changed, 264 insertions(+), 27 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 |   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

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

[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

[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>
---
 drivers/iio/adc/ti_am335x_adc.c      |  254 +++++++++++++++++++++++++++++++---
 include/linux/mfd/ti_am335x_tscadc.h |   13 ++
 2 files changed, 246 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a952538..ae2202b 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -28,12 +28,20 @@
 #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 +64,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
 	return step_en;
 }
 
-static void tiadc_step_config(struct tiadc_device *adc_dev)
+static void tiadc_step_config(struct iio_dev *indio_dev)
 {
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	unsigned int stepconfig;
-	int i, steps;
+	int i, steps, chan;
 
 	/*
 	 * There are 16 configurable steps and 8 analog input
@@ -72,11 +81,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 +96,175 @@ 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));
+	} 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());
+	} else
+		return IRQ_NONE;
+
+	/* If any IRQ flags left, return none. So TSC can handle its IRQs */
+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+	if (status == false)
+		return IRQ_HANDLED;
+	else
+		return IRQ_NONE;
+}
+
+static irqreturn_t tiadc_trigger_h(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	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, stepnum;
+	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) {
+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
+		/*
+		 * There are a total of 16 steps available
+		 * that are shared between ADC and touchscreen.
+		 * We start configuring from step 16 to 0 incase of
+		 * ADC. Hence the relation between input channel
+		 * and step for ADC would be as below.
+		 */
+		stepnum = chan->channel + 9;
+		enb |= (1 << stepnum);
+	}
+
+	adc_dev->buffer_en_ch_steps = enb;
+	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 struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
+{
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return NULL;
+
+	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)
+		return NULL;
+
+	return trig;
 }
 
 static const char * const chan_name_ain[] = {
@@ -120,6 +299,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 +322,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 +351,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);
@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
 					  sizeof(struct tiadc_device));
 	if (indio_dev == NULL) {
 		dev_err(&pdev->dev, "failed to allocate iio device\n");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto err_ret;
 	}
 	adc_dev = iio_priv(indio_dev);
 
@@ -231,28 +406,56 @@ 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;
+		goto err_free_device;
+
+	adc_dev->trig = tiadc_iio_allocate_trigger(indio_dev);
+	if (adc_dev->trig == NULL) {
+		err = -ENOMEM;
+		goto err_free_channels;
+	}
+
+	err = request_irq(adc_dev->irq, tiadc_irq, IRQF_SHARED,
+		indio_dev->name, indio_dev);
+	if (err)
+		goto err_free_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_free_channels;
+		goto err_buffer_unregister;
 
 	platform_set_drvdata(pdev, indio_dev);
 
 	return 0;
 
+err_buffer_unregister:
+	iio_buffer_unregister(indio_dev);
+err_free_irq:
+	free_irq(adc_dev->irq, indio_dev);
+err_free_trigger:
+	iio_trigger_unregister(adc_dev->trig);
+	iio_trigger_free(adc_dev->trig);
 err_free_channels:
 	tiadc_channels_remove(indio_dev);
+err_free_device:
+	iio_device_free(indio_dev);
+err_ret:
 	return err;
 }
 
@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	u32 step_en;
 
+	free_irq(adc_dev->irq, indio_dev);
 	iio_device_unregister(indio_dev);
+	iio_buffer_unregister(indio_dev);
 	tiadc_channels_remove(indio_dev);
 
 	step_en = get_adc_step_mask(adc_dev);
 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
+	iio_device_free(indio_dev);
 
 	return 0;
 }
@@ -298,10 +504,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 2/2] iio: ti_am335x_adc: Add continuous sampling support

[Lee Jones]

On Sun, 25 Aug 2013, 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>
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  254 +++++++++++++++++++++++++++++++---
>  include/linux/mfd/ti_am335x_tscadc.h |   13 ++
>  2 files changed, 246 insertions(+), 21 deletions(-)

MFD stuff looks okay to me.

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

<snip>

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

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

[PATCH] iio: am335x-iio-adc: select triggered buffer

[Sebastian Andrzej Siewior]

"triggered buffer" isn't selected so we end up with:
|ERROR: "iio_triggered_buffer_setup" [drivers/iio/adc/ti_am335x_adc.ko] undefined!

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Please merge this into _this_ patch, it is still required.

 drivers/iio/adc/Kconfig | 1 +
 1 file changed, 1 insertion(+)

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.
-- 
1.8.4.rc3

Re: [PATCH] iio: am335x-iio-adc: select triggered buffer

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 12:31:34PM +0200, Sebastian Andrzej Siewior wrote:
> "triggered buffer" isn't selected so we end up with:
> |ERROR: "iio_triggered_buffer_setup" [drivers/iio/adc/ti_am335x_adc.ko] undefined!
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Please merge this into _this_ patch, it is still required.
> 
>  drivers/iio/adc/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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
Thanks for pointing it out. Added.

ZubairLK

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

[Sebastian Andrzej Siewior]

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

I am mostly happy with it. There are just two things I pointed out.
Besides that, it looks good from my side.

>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_a=
dc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
>@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc=
_dev)
> 		adc_dev->channel_step[i] =3D steps;
> 		steps++;
> 	}
>+}
>+
>+static irqreturn_t tiadc_irq(int irq, void *private)
>+{
>+	struct iio_dev *indio_dev =3D private;
>+	struct tiadc_device *adc_dev =3D iio_priv(indio_dev);
>+	unsigned int status, config;
>+	status =3D 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 =3D tiadc_readl(adc_dev, REG_CTRL);
>+		config &=3D ~(CNTRLREG_TSCSSENB);
>+		tiadc_writel(adc_dev, REG_CTRL, config);
>+		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
>+				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
>+		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
>+	} else if (status & IRQENB_FIFO1THRES) {
>+		/* 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());
>+	} else
>+		return IRQ_NONE;
>+
>+	/* If any IRQ flags left, return none. So TSC can handle its IRQs */
>+	status =3D tiadc_readl(adc_dev, REG_IRQSTATUS);
>+	if (status =3D=3D false)
>+		return IRQ_HANDLED;
>+	else
>+		return IRQ_NONE;

As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
shared line are invoked once an interrupt is detected.

=E2=80=A6

>+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>+{
>+	struct tiadc_device *adc_dev =3D iio_priv(indio_dev);
>+	struct iio_buffer *buffer =3D indio_dev->buffer;
>+	unsigned int enb =3D 0, stepnum;
>+	u8 bit;
>+
>+	adc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>+	if (adc_dev->data =3D=3D NULL)
>+		return -ENOMEM;
>+
>+	tiadc_step_config(indio_dev);
>+	for_each_set_bit(bit, buffer->scan_mask,
>+			adc_dev->channels) {
>+		struct iio_chan_spec const *chan =3D indio_dev->channels + bit;
>+		/*
>+		 * There are a total of 16 steps available
>+		 * that are shared between ADC and touchscreen.
>+		 * We start configuring from step 16 to 0 incase of
>+		 * ADC. Hence the relation between input channel
>+		 * and step for ADC would be as below.
>+		 */
>+		stepnum =3D chan->channel + 9;
>+		enb |=3D (1 << stepnum);

Hmm. This looks odd.
=20
In tiadc_step_config() we do:
| steps =3D TOTAL_STEPS - adc_dev->channels;
|=E2=80=A6
| for (i =3D 0; i < adc_dev->channels; i++) {
|         chan =3D adc_dev->channel_line[i];
|         tiadc_writel(adc_dev,
|                         REG_STEPCONFIG(steps),
|                         stepconfig
|                         |
|                         STEPCONFIG_INP(chan));
|         tiadc_writel(adc_dev,
|                         REG_STEPDELAY(steps),
|                         STEPCONFIG_OPENDLY);
|         adc_dev->channel_step[i]
|                 =3D
|                 steps;
|         steps++;
| }

That means if we have only one channel we enable the last step bit /
topmost that is bit 16. For the "default" four channels we get 0x1e000
as mask which means bit 13 to 16.=20
If you do have only one channel with the number 4 you then
get_adc_step_mask() will compute the correct step mask but here enable
step 14 instead of step 16.

What about the following as replacement?

index 08681d3..3bfcf1b 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_de=
v)
 	return step_en;
 }
=20
+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 =3D iio_priv(indio_dev);
@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indi=
o_dev)
 {
 	struct tiadc_device *adc_dev =3D iio_priv(indio_dev);
 	struct iio_buffer *buffer =3D indio_dev->buffer;
-	unsigned int enb =3D 0, stepnum;
+	unsigned int enb =3D 0;
 	u8 bit;
=20
 	adc_dev->data =3D kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *ind=
io_dev)
 		return -ENOMEM;
=20
 	tiadc_step_config(indio_dev);
-	for_each_set_bit(bit, buffer->scan_mask,
-			adc_dev->channels) {
-		struct iio_chan_spec const *chan =3D indio_dev->channels + bit;
-		/*
-		 * There are a total of 16 steps available
-		 * that are shared between ADC and touchscreen.
-		 * We start configuring from step 16 to 0 incase of
-		 * ADC. Hence the relation between input channel
-		 * and step for ADC would be as below.
-		 */
-		stepnum =3D chan->channel + 9;
-		enb |=3D (1 << stepnum);
-	}
+	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+		enb |=3D get_adc_step_bit(adc_dev, bit);
=20
 	adc_dev->buffer_en_ch_steps =3D enb;
 	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);


Would it work? This is untested but it could work :)

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 03:01:24PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> 
> I am mostly happy with it. There are just two things I pointed out.
> Besides that, it looks good from my side.
Sign-off?

> >+static irqreturn_t tiadc_irq(int irq, void *private)
> >+{
...
> >+	/* If any IRQ flags left, return none. So TSC can handle its IRQs */
> >+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> >+	if (status == false)
> >+		return IRQ_HANDLED;
> >+	else
> >+		return IRQ_NONE;
> 
> As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
> shared line are invoked once an interrupt is detected.
> 
> …
I somehow missed sending these patches to Dmitry.. 
I'll do it next series.

He originally suggested this way.
An alternative was to handle irqs in mfd core which would 
seriously complicate code and patches..

Whenever I change the handlers to the way you suggest.
i.e. Handle what each irq handler does and return IRQ_HANDLED
if the irq handler handled nothing then return IRQ_NONE.

The driver hangs when I'm brutal while touching the TSC and 
continuously sampling the ADC. Not always. But happens.

When I check /proc/interrupts the hardware irqs seem to be firing.
But for some reason the whole thing messes up..

> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	struct iio_buffer *buffer = indio_dev->buffer;
> >+	unsigned int enb = 0, stepnum;
> >+	u8 bit;
> >+
> >+	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) {
> >+		struct iio_chan_spec const *chan = indio_dev->channels + bit;
> >+		/*
> >+		 * There are a total of 16 steps available
> >+		 * that are shared between ADC and touchscreen.
> >+		 * We start configuring from step 16 to 0 incase of
> >+		 * ADC. Hence the relation between input channel
> >+		 * and step for ADC would be as below.
> >+		 */
> >+		stepnum = chan->channel + 9;
> >+		enb |= (1 << stepnum);
> 
> Hmm. This looks odd.
>  
> In tiadc_step_config() we do:
> | steps = TOTAL_STEPS - adc_dev->channels;
> |…
> | for (i = 0; i < adc_dev->channels; i++) {
> |         chan = adc_dev->channel_line[i];
> |         tiadc_writel(adc_dev,
> |                         REG_STEPCONFIG(steps),
> |                         stepconfig
> |                         |
> |                         STEPCONFIG_INP(chan));
> |         tiadc_writel(adc_dev,
> |                         REG_STEPDELAY(steps),
> |                         STEPCONFIG_OPENDLY);
> |         adc_dev->channel_step[i]
> |                 =
> |                 steps;
> |         steps++;
> | }
> 
> That means if we have only one channel we enable the last step bit /
> topmost that is bit 16. For the "default" four channels we get 0x1e000
> as mask which means bit 13 to 16. 
> If you do have only one channel with the number 4 you then
> get_adc_step_mask() will compute the correct step mask but here enable
> step 14 instead of step 16.
> 
> What about the following as replacement?
I sense a mismatch. Probably because of different styles of coding
these bit handling :).

I'll look into it and the alternative way you suggested as well.

Thanks
ZubairLK

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

[Sebastian Andrzej Siewior]

On 08/28/2013 08:43 PM, Zubair Lutfullah : wrote:
> On Wed, Aug 28, 2013 at 03:01:24PM +0200, Sebastian Andrzej Siewior wrote:
>> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>>
>> I am mostly happy with it. There are just two things I pointed out.
>> Besides that, it looks good from my side.
> Sign-off?

Acked-by if anything. For that you should fix the things I pointed out.

>>> +static irqreturn_t tiadc_irq(int irq, void *private)
>>> +{
> ...
>>> +	/* If any IRQ flags left, return none. So TSC can handle its IRQs */
>>> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
>>> +	if (status == false)
>>> +		return IRQ_HANDLED;
>>> +	else
>>> +		return IRQ_NONE;
>>
>> As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
>> shared line are invoked once an interrupt is detected.
>>
>> …
> I somehow missed sending these patches to Dmitry.. 
> I'll do it next series.
> 
> He originally suggested this way.
> An alternative was to handle irqs in mfd core which would 
> seriously complicate code and patches..
> 
> Whenever I change the handlers to the way you suggest.
> i.e. Handle what each irq handler does and return IRQ_HANDLED
> if the irq handler handled nothing then return IRQ_NONE.
> 
> The driver hangs when I'm brutal while touching the TSC and 
> continuously sampling the ADC. Not always. But happens.
> 
> When I check /proc/interrupts the hardware irqs seem to be firing.
> But for some reason the whole thing messes up..

In that case it is likely that the irqs are not acked properly and
therefore fired again. So you should check that.

If you look at handle_irq_event_percpu() in kernel/irq/handle.c. That
is the function that invokes your handler. It looks the following in
short:

	do {
		res = action->handler(irq, action->dev_id);
		retval |= res;
		action = action->next;
	} while (action);
	note_interrupt(irq, desc, retval);

As you see all registered handlers are invoked no matter of the return
code. The common return code is pass over to note_interrupt() which will
disable the irq line if IRQ_NONE is returned too many times.

It is possible that the read of REG_IRQSTATUS will ACK those and
therefore it does not fire anymore. I don't have the manual next to me
right now, so please check how the interrupt is acked by reading the
status register or by writing a bit to the status register etc.
_If_ the status bit is auto-cleared on read but the interrupt is not
acked and remains acktive then you won't see it in the "other" handler,
do nothing but the source will remain active.
This might match your description. In that case I suggest to register
that interrupt in the MFD part let TSC & ADC register via the mfd part.
The MFD part would read the status register and pass it to the two
handlers so it not lost.

In any case: please check what is really going on. This workaround you
have now does not look good.

> Thanks
> ZubairLK


Sebastian

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

[Sebastian Andrzej Siewior]

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

>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_a=
dc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
=E2=80=A6

>+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *ind=
io_dev)
>+{
>+	struct iio_trigger *trig;
>+	int ret;
>+
>+	trig =3D iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
>+	if (trig =3D=3D NULL)
>+		return NULL;
>+
>+	trig->dev.parent =3D indio_dev->dev.parent;
>+	trig->ops =3D &tiadc_trigger_ops;
>+	iio_trigger_set_drvdata(trig, indio_dev);
>+
>+	ret =3D iio_trigger_register(trig);
>+	if (ret)

shouldn't you free the trigger / undo iio_trigger_alloc() here?

>+		return NULL;
>+
>+	return trig;
> }

>=20
> static const char * const chan_name_ain[] =3D {
>@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
> 					  sizeof(struct tiadc_device));
> 	if (indio_dev =3D=3D NULL) {
> 		dev_err(&pdev->dev, "failed to allocate iio device\n");
>-		return -ENOMEM;
>+		err =3D -ENOMEM;
>+		goto err_ret;
Why the jump instead of leave right away?

> 	}
> 	adc_dev =3D iio_priv(indio_dev);
>=20
>@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> 	struct tiadc_device *adc_dev =3D iio_priv(indio_dev);
> 	u32 step_en;
>=20
>+	free_irq(adc_dev->irq, indio_dev);
> 	iio_device_unregister(indio_dev);
>+	iio_buffer_unregister(indio_dev);
> 	tiadc_channels_remove(indio_dev);

I think you need also to

	iio_trigger_unregister(adc_dev->trig);
	iio_trigger_free(adc_dev->trig);

here.
=20
> 	step_en =3D get_adc_step_mask(adc_dev);
> 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>+	iio_device_free(indio_dev);
>=20
> 	return 0;
> }

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> 
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >index a952538..ae2202b 100644
> >--- a/drivers/iio/adc/ti_am335x_adc.c
> >+++ b/drivers/iio/adc/ti_am335x_adc.c
> …
> 
> >+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
...
> >+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> >+	if (trig == NULL)
> >+		return NULL;
> >+
...
> >+	ret = iio_trigger_register(trig);
> >+	if (ret)
> 
> shouldn't you free the trigger / undo iio_trigger_alloc() here?
> 
> >+		return NULL;
> >+
...
Possible. It's done in the probe section just 
to keep error handling bunched there.
> 
> > 
> > static const char * const chan_name_ain[] = {
> >@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
> > 					  sizeof(struct tiadc_device));
> > 	if (indio_dev == NULL) {
> > 		dev_err(&pdev->dev, "failed to allocate iio device\n");
> >-		return -ENOMEM;
> >+		err = -ENOMEM;
> >+		goto err_ret;
> Why the jump instead of leave right away?
> 
> > 	}
Again. Error handling all in one place.

> > 
> >@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> > 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > 	u32 step_en;
> > 
> >+	free_irq(adc_dev->irq, indio_dev);
> > 	iio_device_unregister(indio_dev);
> >+	iio_buffer_unregister(indio_dev);
> > 	tiadc_channels_remove(indio_dev);
> 
> I think you need also to
> 
> 	iio_trigger_unregister(adc_dev->trig);
> 	iio_trigger_free(adc_dev->trig);
> 
> here.
Indeed.
Thanks for pointing it out.

Zubair

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

[Sebastian Andrzej Siewior]

On 08/28/2013 08:22 PM, Zubair Lutfullah : wrote:
> On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:
>> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>>
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index a952538..ae2202b 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> …
>>
>>> +static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> ...
>>> +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
>>> +	if (trig == NULL)
>>> +		return NULL;
>>> +
> ...
>>> +	ret = iio_trigger_register(trig);
>>> +	if (ret)
>>
>> shouldn't you free the trigger / undo iio_trigger_alloc() here?
>>
>>> +		return NULL;
>>> +
> ...
> Possible. It's done in the probe section just 
> to keep error handling bunched there.

The caller of iio_trigger *tiadc_iio_allocate_trigger() should not
cleanup after it in case of a half failure. The function itself should
be in charge of it. However you never assign the pointer so you leak
the memory here.

>>
>>>
>>> static const char * const chan_name_ain[] = {
>>> @@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
>>> 					  sizeof(struct tiadc_device));
>>> 	if (indio_dev == NULL) {
>>> 		dev_err(&pdev->dev, "failed to allocate iio device\n");
>>> -		return -ENOMEM;
>>> +		err = -ENOMEM;
>>> +		goto err_ret;
>> Why the jump instead of leave right away?
>>
>>> 	}
> Again. Error handling all in one place.

But you simply leave at the end of the function, no error handling is
done / required if you leave here. So this a zero change and has
nothing to do with the "continuous sampling support" support.

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 08:38:55PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/28/2013 08:22 PM, Zubair Lutfullah : wrote:
> > On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:
> >> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> >>
> >>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >>> index a952538..ae2202b 100644
> >>> --- a/drivers/iio/adc/ti_am335x_adc.c
> >>> +++ b/drivers/iio/adc/ti_am335x_adc.c
> >> …
> >>
> >>> +static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> > ...
> >>> +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> >>> +	if (trig == NULL)
> >>> +		return NULL;
> >>> +
> > ...
> >>> +	ret = iio_trigger_register(trig);
> >>> +	if (ret)
> >>
> >> shouldn't you free the trigger / undo iio_trigger_alloc() here?
> >>
> >>> +		return NULL;
> >>> +
> > ...
> > Possible. It's done in the probe section just 
> > to keep error handling bunched there.
> 
> The caller of iio_trigger *tiadc_iio_allocate_trigger() should not
> cleanup after it in case of a half failure. The function itself should
> be in charge of it. However you never assign the pointer so you leak
> the memory here.
> 
A very valid point indeed.
I'll fix this.
Thanks.
> >>
> >>>
> >>> static const char * const chan_name_ain[] = {
> >>> @@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
> >>> 					  sizeof(struct tiadc_device));
> >>> 	if (indio_dev == NULL) {
> >>> 		dev_err(&pdev->dev, "failed to allocate iio device\n");
> >>> -		return -ENOMEM;
> >>> +		err = -ENOMEM;
> >>> +		goto err_ret;
> >> Why the jump instead of leave right away?
> >>
> >>> 	}
> > Again. Error handling all in one place.
> 
> But you simply leave at the end of the function, no error handling is
> done / required if you leave here. So this a zero change and has
> nothing to do with the "continuous sampling support" support.
> 
Again. Thanks.

ZubairLK

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

[Sebastian Andrzej Siewior]

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

>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_a=
dc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
>@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
=E2=80=A6
>+err_free_device:
>+	iio_device_free(indio_dev);

I am not sure about this one.

>+err_ret:
> 	return err;
> }
>=20
>@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> 	struct tiadc_device *adc_dev =3D iio_priv(indio_dev);
> 	u32 step_en;
>=20
>+	free_irq(adc_dev->irq, indio_dev);
> 	iio_device_unregister(indio_dev);
>+	iio_buffer_unregister(indio_dev);
> 	tiadc_channels_remove(indio_dev);
>=20
> 	step_en =3D get_adc_step_mask(adc_dev);
> 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>+	iio_device_free(indio_dev);

But this one is wrong. The will be removed via dev_res() and if you do
it here as well then dev_res() will decrement the reference of an unused
object.

>=20
> 	return 0;
> }

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 06:43:08PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> 
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >index a952538..ae2202b 100644
> >--- a/drivers/iio/adc/ti_am335x_adc.c
> >+++ b/drivers/iio/adc/ti_am335x_adc.c
> >@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
> …
> >+err_free_device:
> >+	iio_device_free(indio_dev);
> 
> I am not sure about this one.

If I understand correctly, if devm_iio_device_alloc 
is successful earlier in the code and subsequent stuff fails. 

Then the code jumps to err_free_device and this is needed.

> 
> >+err_ret:
> > 	return err;
> > }
> > 
> >@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> > 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > 	u32 step_en;
> > 
> >+	free_irq(adc_dev->irq, indio_dev);
> > 	iio_device_unregister(indio_dev);
> >+	iio_buffer_unregister(indio_dev);
> > 	tiadc_channels_remove(indio_dev);
> > 
> > 	step_en = get_adc_step_mask(adc_dev);
> > 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
> >+	iio_device_free(indio_dev);
> 
> But this one is wrong. The will be removed via dev_res() and if you do
> it here as well then dev_res() will decrement the reference of an unused
> object.
I was unaware of this aspect.

*but*

iio_simple_dummy.c has it in its remove function.
And I just checked and found it in several drivers as well.

I'll leave this to the more experienced folks on the list..
> > 
> > 	return 0;
> > }
> 
> Sebastian

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

[Sebastian Andrzej Siewior]

On 08/28/2013 08:19 PM, Zubair Lutfullah : wrote:
> On Wed, Aug 28, 2013 at 06:43:08PM +0200, Sebastian Andrzej Siewior wrote:
>> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>>
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index a952538..ae2202b 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>>> @@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
>> …
>>> +err_free_device:
>>> +	iio_device_free(indio_dev);
>>
>> I am not sure about this one.
> 
> If I understand correctly, if devm_iio_device_alloc 
> is successful earlier in the code and subsequent stuff fails. 
> 
> Then the code jumps to err_free_device and this is needed.

Yes, but devm_() helper release the memory on failer _or_ device /
driver removal.

>>> +err_ret:
>>> 	return err;
>>> }
>>>
>>> @@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
>>> 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>>> 	u32 step_en;
>>>
>>> +	free_irq(adc_dev->irq, indio_dev);
>>> 	iio_device_unregister(indio_dev);
>>> +	iio_buffer_unregister(indio_dev);
>>> 	tiadc_channels_remove(indio_dev);
>>>
>>> 	step_en = get_adc_step_mask(adc_dev);
>>> 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>>> +	iio_device_free(indio_dev);
>>
>> But this one is wrong. The will be removed via dev_res() and if you do
>> it here as well then dev_res() will decrement the reference of an unused
>> object.
> I was unaware of this aspect.
> 
> *but*
> 
> iio_simple_dummy.c has it in its remove function.
> And I just checked and found it in several drivers as well.
> 
> I'll leave this to the more experienced folks on the list..

You can belive me one thing or the other :)
Point me one driver please. Do they use devm_iio_device_alloc() like
you do or iio_device_alloc()? In case of the latter you need to
manually free the memory.

>>>
>>> 	return 0;
>>> }

Sebastian

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

[Zubair Lutfullah :]

On Wed, Aug 28, 2013 at 08:35:30PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/28/2013 08:19 PM, Zubair Lutfullah : wrote:
> > On Wed, Aug 28, 2013 at 06:43:08PM +0200, Sebastian Andrzej Siewior wrote:
> >> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
> >>
> >>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >>> index a952538..ae2202b 100644
> >>> --- a/drivers/iio/adc/ti_am335x_adc.c
> >>> +++ b/drivers/iio/adc/ti_am335x_adc.c
> >>> @@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
> >> …
> >>> +err_free_device:
> >>> +	iio_device_free(indio_dev);
> >>
> >> I am not sure about this one.
> > 
> > If I understand correctly, if devm_iio_device_alloc 
> > is successful earlier in the code and subsequent stuff fails. 
> > 
> > Then the code jumps to err_free_device and this is needed.
> 
> Yes, but devm_() helper release the memory on failer _or_ device /
> driver removal.
I see. 

> >>> @@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> >>> 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
...
> >>> 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
> >>> +	iio_device_free(indio_dev);
...
> Point me one driver please. Do they use devm_iio_device_alloc() like
> you do or iio_device_alloc()? In case of the latter you need to
> manually free the memory.
Aah. So the devm_* stuff makes the difference.
Thanks for pointing it out. Will remove it.

ZubairLK

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

[Sebastian Andrzej Siewior]

On 08/28/2013 08:59 PM, Zubair Lutfullah : wrote:
>>>>> 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>>>>> +	iio_device_free(indio_dev);
> ...
>> Point me one driver please. Do they use devm_iio_device_alloc() like
>> you do or iio_device_alloc()? In case of the latter you need to
>> manually free the memory.
> Aah. So the devm_* stuff makes the difference.
> Thanks for pointing it out. Will remove it.

Yes, devm_* stuff. There is also matching kmalloc() version of it for
the very same purpose and a few others…

In future please post a bugfix (adding iio_device_free() to plug a
memory leak) as a separate patch. It has nothing todo with "continuous
sampling support" and it case it is valid it could be applied
independantly and ported back to earlier kernel if necessary. The way
you did here you just "silently" plugged a memory leak.

> 
> ZubairLK
> 

Sebastian