am335x: IIO/ADC fixes if used together with TSC

am335x: IIO/ADC fixes if used together with TSC

[Sebastian Andrzej Siewior]

Hi,

this series got a little bigger than expected with a biggy at the end.
I didn't managed to split the patches across the three subsystems it
touches due to the changes are required to keep it working and break the
logic.
Even in 5/5 which does three logical changes I didn't manage to split it
in three patches because the driver would not be functional (sometimes it would
get worse than before). This is all documented in the changelog. 
Thefore I ask to get this series applied via one tree, the iio tree which contains
or depends on the most changes.

The two bigger issues here are fixed by the last patch incuding
- time outs on the iio/adc side while TSC&ADC is used together
- a lockup issue which occurs while TSC&ADC is used together.

Sebastian

[PATCH 1/5] iio: ti_am335x_adc: adjust the closing bracket in tiadc_read_raw()

[Sebastian Andrzej Siewior]

It somehow looks like the ending bracket belongs to the if statement but
it does belong to the while loop. This patch moves the bracket where it
belongs.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index d4d7482..e948454 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -341,7 +341,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
 		if (time_after(jiffies, timeout))
 			return -EAGAIN;
-		}
+	}
 	map_val = chan->channel + TOTAL_CHANNELS;
 
 	/*
-- 
1.8.5.1

Re: [PATCH 1/5] iio: ti_am335x_adc: adjust the closing bracket in tiadc_read_raw()

[Lee Jones]

On Wed, 18 Dec 2013, Sebastian Andrzej Siewior wrote:

> It somehow looks like the ending bracket belongs to the if statement but
> it does belong to the while loop. This patch moves the bracket where it
> belongs.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] mfd: ti_am335x_tscadc: make am335x_tsc_se_update() local

[Sebastian Andrzej Siewior]

since the "recent" changes, am335x_tsc_se_update() has no longer any
users outside of this file so make it local.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mfd/ti_am335x_tscadc.c       | 3 +--
 include/linux/mfd/ti_am335x_tscadc.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 88718ab..67d0eb4 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -48,11 +48,10 @@ static const struct regmap_config tscadc_regmap_config = {
 	.val_bits = 32,
 };
 
-void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
+static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
 {
 	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
 }
-EXPORT_SYMBOL_GPL(am335x_tsc_se_update);
 
 void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
 {
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index d498d98f..1fe7219 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -176,7 +176,6 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
 	return *tscadc_dev;
 }
 
-void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc);
 void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
 
-- 
1.8.5.1

Re: [PATCH 2/5] mfd: ti_am335x_tscadc: make am335x_tsc_se_update() local

[Lee Jones]

> since the "recent" changes, am335x_tsc_se_update() has no longer any
> users outside of this file so make it local.

Please use formatting dictated by the subsystem when providing
patches. For MFD we capitalise the first word after 'mfd: <dev>: ' and
the same for the first word in the commit body.

Patch looks okay, I'd prefer to take the set in one go.

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

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

[PATCH 3/5] mfd: ti_am335x_tscadc: don't read back REG_SE

[Sebastian Andrzej Siewior]

The purpose of reg_se_cache has been defeated. It should avoid the
read-back of the register to avoid the latency and the fact that the
bits are reset to 0 after the individual conversation took place.

The reason why this is required like this to work, is that read-back of
the register removes the bits of the ADC so they do not start another
conversation after the register is re-written from the TSC side for the
update.
To avoid the unrequired read-back I introduce a "set adc" variant which
does not update the cache mask. After the conversation completes, the
register bit is removed from the SE reister anyway and we don't plan a
new conversation "any time soon".
This is a small preparation for a larger sync-rework.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c      |  2 +-
 drivers/mfd/ti_am335x_tscadc.c       | 12 ++++++++++--
 include/linux/mfd/ti_am335x_tscadc.h |  1 +
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e948454..9ad9bd9 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -335,7 +335,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 		return -EBUSY;
 
 	step_en = get_adc_step_mask(adc_dev);
-	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
+	am335x_tsc_se_adc_set(adc_dev->mfd_tscadc, step_en);
 
 	/* Wait for ADC sequencer to complete sampling */
 	while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 67d0eb4..2948a41 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -58,19 +58,27 @@ void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
 	unsigned long flags;
 
 	spin_lock_irqsave(&tsadc->reg_lock, flags);
