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

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

[Zubair Lutfullah]

Hi,

These apply to togreg branch in iio.

Round 10 updates
- Removed devm_free as not needed

Round 9 updates

- Removed Trigger.
- Restructured everything for INDIO_BUFFERED_HARDWARE mode.
- Threaded IRQ with top/bottom halfs. Top half HW IRQ wakes
bottom half threaded worker that pushes samples to iio/userspace.
- Removed some of the patchwork to the driver that was irrelevant to the
continuous sampling patch.

Cleanup will be done separately once this goes through.
Hope these patches meet the mighty kernel standards :)

Zubair

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           |  216 ++++++++++++++++++++++++++++-
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +-
 include/linux/mfd/ti_am335x_tscadc.h      |    9 ++
 3 files changed, 229 insertions(+), 8 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/input/touchscreen/ti_am335x_tsc.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
 	u32			config_inp[4];
 	u32			bit_xp, bit_xn, bit_yp, bit_yn;
 	u32			inp_xp, inp_xn, inp_yp, inp_yn;
+	u32			step_mask;
 };
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	/* The steps1 … end and bit 0 for TS_Charge */
 	stepenable = (1 << (end_step + 2)) - 1;
-	am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
+	ts_dev->step_mask = stepenable;
+	am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	if (irqclr) {
 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
-		am335x_tsc_se_update(ts_dev->mfd_tscadc);
+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 		return IRQ_HANDLED;
 	}
 	return IRQ_NONE;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

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

[Zubair Lutfullah]

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

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

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

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

diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index e1c5300..24e625c 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -52,6 +52,7 @@ struct titsc {
 	u32			config_inp[4];
 	u32			bit_xp, bit_xn, bit_yp, bit_yn;
 	u32			inp_xp, inp_xn, inp_yp, inp_yn;
+	u32			step_mask;
 };
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
@@ -196,7 +197,8 @@ static void titsc_step_config(struct titsc *ts_dev)
 
 	/* The steps1 … end and bit 0 for TS_Charge */
 	stepenable = (1 << (end_step + 2)) - 1;
-	am335x_tsc_se_set(ts_dev->mfd_tscadc, stepenable);
+	ts_dev->step_mask = stepenable;
+	am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -260,6 +262,10 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 	unsigned int fsm;
 
 	status = titsc_readl(ts_dev, REG_IRQSTATUS);
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO1 interrupts are used by ADC. Handle FIFO0 IRQs here only
+	 */
 	if (status & IRQENB_FIFO0THRES) {
 
 		titsc_read_coordinates(ts_dev, &x, &y, &z1, &z2);
@@ -316,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	if (irqclr) {
 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
-		am335x_tsc_se_update(ts_dev->mfd_tscadc);
+		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 		return IRQ_HANDLED;
 	}
 	return IRQ_NONE;
@@ -389,7 +395,7 @@ static int titsc_probe(struct platform_device *pdev)
 	}
 
 	err = request_irq(ts_dev->irq, titsc_irq,
-			  0, pdev->dev.driver->name, ts_dev);
+			  IRQF_SHARED, pdev->dev.driver->name, ts_dev);
 	if (err) {
 		dev_err(&pdev->dev, "failed to allocate irq.\n");
 		goto err_free_mem;
-- 
1.7.9.5

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

[Zubair Lutfullah]

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

Continuous sampling starts when buffer is enabled.
HW IRQ wakes worker thread that pushes samples to userspace.
Sampling stops when buffer is disabled by userspace.

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 INDIO_BUFFER_HARDWARE mode.

Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c      |  216 +++++++++++++++++++++++++++++++++-
 include/linux/mfd/ti_am335x_tscadc.h |    9 ++
 2 files changed, 220 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a952538..b4b2ea0 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -28,12 +28,19 @@
 #include <linux/iio/driver.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.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 buffer_en_ch_steps;
+	u32 *data;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
 	return step_en;
 }
 
-static void tiadc_step_config(struct tiadc_device *adc_dev)
+static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
 {
+	return 1 << adc_dev->channel_step[chan];
+}
+
+static void tiadc_step_config(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	unsigned int stepconfig;
 	int i, steps;
 
@@ -72,7 +85,11 @@ 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;
@@ -85,7 +102,177 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 		adc_dev->channel_step[i] = steps;
 		steps++;
 	}
+}
+
+static irqreturn_t tiadc_irq_h(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	unsigned int status, config;
+	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
+
+	/*
+	 * ADC and touchscreen share the IRQ line.
+	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
+	 */
+	if (status & IRQENB_FIFO1OVRRUN) {
+		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
+		config = tiadc_readl(adc_dev, REG_CTRL);
+		config &= ~(CNTRLREG_TSCSSENB);
+		tiadc_writel(adc_dev, REG_CTRL, config);
+		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
+				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
+		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+		return IRQ_HANDLED;
+	} else if (status & IRQENB_FIFO1THRES) {
+		/* Disable irq and wake worker thread */
+		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t tiadc_worker_h(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	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);
+
+	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. Needed in corner cases in simultaneous tsc/adc use */
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (i = 0; i < fifo1count; i++)
+		read = tiadc_readl(adc_dev, REG_FIFO1);
+
+	return iio_sw_buffer_preenable(indio_dev);
+}
+
+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct iio_buffer *buffer = indio_dev->buffer;
+	unsigned int enb = 0;
+	u8 bit;
+
+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
+	if (adc_dev->data == NULL)
+		return -ENOMEM;
+
+	tiadc_step_config(indio_dev);
+	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
+	adc_dev->buffer_en_ch_steps = enb;
+
+	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
+
+	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
+				| IRQENB_FIFO1OVRRUN);
+
+	return 0;
+}
+
+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 leftover data in the time it takes to disable adc */
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	for (i = 0; i < fifo1count; i++)
+		read = tiadc_readl(adc_dev, REG_FIFO1);
+
+	return 0;
+}
+
+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,
+};
+
+int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
+	irqreturn_t (*pollfunc_bh)(int irq, void *p),
+	irqreturn_t (*pollfunc_th)(int irq, void *p),
+	int irq,
+	unsigned long flags,
+	const struct iio_buffer_setup_ops *setup_ops)
+{
+	int ret;
+
+	indio_dev->buffer = iio_kfifo_allocate(indio_dev);
+	if (!indio_dev->buffer)
+		return -ENOMEM;
+
+	ret = devm_request_threaded_irq(indio_dev->dev.parent,
+				irq,
+				pollfunc_th, pollfunc_bh,
+				flags, indio_dev->name,
+				indio_dev);
+	if (ret)
+		goto error_kfifo_free;
+
+	indio_dev->setup_ops = setup_ops;
+	indio_dev->modes |= INDIO_BUFFER_HARDWARE;
+
+	ret = iio_buffer_register(indio_dev,
+				  indio_dev->channels,
+				  indio_dev->num_channels);
+	if (ret)
+		goto error_kfifo_free;
+
+	return 0;
+
+error_kfifo_free:
+	iio_kfifo_free(indio_dev->buffer);
+	return ret;
+}
+
+static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
+{
+	iio_kfifo_free(indio_dev->buffer);
+	iio_buffer_unregister(indio_dev);
 }
 
 static const char * const chan_name_ain[] = {
@@ -120,6 +307,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;
@@ -147,6 +335,10 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	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);
 
