Re: [PATCH RFT 2/5] iio: mxs-lradc: move TS config into suitable function

Re: [PATCH RFT 2/5] iio: mxs-lradc: move TS config into suitable function

[Jonathan Cameron]

On 14/04/16 21:01, Marek Vasut wrote:
> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>> This patch moves the touchscreen type configuration into
>> a more suitable function. Btw this simplifies PM ops later.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> 
> Looks OK
> 
> Reviewed-by: Marek Vasut <marex@denx.de>
Please do CC Dmitry and linux-input on any patches 'touching' the
touchscreen parts of the driver.  I'm happy this one is simple
enough to apply anyway, but best to make sure everyone is in the
loop.

Something a little odd happened when applying the patch.  Automerging
failed so I did it by hand. Not sure what had changed!

Anyhow, please sanity check I haven't messed it up.

Applied to the togreg branch of iio.git - initially pushed out as
testing for the autobuilders to play with it.

Thanks,


Jonathan
> 
>> ---
>>  drivers/iio/adc/mxs-lradc.c |   21 +++++++++++----------
>>  1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>> index 0576953..7118fca 100644
>> --- a/drivers/iio/adc/mxs-lradc.c
>> +++ b/drivers/iio/adc/mxs-lradc.c
>> @@ -686,6 +686,17 @@ static void mxs_lradc_prepare_pressure(struct mxs_lradc *lradc)
>>  
>>  static void mxs_lradc_enable_touch_detection(struct mxs_lradc *lradc)
>>  {
>> +	/* Configure the touchscreen type */
>> +	if (lradc->soc == IMX28_LRADC) {
>> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
>> +				    LRADC_CTRL0);
>> +
>> +		if (lradc->use_touchscreen == MXS_LRADC_TOUCHSCREEN_5WIRE)
>> +			mxs_lradc_reg_set(lradc,
>> +					  LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
>> +					  LRADC_CTRL0);
>> +	}
>> +
>>  	mxs_lradc_setup_touch_detection(lradc);
>>  
>>  	lradc->cur_plate = LRADC_TOUCH;
>> @@ -1491,16 +1502,6 @@ static int mxs_lradc_hw_init(struct mxs_lradc *lradc)
>>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(2));
>>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(3));
>>  
>> -	/* Configure the touchscreen type */
>> -	if (lradc->soc == IMX28_LRADC) {
>> -		mxs_lradc_reg_clear(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
>> -				    LRADC_CTRL0);
>> -
>> -	if (lradc->use_touchscreen == MXS_LRADC_TOUCHSCREEN_5WIRE)
>> -		mxs_lradc_reg_set(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
>> -				  LRADC_CTRL0);
>> -	}
>> -
>>  	/* Start internal temperature sensing. */
>>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_CTRL2);
>>  
>>
> 
> 

Re: [PATCH RFT 2/5] iio: mxs-lradc: move TS config into suitable function

[Torokhov]

On Sun, Apr 17, 2016 at 11:13:49AM +0100, Jonathan Cameron wrote:
> On 14/04/16 21:01, Marek Vasut wrote:
> > On 04/14/2016 05:48 PM, Stefan Wahren wrote:
> >> This patch moves the touchscreen type configuration into
> >> a more suitable function. Btw this simplifies PM ops later.
> >>
> >> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > 
> > Looks OK
> > 
> > Reviewed-by: Marek Vasut <marex@denx.de>
> Please do CC Dmitry and linux-input on any patches 'touching' the
> touchscreen parts of the driver.  I'm happy this one is simple
> enough to apply anyway, but best to make sure everyone is in the
> loop.

FWIW:

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> 
> Something a little odd happened when applying the patch.  Automerging
> failed so I did it by hand. Not sure what had changed!
> 
> Anyhow, please sanity check I haven't messed it up.
> 
> Applied to the togreg branch of iio.git - initially pushed out as
> testing for the autobuilders to play with it.
> 
> Thanks,
> 
> 
> Jonathan
> > 
> >> ---
> >>  drivers/iio/adc/mxs-lradc.c |   21 +++++++++++----------
> >>  1 file changed, 11 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> >> index 0576953..7118fca 100644
> >> --- a/drivers/iio/adc/mxs-lradc.c
> >> +++ b/drivers/iio/adc/mxs-lradc.c
> >> @@ -686,6 +686,17 @@ static void mxs_lradc_prepare_pressure(struct mxs_lradc *lradc)
> >>  
> >>  static void mxs_lradc_enable_touch_detection(struct mxs_lradc *lradc)
> >>  {
> >> +	/* Configure the touchscreen type */
> >> +	if (lradc->soc == IMX28_LRADC) {
> >> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
> >> +				    LRADC_CTRL0);
> >> +
> >> +		if (lradc->use_touchscreen == MXS_LRADC_TOUCHSCREEN_5WIRE)
> >> +			mxs_lradc_reg_set(lradc,
> >> +					  LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
> >> +					  LRADC_CTRL0);
> >> +	}
> >> +
> >>  	mxs_lradc_setup_touch_detection(lradc);
> >>  
> >>  	lradc->cur_plate = LRADC_TOUCH;
> >> @@ -1491,16 +1502,6 @@ static int mxs_lradc_hw_init(struct mxs_lradc *lradc)
> >>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(2));
> >>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(3));
> >>  
> >> -	/* Configure the touchscreen type */
> >> -	if (lradc->soc == IMX28_LRADC) {
> >> -		mxs_lradc_reg_clear(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
> >> -				    LRADC_CTRL0);
> >> -
> >> -	if (lradc->use_touchscreen == MXS_LRADC_TOUCHSCREEN_5WIRE)
> >> -		mxs_lradc_reg_set(lradc, LRADC_CTRL0_MX28_TOUCH_SCREEN_TYPE,
> >> -				  LRADC_CTRL0);
> >> -	}
> >> -
> >>  	/* Start internal temperature sensing. */
> >>  	mxs_lradc_reg_wrt(lradc, 0, LRADC_CTRL2);
> >>  
> >>
> > 
> > 
> 