-	tsadc->reg_se_cache = tscadc_readl(tsadc, REG_SE);
 	tsadc->reg_se_cache |= val;
 	am335x_tsc_se_update(tsadc);
 	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
 }
 EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
 
+void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsadc->reg_lock, flags);
+	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache | val);
+	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
+}
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&tsadc->reg_lock, flags);
-	tsadc->reg_se_cache = tscadc_readl(tsadc, REG_SE);
 	tsadc->reg_se_cache &= ~val;
 	am335x_tsc_se_update(tsadc);
 	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 1fe7219..be44a66 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -177,6 +177,7 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
 }
 
 void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val);
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
 
 #endif
-- 
1.8.5.1

Re: [PATCH 3/5] mfd: ti_am335x_tscadc: don't read back REG_SE

[Lee Jones]

On Wed, 18 Dec 2013, Sebastian Andrzej Siewior wrote:

> The purpose of reg_se_cache has been defeated. It should avoid the
> read-back of the register to avoid the latency and the fact that the
> bits are reset to 0 after the individual conversation took place.
> 
> The reason why this is required like this to work, is that read-back of
> the register removes the bits of the ADC so they do not start another
> conversation after the register is re-written from the TSC side for the
> update.
> To avoid the unrequired read-back I introduce a "set adc" variant which
> does not update the cache mask. After the conversation completes, the
> register bit is removed from the SE reister anyway and we don't plan a
> new conversation "any time soon".
> This is a small preparation for a larger sync-rework.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  2 +-
>  drivers/mfd/ti_am335x_tscadc.c       | 12 ++++++++++--
>  include/linux/mfd/ti_am335x_tscadc.h |  1 +
>  3 files changed, 12 insertions(+), 3 deletions(-)

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

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

[PATCH 4/5] mfd: ti_am335x: drop am335x_tsc_se_update() from resume path

[Sebastian Andrzej Siewior]

The update of the SE register in MFD doesn't look right as it has
nothing to do with it. The better place to do it is in TSC driver (which
is already doint it) and in the ADC driver which needs this only in the
continues mode.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c | 2 ++
 drivers/mfd/ti_am335x_tscadc.c  | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 9ad9bd9..e737eef 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -199,6 +199,7 @@ static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
 	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);
+	adc_dev->buffer_en_ch_steps = 0;
 
 	/* Flush FIFO of leftover data in the time it takes to disable adc */
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
@@ -494,6 +495,7 @@ static int tiadc_resume(struct device *dev)
 	tiadc_writel(adc_dev, REG_CTRL, restore);
 
 	tiadc_step_config(indio_dev);
+	am335x_tsc_se_set(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
 
 	return 0;
 }
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 2948a41..ad560c6 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -309,7 +309,6 @@ static int tscadc_resume(struct device *dev)
 
 	if (tscadc_dev->tsc_cell != -1)
 		tscadc_idle_config(tscadc_dev);
-	am335x_tsc_se_update(tscadc_dev);
 	restore = tscadc_readl(tscadc_dev, REG_CTRL);
 	tscadc_writel(tscadc_dev, REG_CTRL,
 			(restore | CNTRLREG_TSCSSENB));
-- 
1.8.5.1

Re: [PATCH 4/5] mfd: ti_am335x: drop am335x_tsc_se_update() from resume path

[Lee Jones]

> The update of the SE register in MFD doesn't look right as it has
> nothing to do with it. The better place to do it is in TSC driver (which
> is already doint it) and in the ADC driver which needs this only in the
> continues mode.

When you resubmit the patch-set complete with Acks, please ensure you
spell check and proof read _all_ of your commit logs.

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 2 ++
>  drivers/mfd/ti_am335x_tscadc.c  | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)