@@ -237,20 +429,33 @@ static int tiadc_probe(struct platform_device *pdev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &tiadc_info;
 
-	tiadc_step_config(adc_dev);
+	tiadc_step_config(indio_dev);
+	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
 
 	err = tiadc_channel_init(indio_dev, adc_dev->channels);
 	if (err < 0)
 		return err;
 
-	err = iio_device_register(indio_dev);
+	err = tiadc_iio_buffered_hardware_setup(indio_dev,
+		&tiadc_worker_h,
+		&tiadc_irq_h,
+		adc_dev->mfd_tscadc->irq,
+		IRQF_SHARED,
+		&tiadc_buffer_setup_ops);
+
 	if (err)
 		goto err_free_channels;
 
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto err_buffer_unregister;
+
 	platform_set_drvdata(pdev, indio_dev);
 
 	return 0;
 
+err_buffer_unregister:
+	tiadc_iio_buffered_hardware_remove(indio_dev);
 err_free_channels:
 	tiadc_channels_remove(indio_dev);
 	return err;
@@ -263,6 +468,7 @@ static int tiadc_remove(struct platform_device *pdev)
 	u32 step_en;
 
 	iio_device_unregister(indio_dev);
+	tiadc_iio_buffered_hardware_remove(indio_dev);
 	tiadc_channels_remove(indio_dev);
 
 	step_en = get_adc_step_mask(adc_dev);
@@ -301,7 +507,7 @@ static int tiadc_resume(struct device *dev)
 	restore &= ~(CNTRLREG_POWERDOWN);
 	tiadc_writel(adc_dev, REG_CTRL, restore);
 
-	tiadc_step_config(adc_dev);
+	tiadc_step_config(indio_dev);
 
 	return 0;
 }
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index db1791b..7d98562 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -46,16 +46,24 @@
 /* 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_SWCNT	STEPCONFIG_MODE(1)
 #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
 #define STEPCONFIG_AVG_MASK	(7 << 2)
 #define STEPCONFIG_AVG(val)	((val) << 2)
@@ -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
-- 
1.7.9.5

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

[Jonathan Cameron]

On 18/09/13 12:23, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds continuous sampling support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> HW IRQ wakes worker thread that pushes samples to userspace.
> Sampling stops when buffer is disabled by userspace.
>
> 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 INDIO_BUFFER_HARDWARE mode.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Russ Dill <Russ.Dill-l0cyMroinI0@public.gmane.org>
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

Very very nearly there. Couple of suggestions in-line.
(about 30 seconds work + testing ;)

I'm still unsure of why we need 32bit storage in the fifo
for the data.  I've proposed the changes I'd make to put it in 16 bit
storage inline.  The fact that the device is working in 32bits
is irrelevant given we only have a 12 bit number coming out and
it is in 12 least significant bits.  I guess there might be a tiny
cost in doing the conversion, but you kfifo will be half the size
as a result and that seems more likely to a worthwhile gain!

Out of interest, are you testing this with generic_buffer.c?
If so, what changes were necessary?  Given this driver will not
have a trigger it would be nice to update that example code to handle
that case if it doesn't already.

Thanks,

Jonathan


> ---
>   drivers/iio/adc/ti_am335x_adc.c      |  216 +++++++++++++++++++++++++++++++++-
>   include/linux/mfd/ti_am335x_tscadc.h |    9 ++
>   2 files changed, 220 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..b4b2ea0 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -28,12 +28,19 @@
>   #include <linux/iio/driver.h>
>
>   #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.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 buffer_en_ch_steps;
> +	u32 *data;
u16 *data;

Also it might actually save memory to simply have a fixed size array of 
the maximum size used here and avoid the extra allocations for the cost
of 16 bytes in here.

Hence,

u16 data[8];
>   };
>
>   static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
>   	return step_en;
>   }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>   {
> +	return 1 << adc_dev->channel_step[chan];
> +}
> +
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>   	unsigned int stepconfig;
>   	int i, steps;
>
> @@ -72,7 +85,11 @@ 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;
> @@ -85,7 +102,177 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>   		adc_dev->channel_step[i] = steps;
>   		steps++;
>   	}
> +}
> +
> +static irqreturn_t tiadc_irq_h(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int status, config;
> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> +	 */
> +	if (status & IRQENB_FIFO1OVRRUN) {
> +		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> +		config = tiadc_readl(adc_dev, REG_CTRL);
> +		config &= ~(CNTRLREG_TSCSSENB);
> +		tiadc_writel(adc_dev, REG_CTRL, config);
> +		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
> +				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> +		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> +		return IRQ_HANDLED;
> +	} else if (status & IRQENB_FIFO1THRES) {
> +		/* Disable irq and wake worker thread */
> +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
I guess this is necessary given the sharing will make IRQF_ONESHOT
tricky. As disabling the source of the interrupts is nice and
easy here this will do the job.
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t tiadc_worker_h(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	int i, k, fifo1count, read;
> +	u32 *data = adc_dev->data;
	u16* 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;
//This is a 12 bit number after the mask so will fit just fine into 16 bits.
> +		}
> +		iio_push_to_buffers(indio_dev, (u8 *) data);
> +	}
>
> +	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> +	tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> +	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. Needed in corner cases in simultaneous tsc/adc use */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	unsigned int enb = 0;
> +	u8 bit;
> +
(can drop this if doing the array with adc_dev as suggested above)
> +	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	if (adc_dev->data == NULL)
> +		return -ENOMEM;
> +
> +	tiadc_step_config(indio_dev);
> +	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> +		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> +	adc_dev->buffer_en_ch_steps = enb;
> +
> +	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> +	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> +	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN);
> +
> +	return 0;
> +}
> +
> +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 leftover data in the time it takes to disable adc */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	return 0;
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +
> +	tiadc_step_config(indio_dev);
(can drop this if doing the array withing adc_dev as suggested above)
> +	kfree(adc_dev->data);
This is also missbalanced with the preenable (which is true of quite
a few drivers - one day I'll clean those up!)
> +
> +	return 0;
> +}

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

