[PATCH 0/6] i.MX7D ADC improvements

[PATCH 0/6] i.MX7D ADC improvements

[Andrey Smirnov]

Everyone:

This series is a number of small improvements I made to i.MX7D ADC
driver. Hopefully each patch is self-explanatory.

Feedback is welcome!

Thanks,
Andrey Smirnov

Andrey Smirnov (6):
  iio: imx7d_adc: Add local struct device pointer in imx7d_adc_probe()
  iio: imx7d_adc: Replace pr_err with dev_err
  iio: imx7d_adc: Use devm_iio_device_register()
  iio: imx7d_adc: Use devm_platform_ioremap_resource()
  iio: imx7d_adc: Use imx7d_adc_resume() in imx7d_adc_probe()
  iio: imx7d_adc: Drop imx7d_adc_remove()

 drivers/iio/adc/imx7d_adc.c | 175 +++++++++++++++---------------------
 1 file changed, 73 insertions(+), 102 deletions(-)

-- 
2.20.1

[PATCH 1/6] iio: imx7d_adc: Add local struct device pointer in imx7d_adc_probe()

[Andrey Smirnov]

Use a local "struct device *dev" in imx7d_adc_probe() for brevity. No
functional change intended.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index ad6764fb2a23..3bbd657409d5 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -438,51 +438,51 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	struct imx7d_adc *info;
 	struct iio_dev *indio_dev;
 	struct resource *mem;
+	struct device *dev = &pdev->dev;
 	int irq;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
 	if (!indio_dev) {
 		dev_err(&pdev->dev, "Failed allocating iio device\n");
 		return -ENOMEM;
 	}
 
 	info = iio_priv(indio_dev);
-	info->dev = &pdev->dev;
+	info->dev = dev;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	info->regs = devm_ioremap_resource(&pdev->dev, mem);
+	info->regs = devm_ioremap_resource(dev, mem);
 	if (IS_ERR(info->regs)) {
 		ret = PTR_ERR(info->regs);
-		dev_err(&pdev->dev,
-			"Failed to remap adc memory, err = %d\n", ret);
+		dev_err(dev, "Failed to remap adc memory, err = %d\n", ret);
 		return ret;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
-		dev_err(&pdev->dev, "No irq resource?\n");
+		dev_err(dev, "No irq resource?\n");
 		return irq;
 	}
 
-	info->clk = devm_clk_get(&pdev->dev, "adc");
+	info->clk = devm_clk_get(dev, "adc");
 	if (IS_ERR(info->clk)) {
 		ret = PTR_ERR(info->clk);
-		dev_err(&pdev->dev, "Failed getting clock, err = %d\n", ret);
+		dev_err(dev, "Failed getting clock, err = %d\n", ret);
 		return ret;
 	}
 
