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

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

[Stefan Wahren]

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(-)

-- 
1.7.9.5

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Stefan Wahren]

After successful touchscreen registration the input device was
never freed. So fix this issue by using devm_input_allocate_device().

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/iio/adc/mxs-lradc.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index 33051b8..0576953 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
 {
 	struct input_dev *input;
 	struct device *dev = lradc->dev;
-	int ret;
 
 	if (!lradc->use_touchscreen)
 		return 0;
 
-	input = input_allocate_device();
+	input = devm_input_allocate_device(dev);
 	if (!input)
 		return -ENOMEM;
 
@@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
 
 	lradc->ts_input = input;
 	input_set_drvdata(input, lradc);
-	ret = input_register_device(input);
-	if (ret)
-		input_free_device(lradc->ts_input);
 
-	return ret;
+	return input_register_device(input);
 }
 
 static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
-- 
1.7.9.5

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Marek Vasut]

On 04/14/2016 05:48 PM, Stefan Wahren wrote:
> After successful touchscreen registration the input device was
> never freed. So fix this issue by using devm_input_allocate_device().
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index 33051b8..0576953 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>  {
>  	struct input_dev *input;
>  	struct device *dev = lradc->dev;
> -	int ret;
>  
>  	if (!lradc->use_touchscreen)
>  		return 0;
>  
> -	input = input_allocate_device();
> +	input = devm_input_allocate_device(dev);
>  	if (!input)
>  		return -ENOMEM;
>  
> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>  
>  	lradc->ts_input = input;
>  	input_set_drvdata(input, lradc);
> -	ret = input_register_device(input);
> -	if (ret)
> -		input_free_device(lradc->ts_input);
>  
> -	return ret;
> +	return input_register_device(input);
>  }
>  
>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
> 

Nice find.

Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
could use a proper fail path. Do you want to send patches or shall I ?

-- 
Best regards,
Marek Vasut

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Jonathan Cameron]

On 14/04/16 21:01, Marek Vasut wrote:
> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>> After successful touchscreen registration the input device was
>> never freed. So fix this issue by using devm_input_allocate_device().
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>> index 33051b8..0576953 100644
>> --- a/drivers/iio/adc/mxs-lradc.c
>> +++ b/drivers/iio/adc/mxs-lradc.c
>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>  {
>>  	struct input_dev *input;
>>  	struct device *dev = lradc->dev;
>> -	int ret;
>>  
>>  	if (!lradc->use_touchscreen)
>>  		return 0;
>>  
>> -	input = input_allocate_device();
>> +	input = devm_input_allocate_device(dev);
>>  	if (!input)
>>  		return -ENOMEM;
>>  
>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>  
>>  	lradc->ts_input = input;
>>  	input_set_drvdata(input, lradc);
>> -	ret = input_register_device(input);
>> -	if (ret)
>> -		input_free_device(lradc->ts_input);
>>  
>> -	return ret;
>> +	return input_register_device(input);
>>  }
>>  
>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>
> 
> Nice find.
> 
> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
> could use a proper fail path. Do you want to send patches or shall I ?
> 
As this has been there a long time I'm not going to rush it in as a fix.
I suspect no platform is probing and removing it enough to get a significant
amount of memory leaked.

Good to clear it up though!

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

Thanks,

Jonathan

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Marek Vasut]

On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
> On 14/04/16 21:01, Marek Vasut wrote:
>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>> After successful touchscreen registration the input device was
>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>> index 33051b8..0576953 100644
>>> --- a/drivers/iio/adc/mxs-lradc.c
>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>  {
>>>  	struct input_dev *input;
>>>  	struct device *dev = lradc->dev;
>>> -	int ret;
>>>  
>>>  	if (!lradc->use_touchscreen)
>>>  		return 0;
>>>  
>>> -	input = input_allocate_device();
>>> +	input = devm_input_allocate_device(dev);
>>>  	if (!input)
>>>  		return -ENOMEM;
>>>  
>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>  
>>>  	lradc->ts_input = input;
>>>  	input_set_drvdata(input, lradc);
>>> -	ret = input_register_device(input);
>>> -	if (ret)
>>> -		input_free_device(lradc->ts_input);
>>>  
>>> -	return ret;
>>> +	return input_register_device(input);
>>>  }
>>>  
>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>
>>
>> Nice find.
>>
>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>> could use a proper fail path. Do you want to send patches or shall I ?
>>
> As this has been there a long time I'm not going to rush it in as a fix.

