Re: [PATCH 3/5] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save

2014-11-10 Thread Lee Jones 
On Fri, 07 Nov 2014, Vignesh R wrote:

 In one shot mode, sequencer automatically disables all enabled steps at
 the end of each cycle. (both ADC steps and TSC steps) Hence these steps
 need not be saved in reg_se_cache for clearing these steps at a later
 stage.
 Also, when ADC wakes up Sequencer should not be busy executing any of the
 config steps except for the charge step. Previously charge step was 1 ADC
 clock cycle and hence it was ignored.
 
 Signed-off-by: Vignesh R vigne...@ti.com
 ---
  drivers/mfd/ti_am335x_tscadc.c   | 7 +--
  include/linux/mfd/ti_am335x_tscadc.h | 1 +
  2 files changed, 6 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
 index d877e777cce6..94ef8992f46b 100644
 --- a/drivers/mfd/ti_am335x_tscadc.c
 +++ b/drivers/mfd/ti_am335x_tscadc.c
 @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
 *tsadc)
   spin_lock_irq(tsadc-reg_lock);
   finish_wait(tsadc-reg_se_wait, wait);
  
 + /*
 +  * Sequencer should either be idle or
 +  * busy applying the charge step.
 +  */
   reg = tscadc_readl(tsadc, REG_ADCFSM);
 - WARN_ON(reg  SEQ_STATUS);
 + WARN_ON(reg  SEQ_STATUS  (!CHARGE_STEP));

This is almost certainly not correct.

Please take another look at the logic.

I'm _assuming_ you mean (reg  SEQ_STATUS  !CHARGE_STEP).

   tsadc-adc_waiting = false;
   }
   tsadc-adc_in_use = true;
 @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
 *tsadc)
  void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
  {
   spin_lock_irq(tsadc-reg_lock);
 - tsadc-reg_se_cache |= val;
   am335x_tscadc_need_adc(tsadc);
  
   tscadc_writel(tsadc, REG_SE, val);
 diff --git a/include/linux/mfd/ti_am335x_tscadc.h 
 b/include/linux/mfd/ti_am335x_tscadc.h
 index c99be5dc0f5c..fcce182e4a35 100644
 --- a/include/linux/mfd/ti_am335x_tscadc.h
 +++ b/include/linux/mfd/ti_am335x_tscadc.h
 @@ -128,6 +128,7 @@
  
  /* Sequencer Status */
  #define SEQ_STATUS BIT(5)
 +#define CHARGE_STEP  0x11
  
  #define ADC_CLK  300
  #define TOTAL_STEPS  16

-- 
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-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save

2014-11-10 Thread Lee Jones 
On Mon, 10 Nov 2014, Lee Jones wrote:

 On Fri, 07 Nov 2014, Vignesh R wrote:
 
  In one shot mode, sequencer automatically disables all enabled steps at
  the end of each cycle. (both ADC steps and TSC steps) Hence these steps
  need not be saved in reg_se_cache for clearing these steps at a later
  stage.
  Also, when ADC wakes up Sequencer should not be busy executing any of the
  config steps except for the charge step. Previously charge step was 1 ADC
  clock cycle and hence it was ignored.
  
  Signed-off-by: Vignesh R vigne...@ti.com
  ---
   drivers/mfd/ti_am335x_tscadc.c   | 7 +--
   include/linux/mfd/ti_am335x_tscadc.h | 1 +
   2 files changed, 6 insertions(+), 2 deletions(-)
  
  diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
  index d877e777cce6..94ef8992f46b 100644
  --- a/drivers/mfd/ti_am335x_tscadc.c
  +++ b/drivers/mfd/ti_am335x_tscadc.c
  @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
  *tsadc)
  spin_lock_irq(tsadc-reg_lock);
  finish_wait(tsadc-reg_se_wait, wait);
   
  +   /*
  +* Sequencer should either be idle or
  +* busy applying the charge step.
  +*/
  reg = tscadc_readl(tsadc, REG_ADCFSM);
  -   WARN_ON(reg  SEQ_STATUS);
  +   WARN_ON(reg  SEQ_STATUS  (!CHARGE_STEP));
 
 This is almost certainly not correct.
 
 Please take another look at the logic.
 
 I'm _assuming_ you mean (reg  SEQ_STATUS  !CHARGE_STEP).

So I just saw that CHARGE_STEP is actually the new macro below.

So you're currently ANDing these together.

     reg
   0010  #define SEQ_STATUS   BIT(5)
   00010001  #define CHARGE_STEP  0x11

