[PATCH RFT V2 0/3] iio: mxs-lradc: implement PM ops

[PATCH RFT V2 0/3] iio: mxs-lradc: implement PM ops

[Stefan Wahren]

This patch series simplifies TS registration and implements suspend/resume
support for mxs-lradc.

Patch 1: simplify TS registration
Patch 2: refactor mxs-lradc in order to prepare PM implementation
Patch 3: 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.

Changes in V2:
- rebase on iio/testing since patch 2 and 3 from V1 has been applied
- also remove input_unregister_device in patch 1
- use mask of virtual channels for IRQ disabling in patch 2

Stefan Wahren (3):
  iio: mxs-lradc: simplify TS registration
  iio: mxs-lradc: disable only masked channels in mxs_lradc_hw_stop
  iio: mxs-lradc: implement suspend/resume support

 drivers/iio/adc/mxs-lradc.c |   81 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCH RFT V2 1/3] iio: mxs-lradc: simplify TS registration

[Stefan Wahren]

This patch simplifies the TS registration of mxs-lradc by
using devm_input_allocate_device.

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

diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index ad26da1..223650b 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -1120,12 +1120,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;
 
@@ -1146,11 +1145,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)
@@ -1159,7 +1155,6 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
 		return;
 
 	mxs_lradc_disable_ts(lradc);