I did take a proper look today and it seems they do the right thing
afterall. I checked them with kmemleak too to be sure.

> I suspect no platform is probing and removing it enough to get a significant
> amount of memory leaked.
> 
> Good to clear it up though!
> 
> Applied to the togreg branch of iio.git, initially pushed out as testing for
> the autobuilders to play with it.
> 
> Thanks,
> 
> Jonathan
> 


-- 
Best regards,
Marek Vasut

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Stefan Wahren]

Hi Marek,

Am 18.04.2016 um 19:16 schrieb Marek Vasut:
> On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
>> On 14/04/16 21:01, Marek Vasut wrote:
>>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>>> After successful touchscreen registration the input device was
>>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>>
>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>> ---
>>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>> index 33051b8..0576953 100644
>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>  {
>>>>  	struct input_dev *input;
>>>>  	struct device *dev = lradc->dev;
>>>> -	int ret;
>>>>  
>>>>  	if (!lradc->use_touchscreen)
>>>>  		return 0;
>>>>  
>>>> -	input = input_allocate_device();
>>>> +	input = devm_input_allocate_device(dev);
>>>>  	if (!input)
>>>>  		return -ENOMEM;
>>>>  
>>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>  
>>>>  	lradc->ts_input = input;
>>>>  	input_set_drvdata(input, lradc);
>>>> -	ret = input_register_device(input);
>>>> -	if (ret)
>>>> -		input_free_device(lradc->ts_input);
>>>>  
>>>> -	return ret;
>>>> +	return input_register_device(input);
>>>>  }
>>>>  
>>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>>
>>> Nice find.
>>>
>>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>>> could use a proper fail path. Do you want to send patches or shall I ?
>>>
>> As this has been there a long time I'm not going to rush it in as a fix.
> I did take a proper look today and it seems they do the right thing
> afterall. I checked them with kmemleak too to be sure.

thanks, input_unregister_device already free the memory.

Sorry for the mess :-(

I think it would be the best to remove / revert this patch.

Stefan

>
>> I suspect no platform is probing and removing it enough to get a significant
>> amount of memory leaked.
>>
>> Good to clear it up though!
>>
>> Applied to the togreg branch of iio.git, initially pushed out as testing for
>> the autobuilders to play with it.
>>
>> Thanks,
>>
>> Jonathan
>>
>

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Marek Vasut]

On 04/19/2016 08:33 AM, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 18.04.2016 um 19:16 schrieb Marek Vasut:
>> On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
>>> On 14/04/16 21:01, Marek Vasut wrote:
>>>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>>>> After successful touchscreen registration the input device was
>>>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>>>
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>> ---
>>>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>>> index 33051b8..0576953 100644
>>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>  {
>>>>>  	struct input_dev *input;
>>>>>  	struct device *dev = lradc->dev;
>>>>> -	int ret;
>>>>>  
>>>>>  	if (!lradc->use_touchscreen)
>>>>>  		return 0;
>>>>>  
>>>>> -	input = input_allocate_device();
>>>>> +	input = devm_input_allocate_device(dev);
>>>>>  	if (!input)
>>>>>  		return -ENOMEM;
>>>>>  
>>>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>  
>>>>>  	lradc->ts_input = input;
>>>>>  	input_set_drvdata(input, lradc);
>>>>> -	ret = input_register_device(input);
>>>>> -	if (ret)
>>>>> -		input_free_device(lradc->ts_input);
>>>>>  
>>>>> -	return ret;
>>>>> +	return input_register_device(input);
>>>>>  }
>>>>>  
>>>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>>>
>>>> Nice find.
>>>>
>>>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>>>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>>>> could use a proper fail path. Do you want to send patches or shall I ?
>>>>
>>> As this has been there a long time I'm not going to rush it in as a fix.
>> I did take a proper look today and it seems they do the right thing
>> afterall. I checked them with kmemleak too to be sure.
> 
> thanks, input_unregister_device already free the memory.
> 
> Sorry for the mess :-(
> 
> I think it would be the best to remove / revert this patch.

This one? Why exactly? Please elaborate some more, so it's possible to
understand the reasoning :)