[Jonathan Cameron]

On 18/09/13 12:23, Zubair Lutfullah wrote:
> Previously the driver had only one-shot reading functionality.
> This patch adds continuous sampling support to the driver.
>
> Continuous sampling starts when buffer is enabled.
> HW IRQ wakes worker thread that pushes samples to userspace.
> Sampling stops when buffer is disabled by userspace.
>
> 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 INDIO_BUFFER_HARDWARE mode.
>
> Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Russ Dill <Russ.Dill@ti.com>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Very very nearly there. Couple of suggestions in-line.
(about 30 seconds work + testing ;)

I'm still unsure of why we need 32bit storage in the fifo
for the data.  I've proposed the changes I'd make to put it in 16 bit
storage inline.  The fact that the device is working in 32bits
is irrelevant given we only have a 12 bit number coming out and
it is in 12 least significant bits.  I guess there might be a tiny
cost in doing the conversion, but you kfifo will be half the size
as a result and that seems more likely to a worthwhile gain!

Out of interest, are you testing this with generic_buffer.c?
If so, what changes were necessary?  Given this driver will not
have a trigger it would be nice to update that example code to handle
that case if it doesn't already.

Thanks,

Jonathan


> ---
>   drivers/iio/adc/ti_am335x_adc.c      |  216 +++++++++++++++++++++++++++++++++-
>   include/linux/mfd/ti_am335x_tscadc.h |    9 ++
>   2 files changed, 220 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index a952538..b4b2ea0 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -28,12 +28,19 @@
>   #include <linux/iio/driver.h>
>
>   #include <linux/mfd/ti_am335x_tscadc.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.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 buffer_en_ch_steps;
> +	u32 *data;
u16 *data;

Also it might actually save memory to simply have a fixed size array of 
the maximum size used here and avoid the extra allocations for the cost
of 16 bytes in here.

Hence,

u16 data[8];
>   };
>
>   static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -56,8 +63,14 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
>   	return step_en;
>   }
>
> -static void tiadc_step_config(struct tiadc_device *adc_dev)
> +static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>   {
> +	return 1 << adc_dev->channel_step[chan];
> +}
> +
> +static void tiadc_step_config(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>   	unsigned int stepconfig;
>   	int i, steps;
>
> @@ -72,7 +85,11 @@ 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;
> @@ -85,7 +102,177 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>   		adc_dev->channel_step[i] = steps;
>   		steps++;
>   	}
> +}
> +
> +static irqreturn_t tiadc_irq_h(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	unsigned int status, config;
> +	status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> +
> +	/*
> +	 * ADC and touchscreen share the IRQ line.
> +	 * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
> +	 */
> +	if (status & IRQENB_FIFO1OVRRUN) {
> +		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
> +		config = tiadc_readl(adc_dev, REG_CTRL);
> +		config &= ~(CNTRLREG_TSCSSENB);
> +		tiadc_writel(adc_dev, REG_CTRL, config);
> +		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
> +				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
> +		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
> +		return IRQ_HANDLED;
> +	} else if (status & IRQENB_FIFO1THRES) {
> +		/* Disable irq and wake worker thread */
> +		tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
I guess this is necessary given the sharing will make IRQF_ONESHOT
tricky. As disabling the source of the interrupts is nice and
easy here this will do the job.
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static irqreturn_t tiadc_worker_h(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	int i, k, fifo1count, read;
> +	u32 *data = adc_dev->data;
	u16* 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;
//This is a 12 bit number after the mask so will fit just fine into 16 bits.
> +		}
> +		iio_push_to_buffers(indio_dev, (u8 *) data);
> +	}
>
> +	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
> +	tiadc_writel(adc_dev, REG_IRQENABLE, IRQENB_FIFO1THRES);
> +
> +	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. Needed in corner cases in simultaneous tsc/adc use */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	return iio_sw_buffer_preenable(indio_dev);
> +}
> +
> +static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct iio_buffer *buffer = indio_dev->buffer;
> +	unsigned int enb = 0;
> +	u8 bit;
> +
(can drop this if doing the array with adc_dev as suggested above)
> +	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> +	if (adc_dev->data == NULL)
> +		return -ENOMEM;
> +
> +	tiadc_step_config(indio_dev);
> +	for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
> +		enb |= (get_adc_step_bit(adc_dev, bit) << 1);
> +	adc_dev->buffer_en_ch_steps = enb;
> +
> +	am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
> +
> +	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> +	tiadc_writel(adc_dev,  REG_IRQENABLE, IRQENB_FIFO1THRES
> +				| IRQENB_FIFO1OVRRUN);
> +
> +	return 0;
> +}
> +
> +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 leftover data in the time it takes to disable adc */
> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> +	for (i = 0; i < fifo1count; i++)
> +		read = tiadc_readl(adc_dev, REG_FIFO1);
> +
> +	return 0;
> +}
> +
> +static int tiadc_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +
> +	tiadc_step_config(indio_dev);
(can drop this if doing the array withing adc_dev as suggested above)
> +	kfree(adc_dev->data);
This is also missbalanced with the preenable (which is true of quite
a few drivers - one day I'll clean those up!)
> +
> +	return 0;
> +}

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

