[PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Matthias Kaehlcke]

The calculation of the old conversion timeout value was based on the number of
channels used by this driver. This doesn't take into account that other channels
can be used by the touchscreen driver. Adjust the timeout value to the maximum
if the touchscreen driver is enabled

Signed-off-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 drivers/iio/adc/ti_am335x_adc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 3ceac3e..898fc78 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -146,7 +146,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	bool found = false;
 	u32 step_en;
 	unsigned long timeout = jiffies + usecs_to_jiffies
+#ifdef CONFIG_TOUCHSCREEN_TI_AM335X_TSC
+				(IDLE_TIMEOUT * TOTAL_CHANNELS);
+#else
 				(IDLE_TIMEOUT * adc_dev->channels);
+#endif
 	step_en = get_adc_step_mask(adc_dev);
 	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
 
-- 
1.8.4.rc3

Re: [PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Jonathan Cameron]

On 09/10/13 22:02, Matthias Kaehlcke wrote:
> The calculation of the old conversion timeout value was based on the number of
> channels used by this driver. This doesn't take into account that other channels
> can be used by the touchscreen driver. Adjust the timeout value to the maximum
> if the touchscreen driver is enabled
> 
> Signed-off-by: Matthias Kaehlcke <matthias@kaehlcke.net>
Hmm... This is a bit of an uggly solution.  Can we do anything neater via some
callbacks in the underlying mfd?  Rachna, any thoughts on this?

If not I'm happy to take this as fixing a real problem and we can tidy up later
if needed.

Jonathan
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 3ceac3e..898fc78 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -146,7 +146,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>  	bool found = false;
>  	u32 step_en;
>  	unsigned long timeout = jiffies + usecs_to_jiffies
> +#ifdef CONFIG_TOUCHSCREEN_TI_AM335X_TSC
> +				(IDLE_TIMEOUT * TOTAL_CHANNELS);
> +#else
>  				(IDLE_TIMEOUT * adc_dev->channels);
> +#endif
>  	step_en = get_adc_step_mask(adc_dev);
>  	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>  
> 

Re: [PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Jonathan Cameron]

On 09/15/13 17:17, Jonathan Cameron wrote:
> On 09/10/13 22:02, Matthias Kaehlcke wrote:
>> The calculation of the old conversion timeout value was based on the number of
>> channels used by this driver. This doesn't take into account that other channels
>> can be used by the touchscreen driver. Adjust the timeout value to the maximum
>> if the touchscreen driver is enabled
>>
>> Signed-off-by: Matthias Kaehlcke <matthias@kaehlcke.net>
> Hmm... This is a bit of an uggly solution.  Can we do anything neater via some
> callbacks in the underlying mfd?  Rachna, any thoughts on this?
Ah, Rachna's email is dead.  So, anyone else care to comment?
> 
> If not I'm happy to take this as fixing a real problem and we can tidy up later
> if needed.
> 
> Jonathan
>> ---
>>  drivers/iio/adc/ti_am335x_adc.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 3ceac3e..898fc78 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -146,7 +146,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>>  	bool found = false;
>>  	u32 step_en;
>>  	unsigned long timeout = jiffies + usecs_to_jiffies
>> +#ifdef CONFIG_TOUCHSCREEN_TI_AM335X_TSC
>> +				(IDLE_TIMEOUT * TOTAL_CHANNELS);
>> +#else
>>  				(IDLE_TIMEOUT * adc_dev->channels);
>> +#endif
>>  	step_en = get_adc_step_mask(adc_dev);
>>  	am335x_tsc_se_set(adc_dev->mfd_tscadc, step_en);
>>  
>>
> --
> 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
> 

Re: [PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Matthias Kaehlcke]

Hi Jonathan,

thanks for your comments

El Sun, Sep 15, 2013 at 05:17:30PM +0100 Jonathan Cameron ha dit:

> On 09/10/13 22:02, Matthias Kaehlcke wrote:
> > The calculation of the old conversion timeout value was based on the number of
> > channels used by this driver. This doesn't take into account that other channels
> > can be used by the touchscreen driver. Adjust the timeout value to the maximum
> > if the touchscreen driver is enabled
> > 
> > Signed-off-by: Matthias Kaehlcke <matthias@kaehlcke.net>
> Hmm... This is a bit of an uggly solution.  Can we do anything neater via some
> callbacks in the underlying mfd?

i agree, it isn't a very elegant solution. as it's a max timeout which
should never be hit i thought it might be good enough. using a
callback in the mfd is feasible, the mfd has information about the
number of steps used by the touchscreen driver. i'll send a reworked
version soon

thanks

-- 
Matthias Kaehlcke
Embedded Linux Developer
Amsterdam

             "The only important thing Windows does better
            than Debian is implementing the win32 platform"
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

Re: [PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Sebastian Andrzej Siewior]

On 09/10/2013 11:02 PM, Matthias Kaehlcke wrote:
> The calculation of the old conversion timeout value was based on the number of
> channels used by this driver. This doesn't take into account that other channels
> can be used by the touchscreen driver. Adjust the timeout value to the maximum
> if the touchscreen driver is enabled

What bug / miss behave are you trying to fix?
The difference in timming is minimal and therefore I would prefer to get
rid of this ifdef and assume the max value of those two instead.

Sebastian

Re: [PATCH 2/2] iio: ti_am335x_adc: Take touchscreen channels into account for conversion timeout

[Matthias Kaehlcke]

Hi Sebastian,

El Mon, Sep 23, 2013 at 03:31:14PM +0200 Sebastian Andrzej Siewior ha dit:

> On 09/10/2013 11:02 PM, Matthias Kaehlcke wrote:
> > The calculation of the old conversion timeout value was based on the number of
> > channels used by this driver. This doesn't take into account that other channels
> > can be used by the touchscreen driver. Adjust the timeout value to the maximum
> > if the touchscreen driver is enabled
> 
> What bug / miss behave are you trying to fix?

I ran into timeouts when using the touchscreen driver at the same time
as the general purpose ADC and reviewed the timeout calculations. I found
that the IDLE_TIMEOUT value is/was wrong (I submitted another patch
for this) and that the ADC driver doesn't take into account the
steps used by the touchscreen driver

> The difference in timming is minimal and therefore I would prefer to get
> rid of this ifdef and assume the max value of those two instead.

Jonathan also expressed his concerns about this, I submitted a
follow-up patch without the ifdef (https://lkml.org/lkml/2013/9/16/460).
I would appreciate your comments on this patch

note that the timing difference isn't that minimal with the correct
IDLE_TIMEOUT (~100us instead of 10us), it sums up to a max timeout of
~1.6ms (16 steps) and we are busy looping (though in the non-error
case we will bail out as soon as the conversion cycle is finished)

best regards

-- 
Matthias Kaehlcke
Embedded Linux Developer
Amsterdam

    In the absence of clearly-defined goals, we become strangely loyal to
      performing daily trivia until ultimately we become enslaved by it
                           (Robert Heinlein)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-