-- 
Best regards,
Marek Vasut

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Stefan Wahren]

Hi Marek,

Am 19.04.2016 um 12:32 schrieb Marek Vasut:
> On 04/19/2016 08:33 AM, Stefan Wahren wrote:
>> Hi Marek,
>>
>> Am 18.04.2016 um 19:16 schrieb Marek Vasut:
>>> On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
>>>> On 14/04/16 21:01, Marek Vasut wrote:
>>>>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>>>>> After successful touchscreen registration the input device was
>>>>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>>>>
>>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>> ---
>>>>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>>>> index 33051b8..0576953 100644
>>>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>  {
>>>>>>  	struct input_dev *input;
>>>>>>  	struct device *dev = lradc->dev;
>>>>>> -	int ret;
>>>>>>  
>>>>>>  	if (!lradc->use_touchscreen)
>>>>>>  		return 0;
>>>>>>  
>>>>>> -	input = input_allocate_device();
>>>>>> +	input = devm_input_allocate_device(dev);
>>>>>>  	if (!input)
>>>>>>  		return -ENOMEM;
>>>>>>  
>>>>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>  
>>>>>>  	lradc->ts_input = input;
>>>>>>  	input_set_drvdata(input, lradc);
>>>>>> -	ret = input_register_device(input);
>>>>>> -	if (ret)
>>>>>> -		input_free_device(lradc->ts_input);
>>>>>>  
>>>>>> -	return ret;
>>>>>> +	return input_register_device(input);
>>>>>>  }
>>>>>>  
>>>>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>>>>
>>>>> Nice find.
>>>>>
>>>>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>>>>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>>>>> could use a proper fail path. Do you want to send patches or shall I ?
>>>>>
>>>> As this has been there a long time I'm not going to rush it in as a fix.
>>> I did take a proper look today and it seems they do the right thing
>>> afterall. I checked them with kmemleak too to be sure.
>> thanks, input_unregister_device already free the memory.
>>
>> Sorry for the mess :-(
>>
>> I think it would be the best to remove / revert this patch.
> This one? Why exactly? Please elaborate some more, so it's possible to
> understand the reasoning :)
>
>

as you already pointed out this is not a memory leak, because
input_unregister_device free the input device. So the commit message is
incorrect.

This commit also simplifies the code but in this case we should also
remove input_unregister_device.

Regards
Stefan

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Jonathan Cameron]

On 19/04/16 14:37, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 19.04.2016 um 12:32 schrieb Marek Vasut:
>> On 04/19/2016 08:33 AM, Stefan Wahren wrote:
>>> Hi Marek,
>>>
>>> Am 18.04.2016 um 19:16 schrieb Marek Vasut:
>>>> On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
>>>>> On 14/04/16 21:01, Marek Vasut wrote:
>>>>>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>>>>>> After successful touchscreen registration the input device was
>>>>>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>>>>>
>>>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>> ---
>>>>>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>>>>> index 33051b8..0576953 100644
>>>>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>>>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>>  {
>>>>>>>  	struct input_dev *input;
>>>>>>>  	struct device *dev = lradc->dev;
>>>>>>> -	int ret;
>>>>>>>  
>>>>>>>  	if (!lradc->use_touchscreen)
>>>>>>>  		return 0;
>>>>>>>  
>>>>>>> -	input = input_allocate_device();
>>>>>>> +	input = devm_input_allocate_device(dev);
>>>>>>>  	if (!input)
>>>>>>>  		return -ENOMEM;
>>>>>>>  
>>>>>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>>  
>>>>>>>  	lradc->ts_input = input;
>>>>>>>  	input_set_drvdata(input, lradc);
>>>>>>> -	ret = input_register_device(input);
>>>>>>> -	if (ret)
>>>>>>> -		input_free_device(lradc->ts_input);
>>>>>>>  
>>>>>>> -	return ret;
>>>>>>> +	return input_register_device(input);
>>>>>>>  }
>>>>>>>  
>>>>>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>>>>>
>>>>>> Nice find.
>>>>>>
>>>>>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>>>>>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>>>>>> could use a proper fail path. Do you want to send patches or shall I ?
>>>>>>
>>>>> As this has been there a long time I'm not going to rush it in as a fix.
>>>> I did take a proper look today and it seems they do the right thing
>>>> afterall. I checked them with kmemleak too to be sure.
>>> thanks, input_unregister_device already free the memory.
>>>
>>> Sorry for the mess :-(
>>>
>>> I think it would be the best to remove / revert this patch.
Removed it.  Had never pushed this out in a non rebasable branch so
easiest just to drop it.

Jonathan
>> This one? Why exactly? Please elaborate some more, so it's possible to
>> understand the reasoning :)
>>
>>
> 
> as you already pointed out this is not a memory leak, because
> input_unregister_device free the input device. So the commit message is
> incorrect.
> 
> This commit also simplifies the code but in this case we should also
> remove input_unregister_device.
> 
> Regards
> Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Marek Vasut]

