linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver
@ 2014-04-26 11:37 Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes Naveen Krishna Chatradhi
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23

This patchset fixes the 
1. bug causing a crash during module removal for exynos_adc.ko.
-> The bug was seen by Doug, while trying to compile the whole IIO subsystem
   as module @ https://lkml.org/lkml/2014/4/21/481 from Doug.

2. rearrange the clock and regulator enable/disable calls during
   probe, remove, suspend and resume calls
-> Comments give by Jonathan @ https://lkml.org/lkml/2014/4/23/644

3. reduces the timeout and uses wait_for_completion_timeout instead of the
   interruptible varient.
-> This change was under review @ https://lkml.org/lkml/2013/11/5/92
   Final comments were given by Tomasz, to split and submit.

Naveen Krishna Ch (2):
  iio: exynos_adc: use indio_dev->dev structure to handle child nodes
  iio: exynos_adc: rearrange clk and regulator enable/disable calls

Naveen Krishna Chatradhi (3):
  iio: exynos_adc: reduce timeout and use wait_for_completion_timeout
  iio: exynos_adc: do a soft reset in case of timeout
  iio: exynos_adc: do a reinit_completion before the conversion

 drivers/iio/adc/exynos_adc.c |  138 +++++++++++++++++++++++-------------------
 1 file changed, 75 insertions(+), 63 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes
  2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
@ 2014-04-26 11:37 ` Naveen Krishna Chatradhi
  2014-04-26 12:53   ` Jonathan Cameron
  2014-04-26 11:37 ` [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls Naveen Krishna Chatradhi
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23, Naveen Krishna Ch

From: Naveen Krishna Ch <ch.naveen@samsung.com>

Using pdev->dev with device_for_each_child() would iterate over all
of the children of the platform device and delete them.
Thus, causing crashes during module unload.

We should be using the indio_dev->dev structure for
registering/unregistering child nodes.

Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
Reported-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
---
This change was tested on top of 
   https://lkml.org/lkml/2014/4/21/481 from Doug.

 drivers/iio/adc/exynos_adc.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index d25b262..affa93f 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -344,7 +344,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 
 	exynos_adc_hw_init(info);
 
-	ret = of_platform_populate(np, exynos_adc_match, NULL, &pdev->dev);
+	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed adding child nodes\n");
 		goto err_of_populate;
@@ -353,7 +353,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	return 0;
 
 err_of_populate:
-	device_for_each_child(&pdev->dev, NULL,
+	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
 	regulator_disable(info->vdd);
 	clk_disable_unprepare(info->clk);
@@ -369,7 +369,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct exynos_adc *info = iio_priv(indio_dev);
 
-	device_for_each_child(&pdev->dev, NULL,
+	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
 	regulator_disable(info->vdd);
 	clk_disable_unprepare(info->clk);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls
  2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes Naveen Krishna Chatradhi
@ 2014-04-26 11:37 ` Naveen Krishna Chatradhi
  2014-04-28 22:18   ` Doug Anderson
  2014-04-26 11:37 ` [PATCH 3/5 v2] iio: exynos_adc: reduce timeout and use wait_for_completion_timeout Naveen Krishna Chatradhi
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23, Naveen Krishna Ch

From: Naveen Krishna Ch <ch.naveen@samsung.com>

This patch maintains the following order in
probe(), remove(), resume() and suspend() calls

regulator enable, clk prepare enable
...
clk disable unprepare, regulator disable

While at it,
1. enable the regulator before the iio_device_register()
2. handle the return values for enable/disable calls

Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
---
Changes since v1:
corrected the register/unregister and enabling/disbaling sequences

 drivers/iio/adc/exynos_adc.c |   63 +++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index affa93f..0df8916 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -290,32 +290,30 @@ static int exynos_adc_probe(struct platform_device *pdev)
 
 	init_completion(&info->completion);
 
-	ret = request_irq(info->irq, exynos_adc_isr,
-					0, dev_name(&pdev->dev), info);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
-							info->irq);
-		return ret;
-	}
-
-	writel(1, info->enable_reg);
-
 	info->clk = devm_clk_get(&pdev->dev, "adc");
 	if (IS_ERR(info->clk)) {
 		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
 							PTR_ERR(info->clk));
-		ret = PTR_ERR(info->clk);
-		goto err_irq;
+		return PTR_ERR(info->clk);
 	}
 
 	info->vdd = devm_regulator_get(&pdev->dev, "vdd");
 	if (IS_ERR(info->vdd)) {
 		dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
 							PTR_ERR(info->vdd));
