diff mbox series

[v6,4/9] iio: adc: mcp3911: use resource-managed version of iio_device_register

Message ID 20220815061625.35568-5-marcus.folkesson@gmail.com (mailing list archive)
State Accepted
Headers show
Series Improve MCP3911 driver | expand

Commit Message

Marcus Folkesson Aug. 15, 2022, 6:16 a.m. UTC 
Keep using managed resources as much as possible.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/iio/adc/mcp3911.c | 53 ++++++++++++---------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

Comments

Jonathan Cameron Aug. 20, 2022, 12:41 p.m. UTC | #1 
On Mon, 15 Aug 2022 08:16:20 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Keep using managed resources as much as possible.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/adc/mcp3911.c | 53 ++++++++++++---------------------------
>  1 file changed, 16 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> index 890af7dca62d..7e2efe702e57 100644
> --- a/drivers/iio/adc/mcp3911.c
> +++ b/drivers/iio/adc/mcp3911.c
> @@ -258,6 +258,13 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
>  }
>  
> +static void mcp3911_cleanup_regulator(void *_adc)

Missed this on previous versions, but why not pass
the regulator pointer in as the parameter for the callback?

static void mcp391_cleanup_regulator(void *reg)
{
	regulator_disable(adc->vref);
}

Note this can't use the new devm_regulator_get_enable()
because we need access to the regulator within the driver.

I can tidy this up whilst applying (or given it's really minor I might
not bother :)

Note we are stalled at the moment with this series on getting the
fixes upstream.  I'll probably send that pull request shortly.

> +{
> +	struct mcp3911 *adc = _adc;
> +
> +	regulator_disable(adc->vref);
> +}
> +
>  static int mcp3911_probe(struct spi_device *spi)
>  {
>  	struct iio_dev *indio_dev;
> @@ -286,9 +293,14 @@ static int mcp3911_probe(struct spi_device *spi)
>  		ret = regulator_enable(adc->vref);
>  		if (ret)
>  			return ret;
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +				mcp3911_cleanup_regulator, adc);
> +		if (ret)
> +			return ret;
>  	}
>  
> -	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
> +	adc->clki = devm_clk_get_enabled(&adc->spi->dev, NULL);
>  	if (IS_ERR(adc->clki)) {
>  		if (PTR_ERR(adc->clki) == -ENOENT) {
>  			adc->clki = NULL;
> @@ -296,21 +308,13 @@ static int mcp3911_probe(struct spi_device *spi)
>  			dev_err(&adc->spi->dev,
>  				"failed to get adc clk (%ld)\n",
>  				PTR_ERR(adc->clki));
> -			ret = PTR_ERR(adc->clki);
> -			goto reg_disable;
> -		}
> -	} else {
> -		ret = clk_prepare_enable(adc->clki);
> -		if (ret < 0) {
> -			dev_err(&adc->spi->dev,
> -				"Failed to enable clki: %d\n", ret);
> -			goto reg_disable;
> +			return PTR_ERR(adc->clki);
>  		}
>  	}
>  
>  	ret = mcp3911_config(adc);
>  	if (ret)
> -		goto clk_disable;
> +		return ret;
>  
>  	indio_dev->name = spi_get_device_id(spi)->name;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -322,31 +326,7 @@ static int mcp3911_probe(struct spi_device *spi)
>  
>  	mutex_init(&adc->lock);
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		goto clk_disable;
> -
> -	return ret;
> -
> -clk_disable:
> -	clk_disable_unprepare(adc->clki);
> -reg_disable:
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
> -
> -	return ret;
> -}
> -
> -static void mcp3911_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct mcp3911 *adc = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	clk_disable_unprepare(adc->clki);
> -	if (adc->vref)
> -		regulator_disable(adc->vref);
> +	return devm_iio_device_register(&adc->spi->dev, indio_dev);
>  }
>  
>  static const struct of_device_id mcp3911_dt_ids[] = {
> @@ -367,7 +347,6 @@ static struct spi_driver mcp3911_driver = {
>  		.of_match_table = mcp3911_dt_ids,
>  	},
>  	.probe = mcp3911_probe,
> -	.remove = mcp3911_remove,
>  	.id_table = mcp3911_id,
>  };
>  module_spi_driver(mcp3911_driver);
Marcus Folkesson Sept. 12, 2022, 11:02 a.m. UTC | #2 
Hi,