[Zubair Lutfullah :]

On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
> On 18/09/13 12:23, Zubair Lutfullah wrote:
> >Previously the driver had only one-shot reading functionality.
> >This patch adds continuous sampling support to the driver.
> >
...
> 
> Very very nearly there. Couple of suggestions in-line.
> (about 30 seconds work + testing ;)

Thank-you so much. Makes me want to put in more work and finish it :)

> 
> I'm still unsure of why we need 32bit storage in the fifo
> for the data.  I've proposed the changes I'd make to put it in 16 bit
> storage inline.  The fact that the device is working in 32bits
> is irrelevant given we only have a 12 bit number coming out and
> it is in 12 least significant bits.  I guess there might be a tiny
> cost in doing the conversion, but you kfifo will be half the size
> as a result and that seems more likely to a worthwhile gain!
> 

I understand and will make the changes.

> Out of interest, are you testing this with generic_buffer.c?
> If so, what changes were necessary?  Given this driver will not
> have a trigger it would be nice to update that example code to handle
> that case if it doesn't already.

I simply remove the lines like goto err_trigger etc. :p
Sneaky but works. I wasn't sure how to make the code understand its a 
INDIO_BUFFER_HARDWARE..
But this is a separate discussion..

> 
> 
> >---
> >  drivers/iio/adc/ti_am335x_adc.c      |  216 +++++++++++++++++++++++++++++++++-
> >  include/linux/mfd/ti_am335x_tscadc.h |    9 ++
> >  2 files changed, 220 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
...
> >
> >  struct tiadc_device {
> >  	struct ti_tscadc_dev *mfd_tscadc;
> >  	int channels;
> >  	u8 channel_line[8];
> >  	u8 channel_step[8];
> >+	int buffer_en_ch_steps;
> >+	u32 *data;
> u16 *data;
> 
> Also it might actually save memory to simply have a fixed size array
> of the maximum size used here and avoid the extra allocations for
> the cost
> of 16 bytes in here.
> 
> Hence,
> 
> u16 data[8];
> >  };

Why data[8]? The length is dynamic. 
This would be valid for the usual one sample per trigger case.
But here its continuous sampling and the hardware pushes samples
*quickly*
Dynamic allocation is needed.


...

> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
> >+{
> >+	struct iio_dev *indio_dev = private;
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	int i, k, fifo1count, read;
> >+	u32 *data = adc_dev->data;
> 	u16* 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;
> //This is a 12 bit number after the mask so will fit just fine into 16 bits.

Indeed
...
> >+
> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	struct iio_buffer *buffer = indio_dev->buffer;
> >+	unsigned int enb = 0;
> >+	u8 bit;
> >+
> (can drop this if doing the array with adc_dev as suggested above)
> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >+	if (adc_dev->data == NULL)
> >+		return -ENOMEM;

As explained previously. Large array.. This is needed..

...
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+
> >+	tiadc_step_config(indio_dev);
> (can drop this if doing the array withing adc_dev as suggested above)
> >+	kfree(adc_dev->data);
> This is also missbalanced with the preenable (which is true of quite
> a few drivers - one day I'll clean those up!)

Misbalanced? :s

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

Thanks for the review and feedback.
I'll resend the patches with 16 bit everything and dynamic allocation.

Zubair
> 

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

[Zubair Lutfullah :]

On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
> On 18/09/13 12:23, Zubair Lutfullah wrote:
> >Previously the driver had only one-shot reading functionality.
> >This patch adds continuous sampling support to the driver.
> >
...
> 
> Very very nearly there. Couple of suggestions in-line.
> (about 30 seconds work + testing ;)

Thank-you so much. Makes me want to put in more work and finish it :)

> 
> I'm still unsure of why we need 32bit storage in the fifo
> for the data.  I've proposed the changes I'd make to put it in 16 bit
> storage inline.  The fact that the device is working in 32bits
> is irrelevant given we only have a 12 bit number coming out and
> it is in 12 least significant bits.  I guess there might be a tiny
> cost in doing the conversion, but you kfifo will be half the size
> as a result and that seems more likely to a worthwhile gain!
> 

I understand and will make the changes.

> Out of interest, are you testing this with generic_buffer.c?
> If so, what changes were necessary?  Given this driver will not
> have a trigger it would be nice to update that example code to handle
> that case if it doesn't already.

I simply remove the lines like goto err_trigger etc. :p
Sneaky but works. I wasn't sure how to make the code understand its a 
INDIO_BUFFER_HARDWARE..
But this is a separate discussion..

> 
> 
> >---
> >  drivers/iio/adc/ti_am335x_adc.c      |  216 +++++++++++++++++++++++++++++++++-
> >  include/linux/mfd/ti_am335x_tscadc.h |    9 ++
> >  2 files changed, 220 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
...
> >
> >  struct tiadc_device {
> >  	struct ti_tscadc_dev *mfd_tscadc;
> >  	int channels;
> >  	u8 channel_line[8];
> >  	u8 channel_step[8];
> >+	int buffer_en_ch_steps;
> >+	u32 *data;
> u16 *data;
> 
> Also it might actually save memory to simply have a fixed size array
> of the maximum size used here and avoid the extra allocations for
> the cost
> of 16 bytes in here.
> 
> Hence,
> 
> u16 data[8];
> >  };

Why data[8]? The length is dynamic. 
This would be valid for the usual one sample per trigger case.
But here its continuous sampling and the hardware pushes samples
*quickly*
Dynamic allocation is needed.


...

> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
> >+{
> >+	struct iio_dev *indio_dev = private;
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	int i, k, fifo1count, read;
> >+	u32 *data = adc_dev->data;
> 	u16* 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;
> //This is a 12 bit number after the mask so will fit just fine into 16 bits.

Indeed
...
> >+
> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+	struct iio_buffer *buffer = indio_dev->buffer;
> >+	unsigned int enb = 0;
> >+	u8 bit;
> >+
> (can drop this if doing the array with adc_dev as suggested above)
> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >+	if (adc_dev->data == NULL)
> >+		return -ENOMEM;

As explained previously. Large array.. This is needed..

...
> >+{
> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+
> >+	tiadc_step_config(indio_dev);
> (can drop this if doing the array withing adc_dev as suggested above)
> >+	kfree(adc_dev->data);
> This is also missbalanced with the preenable (which is true of quite
> a few drivers - one day I'll clean those up!)

Misbalanced? :s

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

Thanks for the review and feedback.
I'll resend the patches with 16 bit everything and dynamic allocation.

Zubair
> 

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

[Jonathan Cameron]

"Zubair Lutfullah :" <zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
>> On 18/09/13 12:23, Zubair Lutfullah wrote:
>> >Previously the driver had only one-shot reading functionality.
>> >This patch adds continuous sampling support to the driver.
>> >
>...
>> 
>> Very very nearly there. Couple of suggestions in-line.
>> (about 30 seconds work + testing ;)
>
>Thank-you so much. Makes me want to put in more work and finish it :)
>
>> 
>> I'm still unsure of why we need 32bit storage in the fifo
>> for the data.  I've proposed the changes I'd make to put it in 16 bit
>> storage inline.  The fact that the device is working in 32bits
>> is irrelevant given we only have a 12 bit number coming out and
>> it is in 12 least significant bits.  I guess there might be a tiny
>> cost in doing the conversion, but you kfifo will be half the size
>> as a result and that seems more likely to a worthwhile gain!
>> 
>
>I understand and will make the changes.
>
>> Out of interest, are you testing this with generic_buffer.c?
>> If so, what changes were necessary?  Given this driver will not
>> have a trigger it would be nice to update that example code to handle
>> that case if it doesn't already.
>
>I simply remove the lines like goto err_trigger etc. :p
>Sneaky but works. I wasn't sure how to make the code understand its a 
>INDIO_BUFFER_HARDWARE..
>But this is a separate discussion..
>
>> 
>> 
>> >---
>> >  drivers/iio/adc/ti_am335x_adc.c      |  216
>+++++++++++++++++++++++++++++++++-
>> >  include/linux/mfd/ti_am335x_tscadc.h |    9 ++
>> >  2 files changed, 220 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/drivers/iio/adc/ti_am335x_adc.c
>b/drivers/iio/adc/ti_am335x_adc.c
>...
>> >
>> >  struct tiadc_device {
>> >  	struct ti_tscadc_dev *mfd_tscadc;
>> >  	int channels;
>> >  	u8 channel_line[8];
>> >  	u8 channel_step[8];
>> >+	int buffer_en_ch_steps;
>> >+	u32 *data;
>> u16 *data;
>> 
>> Also it might actually save memory to simply have a fixed size array
>> of the maximum size used here and avoid the extra allocations for
>> the cost
>> of 16 bytes in here.
>> 
>> Hence,
>> 
>> u16 data[8];
>> >  };
>
>Why data[8]? The length is dynamic. 
>This would be valid for the usual one sample per trigger case.
>But here its continuous sampling and the hardware pushes samples
>*quickly*
>Dynamic allocation is needed.

As far as I can see you pull one set of channels into data[] then push that out to the kfifo. 

That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?

>
>
>...
>
>> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
>> >+{
>> >+	struct iio_dev *indio_dev = private;
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	int i, k, fifo1count, read;
>> >+	u32 *data = adc_dev->data;
>> 	u16* 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;

i is only ever up to scanbytes / 4.
Hence max of 8 I think?

Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
>> //This is a 12 bit number after the mask so will fit just fine into
>16 bits.
>
>Indeed
>...
>> >+
>> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	struct iio_buffer *buffer = indio_dev->buffer;
>> >+	unsigned int enb = 0;
>> >+	u8 bit;
>> >+
>> (can drop this if doing the array with adc_dev as suggested above)
>> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> >+	if (adc_dev->data == NULL)
>> >+		return -ENOMEM;
>
>As explained previously. Large array.. This is needed..
>
>...
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+
>> >+	tiadc_step_config(indio_dev);
>> (can drop this if doing the array withing adc_dev as suggested above)
>> >+	kfree(adc_dev->data);
>> This is also missbalanced with the preenable (which is true of quite
>> a few drivers - one day I'll clean those up!)
>
>Misbalanced? :s
>
>> >+
>> >+	return 0;
>> >+}
>> 
>
>Thanks for the review and feedback.
>I'll resend the patches with 16 bit everything and dynamic allocation.
>
>Zubair
>> 

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

[Jonathan Cameron]

"Zubair Lutfullah :" <zubair.lutfullah@gmail.com> wrote:
>On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
>> On 18/09/13 12:23, Zubair Lutfullah wrote:
>> >Previously the driver had only one-shot reading functionality.
>> >This patch adds continuous sampling support to the driver.
>> >
>...
>> 
>> Very very nearly there. Couple of suggestions in-line.
>> (about 30 seconds work + testing ;)
>
>Thank-you so much. Makes me want to put in more work and finish it :)
>
>> 
>> I'm still unsure of why we need 32bit storage in the fifo
>> for the data.  I've proposed the changes I'd make to put it in 16 bit
>> storage inline.  The fact that the device is working in 32bits
>> is irrelevant given we only have a 12 bit number coming out and
>> it is in 12 least significant bits.  I guess there might be a tiny
>> cost in doing the conversion, but you kfifo will be half the size
>> as a result and that seems more likely to a worthwhile gain!
>> 
>
>I understand and will make the changes.
>
>> Out of interest, are you testing this with generic_buffer.c?
>> If so, what changes were necessary?  Given this driver will not
>> have a trigger it would be nice to update that example code to handle
>> that case if it doesn't already.
>
>I simply remove the lines like goto err_trigger etc. :p
>Sneaky but works. I wasn't sure how to make the code understand its a 
>INDIO_BUFFER_HARDWARE..
>But this is a separate discussion..
>
>> 
>> 
>> >---
>> >  drivers/iio/adc/ti_am335x_adc.c      |  216
>+++++++++++++++++++++++++++++++++-
>> >  include/linux/mfd/ti_am335x_tscadc.h |    9 ++
>> >  2 files changed, 220 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/drivers/iio/adc/ti_am335x_adc.c
>b/drivers/iio/adc/ti_am335x_adc.c
>...
>> >
>> >  struct tiadc_device {
>> >  	struct ti_tscadc_dev *mfd_tscadc;
>> >  	int channels;
>> >  	u8 channel_line[8];
>> >  	u8 channel_step[8];
>> >+	int buffer_en_ch_steps;
>> >+	u32 *data;
>> u16 *data;
>> 
>> Also it might actually save memory to simply have a fixed size array
>> of the maximum size used here and avoid the extra allocations for
>> the cost
>> of 16 bytes in here.
>> 
>> Hence,
>> 
>> u16 data[8];
>> >  };
>
>Why data[8]? The length is dynamic. 
>This would be valid for the usual one sample per trigger case.
>But here its continuous sampling and the hardware pushes samples
>*quickly*
>Dynamic allocation is needed.

As far as I can see you pull one set of channels into data[] then push that out to the kfifo. 

That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?

>
>
>...
>
>> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
>> >+{
>> >+	struct iio_dev *indio_dev = private;
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	int i, k, fifo1count, read;
>> >+	u32 *data = adc_dev->data;
>> 	u16* 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;

i is only ever up to scanbytes / 4.
Hence max of 8 I think?

Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
>> //This is a 12 bit number after the mask so will fit just fine into
>16 bits.
>
>Indeed
>...
>> >+
>> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	struct iio_buffer *buffer = indio_dev->buffer;
>> >+	unsigned int enb = 0;
>> >+	u8 bit;
>> >+
>> (can drop this if doing the array with adc_dev as suggested above)
>> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> >+	if (adc_dev->data == NULL)
>> >+		return -ENOMEM;
>
>As explained previously. Large array.. This is needed..
>
>...
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+
>> >+	tiadc_step_config(indio_dev);
>> (can drop this if doing the array withing adc_dev as suggested above)
>> >+	kfree(adc_dev->data);
>> This is also missbalanced with the preenable (which is true of quite
>> a few drivers - one day I'll clean those up!)
>
>Misbalanced? :s
>
>> >+
>> >+	return 0;
>> >+}
>> 
>
>Thanks for the review and feedback.
>I'll resend the patches with 16 bit everything and dynamic allocation.
>
>Zubair
>> 

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

[Jonathan Cameron]

"Zubair Lutfullah :" <zubair.lutfullah@gmail.com> wrote:
>On Wed, Sep 18, 2013 at 02:58:47PM +0100, Jonathan Cameron wrote:
>> On 18/09/13 12:23, Zubair Lutfullah wrote:
>> >Previously the driver had only one-shot reading functionality.
>> >This patch adds continuous sampling support to the driver.
>> >
>...
>> 
>> Very very nearly there. Couple of suggestions in-line.
>> (about 30 seconds work + testing ;)
>
>Thank-you so much. Makes me want to put in more work and finish it :)
>
>> 
>> I'm still unsure of why we need 32bit storage in the fifo
>> for the data.  I've proposed the changes I'd make to put it in 16 bit
>> storage inline.  The fact that the device is working in 32bits
>> is irrelevant given we only have a 12 bit number coming out and
>> it is in 12 least significant bits.  I guess there might be a tiny
>> cost in doing the conversion, but you kfifo will be half the size
>> as a result and that seems more likely to a worthwhile gain!
>> 
>
>I understand and will make the changes.
>
>> Out of interest, are you testing this with generic_buffer.c?
>> If so, what changes were necessary?  Given this driver will not
>> have a trigger it would be nice to update that example code to handle
>> that case if it doesn't already.
>
>I simply remove the lines like goto err_trigger etc. :p
>Sneaky but works. I wasn't sure how to make the code understand its a 
>INDIO_BUFFER_HARDWARE..
>But this is a separate discussion..
>
>> 
>> 
>> >---
>> >  drivers/iio/adc/ti_am335x_adc.c      |  216
>+++++++++++++++++++++++++++++++++-
>> >  include/linux/mfd/ti_am335x_tscadc.h |    9 ++
>> >  2 files changed, 220 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/drivers/iio/adc/ti_am335x_adc.c
>b/drivers/iio/adc/ti_am335x_adc.c
>...
>> >
>> >  struct tiadc_device {
>> >  	struct ti_tscadc_dev *mfd_tscadc;
>> >  	int channels;
>> >  	u8 channel_line[8];
>> >  	u8 channel_step[8];
>> >+	int buffer_en_ch_steps;
>> >+	u32 *data;
>> u16 *data;
>> 
>> Also it might actually save memory to simply have a fixed size array
>> of the maximum size used here and avoid the extra allocations for
>> the cost
>> of 16 bytes in here.
>> 
>> Hence,
>> 
>> u16 data[8];
>> >  };
>
>Why data[8]? The length is dynamic. 
>This would be valid for the usual one sample per trigger case.
>But here its continuous sampling and the hardware pushes samples
>*quickly*
>Dynamic allocation is needed.

As far as I can see you pull one set of channels into data[] then push that out to the kfifo. 

That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?

>
>
>...
>
>> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
>> >+{
>> >+	struct iio_dev *indio_dev = private;
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	int i, k, fifo1count, read;
>> >+	u32 *data = adc_dev->data;
>> 	u16* 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;

i is only ever up to scanbytes / 4.
Hence max of 8 I think?

Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
>> //This is a 12 bit number after the mask so will fit just fine into
>16 bits.
>
>Indeed
>...
>> >+
>> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+	struct iio_buffer *buffer = indio_dev->buffer;
>> >+	unsigned int enb = 0;
>> >+	u8 bit;
>> >+
>> (can drop this if doing the array with adc_dev as suggested above)
>> >+	adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> >+	if (adc_dev->data == NULL)
>> >+		return -ENOMEM;
>
>As explained previously. Large array.. This is needed..
>
>...
>> >+{
>> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> >+
>> >+	tiadc_step_config(indio_dev);
>> (can drop this if doing the array withing adc_dev as suggested above)
>> >+	kfree(adc_dev->data);
>> This is also missbalanced with the preenable (which is true of quite
>> a few drivers - one day I'll clean those up!)
>
>Misbalanced? :s
>
>> >+
>> >+	return 0;
>> >+}
>> 
>
>Thanks for the review and feedback.
>I'll resend the patches with 16 bit everything and dynamic allocation.
>
>Zubair
>> 

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

[Zubair Lutfullah :]

On Thu, Sep 19, 2013 at 06:41:22AM +0100, Jonathan Cameron wrote:
> 
> 
> >...
> >> >
> >> >  struct tiadc_device {
> >> >  	struct ti_tscadc_dev *mfd_tscadc;
> >> >  	int channels;
> >> >  	u8 channel_line[8];
> >> >  	u8 channel_step[8];
> >> >+	int buffer_en_ch_steps;
> >> >+	u32 *data;
> >> u16 *data;
> >> 
> >> Also it might actually save memory to simply have a fixed size array
> >> of the maximum size used here and avoid the extra allocations for
> >> the cost
> >> of 16 bytes in here.
> >> 
> >> Hence,
> >> 
> >> u16 data[8];
> >> >  };
> >
> >Why data[8]? The length is dynamic. 
> >This would be valid for the usual one sample per trigger case.
> >But here its continuous sampling and the hardware pushes samples
> >*quickly*
> >Dynamic allocation is needed.
> 
> As far as I can see you pull one set of channels into data[] then push that out to the kfifo. 
> 
> That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?
> 

You have a good eye :).
I understand and will update.

Thanks
Zubair

> >
> >
> >...
> >
> >> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
> >> >+{
> >> >+	struct iio_dev *indio_dev = private;
> >> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >> >+	int i, k, fifo1count, read;
> >> >+	u32 *data = adc_dev->data;
> >> 	u16* 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;
> 
> i is only ever up to scanbytes / 4.
> Hence max of 8 I think?
> 
> Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
> >> //This is a 12 bit number after the mask so will fit just fine into
> >16 bits.
> >

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

[Zubair Lutfullah :]

On Thu, Sep 19, 2013 at 06:41:22AM +0100, Jonathan Cameron wrote:
> 
> 
> >...
> >> >
> >> >  struct tiadc_device {
> >> >  	struct ti_tscadc_dev *mfd_tscadc;
> >> >  	int channels;
> >> >  	u8 channel_line[8];
> >> >  	u8 channel_step[8];
> >> >+	int buffer_en_ch_steps;
> >> >+	u32 *data;
> >> u16 *data;
> >> 
> >> Also it might actually save memory to simply have a fixed size array
> >> of the maximum size used here and avoid the extra allocations for
> >> the cost
> >> of 16 bytes in here.
> >> 
> >> Hence,
> >> 
> >> u16 data[8];
> >> >  };
> >
> >Why data[8]? The length is dynamic. 
> >This would be valid for the usual one sample per trigger case.
> >But here its continuous sampling and the hardware pushes samples
> >*quickly*
> >Dynamic allocation is needed.
> 
> As far as I can see you pull one set of channels into data[] then push that out to the kfifo. 
> 
> That is repeated lots of times but only up to 8 entries are ever used in this array. If not what is the maximum scanbytes can be?
> 

You have a good eye :).
I understand and will update.

Thanks
Zubair

> >
> >
> >...
> >
> >> >+static irqreturn_t tiadc_worker_h(int irq, void *private)
> >> >+{
> >> >+	struct iio_dev *indio_dev = private;
> >> >+	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >> >+	int i, k, fifo1count, read;
> >> >+	u32 *data = adc_dev->data;
> >> 	u16* 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;
> 
> i is only ever up to scanbytes / 4.
> Hence max of 8 I think?
> 
> Now there is a good argument for adding some bulk filling code for the buffers but that is not happening here.
> >> //This is a 12 bit number after the mask so will fit just fine into
> >16 bits.
> >

Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Sebastian Andrzej Siewior]

Hi Zubair,

I have here am335x-evm board. The output of voltage4 (via `cat
/sys/bus/iio/devices/iio\:device0/in_voltage4_raw´) returns values in
the range 570…580. I tested the continuous sampling mdoe by executing
the following commands:

|echo 1 > /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
|echo 25 > /sys/bus/iio/devices/iio\:device0/buffer/length
|echo 1 > /sys/bus/iio/devices/iio\:device0/buffer/enable
|sleep 1
|echo 0 > /sys/bus/iio/devices/iio\:device0/buffer/enable
|cat /dev/iio\:device0 > iio
|hexdump -C iio

and the output is:
|00000000  37 02 92 02 1f 07 0a 06  20 06 31 06 2f 07 2a 07
|00000010  2b 07 2d 07 2c 07 28 07  2c 07 2b 07 2b 07 0c 06
|00000020  21 06 32 06 2e 07 2c 07  2a 07 2a 07 2c 07 2b 07
|00000030  29 07 27 07 28 07 29 07  2a 07 0d 06 21 06 35 06

The first entry is in the range that I would expect. The second is
slitghly higher. The third and following entries are out of range. Have
you observed something like that? Is my testing close to what you have
done or did I make an mistake here?

Sebastian

Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Sebastian Andrzej Siewior]

Hi Zubair,

I have here am335x-evm board. The output of voltage4 (via `cat
/sys/bus/iio/devices/iio\:device0/in_voltage4_raw´) returns values in
the range 570…580. I tested the continuous sampling mdoe by executing
the following commands:

|echo 1 > /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
|echo 25 > /sys/bus/iio/devices/iio\:device0/buffer/length
|echo 1 > /sys/bus/iio/devices/iio\:device0/buffer/enable
|sleep 1
|echo 0 > /sys/bus/iio/devices/iio\:device0/buffer/enable
|cat /dev/iio\:device0 > iio
|hexdump -C iio

and the output is:
|00000000  37 02 92 02 1f 07 0a 06  20 06 31 06 2f 07 2a 07
|00000010  2b 07 2d 07 2c 07 28 07  2c 07 2b 07 2b 07 0c 06
|00000020  21 06 32 06 2e 07 2c 07  2a 07 2a 07 2c 07 2b 07
|00000030  29 07 27 07 28 07 29 07  2a 07 0d 06 21 06 35 06

The first entry is in the range that I would expect. The second is
slitghly higher. The third and following entries are out of range. Have
you observed something like that? Is my testing close to what you have
done or did I make an mistake here?

Sebastian

Re: Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Zubair Lutfullah :]

On Mon, Oct 07, 2013 at 01:53:17PM +0200, Sebastian Andrzej Siewior wrote:
> Hi Zubair,
> 
> I have here am335x-evm board. The output of voltage4 (via `cat
> /sys/bus/iio/devices/iio\:device0/in_voltage4_raw´) returns values in
> the range 570…580. I tested the continuous sampling mdoe by executing
> the following commands:
> 
> |echo 1 > /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
> |echo 25 > /sys/bus/iio/devices/iio\:device0/buffer/length
> |echo 1 > /sys/bus/iio/devices/iio\:device0/buffer/enable
> |sleep 1
> |echo 0 > /sys/bus/iio/devices/iio\:device0/buffer/enable
> |cat /dev/iio\:device0 > iio
> |hexdump -C iio
> 
> and the output is:
> |00000000  37 02 92 02 1f 07 0a 06  20 06 31 06 2f 07 2a 07
> |00000010  2b 07 2d 07 2c 07 28 07  2c 07 2b 07 2b 07 0c 06
> |00000020  21 06 32 06 2e 07 2c 07  2a 07 2a 07 2c 07 2b 07
> |00000030  29 07 27 07 28 07 29 07  2a 07 0d 06 21 06 35 06
> 
> The first entry is in the range that I would expect. The second is
> slitghly higher. The third and following entries are out of range. Have
> you observed something like that? Is my testing close to what you have
> done or did I make an mistake here?

I can't read the hex dump easily..

Keep in mind that the driver now uses 16 bit storage. 

Also, I used generic_buffer.c after removing the trigger checks.

The driver would report stable values for 1.8V on one channel and gnd
on another channel.

Hope this helps
ZubairLK

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

Re: Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Zubair Lutfullah :]

On Mon, Oct 07, 2013 at 01:53:17PM +0200, Sebastian Andrzej Siewior wrote:
> Hi Zubair,
> 
> I have here am335x-evm board. The output of voltage4 (via `cat
> /sys/bus/iio/devices/iio\:device0/in_voltage4_raw´) returns values in
> the range 570…580. I tested the continuous sampling mdoe by executing
> the following commands:
> 
> |echo 1 > /sys/bus/iio/devices/iio\:device0/scan_elements/in_voltage4_en
> |echo 25 > /sys/bus/iio/devices/iio\:device0/buffer/length
> |echo 1 > /sys/bus/iio/devices/iio\:device0/buffer/enable
> |sleep 1
> |echo 0 > /sys/bus/iio/devices/iio\:device0/buffer/enable
> |cat /dev/iio\:device0 > iio
> |hexdump -C iio
> 
> and the output is:
> |00000000  37 02 92 02 1f 07 0a 06  20 06 31 06 2f 07 2a 07
> |00000010  2b 07 2d 07 2c 07 28 07  2c 07 2b 07 2b 07 0c 06
> |00000020  21 06 32 06 2e 07 2c 07  2a 07 2a 07 2c 07 2b 07
> |00000030  29 07 27 07 28 07 29 07  2a 07 0d 06 21 06 35 06
> 
> The first entry is in the range that I would expect. The second is
> slitghly higher. The third and following entries are out of range. Have
> you observed something like that? Is my testing close to what you have
> done or did I make an mistake here?

I can't read the hex dump easily..

Keep in mind that the driver now uses 16 bit storage. 

Also, I used generic_buffer.c after removing the trigger checks.

The driver would report stable values for 1.8V on one channel and gnd
on another channel.

Hope this helps
ZubairLK

> 
> Sebastian

Re: Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Sebastian Andrzej Siewior]

On 10/07/2013 02:50 PM, Zubair Lutfullah : wrote:
>> and the output is:
>> |00000000  37 02 92 02 1f 07 0a 06  20 06 31 06 2f 07 2a 07
>> |00000010  2b 07 2d 07 2c 07 28 07  2c 07 2b 07 2b 07 0c 06
>> |00000020  21 06 32 06 2e 07 2c 07  2a 07 2a 07 2c 07 2b 07
>> |00000030  29 07 27 07 28 07 29 07  2a 07 0d 06 21 06 35 06
>>
>> The first entry is in the range that I would expect. The second is
>> slitghly higher. The third and following entries are out of range. Have
>> you observed something like that? Is my testing close to what you have
>> done or did I make an mistake here?
> 
> I can't read the hex dump easily..

The first one "37 02" => 0x0237 => 567. The next 658 and the following
have mostly a 7 in the upper 8 bit which means the result is 1792+

> Keep in mind that the driver now uses 16 bit storage. 

Yes, I do.

> Also, I used generic_buffer.c after removing the trigger checks.

Do you mind posting it?

> 
> The driver would report stable values for 1.8V on one channel and gnd
> on another channel.

Okay. That sounds good.

> 
> Hope this helps
> ZubairLK


Sebastian

Re: Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Zubair Lutfullah :]

On Mon, Oct 07, 2013 at 02:59:24PM +0200, Sebastian Andrzej Siewior wrote:
> 
> > Also, I used generic_buffer.c after removing the trigger checks.
> 
> Do you mind posting it?
>
Don't have the file handy as its inside the beaglebone black.
And I'm 'technically' on vacation for my wedding :)

If I remember correctly, i simply hacked generic_buffer.c
by commenting trigger related goto error statements.

Hope that helps.
ZubairLK

Re: Testing ti_am335x_adc continuous mode (WAS: Re: [PATCH 2/2]

[Sebastian Andrzej Siewior]

On 10/07/2013 03:18 PM, Zubair Lutfullah : wrote:
> On Mon, Oct 07, 2013 at 02:59:24PM +0200, Sebastian Andrzej Siewior wrote:
>>
>>> Also, I used generic_buffer.c after removing the trigger checks.
>>
>> Do you mind posting it?
>>
> Don't have the file handy as its inside the beaglebone black.
> And I'm 'technically' on vacation for my wedding :)

Ah, okay then. Congratulations to you and your future spouse :)

> If I remember correctly, i simply hacked generic_buffer.c
> by commenting trigger related goto error statements.
> 
> Hope that helps.

yup, should be enough. Thanks.

> ZubairLK

Sebastian