-	info->vref = devm_regulator_get(&pdev->dev, "vref");
+	info->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(info->vref)) {
 		ret = PTR_ERR(info->vref);
-		dev_err(&pdev->dev,
+		dev_err(dev,
 			"Failed getting reference voltage, err = %d\n", ret);
 		return ret;
 	}
 
 	ret = regulator_enable(info->vref);
 	if (ret) {
-		dev_err(&pdev->dev,
+		dev_err(dev,
 			"Can't enable adc reference top voltage, err = %d\n",
 			ret);
 		return ret;
@@ -492,8 +492,8 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 
 	init_completion(&info->completion);
 
-	indio_dev->name = dev_name(&pdev->dev);
-	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(dev);
+	indio_dev->dev.parent = dev;
 	indio_dev->info = &imx7d_adc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = imx7d_adc_iio_channels;
@@ -501,16 +501,15 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 
 	ret = clk_prepare_enable(info->clk);
 	if (ret) {
-		dev_err(&pdev->dev,
-			"Could not prepare or enable the clock.\n");
+		dev_err(dev, "Could not prepare or enable the clock.\n");
 		goto error_adc_clk_enable;
 	}
 
-	ret = devm_request_irq(info->dev, irq,
-				imx7d_adc_isr, 0,
-				dev_name(&pdev->dev), info);
+	ret = devm_request_irq(dev, irq,
+			       imx7d_adc_isr, 0,
+			       dev_name(dev), info);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed requesting irq, irq = %d\n", irq);
+		dev_err(dev, "Failed requesting irq, irq = %d\n", irq);
 		goto error_iio_device_register;
 	}
 
-- 
2.20.1

Re: [PATCH 1/6] iio: imx7d_adc: Add local struct device pointer in imx7d_adc_probe()

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:20 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Use a local "struct device *dev" in imx7d_adc_probe() for brevity. No
> functional change intended.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
+CC Haibo. Not heard from Haibo recently though so may well have moved on...
As such I'm not going to wait for Haibo, but am not planning to do a pull
request this week, so plenty of time for feedback!

This one is in the (hopefully) obviously correct category.
Somewhat minor benefit, but meh, if you are going to work on the driver
then you get to do things like this if you want to ;)

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

Thanks,

Jonathan


> ---
>  drivers/iio/adc/imx7d_adc.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index ad6764fb2a23..3bbd657409d5 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -438,51 +438,51 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	struct imx7d_adc *info;
>  	struct iio_dev *indio_dev;
>  	struct resource *mem;
> +	struct device *dev = &pdev->dev;
>  	int irq;
>  	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
>  	if (!indio_dev) {
>  		dev_err(&pdev->dev, "Failed allocating iio device\n");
>  		return -ENOMEM;
>  	}
>  
>  	info = iio_priv(indio_dev);
> -	info->dev = &pdev->dev;
> +	info->dev = dev;
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	info->regs = devm_ioremap_resource(dev, mem);
>  	if (IS_ERR(info->regs)) {
>  		ret = PTR_ERR(info->regs);
> -		dev_err(&pdev->dev,
> -			"Failed to remap adc memory, err = %d\n", ret);
> +		dev_err(dev, "Failed to remap adc memory, err = %d\n", ret);
>  		return ret;
>  	}
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> -		dev_err(&pdev->dev, "No irq resource?\n");
> +		dev_err(dev, "No irq resource?\n");
>  		return irq;
>  	}
>  
> -	info->clk = devm_clk_get(&pdev->dev, "adc");
> +	info->clk = devm_clk_get(dev, "adc");
>  	if (IS_ERR(info->clk)) {
>  		ret = PTR_ERR(info->clk);
> -		dev_err(&pdev->dev, "Failed getting clock, err = %d\n", ret);
> +		dev_err(dev, "Failed getting clock, err = %d\n", ret);
>  		return ret;
>  	}
>  
> -	info->vref = devm_regulator_get(&pdev->dev, "vref");
> +	info->vref = devm_regulator_get(dev, "vref");
>  	if (IS_ERR(info->vref)) {
>  		ret = PTR_ERR(info->vref);
> -		dev_err(&pdev->dev,
> +		dev_err(dev,
>  			"Failed getting reference voltage, err = %d\n", ret);
>  		return ret;
>  	}
>  
>  	ret = regulator_enable(info->vref);
>  	if (ret) {
> -		dev_err(&pdev->dev,
> +		dev_err(dev,
>  			"Can't enable adc reference top voltage, err = %d\n",
>  			ret);
>  		return ret;
> @@ -492,8 +492,8 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  
>  	init_completion(&info->completion);
>  
> -	indio_dev->name = dev_name(&pdev->dev);
> -	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = dev_name(dev);
> +	indio_dev->dev.parent = dev;
>  	indio_dev->info = &imx7d_adc_iio_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = imx7d_adc_iio_channels;
> @@ -501,16 +501,15 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  
>  	ret = clk_prepare_enable(info->clk);
>  	if (ret) {
> -		dev_err(&pdev->dev,
> -			"Could not prepare or enable the clock.\n");
> +		dev_err(dev, "Could not prepare or enable the clock.\n");
>  		goto error_adc_clk_enable;
>  	}
>  
> -	ret = devm_request_irq(info->dev, irq,
> -				imx7d_adc_isr, 0,
> -				dev_name(&pdev->dev), info);
> +	ret = devm_request_irq(dev, irq,
> +			       imx7d_adc_isr, 0,
> +			       dev_name(dev), info);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "Failed requesting irq, irq = %d\n", irq);
> +		dev_err(dev, "Failed requesting irq, irq = %d\n", irq);
>  		goto error_iio_device_register;
>  	}
>  

[PATCH 2/6] iio: imx7d_adc: Replace pr_err with dev_err

[Andrey Smirnov]

Replace combination of pr_err()/dev_name() with an equivalent call for
dev_err().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 3bbd657409d5..72cfe9834bcb 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -388,8 +388,9 @@ static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
 	 * timeout flags.
 	 */
 	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_CONV_TIME_OUT) {
-		pr_err("%s: ADC got conversion time out interrupt: 0x%08x\n",
-			dev_name(info->dev), status);
+		dev_err(info->dev,
+			"ADC got conversion time out interrupt: 0x%08x\n",
+			status);
 		status &= ~IMX7D_REG_ADC_INT_STATUS_CHANNEL_CONV_TIME_OUT;
 		writel(status, info->regs + IMX7D_REG_ADC_INT_STATUS);
 	}