-		ret = PTR_ERR(info->vdd);
-		goto err_irq;
+		return PTR_ERR(info->vdd);
 	}
 
+	ret = regulator_enable(info->vdd);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret)
+		goto err_disable_reg;
+
+	writel(1, info->enable_reg);
+
 	info->version = exynos_adc_get_version(pdev);
 
 	platform_set_drvdata(pdev, indio_dev);
@@ -332,16 +330,18 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	else
 		indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
 
+	ret = request_irq(info->irq, exynos_adc_isr,
+					0, dev_name(&pdev->dev), info);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
+							info->irq);
+		goto err_disable_clk;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret)
 		goto err_irq;
 
-	ret = regulator_enable(info->vdd);
-	if (ret)
-		goto err_iio_dev;
-
-	clk_prepare_enable(info->clk);
-
 	exynos_adc_hw_init(info);
 
 	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
@@ -355,12 +355,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
 err_of_populate:
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
-	regulator_disable(info->vdd);
-	clk_disable_unprepare(info->clk);
-err_iio_dev:
 	iio_device_unregister(indio_dev);
 err_irq:
 	free_irq(info->irq, info);
+err_disable_clk:
+	writel(0, info->enable_reg);
+	clk_disable_unprepare(info->clk);
+err_disable_reg:
+	regulator_disable(info->vdd);
 	return ret;
 }
 
@@ -371,11 +373,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
 
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
-	regulator_disable(info->vdd);
-	clk_disable_unprepare(info->clk);
-	writel(0, info->enable_reg);
 	iio_device_unregister(indio_dev);
 	free_irq(info->irq, info);
+	writel(0, info->enable_reg);
+	clk_disable_unprepare(info->clk);
+	regulator_disable(info->vdd);
+
 
 	return 0;
 }
@@ -397,8 +400,8 @@ static int exynos_adc_suspend(struct device *dev)
 		writel(con, ADC_V1_CON(info->regs));
 	}
 
-	clk_disable_unprepare(info->clk);
 	writel(0, info->enable_reg);
+	clk_disable_unprepare(info->clk);
 	regulator_disable(info->vdd);
 
 	return 0;
@@ -414,9 +417,11 @@ static int exynos_adc_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	writel(1, info->enable_reg);
-	clk_prepare_enable(info->clk);
+	ret = clk_prepare_enable(info->clk);
+	if (ret)
+		return ret;
 
+	writel(1, info->enable_reg);
 	exynos_adc_hw_init(info);
 
 	return 0;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/5 v2] iio: exynos_adc: reduce timeout and use wait_for_completion_timeout
  2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls Naveen Krishna Chatradhi