On Sat, Aug 20, 2022 at 01:41:50PM +0100, Jonathan Cameron wrote:
> On Mon, 15 Aug 2022 08:16:20 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > Keep using managed resources as much as possible.
> > 
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> >  drivers/iio/adc/mcp3911.c | 53 ++++++++++++---------------------------
> >  1 file changed, 16 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > index 890af7dca62d..7e2efe702e57 100644
> > --- a/drivers/iio/adc/mcp3911.c
> > +++ b/drivers/iio/adc/mcp3911.c
> > @@ -258,6 +258,13 @@ static int mcp3911_config(struct mcp3911 *adc)
> >  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> >  }
> >  
> > +static void mcp3911_cleanup_regulator(void *_adc)
> 
> Missed this on previous versions, but why not pass
> the regulator pointer in as the parameter for the callback?
> 
> static void mcp391_cleanup_regulator(void *reg)
> {
> 	regulator_disable(adc->vref);
> }
> 
> Note this can't use the new devm_regulator_get_enable()
> because we need access to the regulator within the driver.
> 
> I can tidy this up whilst applying (or given it's really minor I might
> not bother :)
> 
> Note we are stalled at the moment with this series on getting the
> fixes upstream.  I'll probably send that pull request shortly.

Just a friendly reminder to not forget to pick up this series.

Thanks,

/Marcus
Jonathan Cameron Sept. 19, 2022, 3:36 p.m. UTC | #3 
On Mon, 12 Sep 2022 13:02:19 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Hi,
> 
> On Sat, Aug 20, 2022 at 01:41:50PM +0100, Jonathan Cameron wrote:
> > On Mon, 15 Aug 2022 08:16:20 +0200
> > Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> >   
> > > Keep using managed resources as much as possible.
> > > 
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > ---
> > >  drivers/iio/adc/mcp3911.c | 53 ++++++++++++---------------------------
> > >  1 file changed, 16 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > > index 890af7dca62d..7e2efe702e57 100644
> > > --- a/drivers/iio/adc/mcp3911.c
> > > +++ b/drivers/iio/adc/mcp3911.c
> > > @@ -258,6 +258,13 @@ static int mcp3911_config(struct mcp3911 *adc)
> > >  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > >  }
> > >  
> > > +static void mcp3911_cleanup_regulator(void *_adc)  
> > 
> > Missed this on previous versions, but why not pass
> > the regulator pointer in as the parameter for the callback?
> > 
> > static void mcp391_cleanup_regulator(void *reg)
> > {
> > 	regulator_disable(adc->vref);
> > }
> > 
> > Note this can't use the new devm_regulator_get_enable()
> > because we need access to the regulator within the driver.
> > 
> > I can tidy this up whilst applying (or given it's really minor I might
> > not bother :)
> > 
> > Note we are stalled at the moment with this series on getting the
> > fixes upstream.  I'll probably send that pull request shortly.  
> 
> Just a friendly reminder to not forget to pick up this series.

If things go according to plan, Greg will take the pull request
I sent the other day and I can fast forward the tree such that
the first 3 patches are in my upstream, then do a second pull
request with these applied. They aim here being to keep the
history reasonably clean rather than spaghetti merges.
Fingers crossed on timing working out. This all got delayed
because I sat on the pull request for a week due to travel and
dodgy airport wifi etc.

Jonathan

