diff mbox

[PATCHv6] staging/iio/adc: change the MXS touchscreen driver implementation

Message ID 201402241416.24270.jbe@pengutronix.de
State New
Headers show

Commit Message

Juergen Borleis Feb. 24, 2014, 1:16 p.m. UTC 
Hi Alexandre,

> While I don't have much experience with the TS part of the code but I
> can investigate if you don't have any idea.

can you please test the following patch?
After releasing the touchscreen the internal state machine was left in a
wrong state.


Regards,
Juergen

Comments

Dan Carpenter Feb. 24, 2014, 1:33 p.m. UTC | #1 
On Mon, Feb 24, 2014 at 02:16:24PM +0100, Juergen Beisert wrote:
> Hi Alexandre,
> 
> > While I don't have much experience with the TS part of the code but I
> > can investigate if you don't have any idea.
> 
> can you please test the following patch?
> After releasing the touchscreen the internal state machine was left in a
> wrong state.
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index e2dd783..558a76c 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -698,6 +698,7 @@ static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
>         }
> 
>         /* if it is released, wait for the next touch via IRQ */
> +       lradc->cur_plate = LRADC_TOUCH;
>         mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>         mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
>  }

I am confused.  Why don't you send this like a normal patch?

regards,
dan carpenter
Alexandre Belloni Feb. 24, 2014, 2:26 p.m. UTC | #2 
Hi Juergen,

On 24/02/2014 at 14:16:24 +0100, Juergen Beisert wrote :
> Hi Alexandre,
> 
> > While I don't have much experience with the TS part of the code but I
> > can investigate if you don't have any idea.
> 
> can you please test the following patch?
> After releasing the touchscreen the internal state machine was left in a
> wrong state.
> 

This seems to solve the issue. You can send the patch with my Tested-by.

It is probably worth trying to get it in 3.14.

Thanks !

> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index e2dd783..558a76c 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -698,6 +698,7 @@ static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
>         }
> 
>         /* if it is released, wait for the next touch via IRQ */
> +       lradc->cur_plate = LRADC_TOUCH;
>         mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>         mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
>  }
> 
> Regards,
> Juergen
> 
> -- 
> Pengutronix e.K.                              | Juergen Beisert             |
> Linux Solutions for Science and Industry      | Phone: +49-5121-206917-5128 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany    | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |
Juergen Borleis Feb. 24, 2014, 2:38 p.m. UTC | #3 
Hi Dan,

On Monday 24 February 2014 14:33:16 Dan Carpenter wrote:
> On Mon, Feb 24, 2014 at 02:16:24PM +0100, Juergen Beisert wrote:
> > > While I don't have much experience with the TS part of the code but I
> > > can investigate if you don't have any idea.
> >
> > can you please test the following patch?
> > After releasing the touchscreen the internal state machine was left in a
> > wrong state.
> >
> > diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> > index e2dd783..558a76c 100644
> > --- a/drivers/staging/iio/adc/mxs-lradc.c  
> > +++ b/drivers/staging/iio/adc/mxs-lradc.c
> > @@ -698,6 +698,7 @@ static void mxs_lradc_finish_touch_event(struct
> > mxs_lradc *lradc, bool valid) }
> >
> >         /* if it is released, wait for the next touch via IRQ */
> > +       lradc->cur_plate = LRADC_TOUCH;
> >         mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
> >         mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> >         } 
>
> I am confused.  Why don't you send this like a normal patch?

For testing fist. But you are right, patch will follow.

Regards,
Juergen
Dan Carpenter Feb. 24, 2014, 2:47 p.m. UTC | #4 
On Mon, Feb 24, 2014 at 03:38:26PM +0100, Juergen Beisert wrote:
> Hi Dan,
> 
> On Monday 24 February 2014 14:33:16 Dan Carpenter wrote:
> > On Mon, Feb 24, 2014 at 02:16:24PM +0100, Juergen Beisert wrote:
> > > > While I don't have much experience with the TS part of the code but I
> > > > can investigate if you don't have any idea.
> > >
> > > can you please test the following patch?
> > > After releasing the touchscreen the internal state machine was left in a
> > > wrong state.
> > >
> > > diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> > > index e2dd783..558a76c 100644
> > > --- a/drivers/staging/iio/adc/mxs-lradc.c  
> > > +++ b/drivers/staging/iio/adc/mxs-lradc.c
> > > @@ -698,6 +698,7 @@ static void mxs_lradc_finish_touch_event(struct
> > > mxs_lradc *lradc, bool valid) }
> > >
> > >         /* if it is released, wait for the next touch via IRQ */
> > > +       lradc->cur_plate = LRADC_TOUCH;
> > >         mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
> > >         mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> > >         } 
> >
> > I am confused.  Why don't you send this like a normal patch?
> 
> For testing fist. But you are right, patch will follow.
> 

Never mind, I suck at reading.

