Skip to contentSkip to conversation

iio: input: ti_am335x_adc: Add continuous sampling support

PATCH Superseded Zubair Lutfullah 3 participants 20 messages Aug 25, 2013 – Aug 29, 2013

Subsystems (4)
HID IIO Input drivers Multifunction devices (MFD)

v6 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Zubair Lutfullah

author
#0 lore raw

v6 cover iio: input: ti_am335x_adc: Add continuous sampling support

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(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
drivers/input/touchscreen/ti_am335x_tsc.c +18 -6
index e1c5300..4124e580 100644
@@ -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;
Signature · 1 line
1.7.9.5
#2 lore raw

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

Quoted 12 lines · * Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:
drivers/input/touchscreen/ti_am335x_tsc.c +1 -3
index e1c5300..4124e580 100644
@@ -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));

drivers/input/touchscreen/ti_am335x_tsc.c static irqreturn_t titsc_irq(int irq, void *dev) · line 333

Quoted 10 lines
drivers/input/touchscreen/ti_am335x_tsc.c +8 -1
+		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.

drivers/input/touchscreen/ti_am335x_tsc.c static irqreturn_t titsc_irq(int irq, void *dev) · line 335

Quoted 2 lines
drivers/input/touchscreen/ti_am335x_tsc.c +1 -0
+
 }

Sebastian

author
#3 lore raw

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

Quoted 7 lines · Sebastian Andrzej Siewior
Quoted 5 lines · * Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:
drivers/input/touchscreen/ti_am335x_tsc.c +0 -0
index e1c5300..4124e580 100644
@@ -315,11 +321,17 @@ static irqreturn_t titsc_irq(int irq, void *dev)

...

Quoted 9 lines

drivers/input/touchscreen/ti_am335x_tsc.c static irqreturn_t titsc_irq(int irq, void *dev) · line 333