> 
> Thanks,
> 
> /Marcus
Jonathan Cameron Sept. 21, 2022, 8:13 p.m. UTC | #4 
On Mon, 19 Sep 2022 16:36:00 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 12 Sep 2022 13:02:19 +0200
> Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> 
> > Hi,
> > 
> > On Sat, Aug 20, 2022 at 01:41:50PM +0100, Jonathan Cameron wrote:  
> > > On Mon, 15 Aug 2022 08:16:20 +0200
> > > Marcus Folkesson <marcus.folkesson@gmail.com> wrote:
> > >     
> > > > Keep using managed resources as much as possible.
> > > > 
> > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > ---
> > > >  drivers/iio/adc/mcp3911.c | 53 ++++++++++++---------------------------
> > > >  1 file changed, 16 insertions(+), 37 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
> > > > index 890af7dca62d..7e2efe702e57 100644
> > > > --- a/drivers/iio/adc/mcp3911.c
> > > > +++ b/drivers/iio/adc/mcp3911.c
> > > > @@ -258,6 +258,13 @@ static int mcp3911_config(struct mcp3911 *adc)
> > > >  	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
> > > >  }
> > > >  
> > > > +static void mcp3911_cleanup_regulator(void *_adc)    
> > > 
> > > Missed this on previous versions, but why not pass
> > > the regulator pointer in as the parameter for the callback?
> > > 
> > > static void mcp391_cleanup_regulator(void *reg)
> > > {
> > > 	regulator_disable(adc->vref);
> > > }
> > > 
> > > Note this can't use the new devm_regulator_get_enable()
> > > because we need access to the regulator within the driver.
> > > 
> > > I can tidy this up whilst applying (or given it's really minor I might
> > > not bother :)
> > > 
> > > Note we are stalled at the moment with this series on getting the
> > > fixes upstream.  I'll probably send that pull request shortly.    
> > 
> > Just a friendly reminder to not forget to pick up this series.  
> 
> If things go according to plan, Greg will take the pull request
> I sent the other day and I can fast forward the tree such that
> the first 3 patches are in my upstream, then do a second pull
> request with these applied. They aim here being to keep the
> history reasonably clean rather than spaghetti merges.
> Fingers crossed on timing working out. This all got delayed
> because I sat on the pull request for a week due to travel and
> dodgy airport wifi etc.
> 

Now applied to the togreg branch of iio.git.
Pushed out as testing for some brief testing by 0day etc.

Jonathan

> Jonathan
> 
> > 
> > Thanks,
> > 
> > /Marcus  
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c
index 890af7dca62d..7e2efe702e57 100644
--- a/drivers/iio/adc/mcp3911.c
+++ b/drivers/iio/adc/mcp3911.c
@@ -258,6 +258,13 @@  static int mcp3911_config(struct mcp3911 *adc)
 	return  mcp3911_write(adc, MCP3911_REG_CONFIG, configreg, 2);
 }
 
+static void mcp3911_cleanup_regulator(void *_adc)
+{
+	struct mcp3911 *adc = _adc;
+
+	regulator_disable(adc->vref);
+}
+
 static int mcp3911_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -286,9 +293,14 @@  static int mcp3911_probe(struct spi_device *spi)
 		ret = regulator_enable(adc->vref);
 		if (ret)
 			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev,
+				mcp3911_cleanup_regulator, adc);
+		if (ret)
+			return ret;
 	}
 
-	adc->clki = devm_clk_get(&adc->spi->dev, NULL);
+	adc->clki = devm_clk_get_enabled(&adc->spi->dev, NULL);
 	if (IS_ERR(adc->clki)) {
 		if (PTR_ERR(adc->clki) == -ENOENT) {
 			adc->clki = NULL;
@@ -296,21 +308,13 @@  static int mcp3911_probe(struct spi_device *spi)
 			dev_err(&adc->spi->dev,
 				"failed to get adc clk (%ld)\n",
 				PTR_ERR(adc->clki));
-			ret = PTR_ERR(adc->clki);
-			goto reg_disable;
-		}
-	} else {
-		ret = clk_prepare_enable(adc->clki);
-		if (ret < 0) {
-			dev_err(&adc->spi->dev,
-				"Failed to enable clki: %d\n", ret);
-			goto reg_disable;
+			return PTR_ERR(adc->clki);
 		}
 	}
 
 	ret = mcp3911_config(adc);
 	if (ret)
-		goto clk_disable;
+		return ret;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
@@ -322,31 +326,7 @@  static int mcp3911_probe(struct spi_device *spi)
 
 	mutex_init(&adc->lock);
 
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		goto clk_disable;
-
-	return ret;
-
-clk_disable:
-	clk_disable_unprepare(adc->clki);
-reg_disable:
-	if (adc->vref)
-		regulator_disable(adc->vref);
-
-	return ret;
-}
-
-static void mcp3911_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct mcp3911 *adc = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-
-	clk_disable_unprepare(adc->clki);
-	if (adc->vref)
-		regulator_disable(adc->vref);
+	return devm_iio_device_register(&adc->spi->dev, indio_dev);
 }
 
 static const struct of_device_id mcp3911_dt_ids[] = {
@@ -367,7 +347,6 @@  static struct spi_driver mcp3911_driver = {
 		.of_match_table = mcp3911_dt_ids,
 	},
 	.probe = mcp3911_probe,
-	.remove = mcp3911_remove,
 	.id_table = mcp3911_id,
 };
 module_spi_driver(mcp3911_driver);