-	input_unregister_device(lradc->ts_input);
 }
 
 /*
-- 
1.7.9.5

Re: [PATCH RFT V2 1/3] iio: mxs-lradc: simplify TS registration

[Marek Vasut]

On 04/21/2016 10:11 PM, Stefan Wahren wrote:
> This patch simplifies the TS registration of mxs-lradc by
> using devm_input_allocate_device.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
>  drivers/iio/adc/mxs-lradc.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index ad26da1..223650b 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -1120,12 +1120,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;
>  
> @@ -1146,11 +1145,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)
> @@ -1159,7 +1155,6 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>  		return;
>  
>  	mxs_lradc_disable_ts(lradc);
> -	input_unregister_device(lradc->ts_input);
>  }
>  
>  /*
> 
This seems to be done right now :)

Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

-- 
Best regards,
Marek Vasut

Re: [PATCH RFT V2 1/3] iio: mxs-lradc: simplify TS registration

[Dmitry Torokhov]

On Thu, Apr 21, 2016 at 08:11:16PM +0000, Stefan Wahren wrote:
> This patch simplifies the TS registration of mxs-lradc by
> using devm_input_allocate_device.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/iio/adc/mxs-lradc.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index ad26da1..223650b 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -1120,12 +1120,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);

Also please drop "input->dev.parent = dev;" below.

>  	if (!input)
>  		return -ENOMEM;
>  
> @@ -1146,11 +1145,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)
> @@ -1159,7 +1155,6 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>  		return;
>  
>  	mxs_lradc_disable_ts(lradc);

I do not think this is needed, since mxs_lradc_ts_close() does this for
us (and it will be called automatically if input device is open at
unregistration time), and if you remove this you can remove
mxs_lradc_ts_unregister() altogether.

> -	input_unregister_device(lradc->ts_input);
>  }
>  
>  /*
> -- 
> 1.7.9.5
> 

Thanks.

-- 
Dmitry

[PATCH RFT V2 2/3] iio: mxs-lradc: disable only masked channels in mxs_lradc_hw_stop

[Stefan Wahren]

Disabling of the touchscreen IRQs should be done in
mxs_lradc_disable_ts. So disable only the masked virtual channels
in mxs_lradc_hw_stop and finally remove the unused function
mxs_lradc_irq_en_mask.

Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
 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 223650b..cfc558f 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -373,13 +373,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 +1498,9 @@ 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->buffer_vchans << LRADC_CTRL1_LRADC_IRQ_EN_OFFSET,
+		LRADC_CTRL1);
 
 	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
 		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
-- 
1.7.9.5

Re: [PATCH RFT V2 2/3] iio: mxs-lradc: disable only masked channels in mxs_lradc_hw_stop

[Marek Vasut]

On 04/21/2016 10:11 PM, Stefan Wahren wrote:
> Disabling of the touchscreen IRQs should be done in
> mxs_lradc_disable_ts. So disable only the masked virtual channels
> in mxs_lradc_hw_stop and finally remove the unused function
> mxs_lradc_irq_en_mask.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
>  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 223650b..cfc558f 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -373,13 +373,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 +1498,9 @@ 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->buffer_vchans << LRADC_CTRL1_LRADC_IRQ_EN_OFFSET,
> +		LRADC_CTRL1);
>  
>  	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>  		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
> 
Looks sane too:

Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

-- 
Best regards,
Marek Vasut

[PATCH RFT V2 3/3] 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 cfc558f..6bd387f 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

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Marek Vasut]

On 04/21/2016 10:11 PM, 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-eS4NqCHxEME@public.gmane.org>
> ---
>  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 cfc558f..6bd387f 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;

Can lradc->ts_input contain some non-valid pointer ? You should make
sure it's either inited to NULL or valid pointer, which I'm not sure
you do.

> +	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,
> 
Looks OK otherwise:

Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

-- 
Best regards,
Marek Vasut

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Stefan Wahren]

Hi Marek,

Am 21.04.2016 um 22:45 schrieb Marek Vasut:
> On 04/21/2016 10:11 PM, 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-eS4NqCHxEME@public.gmane.org>
>> ---
>>  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 cfc558f..6bd387f 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;
> Can lradc->ts_input contain some non-valid pointer ? You should make
> sure it's either inited to NULL or valid pointer, which I'm not sure
> you do.

the whole content of lradc get initialized with zero by
devm_iio_device_alloc.

Or do you mean the error case that input registration failed and a
suspend occur (if it's possible)|?

Stefan

|
>
>> +	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,
>>
> Looks OK otherwise:
>
> Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Marek Vasut]

On 04/22/2016 03:57 PM, Stefan Wahren wrote:
> Hi Marek,
> 
> Am 21.04.2016 um 22:45 schrieb Marek Vasut:
>> On 04/21/2016 10:11 PM, 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>
>>> ---
>>>  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 cfc558f..6bd387f 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;
>> Can lradc->ts_input contain some non-valid pointer ? You should make
>> sure it's either inited to NULL or valid pointer, which I'm not sure
>> you do.
> 
> the whole content of lradc get initialized with zero by
> devm_iio_device_alloc.

Ah, good.

> Or do you mean the error case that input registration failed and a
> suspend occur (if it's possible)|?

No, you cannot get suspend call before you registered the device.

> Stefan
> 
> |
>>
>>> +	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,
>>>
>> Looks OK otherwise:
>>
>> Reviewed-by: Marek Vasut <marex@denx.de>
>>
> 
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Dmitry Torokhov]

On Thu, Apr 21, 2016 at 08:11:18PM +0000, 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-eS4NqCHxEME@public.gmane.org>
> ---
>  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 cfc558f..6bd387f 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;

I'd rather we had it right after we do:

			ret = enable_irq_wake(lradc->irq[0]);

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

Do we need to fail resume if this call fails?

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

Thanks.

-- 
Dmitry

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Stefan Wahren]

Hi Dmitry,

Am 21.04.2016 um 23:08 schrieb Dmitry Torokhov:
> On Thu, Apr 21, 2016 at 08:11:18PM +0000, 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-eS4NqCHxEME@public.gmane.org>
>> ---
>>  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 cfc558f..6bd387f 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;
> I'd rather we had it right after we do:
>
> 			ret = enable_irq_wake(lradc->irq[0]);

but we must unlock the mutex first before return. An option would be to
place the return inside the input branch.

Stefan

Re: [PATCH RFT V2 3/3] iio: mxs-lradc: implement suspend/resume support

[Dmitry Torokhov]

On Fri, Apr 22, 2016 at 03:32:44PM +0200, Stefan Wahren wrote:
> Hi Dmitry,
> 
> Am 21.04.2016 um 23:08 schrieb Dmitry Torokhov:
> > On Thu, Apr 21, 2016 at 08:11:18PM +0000, 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-eS4NqCHxEME@public.gmane.org>
> >> ---
> >>  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 cfc558f..6bd387f 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;
> > I'd rather we had it right after we do:
> >
> > 			ret = enable_irq_wake(lradc->irq[0]);
> 
> but we must unlock the mutex first before return. An option would be to
> place the return inside the input branch.

Ah, right, ignore me then.

-- 
Dmitry