... which will always equate to 0.

  tsadc-adc_waiting = false;
  }
  tsadc-adc_in_use = true;
  @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
  *tsadc)
   void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
   {
  spin_lock_irq(tsadc-reg_lock);
  -   tsadc-reg_se_cache |= val;

Didn't you add this line a little over 1 month ago?

Why the change of heart?

  am335x_tscadc_need_adc(tsadc);
   
  tscadc_writel(tsadc, REG_SE, val);
  diff --git a/include/linux/mfd/ti_am335x_tscadc.h 
  b/include/linux/mfd/ti_am335x_tscadc.h
  index c99be5dc0f5c..fcce182e4a35 100644
  --- a/include/linux/mfd/ti_am335x_tscadc.h
  +++ b/include/linux/mfd/ti_am335x_tscadc.h
  @@ -128,6 +128,7 @@
   
   /* Sequencer Status */
   #define SEQ_STATUS BIT(5)
  +#define CHARGE_STEP0x11
   
   #define ADC_CLK300
   #define TOTAL_STEPS16
 

-- 
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-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save

2014-11-10 Thread Vignesh R 


On Monday 10 November 2014 02:56 PM, Lee Jones wrote:
 On Mon, 10 Nov 2014, Lee Jones wrote:
 
 On Fri, 07 Nov 2014, Vignesh R wrote:

 In one shot mode, sequencer automatically disables all enabled steps at
 the end of each cycle. (both ADC steps and TSC steps) Hence these steps
 need not be saved in reg_se_cache for clearing these steps at a later
 stage.
 Also, when ADC wakes up Sequencer should not be busy executing any of the
 config steps except for the charge step. Previously charge step was 1 ADC
 clock cycle and hence it was ignored.

 Signed-off-by: Vignesh R vigne...@ti.com
 ---
  drivers/mfd/ti_am335x_tscadc.c   | 7 +--
  include/linux/mfd/ti_am335x_tscadc.h | 1 +
  2 files changed, 6 insertions(+), 2 deletions(-)

 diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
 index d877e777cce6..94ef8992f46b 100644
 --- a/drivers/mfd/ti_am335x_tscadc.c
 +++ b/drivers/mfd/ti_am335x_tscadc.c
 @@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
 *tsadc)
 spin_lock_irq(tsadc-reg_lock);
 finish_wait(tsadc-reg_se_wait, wait);
  
 +   /*
 +* Sequencer should either be idle or
 +* busy applying the charge step.
 +*/
 reg = tscadc_readl(tsadc, REG_ADCFSM);
 -   WARN_ON(reg  SEQ_STATUS);
 +   WARN_ON(reg  SEQ_STATUS  (!CHARGE_STEP));

 This is almost certainly not correct.

 Please take another look at the logic.

 I'm _assuming_ you mean (reg  SEQ_STATUS  !CHARGE_STEP).
 
 So I just saw that CHARGE_STEP is actually the new macro below.
 
 So you're currently ANDing these together.
 
      reg
    0010  #define SEQ_STATUS   BIT(5)
    00010001  #define CHARGE_STEP  0x11
 
 ... which will always equate to 0.
 

Oops.. Sorry.. I meant ((reg  SEQ_STATUS)  !(reg  CHARGE_STEP)).
I will fix this.


 tsadc-adc_waiting = false;
 }
 tsadc-adc_in_use = true;
 @@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
 *tsadc)
  void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
  {
 spin_lock_irq(tsadc-reg_lock);
 -   tsadc-reg_se_cache |= val;
 
 Didn't you add this line a little over 1 month ago?
 
 Why the change of heart?
 

Previously, TSC did not reliably re-enable its steps as the TSC irq
handler received false pen up events. Hence, in order to use TSC after
ADC operation, it was necessary to save and re-enable TSC steps
(basically, to keep TSC steps enabled always).
The change was more of a workaround to overcome limitation of TSC irq
handler. With this series of patches, TSC irq handler is very reliable
and the workaround is no longer required.

 am335x_tscadc_need_adc(tsadc);
  
 tscadc_writel(tsadc, REG_SE, val);
 diff --git a/include/linux/mfd/ti_am335x_tscadc.h 
 b/include/linux/mfd/ti_am335x_tscadc.h
 index c99be5dc0f5c..fcce182e4a35 100644
 --- a/include/linux/mfd/ti_am335x_tscadc.h
 +++ b/include/linux/mfd/ti_am335x_tscadc.h
 @@ -128,6 +128,7 @@
  
  /* Sequencer Status */
  #define SEQ_STATUS BIT(5)
 +#define CHARGE_STEP0x11
  
  #define ADC_CLK300
  #define TOTAL_STEPS16

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

[PATCH 3/5] mfd: ti_am335x_tscadc: Remove unwanted reg_se_cache save

2014-11-06 Thread Vignesh R 
In one shot mode, sequencer automatically disables all enabled steps at
the end of each cycle. (both ADC steps and TSC steps) Hence these steps
need not be saved in reg_se_cache for clearing these steps at a later
stage.
Also, when ADC wakes up Sequencer should not be busy executing any of the
config steps except for the charge step. Previously charge step was 1 ADC
clock cycle and hence it was ignored.

Signed-off-by: Vignesh R vigne...@ti.com
---
 drivers/mfd/ti_am335x_tscadc.c   | 7 +--
 include/linux/mfd/ti_am335x_tscadc.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index d877e777cce6..94ef8992f46b 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -86,8 +86,12 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
*tsadc)
spin_lock_irq(tsadc-reg_lock);
finish_wait(tsadc-reg_se_wait, wait);
 
+   /*
+* Sequencer should either be idle or
+* busy applying the charge step.
+*/
reg = tscadc_readl(tsadc, REG_ADCFSM);
-   WARN_ON(reg  SEQ_STATUS);
+   WARN_ON(reg  SEQ_STATUS  (!CHARGE_STEP));
tsadc-adc_waiting = false;
}
tsadc-adc_in_use = true;
@@ -96,7 +100,6 @@ static void am335x_tscadc_need_adc(struct ti_tscadc_dev 
*tsadc)
 void am335x_tsc_se_set_once(struct ti_tscadc_dev *tsadc, u32 val)
 {
spin_lock_irq(tsadc-reg_lock);
-   tsadc-reg_se_cache |= val;
am335x_tscadc_need_adc(tsadc);
 
tscadc_writel(tsadc, REG_SE, val);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h 
b/include/linux/mfd/ti_am335x_tscadc.h
index c99be5dc0f5c..fcce182e4a35 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -128,6 +128,7 @@
 
 /* Sequencer Status */
 #define SEQ_STATUS BIT(5)
+#define CHARGE_STEP0x11
 
 #define ADC_CLK300
 #define TOTAL_STEPS16
-- 
1.9.1

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