iio: mxs-lradc: implement PM ops

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

drivers/iio/adc/mxs-lradc.c +2 -6

index ad26da1..223650b 100644
@@ -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)

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

drivers/iio/adc/mxs-lradc.c +0 -0

index ad26da1..223650b 100644

drivers/iio/adc/mxs-lradc.c +1 -4

 	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)

drivers/iio/adc/mxs-lradc.c +0 -1

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

Signature · 1 line

1.7.9.5

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 <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

drivers/iio/adc/mxs-lradc.c +0 -7

index 223650b..cfc558f 100644
@@ -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)

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +1 -0

index cfc558f..6bd387f 100644
@@ -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>

drivers/iio/adc/mxs-lradc.c +52 -0

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

Quoted 24 lines

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +1 -0

index cfc558f..6bd387f 100644
@@ -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;
 }
 

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1753

Quoted 5 lines

drivers/iio/adc/mxs-lradc.c +5 -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.

Quoted 54 lines

drivers/iio/adc/mxs-lradc.c +52 -0

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

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1815

Quoted 10 lines

drivers/iio/adc/mxs-lradc.c +3 -0

+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 <[email protected]>

Hi Marek,

Am 21.04.2016 um 22:45 schrieb Marek Vasut:

Quoted 25 lines · Stefan Wahren

Quoted 24 lines

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +1 -0

index cfc558f..6bd387f 100644
@@ -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;
 }
 

Quoted 8 lines

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1753

Quoted 5 lines

drivers/iio/adc/mxs-lradc.c +5 -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

|

Quoted 68 lines

Quoted 54 lines

drivers/iio/adc/mxs-lradc.c +52 -0

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

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1815

Quoted 10 lines

drivers/iio/adc/mxs-lradc.c +3 -0

+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 <[email protected]>

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +12 -0

index cfc558f..6bd387f 100644
@@ -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 */

drivers/iio/adc/mxs-lradc.c +17 -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)

drivers/iio/adc/mxs-lradc.c +12 -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,

Signature · 1 line

1.7.9.5

Quoted 35 lines

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +12 -0

index cfc558f..6bd387f 100644
@@ -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 */

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1769

Quoted 10 lines

drivers/iio/adc/mxs-lradc.c +10 -0

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

Hi Dmitry,

Am 21.04.2016 um 23:08 schrieb Dmitry Torokhov:

Quoted 48 lines · Stefan Wahren

Quoted 35 lines

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

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/iio/adc/mxs-lradc.c |   61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/adc/mxs-lradc.c +12 -0

index cfc558f..6bd387f 100644
@@ -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 */

drivers/iio/adc/mxs-lradc.c static int mxs_lradc_remove(struct platform_device *pdev) · line 1769

Quoted 10 lines

drivers/iio/adc/mxs-lradc.c +10 -0

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

Quoted 1 line

			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.