For the MFD part:
  Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

It's probably best that we suck these changes up through the MFD tree,
so once the review comments have been satisfied and we have all
relevant Acks I can take them in and create an immutable branch for
you guys to pull from if you like.

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

[PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation

[Sebastian Andrzej Siewior]

The ADC driver always programms all possible ADC values and discards
them except for the value IIO asked for. On the am335x-evm the driver
programs four values and it takes 500us to gather them. Reducing the number
of coversations down to (required) one also reduces the busy loop down to
125us.
This leads to another error, namely the FIFOCOUNT register is sometimes
(like one out of 10 attempts) not updated in time leading to timeouts.
The next read has the fifocount register updated.
Checking for the ADCStatus status register for beeing idle isn't a good
choice either. The problem is that it often times out if the TSC is used
at the same time. The problem is that the HW compelted the conversaton for
the ADC *and* before the driver noticed it, the HW began to perfom a
TSC conversation and so the driver never seen the HW idle. The next time
we would have two values in the fifo but since the driver reads
everything we always see the current one.
So instead of polling for the IDLE bit, we check for the fifocount. It
should be one instead of zero because we request one value.
This change in turn lead to another error :) Sometimes if TSC & ADC are
used together the TSC starts becomming interrupts even if nobody
actually touched the touchscreen. The interrupts are valid because TSC's
fifo is filled with values. This condition stops after a few ADC reads but
will occur once TSC & ADC are used at the same time. So not good.

On top of this (even without the changes I just mentioned) there is a ADC
& TSC lockup condition which was reported to my Jeff Lance including a test
case:
A busyloop of loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
and a mug on touch screen. Whith this setup, the hardware will lockup after
something between 20min and it could take up to a couple of hours. During that
lockup, the ADCSTAT says 0x30 (or 0x70) which means STEP_ID = IDLE and
FSM_BUSY = yes. That means it says that it is idle and busy at the same
time which is an invalid condition.

For all this reasons I decided to rework this TSC/ADC part and add a
handshake / synchronisation here:
First the ADC signals that it needs the HW and writes a 0 mask into the
SE register. The HW (if active) will complete the current conversation
and become idle. The TSC driver will gather the values from the FIFO
(woken up by an interrupt) and won't "enable" another conversation.
Instead it will wake up the ADC driver which is already waiting. The ADC
driver will start "its" converstation and once it is done, it will
enable the TSC events so the TSC will work again.

After this rework I haven't observed the lockup so far. Plus the busy
loop has been reduced from 500us to 125us.

The continues-read mode remains unchanged.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c           | 60 +++++++++++++++++++-------
 drivers/input/touchscreen/ti_am335x_tsc.c |  4 +-
 drivers/mfd/ti_am335x_tscadc.c            | 71 +++++++++++++++++++++++++++----
 include/linux/mfd/ti_am335x_tscadc.h      |  5 +++
 4 files changed, 114 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e737eef..252a2b5 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -60,6 +60,24 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
 	return step_en;
 }
 