On 04/19/2016 03:37 PM, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 19.04.2016 um 12:32 schrieb Marek Vasut:
>> On 04/19/2016 08:33 AM, Stefan Wahren wrote:
>>> Hi Marek,
>>>
>>> Am 18.04.2016 um 19:16 schrieb Marek Vasut:
>>>> On 04/17/2016 12:08 PM, Jonathan Cameron wrote:
>>>>> On 14/04/16 21:01, Marek Vasut wrote:
>>>>>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>>>>>> After successful touchscreen registration the input device was
>>>>>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>>>>>
>>>>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>> ---
>>>>>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>>>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>>>>> index 33051b8..0576953 100644
>>>>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>>>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>>  {
>>>>>>>  	struct input_dev *input;
>>>>>>>  	struct device *dev = lradc->dev;
>>>>>>> -	int ret;
>>>>>>>  
>>>>>>>  	if (!lradc->use_touchscreen)
>>>>>>>  		return 0;
>>>>>>>  
>>>>>>> -	input = input_allocate_device();
>>>>>>> +	input = devm_input_allocate_device(dev);
>>>>>>>  	if (!input)
>>>>>>>  		return -ENOMEM;
>>>>>>>  
>>>>>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>>>>>  
>>>>>>>  	lradc->ts_input = input;
>>>>>>>  	input_set_drvdata(input, lradc);
>>>>>>> -	ret = input_register_device(input);
>>>>>>> -	if (ret)
>>>>>>> -		input_free_device(lradc->ts_input);
>>>>>>>  
>>>>>>> -	return ret;
>>>>>>> +	return input_register_device(input);
>>>>>>>  }
>>>>>>>  
>>>>>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>>>>>
>>>>>> Nice find.
>>>>>>
>>>>>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>>>>>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>>>>>> could use a proper fail path. Do you want to send patches or shall I ?
>>>>>>
>>>>> As this has been there a long time I'm not going to rush it in as a fix.
>>>> I did take a proper look today and it seems they do the right thing
>>>> afterall. I checked them with kmemleak too to be sure.
>>> thanks, input_unregister_device already free the memory.
>>>
>>> Sorry for the mess :-(
>>>
>>> I think it would be the best to remove / revert this patch.
>> This one? Why exactly? Please elaborate some more, so it's possible to
>> understand the reasoning :)
>>
>>
> 
> as you already pointed out this is not a memory leak, because
> input_unregister_device free the input device. So the commit message is
> incorrect.

I see, yes, it's not a memleak.

> This commit also simplifies the code but in this case we should also
> remove input_unregister_device.

This matches the explanation in drivers/input/input.c above
input_register_device() , agreed.

> Regards
> Stefan
> 


-- 
Best regards,
Marek Vasut

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Stefan Wahren]

Hi Marek,

