[RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Eric Millbrandt]

Saving the state of the digitizers seems unnecessary since the next
sample will rewrite those registers.  It also causes an unnecessary
sample to occur if the previous state was a sample.  This will cause
the wrong sample to be returned on the next call to wm97xx_read_aux_adc().
---
 drivers/input/touchscreen/wm97xx-core.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index cbfef1e..b4bcb2c 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -138,9 +138,6 @@ int wm97xx_read_aux_adc(struct wm97xx *wm, u16 adcsel)
                wm97xx_reg_write(wm, AC97_EXTENDED_MID, power & 0x7fff);
        }

-       /* Prepare the codec for AUX reading */
-       wm->codec->aux_prepare(wm);
-
        /* Turn polling mode on to read AUX ADC */
        wm->pen_probably_down = 1;
        wm->codec->poll_sample(wm, adcsel, &auxval);
@@ -148,8 +145,6 @@ int wm97xx_read_aux_adc(struct wm97xx *wm, u16 adcsel)
        if (power_adc)
                wm97xx_reg_write(wm, AC97_EXTENDED_MID, power | 0x8000);

-       wm->codec->dig_restore(wm);
-
        wm->pen_probably_down = 0;

        mutex_unlock(&wm->codec_mutex);
--
1.6.3.1

-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-

This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Mark Brown]

On Wed, Sep 01, 2010 at 10:52:10AM -0400, Eric Millbrandt wrote:

> Saving the state of the digitizers seems unnecessary since the next
> sample will rewrite those registers.  It also causes an unnecessary
> sample to occur if the previous state was a sample.  This will cause
> the wrong sample to be returned on the next call to wm97xx_read_aux_adc().

You need to explain how this interacts with both polling and continuous
touchscreen modes - saying that it seems unneeded feels a little fuzzy.
Note that this will also clear the current digitiser configuration
before it starts the AUXADC so it's not a simple save of the values.

RE: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Eric Millbrandt]

On Wed, 1 Sep 2010 at 13:19:25 Mark Brown wrote:
> On Wed, Sep 01, 2010 at 10:52:10AM -0400, Eric Millbrandt wrote:
>
>> Saving the state of the digitizers seems unnecessary since the next
>> sample will rewrite those registers.  It also causes an unnecessary
>> sample to occur if the previous state was a sample.  This will cause
>> the wrong sample to be returned on the next call to
> wm97xx_read_aux_adc().
>
> You need to explain how this interacts with both polling and continuous
> touchscreen modes - saying that it seems unneeded feels a little fuzzy.
> Note that this will also clear the current digitiser configuration before
> it starts the AUXADC so it's not a simple save of the values.

Hi Mark,

You are right; the current digitizer settings do need to be cleared.  I was testing in the polling touchscreen mode and overlooked that fact.  But do we want to restore the POLL bit if it happened to be set?  Continuous mode does not need it and it looks like wm9712_poll_*() will set the POLL bit again when it needs a touchscreen measurement.

Eric

-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-

This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Mark Brown]

On Wed, Sep 01, 2010 at 02:46:59PM -0400, Eric Millbrandt wrote:
> On Wed, 1 Sep 2010 at 13:19:25 Mark Brown wrote:

[Reflowed your mail into 80 columns for legibility - you might want to
look at your MUA configuration]

> > You need to explain how this interacts with both polling and continuous
> > touchscreen modes - saying that it seems unneeded feels a little fuzzy.
> > Note that this will also clear the current digitiser configuration before
> > it starts the AUXADC so it's not a simple save of the values.

> You are right; the current digitizer settings do need to be cleared.
> I was testing in the polling touchscreen mode and overlooked that fact.
> But do we want to restore the POLL bit if it happened to be set?
> Continuous mode does not need it and it looks like wm9712_poll_*() will
> set the POLL bit again when it needs a touchscreen measurement.

I've got a feeling that at least some machine-specific semi accelerated
drivers make use of this but can't quite remember.  If it just causes an
extra sample sometimes I'm inclined to feel it's safer to leave things
as-is.