+static u32 get_adc_chan_step_mask(struct tiadc_device *adc_dev,
+		struct iio_chan_spec const *chan)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(adc_dev->channel_step); i++) {
+		if (chan->channel == adc_dev->channel_line[i]) {
+			u32 step;
+
+			step = adc_dev->channel_step[i];
+			/* +1 for the charger */
+			return 1 << (step + 1);
+		}
+	}
+	WARN_ON(1);
+	return 0;
+}
+
 static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
 {
 	return 1 << adc_dev->channel_step[chan];
@@ -329,34 +347,43 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	unsigned int fifo1count, read, stepid;
 	bool found = false;
 	u32 step_en;
-	unsigned long timeout = jiffies + usecs_to_jiffies
-				(IDLE_TIMEOUT * adc_dev->channels);
+	unsigned long timeout;
 
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
 
-	step_en = get_adc_step_mask(adc_dev);
+	step_en = get_adc_chan_step_mask(adc_dev, chan);
+	if (!step_en)
+		return -EINVAL;
+
+	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+	while (fifo1count--)
+		tiadc_readl(adc_dev, REG_FIFO1);
+
 	am335x_tsc_se_adc_set(adc_dev->mfd_tscadc, step_en);
 
-	/* Wait for ADC sequencer to complete sampling */
-	while (tiadc_readl(adc_dev, REG_ADCFSM) & SEQ_STATUS) {
-		if (time_after(jiffies, timeout))
+	timeout = jiffies + usecs_to_jiffies
+				(IDLE_TIMEOUT * adc_dev->channels);
+	/* Wait for Fifo threshold interrupt */
+	while (1) {
+		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
+		if (fifo1count)
+			break;
+
+		if (time_after(jiffies, timeout)) {
+			am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
 			return -EAGAIN;
+		}
 	}
 	map_val = chan->channel + TOTAL_CHANNELS;
 
 	/*
-	 * When the sub-system is first enabled,
-	 * the sequencer will always start with the
-	 * lowest step (1) and continue until step (16).
-	 * For ex: If we have enabled 4 ADC channels and
-	 * currently use only 1 out of them, the
-	 * sequencer still configures all the 4 steps,
-	 * leading to 3 unwanted data.
-	 * Hence we need to flush out this data.
+	 * We check the complete FIFO. We programmed just one entry but in case
+	 * something went wrong we left empty handed (-EAGAIN previously) and
+	 * then the value apeared somehow in the FIFO we would have two entries.
+	 * Therefore we read every item and keep only the latest version of the
+	 * requested channel.
 	 */
-
-	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (i = 0; i < fifo1count; i++) {
 		read = tiadc_readl(adc_dev, REG_FIFO1);
 		stepid = read & FIFOREAD_CHNLID_MASK;
@@ -368,6 +395,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 			*val = (u16) read;
 		}
 	}
+	am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
 
 	if (found == false)
 		return -EBUSY;
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 68beada..5d46fff 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -198,7 +198,7 @@ static void titsc_step_config(struct titsc *ts_dev)
 	/* The steps1 … end and bit 0 for TS_Charge */
 	stepenable = (1 << (end_step + 2)) - 1;
 	ts_dev->step_mask = stepenable;
-	am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
+	am335x_tsc_se_tsc_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 }
 
 static void titsc_read_coordinates(struct titsc *ts_dev,
@@ -322,7 +322,7 @@ static irqreturn_t titsc_irq(int irq, void *dev)
 
 	if (irqclr) {
 		titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
-		am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
+		am335x_tsc_se_tsc_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
 		return IRQ_HANDLED;
 	}
 	return IRQ_NONE;
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index ad560c6..dd950ec 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -24,6 +24,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/sched.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -48,31 +49,83 @@ static const struct regmap_config tscadc_regmap_config = {
 	.val_bits = 32,
 };
 
-static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
+void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsadc->reg_lock, flags);
+	tsadc->reg_se_cache |= val;
 	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
+	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
 }
+EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
 
-void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
+void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&tsadc->reg_lock, flags);
-	tsadc->reg_se_cache |= val;
-	am335x_tsc_se_update(tsadc);
+	tsadc->reg_se_cache = val;
+	if (tsadc->adc_in_use || tsadc->adc_waiting) {
+		if (tsadc->adc_waiting)
+			wake_up(&tsadc->reg_se_wait);
+	} else {
+		tscadc_writel(tsadc, REG_SE, val);
+	}
 	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
 }
-EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_tsc_set);
+
+static void am335x_tscadc_need_adc(struct ti_tscadc_dev *tsadc)
+{
+	DEFINE_WAIT(wait);
+	u32 reg;
+
+	/*
+	 * disable TSC steps so it does not run while the ADC is using it. If
+	 * write 0 while it is running (it just started or was already running)
+	 * then it completes all steps that were enabled and stops then.
+	 */
+	tscadc_writel(tsadc, REG_SE, 0);
+	reg = tscadc_readl(tsadc, REG_ADCFSM);
+	if (reg & SEQ_STATUS) {
+		tsadc->adc_waiting = true;
+		prepare_to_wait(&tsadc->reg_se_wait, &wait,
+				TASK_UNINTERRUPTIBLE);
+		spin_unlock_irq(&tsadc->reg_lock);
+
+		schedule();
+
+		spin_lock_irq(&tsadc->reg_lock);
+		finish_wait(&tsadc->reg_se_wait, &wait);
+
+		reg = tscadc_readl(tsadc, REG_ADCFSM);
+		WARN_ON(reg & SEQ_STATUS);
+		tsadc->adc_waiting = false;
+	}
+	tsadc->adc_in_use = true;
+}
 
 void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val)
 {
+	spin_lock_irq(&tsadc->reg_lock);
+	am335x_tscadc_need_adc(tsadc);
+
+	tscadc_writel(tsadc, REG_SE, val);
+	spin_unlock_irq(&tsadc->reg_lock);
+}
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+
+void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc)
+{
 	unsigned long flags;
 
 	spin_lock_irqsave(&tsadc->reg_lock, flags);
-	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache | val);
+	tsadc->adc_in_use = false;
+	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
 	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
 }
-EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_set);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_done);
 
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
 {
@@ -80,7 +133,7 @@ void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
 
 	spin_lock_irqsave(&tsadc->reg_lock, flags);
 	tsadc->reg_se_cache &= ~val;
-	am335x_tsc_se_update(tsadc);
+	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
 	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
 }
 EXPORT_SYMBOL_GPL(am335x_tsc_se_clr);
@@ -188,6 +241,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	}
 
 	spin_lock_init(&tscadc->reg_lock);
+	init_waitqueue_head(&tscadc->reg_se_wait);
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index be44a66..3cd4f3d3 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -159,6 +159,9 @@ struct ti_tscadc_dev {
 	int adc_cell;	/* -1 if not used */
 	struct mfd_cell cells[TSCADC_CELLS];
 	u32 reg_se_cache;
+	bool adc_waiting;
+	bool adc_in_use;
+	wait_queue_head_t reg_se_wait;
 	spinlock_t reg_lock;
 	unsigned int clk_div;
 
@@ -177,7 +180,9 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
 }
 
 void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val);
 void am335x_tsc_se_adc_set(struct ti_tscadc_dev *tsadc, u32 val);
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_adc_done(struct ti_tscadc_dev *tsadc);
 
 #endif
-- 
1.8.5.1

Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation

[Lee Jones]

> The ADC driver always programms all possible ADC values and discards
> them except for the value IIO asked for. On the am335x-evm the driver
> programs four values and it takes 500us to gather them. Reducing the number
> of coversations down to (required) one also reduces the busy loop down to
> 125us.
> This leads to another error, namely the FIFOCOUNT register is sometimes
> (like one out of 10 attempts) not updated in time leading to timeouts.
> The next read has the fifocount register updated.
> Checking for the ADCStatus status register for beeing idle isn't a good
> choice either. The problem is that it often times out if the TSC is used
> at the same time. The problem is that the HW compelted the conversaton for
> the ADC *and* before the driver noticed it, the HW began to perfom a
> TSC conversation and so the driver never seen the HW idle. The next time
> we would have two values in the fifo but since the driver reads
> everything we always see the current one.
> So instead of polling for the IDLE bit, we check for the fifocount. It
> should be one instead of zero because we request one value.
> This change in turn lead to another error :) Sometimes if TSC & ADC are
> used together the TSC starts becomming interrupts even if nobody
> actually touched the touchscreen. The interrupts are valid because TSC's
> fifo is filled with values. This condition stops after a few ADC reads but
> will occur once TSC & ADC are used at the same time. So not good.
> 
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to my Jeff Lance including a test
> case:
> A busyloop of loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. Whith this setup, the hardware will lockup after
> something between 20min and it could take up to a couple of hours. During that
> lockup, the ADCSTAT says 0x30 (or 0x70) which means STEP_ID = IDLE and
> FSM_BUSY = yes. That means it says that it is idle and busy at the same
> time which is an invalid condition.
> 
> For all this reasons I decided to rework this TSC/ADC part and add a
> handshake / synchronisation here:
> First the ADC signals that it needs the HW and writes a 0 mask into the
> SE register. The HW (if active) will complete the current conversation
> and become idle. The TSC driver will gather the values from the FIFO
> (woken up by an interrupt) and won't "enable" another conversation.
> Instead it will wake up the ADC driver which is already waiting. The ADC
> driver will start "its" converstation and once it is done, it will
> enable the TSC events so the TSC will work again.