Am 14.04.2016 um 22:01 schrieb Marek Vasut:
> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>> After successful touchscreen registration the input device was
>> never freed. So fix this issue by using devm_input_allocate_device().
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>> index 33051b8..0576953 100644
>> --- a/drivers/iio/adc/mxs-lradc.c
>> +++ b/drivers/iio/adc/mxs-lradc.c
>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>  {
>>  	struct input_dev *input;
>>  	struct device *dev = lradc->dev;
>> -	int ret;
>>  
>>  	if (!lradc->use_touchscreen)
>>  		return 0;
>>  
>> -	input = input_allocate_device();
>> +	input = devm_input_allocate_device(dev);
>>  	if (!input)
>>  		return -ENOMEM;
>>  
>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>  
>>  	lradc->ts_input = input;
>>  	input_set_drvdata(input, lradc);
>> -	ret = input_register_device(input);
>> -	if (ret)
>> -		input_free_device(lradc->ts_input);
>>  
>> -	return ret;
>> +	return input_register_device(input);
>>  }
>>  
>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>
> Nice find.
>
> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
> could use a proper fail path. Do you want to send patches or shall I ?
>

since you found them it would be better if you handle them.

Regards
Stefan

[PATCH RFT 1/5] iio: mxs-lradc: fix memory leak

[Marek Vasut]

On 04/18/2016 08:19 AM, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 14.04.2016 um 22:01 schrieb Marek Vasut:
>> On 04/14/2016 05:48 PM, Stefan Wahren wrote:
>>> After successful touchscreen registration the input device was
>>> never freed. So fix this issue by using devm_input_allocate_device().
>>>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>>  drivers/iio/adc/mxs-lradc.c |    8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>> index 33051b8..0576953 100644
>>> --- a/drivers/iio/adc/mxs-lradc.c
>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>> @@ -1109,12 +1109,11 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>  {
>>>  	struct input_dev *input;
>>>  	struct device *dev = lradc->dev;
>>> -	int ret;
>>>  
>>>  	if (!lradc->use_touchscreen)
>>>  		return 0;
>>>  
>>> -	input = input_allocate_device();
>>> +	input = devm_input_allocate_device(dev);
>>>  	if (!input)
>>>  		return -ENOMEM;
>>>  
>>> @@ -1134,11 +1133,8 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
>>>  
>>>  	lradc->ts_input = input;
>>>  	input_set_drvdata(input, lradc);
>>> -	ret = input_register_device(input);
>>> -	if (ret)
>>> -		input_free_device(lradc->ts_input);
>>>  
>>> -	return ret;
>>> +	return input_register_device(input);
>>>  }
>>>  
>>>  static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>>>
>> Nice find.
>>
>> Looks like at91_adc.c and exynos_adc.c suffer from the exact same issue.
>> The leak looks a bit more severe on exynos even, exynos_adc_ts_init()
>> could use a proper fail path. Do you want to send patches or shall I ?
>>
> 
> since you found them it would be better if you handle them.

Roger. Since Atmel was so kind to provide sama5d2 and I also have
sama5d4 board here, I will check the Atmel one at least.

-- 
Best regards,
Marek Vasut

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

[Stefan Wahren]

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>
---
 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);
 
-- 
1.7.9.5

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

[Marek Vasut]

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>

> ---
>  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);
>  
> 


-- 
Best regards,
Marek Vasut

[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);
>>  
>>
> 
> 