Quoted 6 lines
drivers/input/touchscreen/ti_am335x_tsc.c +6 -0
+	/* 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

author
#4 lore raw

PATCH v6 2/2 iio: ti_am335x_adc: Add continuous sampling support

In reply to 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 <[email protected]>
Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Russ Dill <[email protected]>
---
 drivers/iio/adc/ti_am335x_adc.c      |  254 +++++++++++++++++++++++++++++++---
 include/linux/mfd/ti_am335x_tscadc.h |   13 ++
 2 files changed, 246 insertions(+), 21 deletions(-)
drivers/iio/adc/ti_am335x_adc.c +233 -21
index a952538..ae2202b 100644
@@ -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;
 }
include/linux/mfd/ti_am335x_tscadc.h +13 -0
index db1791b..a372ebf 100644
@@ -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)
Signature · 1 line
1.7.9.5
Acked-by on v6 2/2
#5 lore raw

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

Quoted 19 lines · 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.

Quoted 7 lines
Signed-off-by Zubair Lutfullah <[email protected]>
Acked-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Russ Dill <[email protected]>
---
 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 <[email protected]>

<snip>

Quoted 49 lines
include/linux/mfd/ti_am335x_tscadc.h +13 -0
index db1791b..a372ebf 100644
@@ -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)
Signature · 4 lines
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
#6 lore raw

PATCH 1/1 iio: am335x-iio-adc: select triggered buffer

In reply to Zubair Lutfullah

"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 <[email protected]>

---
Please merge this into _this_ patch, it is still required.

 drivers/iio/adc/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/iio/adc/Kconfig +1 -0
index 09371cb..cfa2039 100644
@@ -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.
Signature · 1 line
1.8.4.rc3
author
#7 lore raw

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

Quoted 15 lines · 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 <[email protected]>

---
Please merge this into _this_ patch, it is still required.

 drivers/iio/adc/Kconfig | 1 +
 1 file changed, 1 insertion(+)
drivers/iio/adc/Kconfig +0 -0
index 09371cb..cfa2039 100644

drivers/iio/adc/Kconfig config TI_ADC081C · line 170

Quoted 4 lines
drivers/iio/adc/Kconfig +1 -0
@@ -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

#8 lore raw

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

In reply to Zubair Lutfullah

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

drivers/iio/adc/ti_am335x_a=

Quoted 1 line
drivers/iio/adc/ti_am335x_a= renamed from drivers/iio/adc/ti_am335x_adc.c +0 -0

dc.c

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc=

Quoted 4 lines

index a952538..ae2202b 100644

drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc=

_dev)

Quoted 27 lines
drivers/iio/adc/ti_am335x_adc.c +24 -0
 		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);

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc= · line 132

Quoted 10 lines
drivers/iio/adc/ti_am335x_adc.c +10 -0
+		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

Quoted 14 lines
drivers/iio/adc/ti_am335x_adc.c +14 -0
+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) {

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc= · line 156

Quoted 10 lines
drivers/iio/adc/ti_am335x_adc.c +10 -0
+		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

drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -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

author
#9 lore raw

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

Quoted 4 lines · 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.

Sign-off?

Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+static irqreturn_t tiadc_irq(int irq, void *private)
+{

...

Quoted 11 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+	/* 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..

Quoted 46 lines
Quoted 14 lines
(quoted diff) +14 -0
+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) {

Review

Quoted 10 lines
(quoted diff) +10 -0
+		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++;
| }
Quoted 8 lines

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

#10 lore raw

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

Quoted 6 lines · Zubair Lutfullah
Quoted 4 lines · 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.

Sign-off?

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

Quoted 25 lines
Quoted 2 lines
Quoted 2 lines
(quoted diff) +2 -0
+static irqreturn_t tiadc_irq(int irq, void *private)
+{

...

Quoted 11 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+	/* 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.

Quoted 5 lines

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.

Quoted 2 lines

Thanks
ZubairLK

Sebastian

#11 lore raw

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

In reply to Zubair Lutfullah

drivers/iio/adc/ti_am335x_a=

Quoted 1 line · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_a= renamed from drivers/iio/adc/ti_am335x_adc.c +0 -0

dc.c

drivers/iio/adc/ti_am335x_adc.c

Quoted 3 lines

index a952538..ae2202b 100644

drivers/iio/adc/ti_am335x_adc.c +0 -0

=E2=80=A6

Quoted 1 line

+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *ind=

io_dev)

Quoted 4 lines
drivers/iio/adc/ti_am335x_adc.c +4 -0
+{
+	struct iio_trigger *trig;
+	int ret;
+

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc_dev) · line 264

Quoted 10 lines
drivers/iio/adc/ti_am335x_adc.c +10 -0
+	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?

Quoted 8 lines
drivers/iio/adc/ti_am335x_adc.c +3 -0
+		return NULL;
+
+	return trig;

}

=20
static const char * const chan_name_ain[] =3D {

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 398

Quoted 6 lines
drivers/iio/adc/ti_am335x_adc.c +2 -1
@@ -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?

Quoted 11 lines
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	}
 	adc_dev =3D iio_priv(indio_dev);

=20

drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -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

drivers/iio/adc/ti_am335x_adc.c +1 -0
+	free_irq(adc_dev->irq, indio_dev);

	iio_device_unregister(indio_dev);
drivers/iio/adc/ti_am335x_adc.c +1 -0
+	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

Quoted 6 lines
	step_en =3D get_adc_step_mask(adc_dev);
	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
drivers/iio/adc/ti_am335x_adc.c +1 -0
+	iio_device_free(indio_dev);

=20

	return 0;
}

Sebastian

author
#12 lore raw

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

Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 4 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644

…

Quoted 1 line

+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)

...

Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti_am335x_adc.c +4 -0
+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return NULL;
+

...

Quoted 7 lines

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc_dev) · line 264

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+	ret = iio_trigger_register(trig);
+	if (ret)

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

drivers/iio/adc/ti_am335x_adc.c

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+		return NULL;
+

...
Possible. It's done in the probe section just
to keep error handling bunched there.

Quoted 13 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 398

Quoted 10 lines

static const char * const chan_name_ain[] = {

drivers/iio/adc/ti_am335x_adc.c +2 -1
@@ -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?

Quoted 1 line
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	}

Again. Error handling all in one place.

Quoted 12 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_remove(struct platform_device *pdev) · line 471

Quoted 9 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
@@ -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

Quoted 4 lines
	iio_trigger_unregister(adc_dev->trig);
	iio_trigger_free(adc_dev->trig);

here.

Indeed.
Thanks for pointing it out.

Zubair

#13 lore raw

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

Quoted 21 lines · Zubair Lutfullah
Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 4 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644

…

Quoted 1 line

+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)

...

Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti_am335x_adc.c +4 -0
+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return NULL;
+

...

Quoted 5 lines

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc_dev) · line 264

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+	ret = iio_trigger_register(trig);
+	if (ret)

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

Quoted 5 lines
Quoted 2 lines

drivers/iio/adc/ti_am335x_adc.c

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+		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.

Quoted 12 lines
Quoted 12 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 398

Quoted 10 lines

static const char * const chan_name_ain[] = {

drivers/iio/adc/ti_am335x_adc.c +2 -1
@@ -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?

Quoted 2 lines
Quoted 1 line

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 399

Quoted 1 line
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	}

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

author
#14 lore raw

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

Quoted 28 lines · Sebastian Andrzej Siewior
Quoted 21 lines · Zubair Lutfullah
Quoted 9 lines · Sebastian Andrzej Siewior
Quoted 4 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644

…

Quoted 1 line

+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)

...

Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ti_am335x_adc.c +4 -0
+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return NULL;
+

...

Quoted 5 lines

drivers/iio/adc/ti_am335x_adc.c static void tiadc_step_config(struct tiadc_device *adc_dev) · line 264

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+	ret = iio_trigger_register(trig);
+	if (ret)

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

Quoted 5 lines
Quoted 2 lines

drivers/iio/adc/ti_am335x_adc.c

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+		return NULL;
+

...
Possible. It's done in the probe section just
to keep error handling bunched there.

Quoted 5 lines

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.

Quoted 12 lines
Quoted 12 lines
Quoted 12 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 398

Quoted 10 lines

static const char * const chan_name_ain[] = {

drivers/iio/adc/ti_am335x_adc.c +2 -1
@@ -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?

Quoted 7 lines
Quoted 2 lines
Quoted 1 line

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 399

Quoted 1 line
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	}

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

#15 lore raw

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

In reply to Zubair Lutfullah

drivers/iio/adc/ti_am335x_a=

Quoted 1 line · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_a= renamed from drivers/iio/adc/ti_am335x_adc.c +0 -0

dc.c

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev)

Quoted 4 lines

index a952538..ae2202b 100644

drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)

=E2=80=A6

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 457

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+err_free_device:
+	iio_device_free(indio_dev);

I am not sure about this one.

Quoted 15 lines
drivers/iio/adc/ti_am335x_adc.c +1 -0
+err_ret:
 	return err;
 }

=20

drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -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

drivers/iio/adc/ti_am335x_adc.c +1 -0
+	free_irq(adc_dev->irq, indio_dev);

	iio_device_unregister(indio_dev);
drivers/iio/adc/ti_am335x_adc.c +1 -0
+	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);

drivers/iio/adc/ti_am335x_adc.c

Quoted 1 line
drivers/iio/adc/ti_am335x_adc.c +1 -0
+	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.

Quoted 3 lines

=20

	return 0;
}

Sebastian

author
#16 lore raw

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

Quoted 8 lines · Sebastian Andrzej Siewior
Quoted 5 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644
@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)

…

Quoted 4 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 457

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+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.

Quoted 14 lines
Quoted 14 lines

+err_ret:

	return err;
}
drivers/iio/adc/ti_am335x_adc.c +2 -0
@@ -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);
Quoted 7 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_remove(struct platform_device *pdev) · line 475

Quoted 3 lines
drivers/iio/adc/ti_am335x_adc.c +1 -0
 	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..

Quoted 5 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_remove(struct platform_device *pdev) · line 477

Quoted 3 lines
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	return 0;
 }

Sebastian

#17 lore raw

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

Quoted 14 lines · Zubair Lutfullah
Quoted 8 lines · Sebastian Andrzej Siewior
Quoted 5 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644
@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)

…

Quoted 4 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 457

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+err_free_device:
+	iio_device_free(indio_dev);

I am not sure about this one.

Quoted 4 lines

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.

Quoted 20 lines
Quoted 13 lines
Quoted 13 lines

+err_ret:

	return err;
}
drivers/iio/adc/ti_am335x_adc.c +2 -0
@@ -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);
Quoted 7 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_remove(struct platform_device *pdev) · line 475

Quoted 3 lines
drivers/iio/adc/ti_am335x_adc.c +1 -0
 	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.

Quoted 8 lines

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.

Quoted 3 lines
Quoted 3 lines

drivers/iio/adc/ti_am335x_adc.c static int tiadc_remove(struct platform_device *pdev) · line 477

Quoted 3 lines
drivers/iio/adc/ti_am335x_adc.c +0 -0
 	return 0;
 }

Sebastian

author
#18 lore raw

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

Quoted 15 lines · Sebastian Andrzej Siewior
Quoted 14 lines · Zubair Lutfullah
Quoted 12 lines · Sebastian Andrzej Siewior
Quoted 5 lines · * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
drivers/iio/adc/ti_am335x_adc.c +0 -0
index a952538..ae2202b 100644
@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)

…

drivers/iio/adc/ti_am335x_adc.c static int tiadc_probe(struct platform_device *pdev) · line 457

Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +2 -0
+err_free_device:
+	iio_device_free(indio_dev);

I am not sure about this one.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iio/adc/ti_am335x_adc.c +0 -0
@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);

...

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
drivers/iio/adc/ti_am335x_adc.c +1 -0
+	iio_device_free(indio_dev);

...

Quoted 3 lines

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

#19 lore raw

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

Quoted 8 lines · Zubair Lutfullah
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
(quoted diff) +1 -0
+	iio_device_free(indio_dev);

...

Quoted 3 lines

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.

Quoted 3 lines

ZubairLK

Sebastian