Spell check this entire block.

Smileys in commit messages are generally a bad idea.

Please insert '\n's between paragraphs.

Proof read, as some of the sentences are not comprehensible.

/* MFD Parts */

> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -24,6 +24,7 @@

> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
> +void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsadc->reg_lock, flags);
> +	tsadc->reg_se_cache |= val;
>  	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
> +	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>  }
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
>  
> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
> +void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)

What's the difference between:

am335x_tsc_se_set()
and
am335x_tsc_se_tsc_set()

Why don't you have a am335x_tsc_se_tsc_set_tsc_se_tsc_set() ;)

Perhaps a better function name might be in order.

>  {
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&tsadc->reg_lock, flags);
> -	tsadc->reg_se_cache |= val;
> -	am335x_tsc_se_update(tsadc);
> +	tsadc->reg_se_cache = val;
> +	if (tsadc->adc_in_use || tsadc->adc_waiting) {
> +		if (tsadc->adc_waiting)
> +			wake_up(&tsadc->reg_se_wait);

So if we're either in-use or waiting, the step mask is never set?

But I guess that the touchscreen driver assumes it has been set.

Is this okay?

> +	} else {
> +		tscadc_writel(tsadc, REG_SE, val);
> +	}

I think this would be better represented as:

        if (tsadc->adc_waiting)
                wake_up(&tsadc->reg_se_wait);
        else if (!tsadc->adc_in_use)
                tscadc_writel(tsadc, REG_SE, val);

>  	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>  }
> -EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_tsc_set);

<snip>

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

Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation

[Sebastian Andrzej Siewior]

On 12/19/2013 09:42 AM, Lee Jones wrote:
> Spell check this entire block.

will do.

> Smileys in commit messages are generally a bad idea.

will drop.

> Please insert '\n's between paragraphs.

Okay.

> Proof read, as some of the sentences are not comprehensible.

I am going to retry.

> /* MFD Parts */
> 
>> --- a/drivers/mfd/ti_am335x_tscadc.c
>> +++ b/drivers/mfd/ti_am335x_tscadc.c
>> @@ -24,6 +24,7 @@
> 
>> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
>> +void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>>  {
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&tsadc->reg_lock, flags);
>> +	tsadc->reg_se_cache |= val;
>>  	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
>> +	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>>  }
>> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
>>  
>> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>> +void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)
> 
> What's the difference between:
> 
> am335x_tsc_se_set()
> and
> am335x_tsc_se_tsc_set()

The code is the same. I tried to avoid to call the function with the
tsc in its name from the adc part. The continuous mode is still using
this interface because its content has to remain while the TSC is
updating the register. Only the write of the ADC in single-shot-mode is
used as a "one-time-thing" and not required later.

> Why don't you have a am335x_tsc_se_tsc_set_tsc_se_tsc_set() ;)
> 
> Perhaps a better function name might be in order.

the am335x_tsc is the namespace. se the register followed by the user
which is tsc or adc but I know what you mean. I will try to replace it
with _cached and _once so we get rid of the part where is twice in the
name of the function.