regards,
dan carpenter
Alexandre Belloni Feb. 24, 2014, 4:48 p.m. UTC | #5 
On 24/02/2014 at 15:39:53 +0100, Juergen Beisert wrote :
> Releasing the touchscreen lets the internal statemachine left in a wrong state.
> Due to this the release coordinate will be reported again by accident when the next
> touchscreen event happens. This change sets up the correct state when waiting
> for the next touchscreen event.
>     
> Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> 

Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 7fc66a6..514844e 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -757,6 +757,7 @@ static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
>  	}
>  
>  	/* if it is released, wait for the next touch via IRQ */
> +	lradc->cur_plate = LRADC_TOUCH;
>  	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>  	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
>  }
> 
> -- 
> Pengutronix e.K.                              | Juergen Beisert             |
> Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |
Jonathan Cameron Feb. 24, 2014, 5:14 p.m. UTC | #6 
On February 24, 2014 2:26:58 PM GMT+00:00, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
>Hi Juergen,
>
>On 24/02/2014 at 14:16:24 +0100, Juergen Beisert wrote :
>> Hi Alexandre,
>> 
>> > While I don't have much experience with the TS part of the code but
>I
>> > can investigate if you don't have any idea.
>> 
>> can you please test the following patch?
>> After releasing the touchscreen the internal state machine was left
>in a
>> wrong state.
>> 
>
>This seems to solve the issue. You can send the patch with my
>Tested-by.
>
>It is probably worth trying to get it in 3.14.
Definitely. There is time.
>
>Thanks !
>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>b/drivers/staging/iio/adc/mxs-lradc.c
>> index e2dd783..558a76c 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -698,6 +698,7 @@ static void mxs_lradc_finish_touch_event(struct
>mxs_lradc *lradc, bool valid)
>>         }
>> 
>>         /* if it is released, wait for the next touch via IRQ */
>> +       lradc->cur_plate = LRADC_TOUCH;
>>         mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ,
>LRADC_CTRL1);
>>         mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
>LRADC_CTRL1);
>>  }
>> 
>> Regards,
>> Juergen
>> 
>> -- 
>> Pengutronix e.K.                              | Juergen Beisert      
>      |
>> Linux Solutions for Science and Industry      | Phone:
>+49-5121-206917-5128 |
>> Peiner Str. 6-8, 31137 Hildesheim, Germany    | Fax:  
>+49-5121-206917-5555 |
>> Amtsgericht Hildesheim, HRA 2686              |
>http://www.pengutronix.de/  |
Jonathan Cameron Feb. 24, 2014, 9:14 p.m. UTC | #7 
On 24/02/14 16:48, Alexandre Belloni wrote:
> On 24/02/2014 at 15:39:53 +0100, Juergen Beisert wrote :
>> Releasing the touchscreen lets the internal statemachine left in a wrong state.
>> Due to this the release coordinate will be reported again by accident when the next
>> touchscreen event happens. This change sets up the correct state when waiting
>> for the next touchscreen event.
>>
>> Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
>>
>
> Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Applied to the fixes-togreg branch of iio.git  Thanks

Jonathan
>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>> index 7fc66a6..514844e 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -757,6 +757,7 @@ static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
>>   	}
>>
>>   	/* if it is released, wait for the next touch via IRQ */
>> +	lradc->cur_plate = LRADC_TOUCH;
>>   	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>>   	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
>>   }
>>
>> --
>> Pengutronix e.K.                              | Juergen Beisert             |
>> Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |
>
Jonathan Cameron Feb. 24, 2014, 10:04 p.m. UTC | #8 
On 24/02/14 21:14, Jonathan Cameron wrote:
> On 24/02/14 16:48, Alexandre Belloni wrote:
>> On 24/02/2014 at 15:39:53 +0100, Juergen Beisert wrote :
>>> Releasing the touchscreen lets the internal statemachine left in a wrong state.
>>> Due to this the release coordinate will be reported again by accident when the next
>>> touchscreen event happens. This change sets up the correct state when waiting
>>> for the next touchscreen event.
>>>
>>> Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
>>>
>>
>> Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Applied to the fixes-togreg branch of iio.git  Thanks
I just forced an update to the tree having realised that I'd not
marked it for stable (the change went into the 3.13 tree) and that
a few more details were needed to explain the effects of the problem
and where it came from. Will let it sit overnight then send on to
Greg sometime in the next few days.  Good to have this one cleared up.

Jonathan
>
> Jonathan
>>
>>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>>> index 7fc66a6..514844e 100644
>>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>>> @@ -757,6 +757,7 @@ static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
>>>       }
>>>
>>>       /* if it is released, wait for the next touch via IRQ */
>>> +    lradc->cur_plate = LRADC_TOUCH;
>>>       mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>>>       mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
>>>   }
>>>
>>> --
>>> Pengutronix e.K.                              | Juergen Beisert             |
>>> Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |
>>
>
> --
> 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
diff mbox

Patch

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index e2dd783..558a76c 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -698,6 +698,7 @@  static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
        }

        /* if it is released, wait for the next touch via IRQ */
+       lradc->cur_plate = LRADC_TOUCH;
        mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
        mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
 }