@ 2014-04-26 11:37 ` Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 4/5 v2] iio: exynos_adc: do a soft reset in case of timeout Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 5/5 v2] iio: exynos_adc: do a reinit_completion before the conversion Naveen Krishna Chatradhi
  4 siblings, 0 replies; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23

ADC module on Exynos5 SoCs runs at 600KSPS. At this conversion rate,
waiting for 1000 msecs is wasteful (incase of h/w failure).

Hence, reduce the time out to 100msecs and use
wait_for_completion_timeout() instead of
wait_for_completion_interruptible_timeout()

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
This change is a part of the patch reviewd at https://lkml.org/lkml/2013/11/5/92

 drivers/iio/adc/exynos_adc.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index a2b8b1a..4d2467a 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -82,7 +82,7 @@ enum adc_version {
 #define ADC_CON_EN_START	(1u << 0)
 #define ADC_DATX_MASK		0xFFF
 
-#define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(1000))
+#define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
 
 struct exynos_adc {
 	void __iomem		*regs;
@@ -121,6 +121,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	struct exynos_adc *info = iio_priv(indio_dev);
 	unsigned long timeout;
 	u32 con1, con2;
+	int ret;
 
 	if (mask != IIO_CHAN_INFO_RAW)
 		return -EINVAL;
@@ -145,16 +146,19 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 				ADC_V1_CON(info->regs));
 	}
 
-	timeout = wait_for_completion_interruptible_timeout
+	timeout = wait_for_completion_timeout
 			(&info->completion, EXYNOS_ADC_TIMEOUT);
-	*val = info->value;
+	if (timeout == 0) {
+		ret = -ETIMEDOUT;
+	} else {
+		*val = info->value;
+		*val2 = 0;
+		ret = IIO_VAL_INT;
+	}
 
 	mutex_unlock(&indio_dev->mlock);
 
-	if (timeout == 0)
-		return -ETIMEDOUT;
-
-	return IIO_VAL_INT;
+	return ret;
 }
 
 static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/5 v2] iio: exynos_adc: do a soft reset in case of timeout
  2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
                   ` (2 preceding siblings ...)
  2014-04-26 11:37 ` [PATCH 3/5 v2] iio: exynos_adc: reduce timeout and use wait_for_completion_timeout Naveen Krishna Chatradhi
@ 2014-04-26 11:37 ` Naveen Krishna Chatradhi
  2014-04-26 11:37 ` [PATCH 5/5 v2] iio: exynos_adc: do a reinit_completion before the conversion Naveen Krishna Chatradhi
  4 siblings, 0 replies; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23

Do a soft reset software if a timeout happens.
This is applicable only for ADC_V2.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
This change is a part of the patch reviewed at https://lkml.org/lkml/2013/11/5/92

 drivers/iio/adc/exynos_adc.c |   50 ++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 4d2467a..805c9f6 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -112,6 +112,30 @@ static inline unsigned int exynos_adc_get_version(struct platform_device *pdev)
 	return (unsigned int)match->data;
 }
 
+static void exynos_adc_hw_init(struct exynos_adc *info)
+{
+	u32 con1, con2;
+
+	if (info->version == ADC_V2) {
+		con1 = ADC_V2_CON1_SOFT_RESET;
+		writel(con1, ADC_V2_CON1(info->regs));
+
+		con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
+			ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
+		writel(con2, ADC_V2_CON2(info->regs));
+
+		/* Enable interrupts */
+		writel(1, ADC_V2_INT_EN(info->regs));
+	} else {
+		/* set default prescaler values and Enable prescaler */
+		con1 =  ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
+
+		/* Enable 12-bit ADC resolution */
+		con1 |= ADC_V1_CON_RES;
+		writel(con1, ADC_V1_CON(info->regs));
+	}
+}
+
 static int exynos_read_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int *val,
@@ -149,6 +173,8 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	timeout = wait_for_completion_timeout
 			(&info->completion, EXYNOS_ADC_TIMEOUT);
 	if (timeout == 0) {
+		dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
+		exynos_adc_hw_init(info);
 		ret = -ETIMEDOUT;
 	} else {
 		*val = info->value;
@@ -230,30 +256,6 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
 	return 0;
 }
 
-static void exynos_adc_hw_init(struct exynos_adc *info)
-{
-	u32 con1, con2;
-
-	if (info->version == ADC_V2) {
-		con1 = ADC_V2_CON1_SOFT_RESET;
-		writel(con1, ADC_V2_CON1(info->regs));
-
-		con2 = ADC_V2_CON2_OSEL | ADC_V2_CON2_ESEL |
-			ADC_V2_CON2_HIGHF | ADC_V2_CON2_C_TIME(0);
-		writel(con2, ADC_V2_CON2(info->regs));
-
-		/* Enable interrupts */
-		writel(1, ADC_V2_INT_EN(info->regs));
-	} else {
-		/* set default prescaler values and Enable prescaler */
-		con1 =  ADC_V1_CON_PRSCLV(49) | ADC_V1_CON_PRSCEN;
-
-		/* Enable 12-bit ADC resolution */
-		con1 |= ADC_V1_CON_RES;
-		writel(con1, ADC_V1_CON(info->regs));
-	}
-}
-
 static int exynos_adc_probe(struct platform_device *pdev)
 {
 	struct exynos_adc *info = NULL;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/5 v2] iio: exynos_adc: do a reinit_completion before the conversion
  2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
                   ` (3 preceding siblings ...)
  2014-04-26 11:37 ` [PATCH 4/5 v2] iio: exynos_adc: do a soft reset in case of timeout Naveen Krishna Chatradhi
@ 2014-04-26 11:37 ` Naveen Krishna Chatradhi
  4 siblings, 0 replies; 8+ messages in thread
From: Naveen Krishna Chatradhi @ 2014-04-26 11:37 UTC (permalink / raw)
  To: linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler, jic23

Add reinit_completion() before the wait_for_completion_timeout in
raw_read() call.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Reviewed-by: Doug Anderson <dianders@chromium.org>
---
 drivers/iio/adc/exynos_adc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 805c9f6..32290e6 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -151,6 +151,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	mutex_lock(&indio_dev->mlock);
