am335x: IIO/ADC fixes if used together with TSC, v2

am335x: IIO/ADC fixes if used together with TSC, v2

[Sebastian Andrzej Siewior]

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.

v1…v2:
 - spelled checked and re-read each commit message and formated to
   subject line as Lee suggested.
 - removed am335x_tsc_se_set(), replaced am335x_tsc_se_tsc_set() by 
   am335x_tsc_se_set_cache() and adc variant is now named
   am335x_tsc_se_set_once()
 - Added Lee Jones' tags

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.

Reviewed-by: Lee Jones <lee.jones@linaro.org>
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()

[Jonathan Cameron]

On 12/19/13 15:28, 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.
> 
> Reviewed-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>

Lee, I'm assuming you are fine taking the whole series!
> ---
>  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;
>  
>  	/*
> 

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

[Lee Jones]

On Sun, 22 Dec 2013, Jonathan Cameron wrote:

> On 12/19/13 15:28, 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.
> > 
> > Reviewed-by: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> 
> Lee, I'm assuming you are fine taking the whole series!

Sure, no problem.

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

On Thu, 19 Dec 2013, Sebastian Andrzej Siewior wrote:

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

I'm pretty sure I Acked this already.

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

-- 
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 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 not required read-back I introduce a "set once" variant which
does not update the cache mask. After the conversation completes, the
bit is removed from the SE register anyway and we don't plan a new
conversation "any time soon". The current set function is renamed to
set_cache to distinguish the two operations.
This is a small preparation for a larger sync-rework.

Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/iio/adc/ti_am335x_adc.c           |  4 ++--
 drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
 drivers/mfd/ti_am335x_tscadc.c            | 16 ++++++++++++----
 include/linux/mfd/ti_am335x_tscadc.h      |  3 ++-
 4 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e948454..b5197a0 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -181,7 +181,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
 		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);
+	am335x_tsc_se_set_cache(adc_dev->mfd_tscadc, enb);
 
 	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
 				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
@@ -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_set_once(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/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 68beada..2ca5a7b 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_set_cache(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_set_cache(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 67d0eb4..cb0c211 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -53,24 +53,32 @@ static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
 	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
 }
 
-void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
+void am335x_tsc_se_set_cache(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);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_set_cache);
+
+void am335x_tsc_se_set_once(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_set_once);
 
 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..2fa9c06 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -176,7 +176,8 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
 	return *tscadc_dev;
 }
 
-void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val);
+void am335x_tsc_se_set_once(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

[Jonathan Cameron]

On 12/19/13 15:28, 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 not required read-back I introduce a "set once" variant which
> does not update the cache mask. After the conversation completes, the
> bit is removed from the SE register anyway and we don't plan a new
> conversation "any time soon". The current set function is renamed to
> set_cache to distinguish the two operations.
> This is a small preparation for a larger sync-rework.
> 
> Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

> ---
>  drivers/iio/adc/ti_am335x_adc.c           |  4 ++--
>  drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
>  drivers/mfd/ti_am335x_tscadc.c            | 16 ++++++++++++----
>  include/linux/mfd/ti_am335x_tscadc.h      |  3 ++-
>  4 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index e948454..b5197a0 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -181,7 +181,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>  		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);
> +	am335x_tsc_se_set_cache(adc_dev->mfd_tscadc, enb);
>  
>  	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
>  				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
> @@ -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_set_once(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/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 68beada..2ca5a7b 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_set_cache(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_set_cache(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 67d0eb4..cb0c211 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -53,24 +53,32 @@ static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
>  	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
>  }
>  
> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
> +void am335x_tsc_se_set_cache(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);
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set_cache);
> +
> +void am335x_tsc_se_set_once(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_set_once);
>  
>  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..2fa9c06 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -176,7 +176,8 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>  	return *tscadc_dev;
>  }
>  
> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val);
> +void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val);
> +void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val);
>  void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val);
>  
>  #endif
> 

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

[Lee Jones]

On Thu, 19 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 not required read-back I introduce a "set once" variant which
> does not update the cache mask. After the conversation completes, the
> bit is removed from the SE register anyway and we don't plan a new
> conversation "any time soon". The current set function is renamed to
> set_cache to distinguish the two operations.
> This is a small preparation for a larger sync-rework.
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/iio/adc/ti_am335x_adc.c           |  4 ++--
>  drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--

Just need Dmitry's Ack now.

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

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

[Dmitry Torokhov]

On Mon, Jan 06, 2014 at 09:35:25AM +0000, Lee Jones wrote:
> On Thu, 19 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 not required read-back I introduce a "set once" variant which
> > does not update the cache mask. After the conversation completes, the
> > bit is removed from the SE register anyway and we don't plan a new
> > conversation "any time soon". The current set function is renamed to
> > set_cache to distinguish the two operations.
> > This is a small preparation for a larger sync-rework.
> > 
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  drivers/iio/adc/ti_am335x_adc.c           |  4 ++--
> >  drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
> 
> Just need Dmitry's Ack now.

Well, that has nothing to do with input per se but rather inner workings
of the device. But yes, FWIW:

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

Thanks.

-- 
Dmitry

[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 doing it) and in the ADC driver which needs this only in the
continues mode.

Acked-by: Lee Jones <lee.jones@linaro.org>  [MFD part]
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 b5197a0..6822b9f 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 cb0c211..157f569 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

[Jonathan Cameron]

On 12/19/13 15:28, Sebastian Andrzej Siewior wrote:
> 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 doing it) and in the ADC driver which needs this only in the
> continues mode.
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>  [MFD part]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  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 b5197a0..6822b9f 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 cb0c211..157f569 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));
> 

[PATCH 5/5] mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

[Sebastian Andrzej Siewior]

The ADC driver always programs 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 conversations down to the (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 EBUSY.
The next read has the FIFOCOUNT register updated.
Checking for the ADCSTAT register for being idle isn't a good choice either.
The problem is that if TSC is used at the same time, the HW completes the
conversation for ADC *and* before the driver noticed it, the HW begins to
perform 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 in ADCStatus register, we should
check the FIFOCOUNT register. It should be one instead of zero because we
request one value.

This change in turn leads to another error. Sometimes if TSC & ADC are
used together the TSC starts becoming interrupts even if nobody
actually touched the touchscreen. The interrupts seem valid because TSC's
FIFO is filled with values for each channel of the TSC. This condition stops
after a few ADC reads but will occur again. Not good.

On top of this (even without the changes I just mentioned) there is a ADC
& TSC lockup condition which was reported to me by Jeff Lance including the
following test case:
A busy loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
and a mug on touch screen. With this setup, the hardware will lockup after
something between 20 minutes and it could take up to a couple of hours.
During that lockup, the ADCSTAT register says 0x30 (or 0x70) which means
STEP_ID = IDLE and FSM_BUSY = yes. That means the hardware 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 / synchronization 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" conversation and once it is done, it will
enable the TSC steps 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      | 64 ++++++++++++++++++++++++++----------
 drivers/mfd/ti_am335x_tscadc.c       | 63 +++++++++++++++++++++++++++++------
 include/linux/mfd/ti_am335x_tscadc.h |  4 +++
 3 files changed, 103 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 6822b9f..31e786e 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_set_once(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;
@@ -495,8 +523,8 @@ 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);
-
+	am335x_tsc_se_set_cache(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 157f569..d4e8604 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,71 @@ static const struct regmap_config tscadc_regmap_config = {
 	.val_bits = 32,
 };
 
-static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
-{
-	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
-}
-
 void am335x_tsc_se_set_cache(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_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_cache);
 
+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_set_once(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_set_once);
+
+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_set_once);
+EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_done);
 
 void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
 {
@@ -80,7 +121,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 +229,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 2fa9c06..fb96c84 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;
 
@@ -179,5 +182,6 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
 void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val);
 void am335x_tsc_se_set_once(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 synchronization

[Lee Jones]

On Thu, 19 Dec 2013, Sebastian Andrzej Siewior wrote:

> The ADC driver always programs 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 conversations down to the (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 EBUSY.
> The next read has the FIFOCOUNT register updated.
> Checking for the ADCSTAT register for being idle isn't a good choice either.
> The problem is that if TSC is used at the same time, the HW completes the
> conversation for ADC *and* before the driver noticed it, the HW begins to
> perform 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 in ADCStatus register, we should
> check the FIFOCOUNT register. It should be one instead of zero because we
> request one value.
> 
> This change in turn leads to another error. Sometimes if TSC & ADC are
> used together the TSC starts becoming interrupts even if nobody
> actually touched the touchscreen. The interrupts seem valid because TSC's
> FIFO is filled with values for each channel of the TSC. This condition stops
> after a few ADC reads but will occur again. Not good.
> 
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to me by Jeff Lance including the
> following test case:
> A busy loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. With this setup, the hardware will lockup after
> something between 20 minutes and it could take up to a couple of hours.
> During that lockup, the ADCSTAT register says 0x30 (or 0x70) which means
> STEP_ID = IDLE and FSM_BUSY = yes. That means the hardware 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 / synchronization 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" conversation and once it is done, it will
> enable the TSC steps 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      | 64 ++++++++++++++++++++++++++----------
>  drivers/mfd/ti_am335x_tscadc.c       | 63 +++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |  4 +++

Assuming nothing else has changed since the answers you gave me:
  Acked-by: Lee Jones <lee.jones@linaro.org>

Once we have the remaining Acks I'll happily apply this patch-set.

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

Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

[Sebastian Andrzej Siewior]

On 12/19/2013 06:18 PM, Lee Jones wrote:

> Assuming nothing else has changed since the answers you gave me:
>   Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> Once we have the remaining Acks I'll happily apply this patch-set.

Thank you.

Sebastian

Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

[Zubair Lutfullah :]

On Thu, Dec 19, 2013 at 04:28:31PM +0100, Sebastian Andrzej Siewior wrote:
> The ADC driver always programs 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 conversations down to the (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 EBUSY.
> The next read has the FIFOCOUNT register updated.
> Checking for the ADCSTAT register for being idle isn't a good choice either.
> The problem is that if TSC is used at the same time, the HW completes the
> conversation for ADC *and* before the driver noticed it, the HW begins to
> perform 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 in ADCStatus register, we should
> check the FIFOCOUNT register. It should be one instead of zero because we
> request one value.
> 
> This change in turn leads to another error. Sometimes if TSC & ADC are
> used together the TSC starts becoming interrupts even if nobody
> actually touched the touchscreen. The interrupts seem valid because TSC's
> FIFO is filled with values for each channel of the TSC. This condition stops
> after a few ADC reads but will occur again. Not good.
> 
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to me by Jeff Lance including the
> following test case:
> A busy loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. With this setup, the hardware will lockup after
> something between 20 minutes and it could take up to a couple of hours.
> During that lockup, the ADCSTAT register says 0x30 (or 0x70) which means
> STEP_ID = IDLE and FSM_BUSY = yes. That means the hardware says that it is
> idle and busy at the same time which is an invalid condition.
I knew about FSM messing up. But couldn't investigate thouroughly
and didn't want to rework so much while adding continuous mode. 

Good work :)
> 
> For all this reasons I decided to rework this TSC/ADC part and add a
> handshake / synchronization 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" conversation and once it is done, it will
> enable the TSC steps 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.

For one-shot reading from ADC, the TSC is 'disabled' for that moment.
Ok. But for continuous mode, reading from ADC disables TSC for that long time?

If it doesn't disable the TSC, does that mean that this fix would
correct the one-shot read. But the continuous mode is still prone to the 
FSM hanging up?

> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
...
> @@ -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);
> +
Would this flush be needed ideally?

Thanks
Zubair 

Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

[Jonathan Cameron]

On 12/19/13 15:28, Sebastian Andrzej Siewior wrote:
> The ADC driver always programs 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 conversations down to the (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 EBUSY.
> The next read has the FIFOCOUNT register updated.
> Checking for the ADCSTAT register for being idle isn't a good choice either.
> The problem is that if TSC is used at the same time, the HW completes the
> conversation for ADC *and* before the driver noticed it, the HW begins to
> perform 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 in ADCStatus register, we should
> check the FIFOCOUNT register. It should be one instead of zero because we
> request one value.
> 
> This change in turn leads to another error. Sometimes if TSC & ADC are
> used together the TSC starts becoming interrupts even if nobody
becoming -> generating?
> actually touched the touchscreen. The interrupts seem valid because TSC's
> FIFO is filled with values for each channel of the TSC. This condition stops
> after a few ADC reads but will occur again. Not good.
> 
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to me by Jeff Lance including the
> following test case:
> A busy loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. With this setup, the hardware will lockup after
> something between 20 minutes and it could take up to a couple of hours.
> During that lockup, the ADCSTAT register says 0x30 (or 0x70) which means
> STEP_ID = IDLE and FSM_BUSY = yes. That means the hardware says that it is
> idle and busy at the same time which is an invalid condition.
> 
yikes.
> For all this reasons I decided to rework this TSC/ADC part and add a
> handshake / synchronization 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" conversation and once it is done, it will
> enable the TSC steps 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.
>
Thanks for the detailed explanation.

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Hmm. I'm not really an expert in this complex driver, but your explanation is clear
and the code appears to implement what you describe.  Hopefully we'll see a fix for
the continuous reads as well.

Fiddly little device!

Acked-by: Jonathan Cameron <jic23@kernel.org>

> ---
>  drivers/iio/adc/ti_am335x_adc.c      | 64 ++++++++++++++++++++++++++----------
>  drivers/mfd/ti_am335x_tscadc.c       | 63 +++++++++++++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |  4 +++
>  3 files changed, 103 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 6822b9f..31e786e 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_set_once(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;
> @@ -495,8 +523,8 @@ 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);
> -
> +	am335x_tsc_se_set_cache(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 157f569..d4e8604 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,71 @@ static const struct regmap_config tscadc_regmap_config = {
>  	.val_bits = 32,
>  };
>  
> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
> -{
> -	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
> -}
> -
>  void am335x_tsc_se_set_cache(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_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_cache);
>  
> +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_set_once(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_set_once);
> +
> +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_set_once);
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_adc_done);
>  
>  void am335x_tsc_se_clr(struct ti_tscadc_dev *tsadc, u32 val)
>  {
> @@ -80,7 +121,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 +229,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 2fa9c06..fb96c84 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;
>  
> @@ -179,5 +182,6 @@ static inline struct ti_tscadc_dev *ti_tscadc_dev_get(struct platform_device *p)
>  void am335x_tsc_se_set_cache(struct ti_tscadc_dev *tsadc, u32 val);
>  void am335x_tsc_se_set_once(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
> 

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

[Lee Jones]

Dmitry, Jonathan,

This is for you.

The following changes since commit 6ce4eac1f600b34f2f7f58f9cd8f0503d79e42ae:

  Linux 3.13-rc1 (2013-11-22 11:30:55 -0800)

are available in the git repository at:

  git://git.linaro.org/people/ljones/mfd.git tags/ib-iio-input-3.13-1

for you to fetch changes up to 7ca6740cd1cd410828a01151a044b51910d06eff:

  mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization (2014-01-07 08:45:00 +0000)

----------------------------------------------------------------
Immutable branch for IIO and Input

----------------------------------------------------------------
Sebastian Andrzej Siewior (5):
      iio: ti_am335x_adc: Adjust the closing bracket in tiadc_read_raw()
      mfd: ti_am335x_tscadc: Make am335x_tsc_se_update() local
      mfd: ti_am335x_tscadc: Don't read back REG_SE
      mfd: ti_am335x: Drop am335x_tsc_se_update() from resume path
      mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

 drivers/iio/adc/ti_am335x_adc.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
 drivers/mfd/ti_am335x_tscadc.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 include/linux/mfd/ti_am335x_tscadc.h      |  8 ++++++--
 4 files changed, 117 insertions(+), 34 deletions(-)

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

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

[Dmitry Torokhov]

On Tue, Jan 07, 2014 at 08:54:56AM +0000, Lee Jones wrote:
> Dmitry, Jonathan,
> 
> This is for you.
> 
> The following changes since commit 6ce4eac1f600b34f2f7f58f9cd8f0503d79e42ae:
> 
>   Linux 3.13-rc1 (2013-11-22 11:30:55 -0800)
> 
> are available in the git repository at:
> 
>   git://git.linaro.org/people/ljones/mfd.git tags/ib-iio-input-3.13-1
> 
> for you to fetch changes up to 7ca6740cd1cd410828a01151a044b51910d06eff:
> 
>   mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization (2014-01-07 08:45:00 +0000)
> 
> ----------------------------------------------------------------
> Immutable branch for IIO and Input
> 
> ----------------------------------------------------------------
> Sebastian Andrzej Siewior (5):
>       iio: ti_am335x_adc: Adjust the closing bracket in tiadc_read_raw()
>       mfd: ti_am335x_tscadc: Make am335x_tsc_se_update() local
>       mfd: ti_am335x_tscadc: Don't read back REG_SE
>       mfd: ti_am335x: Drop am335x_tsc_se_update() from resume path
>       mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization
> 
>  drivers/iio/adc/ti_am335x_adc.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------
>  drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
>  drivers/mfd/ti_am335x_tscadc.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  include/linux/mfd/ti_am335x_tscadc.h      |  8 ++++++--
>  4 files changed, 117 insertions(+), 34 deletions(-)

I do not have any pending changes to ti_am335x_tsc.c so it can all go
through Jonathan's tree.

Thanks.

-- 
Dmitry

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

[Lee Jones]

> > Dmitry, Jonathan,
> > 
> > This is for you.
> > 
> > The following changes since commit 6ce4eac1f600b34f2f7f58f9cd8f0503d79e42ae:
> > 
> >   Linux 3.13-rc1 (2013-11-22 11:30:55 -0800)
> > 
> > are available in the git repository at:
> > 
> >   git://git.linaro.org/people/ljones/mfd.git tags/ib-iio-input-3.13-1
> > 
> > for you to fetch changes up to 7ca6740cd1cd410828a01151a044b51910d06eff:
> > 
> >   mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization (2014-01-07 08:45:00 +0000)
> > 
> > ----------------------------------------------------------------
> > Immutable branch for IIO and Input
> > 
> > ----------------------------------------------------------------
> > Sebastian Andrzej Siewior (5):
> >       iio: ti_am335x_adc: Adjust the closing bracket in tiadc_read_raw()
> >       mfd: ti_am335x_tscadc: Make am335x_tsc_se_update() local
> >       mfd: ti_am335x_tscadc: Don't read back REG_SE
> >       mfd: ti_am335x: Drop am335x_tsc_se_update() from resume path
> >       mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization
> > 
> >  drivers/iio/adc/ti_am335x_adc.c           | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-------------------
> >  drivers/input/touchscreen/ti_am335x_tsc.c |  4 ++--
> >  drivers/mfd/ti_am335x_tscadc.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------
> >  include/linux/mfd/ti_am335x_tscadc.h      |  8 ++++++--
> >  4 files changed, 117 insertions(+), 34 deletions(-)
> 
> I do not have any pending changes to ti_am335x_tsc.c so it can all go
> through Jonathan's tree.

Well the patches are going through my tree, but in case either of you
have any changes to the above files you can merge this and Git will do
the rest during the merge-window. If neither of you do then no need to
pull. :)

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

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