> 
>>  {
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(&tsadc->reg_lock, flags);
>> -	tsadc->reg_se_cache |= val;
>> -	am335x_tsc_se_update(tsadc);
>> +	tsadc->reg_se_cache = val;
>> +	if (tsadc->adc_in_use || tsadc->adc_waiting) {
>> +		if (tsadc->adc_waiting)
>> +			wake_up(&tsadc->reg_se_wait);
> 
> So if we're either in-use or waiting, the step mask is never set?
> 
> But I guess that the touchscreen driver assumes it has been set.
> 
> Is this okay?

Yes this is okay because the ADC driver is waiting to use it
exclusively. The ADC driver invokes am335x_tsc_se_adc_done() once it is
done so TSC's mask which is saved here will then be written to the
register.

> 
>> +	} else {
>> +		tscadc_writel(tsadc, REG_SE, val);
>> +	}
> 
> I think this would be better represented as:
> 
>         if (tsadc->adc_waiting)
>                 wake_up(&tsadc->reg_se_wait);
>         else if (!tsadc->adc_in_use)
>                 tscadc_writel(tsadc, REG_SE, val);

I think that looks fine. I will double check it and take your proposal
if nothing goes wrong.

Sebastian

Re: am335x: IIO/ADC fixes if used together with TSC

[Jonathan Cameron]

Given timing I suspect that by the we have reviewed these will be too late in this cycle so will hit in the merge window plus stable.

Also some of these are cleanups rather than fixes so should normally be separate from fixes and applied after them as they are not 
Stable material.

Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>Hi,
>
>this series got a little bigger than expected with a biggy at the end.
>I didn't managed to split the patches across the three subsystems it
>touches due to the changes are required to keep it working and break
>the
>logic.
>Even in 5/5 which does three logical changes I didn't manage to split
>it
>in three patches because the driver would not be functional (sometimes
>it would
>get worse than before). This is all documented in the changelog. 
>Thefore I ask to get this series applied via one tree, the iio tree
>which contains
>or depends on the most changes.
>
>The two bigger issues here are fixed by the last patch incuding
>- time outs on the iio/adc side while TSC&ADC is used together
>- a lockup issue which occurs while TSC&ADC is used together.
>
>Sebastian
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: am335x: IIO/ADC fixes if used together with TSC

[Sebastian Andrzej Siewior]

On 12/18/2013 06:25 PM, Jonathan Cameron wrote:
> Given timing I suspect that by the we have reviewed these will be too late in this cycle so will hit in the merge window plus stable.
> 
> Also some of these are cleanups rather than fixes so should normally be separate from fixes and applied after them as they are not 
> Stable material.

I tried to keep 5/5 as small as possible so I factor individual and
separate changes as good as possible as long as the drivers remained
functional. That means 1-4 are mostly pre-work for 5/5 and that makes
5/5 depends on them.

Tagging 5/5 stable I would prefer to take 1-4, too to keep the backport
effort small. The clash due to the continues mode which is v3.13 is
probably small. This said, I think it is the best to backport it stable
if someone asks for it (unless the majority here thinks otherwise).

Are you saying this is too late for v3.13 or v3.14-rc1?

Sebastian

Re: am335x: IIO/ADC fixes if used together with TSC

[Lee Jones]

> >> Given timing I suspect that by the we have reviewed these will be too
> >late in this cycle so will hit in the merge window plus stable.
> >> 
> >> Also some of these are cleanups rather than fixes so should normally
> >be separate from fixes and applied after them as they are not 
> >> Stable material.
> >
> >I tried to keep 5/5 as small as possible so I factor individual and
> >separate changes as good as possible as long as the drivers remained
> >functional. That means 1-4 are mostly pre-work for 5/5 and that makes
> >5/5 depends on them.
> >
> >Tagging 5/5 stable I would prefer to take 1-4, too to keep the backport
> >effort small. The clash due to the continues mode which is v3.13 is
> >probably small. This said, I think it is the best to backport it stable
> >if someone asks for it (unless the majority here thinks otherwise).
> >
> >Are you saying this is too late for v3.13 or v3.14-rc1?
> >
> >Sebastian
> 
> Too late for 3.13. Probably fine for next merge window if reviews are all good or as a
>  fix post 3.14-rc1 of it misses the cutoff for the merge window.

I think this set should go in via -next. So it will hit the v3.14
merge window.

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