-- 
2.20.1

Re: [PATCH 2/6] iio: imx7d_adc: Replace pr_err with dev_err

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:21 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Replace combination of pr_err()/dev_name() with an equivalent call for
> dev_err().
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Applied, thanks,

Jonathan

> ---
>  drivers/iio/adc/imx7d_adc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 3bbd657409d5..72cfe9834bcb 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -388,8 +388,9 @@ static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
>  	 * timeout flags.
>  	 */
>  	if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_CONV_TIME_OUT) {
> -		pr_err("%s: ADC got conversion time out interrupt: 0x%08x\n",
> -			dev_name(info->dev), status);
> +		dev_err(info->dev,
> +			"ADC got conversion time out interrupt: 0x%08x\n",
> +			status);
>  		status &= ~IMX7D_REG_ADC_INT_STATUS_CHANNEL_CONV_TIME_OUT;
>  		writel(status, info->regs + IMX7D_REG_ADC_INT_STATUS);
>  	}

[PATCH 3/6] iio: imx7d_adc: Use devm_iio_device_register()

[Andrey Smirnov]

Use devm_iio_device_register() and drop explicit call to
iio_device_unregister().

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 72cfe9834bcb..9a46838ec7cf 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -517,7 +517,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	imx7d_adc_feature_config(info);
 	imx7d_adc_hw_init(info);
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret) {
 		imx7d_adc_power_down(info);
 		dev_err(&pdev->dev, "Couldn't register the device.\n");
@@ -539,8 +539,6 @@ static int imx7d_adc_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct imx7d_adc *info = iio_priv(indio_dev);
 
-	iio_device_unregister(indio_dev);
-
 	imx7d_adc_power_down(info);
 
 	clk_disable_unprepare(info->clk);
-- 
2.20.1

Re: [PATCH 3/6] iio: imx7d_adc: Use devm_iio_device_register()

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:22 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Use devm_iio_device_register() and drop explicit call to
> iio_device_unregister().
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
No to this one.  The thing to think about is the resulting order
of the unwinding that happens in remove.

Do take a look at the code flow, but in short what happens is:

1. driver.remove()
2. Devm release functions run in the opposite order to they were
   called during setup.

The upshot of the change you just made here is that we turn the power
off to the device before we remove the userspace interfaces, giving
potentially interesting failures..

There are two options to avoid this:

1. Make everything use devm_ calls (often using devm_add_action_or_reset
for the ones that don't have their own versions).

2. Stop using devm managed functions at the first non devm_ call that needs
unwinding.  From that point onwards in probe / remove you have to do everything
manually.

Jonathan

> ---
>  drivers/iio/adc/imx7d_adc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 72cfe9834bcb..9a46838ec7cf 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -517,7 +517,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	imx7d_adc_feature_config(info);
>  	imx7d_adc_hw_init(info);
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
>  		imx7d_adc_power_down(info);
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
> @@ -539,8 +539,6 @@ static int imx7d_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct imx7d_adc *info = iio_priv(indio_dev);
>  
> -	iio_device_unregister(indio_dev);
> -
>  	imx7d_adc_power_down(info);
>  
>  	clk_disable_unprepare(info->clk);

Re: [PATCH 3/6] iio: imx7d_adc: Use devm_iio_device_register()

[Andrey Smirnov]

On Sun, Apr 7, 2019 at 4:07 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed,  3 Apr 2019 00:03:22 -0700
> Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> > Use devm_iio_device_register() and drop explicit call to
> > iio_device_unregister().
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Hartmut Knaack <knaack.h@gmx.de>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: linux-iio@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> No to this one.  The thing to think about is the resulting order
> of the unwinding that happens in remove.
>
> Do take a look at the code flow, but in short what happens is:
>
> 1. driver.remove()
> 2. Devm release functions run in the opposite order to they were
>    called during setup.
>
> The upshot of the change you just made here is that we turn the power
> off to the device before we remove the userspace interfaces, giving
> potentially interesting failures..
>
> There are two options to avoid this:
>
> 1. Make everything use devm_ calls (often using devm_add_action_or_reset
> for the ones that don't have their own versions).
>
> 2. Stop using devm managed functions at the first non devm_ call that needs
> unwinding.  From that point onwards in probe / remove you have to do everything
> manually.
>

Yeah, missed the ordering proble, sorry about that. I'll re-order
changes such that this conversion happens last to avoid said problem
in v2.

Thanks,
Andrey Smirnov

[PATCH 4/6] iio: imx7d_adc: Use devm_platform_ioremap_resource()

[Andrey Smirnov]

Use devm_platform_ioremap_resource() to be able to drop a bit of
explicit boilerplate code.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 9a46838ec7cf..8bba92611d54 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -438,7 +438,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 {
 	struct imx7d_adc *info;
 	struct iio_dev *indio_dev;
-	struct resource *mem;
 	struct device *dev = &pdev->dev;
 	int irq;
 	int ret;
@@ -452,8 +451,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	info = iio_priv(indio_dev);
 	info->dev = dev;
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	info->regs = devm_ioremap_resource(dev, mem);
+	info->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(info->regs)) {
 		ret = PTR_ERR(info->regs);
 		dev_err(dev, "Failed to remap adc memory, err = %d\n", ret);
-- 
2.20.1

Re: [PATCH 4/6] iio: imx7d_adc: Use devm_platform_ioremap_resource()

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:23 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Use devm_platform_ioremap_resource() to be able to drop a bit of
> explicit boilerplate code.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Great. That one is new to me. Looks like it only got added this cycle.

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

Thanks,

Jonathan
> ---
>  drivers/iio/adc/imx7d_adc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 9a46838ec7cf..8bba92611d54 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -438,7 +438,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  {
>  	struct imx7d_adc *info;
>  	struct iio_dev *indio_dev;
> -	struct resource *mem;
>  	struct device *dev = &pdev->dev;
>  	int irq;
>  	int ret;
> @@ -452,8 +451,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	info = iio_priv(indio_dev);
>  	info->dev = dev;
>  
> -	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	info->regs = devm_ioremap_resource(dev, mem);
> +	info->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(info->regs)) {
>  		ret = PTR_ERR(info->regs);
>  		dev_err(dev, "Failed to remap adc memory, err = %d\n", ret);

[PATCH 5/6] iio: imx7d_adc: Use imx7d_adc_resume() in imx7d_adc_probe()

[Andrey Smirnov]

Initialization sequence performed in imx7d_adc_resume() is exactley
the same as what's being done in imx7d_adc_probe(). Make use of the
former in the latter to avoid code duplication.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 86 +++++++++++++++----------------------
 1 file changed, 35 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index 8bba92611d54..c2fa0ab923ee 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -434,6 +434,33 @@ static void imx7d_adc_power_down(struct imx7d_adc *info)
 	writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
 }
 
+static int imx7d_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx7d_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	ret = regulator_enable(info->vref);
+	if (ret) {
+		dev_err(info->dev,
+			"Can't enable adc reference top voltage, err = %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(info->dev,
+			"Could not prepare or enable clock.\n");
+		regulator_disable(info->vref);
+		return ret;
+	}
+
+	imx7d_adc_hw_init(info);
+
+	return 0;
+}
+
 static int imx7d_adc_probe(struct platform_device *pdev)
 {
 	struct imx7d_adc *info;
@@ -479,14 +506,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = regulator_enable(info->vref);
-	if (ret) {
-		dev_err(dev,
-			"Can't enable adc reference top voltage, err = %d\n",
-			ret);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, indio_dev);
 
 	init_completion(&info->completion);
@@ -498,38 +517,30 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	indio_dev->channels = imx7d_adc_iio_channels;
 	indio_dev->num_channels = ARRAY_SIZE(imx7d_adc_iio_channels);
 
-	ret = clk_prepare_enable(info->clk);
-	if (ret) {
-		dev_err(dev, "Could not prepare or enable the clock.\n");
-		goto error_adc_clk_enable;
-	}
-
 	ret = devm_request_irq(dev, irq,
 			       imx7d_adc_isr, 0,
 			       dev_name(dev), info);
 	if (ret < 0) {
 		dev_err(dev, "Failed requesting irq, irq = %d\n", irq);
-		goto error_iio_device_register;
+		return ret;
 	}
 
 	imx7d_adc_feature_config(info);
-	imx7d_adc_hw_init(info);
+
+	ret = imx7d_adc_resume(&indio_dev->dev);
+	if (ret)
+		return ret;
 
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret) {
 		imx7d_adc_power_down(info);
+		clk_disable_unprepare(info->clk);
+		regulator_disable(info->vref);
 		dev_err(&pdev->dev, "Couldn't register the device.\n");
-		goto error_iio_device_register;
+		return ret;
 	}
 
 	return 0;
-
-error_iio_device_register:
-	clk_disable_unprepare(info->clk);
-error_adc_clk_enable:
-	regulator_disable(info->vref);
-
-	return ret;
 }
 
 static int imx7d_adc_remove(struct platform_device *pdev)
@@ -558,33 +569,6 @@ static int __maybe_unused imx7d_adc_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused imx7d_adc_resume(struct device *dev)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct imx7d_adc *info = iio_priv(indio_dev);
-	int ret;
-
-	ret = regulator_enable(info->vref);
-	if (ret) {
-		dev_err(info->dev,
-			"Can't enable adc reference top voltage, err = %d\n",
-			ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(info->clk);
-	if (ret) {
-		dev_err(info->dev,
-			"Could not prepare or enable clock.\n");
-		regulator_disable(info->vref);
-		return ret;
-	}
-
-	imx7d_adc_hw_init(info);
-
-	return 0;
-}
-
 static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
 
 static struct platform_driver imx7d_adc_driver = {
-- 
2.20.1

Re: [PATCH 5/6] iio: imx7d_adc: Use imx7d_adc_resume() in imx7d_adc_probe()

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:24 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Initialization sequence performed in imx7d_adc_resume() is exactley
> the same as what's being done in imx7d_adc_probe(). Make use of the
> former in the latter to avoid code duplication.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
I'm not to going to apply this now, due to the ordering issue with the earlier
and later patches.  However, it is fine in of itself.

I do wonder if we want to rename resume to something else to reflect
it's new usage?  imx7d_adc_enable perhaps?
Similar question for suspend in the next patch.

Thanks,

Jonathan



> ---
>  drivers/iio/adc/imx7d_adc.c | 86 +++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 8bba92611d54..c2fa0ab923ee 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -434,6 +434,33 @@ static void imx7d_adc_power_down(struct imx7d_adc *info)
>  	writel(adc_cfg, info->regs + IMX7D_REG_ADC_ADC_CFG);
>  }
>  
> +static int imx7d_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_enable(info->vref);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"Can't enable adc reference top voltage, err = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret) {
> +		dev_err(info->dev,
> +			"Could not prepare or enable clock.\n");
> +		regulator_disable(info->vref);
> +		return ret;
> +	}
> +
> +	imx7d_adc_hw_init(info);
> +
> +	return 0;
> +}
> +
>  static int imx7d_adc_probe(struct platform_device *pdev)
>  {
>  	struct imx7d_adc *info;
> @@ -479,14 +506,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = regulator_enable(info->vref);
> -	if (ret) {
> -		dev_err(dev,
> -			"Can't enable adc reference top voltage, err = %d\n",
> -			ret);
> -		return ret;
> -	}
> -
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	init_completion(&info->completion);
> @@ -498,38 +517,30 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	indio_dev->channels = imx7d_adc_iio_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(imx7d_adc_iio_channels);
>  
> -	ret = clk_prepare_enable(info->clk);
> -	if (ret) {
> -		dev_err(dev, "Could not prepare or enable the clock.\n");
> -		goto error_adc_clk_enable;
> -	}
> -
>  	ret = devm_request_irq(dev, irq,
>  			       imx7d_adc_isr, 0,
>  			       dev_name(dev), info);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed requesting irq, irq = %d\n", irq);
> -		goto error_iio_device_register;
> +		return ret;
>  	}
>  
>  	imx7d_adc_feature_config(info);
> -	imx7d_adc_hw_init(info);
> +
> +	ret = imx7d_adc_resume(&indio_dev->dev);
> +	if (ret)
> +		return ret;
>  
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
>  		imx7d_adc_power_down(info);
> +		clk_disable_unprepare(info->clk);
> +		regulator_disable(info->vref);
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
> -		goto error_iio_device_register;
> +		return ret;
>  	}
>  
>  	return 0;
> -
> -error_iio_device_register:
> -	clk_disable_unprepare(info->clk);
> -error_adc_clk_enable:
> -	regulator_disable(info->vref);
> -
> -	return ret;
>  }
>  
>  static int imx7d_adc_remove(struct platform_device *pdev)
> @@ -558,33 +569,6 @@ static int __maybe_unused imx7d_adc_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int __maybe_unused imx7d_adc_resume(struct device *dev)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct imx7d_adc *info = iio_priv(indio_dev);
> -	int ret;
> -
> -	ret = regulator_enable(info->vref);
> -	if (ret) {
> -		dev_err(info->dev,
> -			"Can't enable adc reference top voltage, err = %d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(info->clk);
> -	if (ret) {
> -		dev_err(info->dev,
> -			"Could not prepare or enable clock.\n");
> -		regulator_disable(info->vref);
> -		return ret;
> -	}
> -
> -	imx7d_adc_hw_init(info);
> -
> -	return 0;
> -}
> -
>  static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
>  
>  static struct platform_driver imx7d_adc_driver = {

Re: [PATCH 5/6] iio: imx7d_adc: Use imx7d_adc_resume() in imx7d_adc_probe()

[Andrey Smirnov]

On Sun, Apr 7, 2019 at 4:15 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed,  3 Apr 2019 00:03:24 -0700
> Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> > Initialization sequence performed in imx7d_adc_resume() is exactley
> > the same as what's being done in imx7d_adc_probe(). Make use of the
> > former in the latter to avoid code duplication.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Cc: Hartmut Knaack <knaack.h@gmx.de>
> > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > Cc: Chris Healy <cphealy@gmail.com>
> > Cc: linux-iio@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> I'm not to going to apply this now, due to the ordering issue with the earlier
> and later patches.  However, it is fine in of itself.
>
> I do wonder if we want to rename resume to something else to reflect
> it's new usage?  imx7d_adc_enable perhaps?
> Similar question for suspend in the next patch.

Sure, will rename in v2.

Thanks,
Andrey Smirnov

[PATCH 6/6] iio: imx7d_adc: Drop imx7d_adc_remove()

[Andrey Smirnov]

Since imx7d_adc_remove() does exactly the same thing as
imx7d_adc_suspend() we can use the latter together with
devm_add_action_or_reset() to get rid of the former.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Chris Healy <cphealy@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/iio/adc/imx7d_adc.c | 55 ++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
index c2fa0ab923ee..ed95909eada2 100644
--- a/drivers/iio/adc/imx7d_adc.c
+++ b/drivers/iio/adc/imx7d_adc.c
@@ -461,6 +461,24 @@ static int imx7d_adc_resume(struct device *dev)
 	return 0;
 }
 
+static int imx7d_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct imx7d_adc *info = iio_priv(indio_dev);
+
+	imx7d_adc_power_down(info);
+
+	clk_disable_unprepare(info->clk);
+	regulator_disable(info->vref);
+
+	return 0;
+}
+
+static void __imx7d_adc_suspend(void *data)
+{
+	imx7d_adc_suspend(data);
+}
+
 static int imx7d_adc_probe(struct platform_device *pdev)
 {
 	struct imx7d_adc *info;
@@ -506,8 +524,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	platform_set_drvdata(pdev, indio_dev);
-
 	init_completion(&info->completion);
 
 	indio_dev->name = dev_name(dev);
@@ -531,11 +547,13 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, __imx7d_adc_suspend,
+				       &indio_dev->dev);
+	if (ret)
+		return ret;
+
 	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret) {
-		imx7d_adc_power_down(info);
-		clk_disable_unprepare(info->clk);
-		regulator_disable(info->vref);
 		dev_err(&pdev->dev, "Couldn't register the device.\n");
 		return ret;
 	}