-- 
Dmitry

Re: [PATCH RFT 0/5] iio: mxs-lradc: fix leak and implement PM ops

[Stefan Wahren]

Am 14.04.2016 um 21:55 schrieb Marek Vasut:
> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>> This patch series fix a memory leak and implements suspend/resume
>> support for mxs-lradc.
>>
>> Patch 1: fix the memory leak
>> Patch 2-4: refactor mxs-lradc in order to prepare PM implementation
>> Patch 5: implement suspend/resume support
>>
>> These patches has been tested with i.MX23 and i.MX28 but without
>> a touchscreen. I added only a bogus touchscreen in devicetree.
>>
>> It would be nice if someone with a real touchscreen could test it.
>>
>> Stefan Wahren (5):
>>   iio: mxs-lradc: fix memory leak
>>   iio: mxs-lradc: move TS config into suitable function
>>   iio: mxs-lradc: move STMP reset out of ADC init
>>   iio: mxs-lradc: disable only mapped channels in mxs_lradc_hw_stop
>>   iio: mxs-lradc: implement suspend/resume support
>>
>>  drivers/iio/adc/mxs-lradc.c |  116 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 82 insertions(+), 34 deletions(-)
>>
> On M28EVK:
>
> Tested-by: Marek Vasut <marex@denx.de>
>


Thanks. Only one question, does it imply a test with a touchscreen?

Re: [PATCH RFT 2/5] iio: mxs-lradc: move TS config into suitable function

[Stefan Wahren]

Am 17.04.2016 um 12:13 schrieb Jonathan Cameron:
> On 14/04/16 21:01, Marek Vasut wrote:
>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>> This patch moves the touchscreen type configuration into
>>> a more suitable function. Btw this simplifies PM ops later.
>>>
>>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>> Looks OK
>>
>> Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Please do CC Dmitry and linux-input on any patches 'touching' the
> touchscreen parts of the driver.  I'm happy this one is simple
> enough to apply anyway, but best to make sure everyone is in the
> loop.
>
> Something a little odd happened when applying the patch.  Automerging
> failed so I did it by hand. Not sure what had changed!
>
> Anyhow, please sanity check I haven't messed it up.

Everything is fine. There was a indention conflict between Ksenija's and
my patch.

>
> Applied to the togreg branch of iio.git - initially pushed out as
> testing for the autobuilders to play with it.
>
> Thanks,
>
>
> Jonathan

Re: [PATCH RFT 4/5] iio: mxs-lradc: disable only mapped channels in mxs_lradc_hw_stop

[Stefan Wahren]

Hi Jonathan,

Am 17.04.2016 um 12:47 schrieb Jonathan Cameron:
> On 14/04/16 16:48, Stefan Wahren wrote:
>> Disabling of the touchscreen IRQs is already done in
>> mxs_lradc_disable_ts. There is no need to disable them in
>> mxs_lradc_hw_stop again.
> We could do with a little more documentation in the driver on
> which bits in this register are doing what.
>
> The disable_ts deals with bits 22-24... 
> The only other bit I think is ever used by the driver currently is
> 16 (channel 0).  
>
> So whilst this change probably works, I'm not sure of the logic behind it.
> As far as I can tell your new clear is fine but in reality you could
> just clear (0x3f << 16) and get the same result (which would be clearer
> to my mind).
>
> The key think here is that we don't support the higher bits on i.mx28
> yet... Which are for button detection and threshold detection.
>
> Have I understood this correctly?

Yes, you are. I've have looked to long in the reference manual instead
of the code.
I've missed the point that virtual channel 6 and 7 are used in case of a
connected touchscreen.

I should use the member buffer_vchans for masking the enable IRQs.

Regards
Stefan

>
> Jonathan
>  So we only need to care of the

Re: [PATCH RFT 0/5] iio: mxs-lradc: fix leak and implement PM ops

[Marek Vasut]

On 04/18/2016 08:17 AM, Stefan Wahren wrote:
> Am 14.04.2016 um 21:55 schrieb Marek Vasut:
>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>> This patch series fix a memory leak and implements suspend/resume
>>> support for mxs-lradc.
>>>
>>> Patch 1: fix the memory leak
>>> Patch 2-4: refactor mxs-lradc in order to prepare PM implementation
>>> Patch 5: implement suspend/resume support
>>>
>>> These patches has been tested with i.MX23 and i.MX28 but without
>>> a touchscreen. I added only a bogus touchscreen in devicetree.
>>>
>>> It would be nice if someone with a real touchscreen could test it.
>>>
>>> Stefan Wahren (5):
>>>   iio: mxs-lradc: fix memory leak
>>>   iio: mxs-lradc: move TS config into suitable function
>>>   iio: mxs-lradc: move STMP reset out of ADC init
>>>   iio: mxs-lradc: disable only mapped channels in mxs_lradc_hw_stop
>>>   iio: mxs-lradc: implement suspend/resume support
>>>
>>>  drivers/iio/adc/mxs-lradc.c |  116 ++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 82 insertions(+), 34 deletions(-)
>>>
>> On M28EVK:
>>
>> Tested-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>>
> 
> 
> Thanks. Only one question, does it imply a test with a touchscreen?
> 

Yes, this one [1]. It's an Ampire 800480 display and resistive touch
on top of it.

[1] http://www.denx-cs.de/?q=MxxDK

-- 
Best regards,
Marek Vasut