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 <[email protected]>
---
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
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 <[email protected]>
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);
>
>
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 <[email protected]>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> 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 `-
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
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 `-