+	reinit_completion(&info->completion);
 
 	/* Select the channel to be used and Trigger conversion */
 	if (info->version == ADC_V2) {
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes
  2014-04-26 11:37 ` [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes Naveen Krishna Chatradhi
@ 2014-04-26 12:53   ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2014-04-26 12:53 UTC (permalink / raw)
  To: Naveen Krishna Chatradhi, linux-iio
  Cc: linux-kernel, linux-samsung-soc, dianders, gregkh,
	naveenkrishna.ch, lars, cpgs, grundler

On 26/04/14 12:37, Naveen Krishna Chatradhi wrote:
> From: Naveen Krishna Ch <ch.naveen@samsung.com>
>
> Using pdev->dev with device_for_each_child() would iterate over all
> of the children of the platform device and delete them.
> Thus, causing crashes during module unload.
>
> We should be using the indio_dev->dev structure for
> registering/unregistering child nodes.
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
> Reported-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> ---
Oops, I applied this one earlier (from v1) but didn't send the email...
Never mind as there were no changes.
> This change was tested on top of
>     https://lkml.org/lkml/2014/4/21/481 from Doug.
>
>   drivers/iio/adc/exynos_adc.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index d25b262..affa93f 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -344,7 +344,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
>   	exynos_adc_hw_init(info);
>
> -	ret = of_platform_populate(np, exynos_adc_match, NULL, &pdev->dev);
> +	ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev, "failed adding child nodes\n");
>   		goto err_of_populate;
> @@ -353,7 +353,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
>   	return 0;
>
>   err_of_populate:
> -	device_for_each_child(&pdev->dev, NULL,
> +	device_for_each_child(&indio_dev->dev, NULL,
>   				exynos_adc_remove_devices);
>   	regulator_disable(info->vdd);
>   	clk_disable_unprepare(info->clk);
> @@ -369,7 +369,7 @@ static int exynos_adc_remove(struct platform_device *pdev)
>   	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>   	struct exynos_adc *info = iio_priv(indio_dev);
>
> -	device_for_each_child(&pdev->dev, NULL,
> +	device_for_each_child(&indio_dev->dev, NULL,
>   				exynos_adc_remove_devices);
>   	regulator_disable(info->vdd);
>   	clk_disable_unprepare(info->clk);
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls
  2014-04-26 11:37 ` [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls Naveen Krishna Chatradhi
@ 2014-04-28 22:18   ` Doug Anderson
  0 siblings, 0 replies; 8+ messages in thread
From: Doug Anderson @ 2014-04-28 22:18 UTC (permalink / raw)
  To: Naveen Krishna Chatradhi
  Cc: linux-iio, linux-kernel, linux-samsung-soc, Greg Kroah-Hartman,
	Naveen Krishna, Lars-Peter Clausen, cpgs .,
	Grant Grundler, Jonathan Cameron

Naveen,

On Sat, Apr 26, 2014 at 4:37 AM, Naveen Krishna Chatradhi
<ch.naveen@samsung.com> wrote:
> From: Naveen Krishna Ch <ch.naveen@samsung.com>
>
> This patch maintains the following order in
> probe(), remove(), resume() and suspend() calls
>
> regulator enable, clk prepare enable
> ...
> clk disable unprepare, regulator disable
>
> While at it,
> 1. enable the regulator before the iio_device_register()
> 2. handle the return values for enable/disable calls
>
> Signed-off-by: Naveen Krishna Ch <ch.naveen@samsung.com>
> ---
> Changes since v1:
> corrected the register/unregister and enabling/disbaling sequences
>
>  drivers/iio/adc/exynos_adc.c |   63 +++++++++++++++++++++++-------------------
>  1 file changed, 34 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index affa93f..0df8916 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -290,32 +290,30 @@ static int exynos_adc_probe(struct platform_device *pdev)
>
>         init_completion(&info->completion);
>
> -       ret = request_irq(info->irq, exynos_adc_isr,
> -                                       0, dev_name(&pdev->dev), info);
> -       if (ret < 0) {
> -               dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
> -                                                       info->irq);
> -               return ret;
> -       }
> -
> -       writel(1, info->enable_reg);
> -
>         info->clk = devm_clk_get(&pdev->dev, "adc");
>         if (IS_ERR(info->clk)) {
>                 dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
>                                                         PTR_ERR(info->clk));
> -               ret = PTR_ERR(info->clk);
> -               goto err_irq;
> +               return PTR_ERR(info->clk);
>         }
>
>         info->vdd = devm_regulator_get(&pdev->dev, "vdd");
>         if (IS_ERR(info->vdd)) {
>                 dev_err(&pdev->dev, "failed getting regulator, err = %ld\n",
>                                                         PTR_ERR(info->vdd));
> -               ret = PTR_ERR(info->vdd);
> -               goto err_irq;
> +               return PTR_ERR(info->vdd);
>         }
>
> +       ret = regulator_enable(info->vdd);
> +       if (ret)
> +               return ret;
> +
> +       ret = clk_prepare_enable(info->clk);
> +       if (ret)
> +               goto err_disable_reg;
> +
> +       writel(1, info->enable_reg);
> +
>         info->version = exynos_adc_get_version(pdev);
>
>         platform_set_drvdata(pdev, indio_dev);
> @@ -332,16 +330,18 @@ static int exynos_adc_probe(struct platform_device *pdev)
>         else
>                 indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
>
> +       ret = request_irq(info->irq, exynos_adc_isr,
> +                                       0, dev_name(&pdev->dev), info);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
> +                                                       info->irq);
> +               goto err_disable_clk;
> +       }
> +
>         ret = iio_device_register(indio_dev);
>         if (ret)
>                 goto err_irq;
>
> -       ret = regulator_enable(info->vdd);
> -       if (ret)
> -               goto err_iio_dev;
> -
> -       clk_prepare_enable(info->clk);
> -
>         exynos_adc_hw_init(info);
>
>         ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
> @@ -355,12 +355,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
>  err_of_populate:
>         device_for_each_child(&indio_dev->dev, NULL,
>                                 exynos_adc_remove_devices);
> -       regulator_disable(info->vdd);
> -       clk_disable_unprepare(info->clk);
> -err_iio_dev:
>         iio_device_unregister(indio_dev);
>  err_irq:
>         free_irq(info->irq, info);
> +err_disable_clk:
> +       writel(0, info->enable_reg);
> +       clk_disable_unprepare(info->clk);
> +err_disable_reg:
> +       regulator_disable(info->vdd);
>         return ret;
>  }
>
> @@ -371,11 +373,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
>
>         device_for_each_child(&indio_dev->dev, NULL,
>                                 exynos_adc_remove_devices);
> -       regulator_disable(info->vdd);
> -       clk_disable_unprepare(info->clk);
> -       writel(0, info->enable_reg);
>         iio_device_unregister(indio_dev);
>         free_irq(info->irq, info);
> +       writel(0, info->enable_reg);
> +       clk_disable_unprepare(info->clk);
> +       regulator_disable(info->vdd);
> +

nit: one too many blank lines here

>         return 0;
>  }
> @@ -397,8 +400,8 @@ static int exynos_adc_suspend(struct device *dev)
>                 writel(con, ADC_V1_CON(info->regs));
>         }
>
> -       clk_disable_unprepare(info->clk);
>         writel(0, info->enable_reg);
> +       clk_disable_unprepare(info->clk);
>         regulator_disable(info->vdd);
>
>         return 0;
> @@ -414,9 +417,11 @@ static int exynos_adc_resume(struct device *dev)
>         if (ret)
>                 return ret;
>
> -       writel(1, info->enable_reg);
> -       clk_prepare_enable(info->clk);
> +       ret = clk_prepare_enable(info->clk);
> +       if (ret)
> +               return ret;
>
> +       writel(1, info->enable_reg);
>         exynos_adc_hw_init(info);
>
>         return 0;
> --
> 1.7.9.5
>

Other than nit, looks good to me.

Reviewed-by: Doug Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-04-28 22:18 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-26 11:37 [PATCH 0/5 v2] iio: exynos_adc: fix minor nits in the driver Naveen Krishna Chatradhi
2014-04-26 11:37 ` [PATCH 1/5 v2] iio: exynos_adc: use indio_dev->dev structure to handle child nodes Naveen Krishna Chatradhi
2014-04-26 12:53   ` Jonathan Cameron
2014-04-26 11:37 ` [PATCH 2/5 v2] iio: exynos_adc: rearrange clk and regulator enable/disable calls Naveen Krishna Chatradhi
2014-04-28 22:18   ` Doug Anderson
2014-04-26 11:37 ` [PATCH 3/5 v2] iio: exynos_adc: reduce timeout and use wait_for_completion_timeout Naveen Krishna Chatradhi
2014-04-26 11:37 ` [PATCH 4/5 v2] iio: exynos_adc: do a soft reset in case of timeout Naveen Krishna Chatradhi
2014-04-26 11:37 ` [PATCH 5/5 v2] iio: exynos_adc: do a reinit_completion before the conversion Naveen Krishna Chatradhi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).