RE: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Eric Millbrandt]

On Wed, 1 Sep 2010 at 14:46:50 Mark Brown wrote:
> On Wed, Sep 01, 2010 at 02:46:59PM -0400, Eric Millbrandt wrote:
>> On Wed, 1 Sep 2010 at 13:19:25 Mark Brown wrote:
>
> [Reflowed your mail into 80 columns for legibility - you might want to
> look at your MUA configuration]

Sorry, MS Outlook is dumb.

<snip>

> I've got a feeling that at least some machine-specific semi accelerated
> drivers make use of this but can't quite remember.  If it just causes an
> extra sample sometimes I'm inclined to feel it's safer to leave things
> as-is.

The problem there is the return value of poll_sample currently is not being checked, so wm97xx_read_aux_adc() is returning invalid data.  That could be fixed to either keep polling until RC_VALID or a timeout is hit is returned or return an error to the calling function and letting it decide what to do.  I don't like the second option because it creates a pathologic failure:

-The first call to wm97xx_read_aux_adc() succeeds
-The digitizer settings are restored, generating an extra sample
-A second call to wm97xx_read_aux_adc() fails
-The digitizer settings are restored, generating an extra sample
-The third call fails...

Thoughts?

-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-

This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Mark Brown]

On Wed, Sep 01, 2010 at 03:17:27PM -0400, Eric Millbrandt wrote:

> The problem there is the return value of poll_sample currently is not
> being checked, so wm97xx_read_aux_adc() is returning invalid data.  That
> could be fixed to either keep polling until RC_VALID or a timeout is hit
> is returned or return an error to the calling function and letting it
> decide what to do.  I don't like the second option because it creates a
> pathologic failure:

I think (without having thought through it all so I may have missed
something) that the best thing for the pathalogical case is to have a
limited number of goes at restoring things and then just stop the
digitiser - that will leave things restored while still providing a
guaranteed exit case which should be recoverable (since everything will
be halted).  Does that seem sensible to you?

RE: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Eric Millbrandt]

On Thu, 2 Sep 2010 at 10:40:35 Mark Brown wrote:
> On Wed, Sep 01, 2010 at 03:17:27PM -0400, Eric Millbrandt wrote:
>
>> The problem there is the return value of poll_sample currently is not
>> being checked, so wm97xx_read_aux_adc() is returning invalid data.
>> That could be fixed to either keep polling until RC_VALID or a timeout
>> is hit is returned or return an error to the calling function and
>> letting it decide what to do.  I don't like the second option because
>> it creates a pathologic failure:
>
> I think (without having thought through it all so I may have missed
> something) that the best thing for the pathalogical case is to have a
> limited number of goes at restoring things and then just stop the
> digitiser - that will leave things restored while still providing a
> guaranteed exit case which should be recoverable (since
> everything will be halted).  Does that seem sensible to you?
>

It does.  So I will modify wm97xx_read_aux_adc() to do the following:

-Call poll_sample()
-If the return value is not RC_VALID call poll_sample() again
-If after 5 tries (I'm not sure how many samples can be queued) we have
not succeeded, stop the digitizer and return -1.
-Otherwise return auxadc.

If this sounds reasonable I will create a patch for review.


-DISCLAIMER: an automatically appended disclaimer may follow. By posting-
-to a public e-mail mailing list I hereby grant permission to distribute-
-and copy this message.-

This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [RFC] Input: wm97xx-core - Remove save state from wm97xx_read_aux_adc()

[Mark Brown]

On Thu, Sep 02, 2010 at 11:32:29AM -0400, Eric Millbrandt wrote:

> It does.  So I will modify wm97xx_read_aux_adc() to do the following:

> -Call poll_sample()
> -If the return value is not RC_VALID call poll_sample() again
> -If after 5 tries (I'm not sure how many samples can be queued) we have
> not succeeded, stop the digitizer and return -1.
> -Otherwise return auxadc.

> If this sounds reasonable I will create a patch for review.

Yup.  I'd return -EBUSY or something rather than a made up error code.
5 seems like a reasonable number of times to try.