[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

[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@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.

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

[PATCH RFT 3/5] iio: mxs-lradc: move STMP reset out of ADC init

[Stefan Wahren]

This patch moves the STMP reset out of ADC init function
in order to make it never fail. This is also necessary
for PM ops.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/iio/adc/mxs-lradc.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index 7118fca..95d84c9 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -1482,18 +1482,13 @@ static const struct iio_chan_spec mx28_lradc_chan_spec[] = {
 	MXS_ADC_CHAN(15, IIO_VOLTAGE, "VDD5V"),
 };
 
-static int mxs_lradc_hw_init(struct mxs_lradc *lradc)
+static void mxs_lradc_hw_init(struct mxs_lradc *lradc)
 {
 	/* The ADC always uses DELAY CHANNEL 0. */
 	const u32 adc_cfg =
 		(1 << (LRADC_DELAY_TRIGGER_DELAYS_OFFSET + 0)) |
 		(LRADC_DELAY_TIMER_PER << LRADC_DELAY_DELAY_OFFSET);
 
-	int ret = stmp_reset_block(lradc->base);
-
-	if (ret)
-		return ret;
-
 	/* Configure DELAY CHANNEL 0 for generic ADC sampling. */
 	mxs_lradc_reg_wrt(lradc, adc_cfg, LRADC_DELAY(0));
 
@@ -1504,8 +1499,6 @@ static int mxs_lradc_hw_init(struct mxs_lradc *lradc)
 
 	/* Start internal temperature sensing. */
 	mxs_lradc_reg_wrt(lradc, 0, LRADC_CTRL2);
-
-	return 0;
 }
 
 static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
@@ -1705,11 +1698,13 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* Configure the hardware. */
-	ret = mxs_lradc_hw_init(lradc);
+	ret = stmp_reset_block(lradc->base);
 	if (ret)
 		goto err_dev;
 
+	/* Configure the hardware. */
+	mxs_lradc_hw_init(lradc);
+
 	/* Register the touchscreen input device. */
 	if (touch_ret == 0) {
 		ret = mxs_lradc_ts_register(lradc);
-- 
1.7.9.5

[PATCH RFT 3/5] iio: mxs-lradc: move STMP reset out of ADC init

[Juergen Borleis]

Hi Stefan,

On Thursday 14 April 2016 17:48:27 Stefan Wahren wrote:
> This patch moves the STMP reset out of ADC init function
> in order to make it never fail. This is also necessary
> for PM ops.

What do you mean with "make it never fail"?

> [...]

jb

[PATCH RFT 3/5] iio: mxs-lradc: move STMP reset out of ADC init

[Marek Vasut]

On 04/15/2016 08:48 AM, Juergen Borleis wrote:
> Hi Stefan,
> 
> On Thursday 14 April 2016 17:48:27 Stefan Wahren wrote:
>> This patch moves the STMP reset out of ADC init function
>> in order to make it never fail. This is also necessary
>> for PM ops.
> 
> What do you mean with "make it never fail"?

He wants to make the function invoke only functions which don't return
error value. Not a great wording, but the patch makes it clear.

>> [...]
> 
> jb
> 


-- 
Best regards,
Marek Vasut

[PATCH RFT 3/5] iio: mxs-lradc: move STMP reset out of ADC init

[Jonathan Cameron]

On 15/04/16 11:12, Marek Vasut wrote:
> On 04/15/2016 08:48 AM, Juergen Borleis wrote:
>> Hi Stefan,
>>
>> On Thursday 14 April 2016 17:48:27 Stefan Wahren wrote:
>>> This patch moves the STMP reset out of ADC init function
>>> in order to make it never fail. This is also necessary
>>> for PM ops.
>>
>> What do you mean with "make it never fail"?
> 
> He wants to make the function invoke only functions which don't return
> error value. Not a great wording, but the patch makes it clear.
I've reworded to make this a little clearer and applied it to the togreg branch
of iio.git initially pushed out as testing for the autobuilders
to play with it (also giving time for any more comments on it to come through)

Jonathan
> 
>>> [...]
>>
>> jb
>>
> 
> 

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

[Stefan Wahren]

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. So we only need to care of the
mapped channels which are common for i.MX23 and i.MX28
and we remove the now unused function mxs_lradc_irq_en_mask.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/iio/adc/mxs-lradc.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index 95d84c9..97c993f 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -295,6 +295,7 @@ struct mxs_lradc {
 #define	LRADC_CTRL1_LRADC_IRQ_EN(n)		(1 << ((n) + 16))
 #define	LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK	(0x1fff << 16)
 #define	LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK	(0x01ff << 16)
+#define	LRADC_CTRL1_MAPPED_CHANS_IRQ_EN_MASK	(0x00ff << 16)
 #define	LRADC_CTRL1_LRADC_IRQ_EN_OFFSET		16
 #define	LRADC_CTRL1_TOUCH_DETECT_IRQ		BIT(8)
 #define	LRADC_CTRL1_LRADC_IRQ(n)		(1 << (n))
@@ -373,13 +374,6 @@ static u32 mxs_lradc_plate_mask(struct mxs_lradc *lradc)
 	return LRADC_CTRL0_MX28_PLATE_MASK;
 }
 
-static u32 mxs_lradc_irq_en_mask(struct mxs_lradc *lradc)
-{
-	if (lradc->soc == IMX23_LRADC)
-		return LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK;
-	return LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK;
-}
-
 static u32 mxs_lradc_irq_mask(struct mxs_lradc *lradc)
 {
 	if (lradc->soc == IMX23_LRADC)
@@ -1505,7 +1499,8 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
 {
 	int i;
 
-	mxs_lradc_reg_clear(lradc, mxs_lradc_irq_en_mask(lradc), LRADC_CTRL1);
+	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_MAPPED_CHANS_IRQ_EN_MASK,
+			    LRADC_CTRL1);
 
 	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
 		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
-- 
1.7.9.5

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

[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?

Jonathan
 So we only need to care of the
> mapped channels which are common for i.MX23 and i.MX28
> and we remove the now unused function mxs_lradc_irq_en_mask.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/iio/adc/mxs-lradc.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index 95d84c9..97c993f 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -295,6 +295,7 @@ struct mxs_lradc {
>  #define	LRADC_CTRL1_LRADC_IRQ_EN(n)		(1 << ((n) + 16))
>  #define	LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK	(0x1fff << 16)
>  #define	LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK	(0x01ff << 16)
> +#define	LRADC_CTRL1_MAPPED_CHANS_IRQ_EN_MASK	(0x00ff << 16)
>  #define	LRADC_CTRL1_LRADC_IRQ_EN_OFFSET		16
>  #define	LRADC_CTRL1_TOUCH_DETECT_IRQ		BIT(8)
>  #define	LRADC_CTRL1_LRADC_IRQ(n)		(1 << (n))
> @@ -373,13 +374,6 @@ static u32 mxs_lradc_plate_mask(struct mxs_lradc *lradc)
>  	return LRADC_CTRL0_MX28_PLATE_MASK;
>  }
>  
> -static u32 mxs_lradc_irq_en_mask(struct mxs_lradc *lradc)
> -{
> -	if (lradc->soc == IMX23_LRADC)
> -		return LRADC_CTRL1_MX23_LRADC_IRQ_EN_MASK;
> -	return LRADC_CTRL1_MX28_LRADC_IRQ_EN_MASK;
> -}
> -
>  static u32 mxs_lradc_irq_mask(struct mxs_lradc *lradc)
>  {
>  	if (lradc->soc == IMX23_LRADC)
> @@ -1505,7 +1499,8 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>  {
>  	int i;
>  
> -	mxs_lradc_reg_clear(lradc, mxs_lradc_irq_en_mask(lradc), LRADC_CTRL1);
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_MAPPED_CHANS_IRQ_EN_MASK,
> +			    LRADC_CTRL1);
>  
>  	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>  		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
> 

[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

[PATCH RFT 5/5] iio: mxs-lradc: implement suspend/resume support

[Stefan Wahren]

This patch implements suspend/resume support for mxs-lradc.
It's possible to use the touchscreen as wakeup source.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index 97c993f..64704c9 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -29,6 +29,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
 #include <linux/slab.h>
 #include <linux/stmp_device.h>
 #include <linux/sysfs.h>
@@ -1745,10 +1746,70 @@ static int mxs_lradc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int __maybe_unused mxs_lradc_suspend(struct device *dev)
+{
+	struct iio_dev *iio = dev_get_drvdata(dev);
+	struct mxs_lradc *lradc = iio_priv(iio);
+	struct input_dev *input = lradc->ts_input;
+	int ret = 0;
+
+	if (input) {
+		mutex_lock(&input->mutex);
+
+		/* Enable touchscreen wakeup irq */
+		if (input->users && device_may_wakeup(dev))
+			ret = enable_irq_wake(lradc->irq[0]);
+		else
+			mxs_lradc_disable_ts(lradc);
+
+		mutex_unlock(&input->mutex);
+	}
+
+	if (ret)
+		return ret;
+
+	mxs_lradc_hw_stop(lradc);
+
+	clk_disable_unprepare(lradc->clk);
+
+	return ret;
+}
+
+static int __maybe_unused mxs_lradc_resume(struct device *dev)
+{
+	struct iio_dev *iio = dev_get_drvdata(dev);
+	struct mxs_lradc *lradc = iio_priv(iio);
+	struct input_dev *input = lradc->ts_input;
+	int ret;
+
+	ret = clk_prepare_enable(lradc->clk);
+	if (ret)
+		return ret;
+
+	mxs_lradc_hw_init(lradc);
+
+	if (input) {
+		mutex_lock(&input->mutex);
+
+		/* Disable touchscreen wakeup irq */
+		if (input->users && device_may_wakeup(dev))
+			ret = disable_irq_wake(lradc->irq[0]);
+		else
+			mxs_lradc_enable_touch_detection(lradc);
+
+		mutex_unlock(&input->mutex);
+	}
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(mxs_lradc_pm_ops, mxs_lradc_suspend, mxs_lradc_resume);
+
 static struct platform_driver mxs_lradc_driver = {
 	.driver	= {
 		.name	= DRIVER_NAME,
 		.of_match_table = mxs_lradc_dt_ids,
+		.pm	= &mxs_lradc_pm_ops,
 	},
 	.probe	= mxs_lradc_probe,
 	.remove	= mxs_lradc_remove,
-- 
1.7.9.5

[PATCH RFT 5/5] iio: mxs-lradc: implement suspend/resume support

[Jonathan Cameron]

On 14/04/16 16:48, Stefan Wahren wrote:
> This patch implements suspend/resume support for mxs-lradc.
> It's possible to use the touchscreen as wakeup source.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Looks good to me pending replies to the previous patch comments.
> ---
>  drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index 97c993f..64704c9 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -29,6 +29,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
>  #include <linux/slab.h>
>  #include <linux/stmp_device.h>
>  #include <linux/sysfs.h>
> @@ -1745,10 +1746,70 @@ static int mxs_lradc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int __maybe_unused mxs_lradc_suspend(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct mxs_lradc *lradc = iio_priv(iio);
> +	struct input_dev *input = lradc->ts_input;
> +	int ret = 0;
> +
> +	if (input) {
> +		mutex_lock(&input->mutex);
> +
> +		/* Enable touchscreen wakeup irq */
> +		if (input->users && device_may_wakeup(dev))
> +			ret = enable_irq_wake(lradc->irq[0]);
> +		else
> +			mxs_lradc_disable_ts(lradc);
> +
> +		mutex_unlock(&input->mutex);
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	mxs_lradc_hw_stop(lradc);
> +
> +	clk_disable_unprepare(lradc->clk);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused mxs_lradc_resume(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct mxs_lradc *lradc = iio_priv(iio);
> +	struct input_dev *input = lradc->ts_input;
> +	int ret;
> +
> +	ret = clk_prepare_enable(lradc->clk);
> +	if (ret)
> +		return ret;
> +
> +	mxs_lradc_hw_init(lradc);
> +
> +	if (input) {
> +		mutex_lock(&input->mutex);
> +
> +		/* Disable touchscreen wakeup irq */
> +		if (input->users && device_may_wakeup(dev))
> +			ret = disable_irq_wake(lradc->irq[0]);
> +		else
> +			mxs_lradc_enable_touch_detection(lradc);
> +
> +		mutex_unlock(&input->mutex);
> +	}
> +
> +	return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(mxs_lradc_pm_ops, mxs_lradc_suspend, mxs_lradc_resume);
> +
>  static struct platform_driver mxs_lradc_driver = {
>  	.driver	= {
>  		.name	= DRIVER_NAME,
>  		.of_match_table = mxs_lradc_dt_ids,
> +		.pm	= &mxs_lradc_pm_ops,
>  	},
>  	.probe	= mxs_lradc_probe,
>  	.remove	= mxs_lradc_remove,
> 

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

[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>

-- 
Best regards,
Marek Vasut

[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?

[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@denx.de>
>>
> 
> 
> 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