@@ -543,37 +561,10 @@ static int imx7d_adc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int imx7d_adc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct imx7d_adc *info = iio_priv(indio_dev);
-
-	imx7d_adc_power_down(info);
-
-	clk_disable_unprepare(info->clk);
-	regulator_disable(info->vref);
-
-	return 0;
-}
-
-static int __maybe_unused imx7d_adc_suspend(struct device *dev)
-{
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
-	struct imx7d_adc *info = iio_priv(indio_dev);
-
-	imx7d_adc_power_down(info);
-
-	clk_disable_unprepare(info->clk);
-	regulator_disable(info->vref);
-
-	return 0;
-}
-
 static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
 
 static struct platform_driver imx7d_adc_driver = {
 	.probe		= imx7d_adc_probe,
-	.remove		= imx7d_adc_remove,
 	.driver		= {
 		.name	= "imx7d_adc",
 		.of_match_table = imx7d_adc_match,
-- 
2.20.1

Re: [PATCH 6/6] iio: imx7d_adc: Drop imx7d_adc_remove()

[Jonathan Cameron]

On Wed,  3 Apr 2019 00:03:25 -0700
Andrey Smirnov <andrew.smirnov@gmail.com> wrote:

> Since imx7d_adc_remove() does exactly the same thing as
> imx7d_adc_suspend() we can use the latter together with
> devm_add_action_or_reset() to get rid of the former.
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Chris Healy <cphealy@gmail.com>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Ah. OK, so you end up with one of the options I listed when rejecting
the earlier change for iio_device_register.

Please merge this patch with that one so we have a single change and don't
end up with a race condition prone state in between. Separate patches
are fine as well, but work your way down through probe rather than jumping
around like now.  Hence we'll have something valid at all stages.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/imx7d_adc.c | 55 ++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index c2fa0ab923ee..ed95909eada2 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -461,6 +461,24 @@ static int imx7d_adc_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int imx7d_adc_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct imx7d_adc *info = iio_priv(indio_dev);
> +
> +	imx7d_adc_power_down(info);
> +
> +	clk_disable_unprepare(info->clk);
> +	regulator_disable(info->vref);
> +
> +	return 0;
> +}
> +
> +static void __imx7d_adc_suspend(void *data)
> +{
> +	imx7d_adc_suspend(data);
> +}
> +
>  static int imx7d_adc_probe(struct platform_device *pdev)
>  {
>  	struct imx7d_adc *info;
> @@ -506,8 +524,6 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	platform_set_drvdata(pdev, indio_dev);
> -
>  	init_completion(&info->completion);
>  
>  	indio_dev->name = dev_name(dev);
> @@ -531,11 +547,13 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_add_action_or_reset(dev, __imx7d_adc_suspend,
> +				       &indio_dev->dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
> -		imx7d_adc_power_down(info);
> -		clk_disable_unprepare(info->clk);
> -		regulator_disable(info->vref);
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
>  		return ret;
>  	}
> @@ -543,37 +561,10 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int imx7d_adc_remove(struct platform_device *pdev)
> -{
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct imx7d_adc *info = iio_priv(indio_dev);
> -
> -	imx7d_adc_power_down(info);
> -
> -	clk_disable_unprepare(info->clk);
> -	regulator_disable(info->vref);
> -
> -	return 0;
> -}
> -
> -static int __maybe_unused imx7d_adc_suspend(struct device *dev)
> -{
> -	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> -	struct imx7d_adc *info = iio_priv(indio_dev);
> -
> -	imx7d_adc_power_down(info);
> -
> -	clk_disable_unprepare(info->clk);
> -	regulator_disable(info->vref);
> -
> -	return 0;
> -}
> -
>  static SIMPLE_DEV_PM_OPS(imx7d_adc_pm_ops, imx7d_adc_suspend, imx7d_adc_resume);
>  
>  static struct platform_driver imx7d_adc_driver = {
>  	.probe		= imx7d_adc_probe,
> -	.remove		= imx7d_adc_remove,
>  	.driver		= {
>  		.name	= "imx7d_adc",
>  		.of_match_table = imx7d_adc_match,