linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions
@ 2021-06-25 14:01 Alexandru Ardelean
  2021-06-25 14:01 ` [PATCH 2/2] iio: accel: bma220: make suspend state setting more robust Alexandru Ardelean
  2021-07-04 17:08 ` [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Jonathan Cameron
  0 siblings, 2 replies; 4+ messages in thread
From: Alexandru Ardelean @ 2021-06-25 14:01 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, Alexandru Ardelean

This change converts the driver to use devm_iio_triggered_buffer_setup()
and devm_iio_device_register() for initializing and registering the IIO
device.

The bma220_deinit() is converted into a callback for a
devm_add_action_or_reset() hook, so that the device is put in stand-by when
the driver gets uninitialized.
The return value of the bma220_deinit() function isn't used as it does not
add any value. On the error path of the probe function, this can just
override the actual error with -EBUSY, or can even return 0 (no error), on
the error path.

Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
 drivers/iio/accel/bma220_spi.c | 44 +++++++++-------------------------
 1 file changed, 11 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 0622c7936499..0095931a11f8 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -218,20 +218,14 @@ static int bma220_init(struct spi_device *spi)
 	return 0;
 }
 
-static int bma220_deinit(struct spi_device *spi)
+static void bma220_deinit(void *spi)
 {
 	int ret;
 
 	/* Make sure the chip is powered off */
 	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
 	if (ret == BMA220_SUSPEND_SLEEP)
-		ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
-	if (ret < 0)
-		return ret;
-	if (ret == BMA220_SUSPEND_SLEEP)
-		return -EBUSY;
-
-	return 0;
+		bma220_read_reg(spi, BMA220_REG_SUSPEND);
 }
 
 static int bma220_probe(struct spi_device *spi)
@@ -262,34 +256,19 @@ static int bma220_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
-					 bma220_trigger_handler, NULL);
-	if (ret < 0) {
-		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
-		goto err_suspend;
-	}
+	ret = devm_add_action_or_reset(&spi->dev, bma220_deinit, spi);
+	if (ret)
+		return ret;
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      bma220_trigger_handler, NULL);
 	if (ret < 0) {
-		dev_err(&spi->dev, "iio_device_register failed\n");
-		iio_triggered_buffer_cleanup(indio_dev);
-		goto err_suspend;
+		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
+		return ret;
 	}
 
-	return 0;
-
-err_suspend:
-	return bma220_deinit(spi);
-}
-
-static int bma220_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-
-	return bma220_deinit(spi);
+	return devm_iio_device_register(&spi->dev, indio_dev);
 }
 
 static __maybe_unused int bma220_suspend(struct device *dev)
@@ -326,7 +305,6 @@ static struct spi_driver bma220_driver = {
 		.acpi_match_table = bma220_acpi_id,
 	},
 	.probe =            bma220_probe,
-	.remove =           bma220_remove,
 	.id_table =         bma220_spi_id,
 };
 module_spi_driver(bma220_driver);
-- 
2.31.1


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

* [PATCH 2/2] iio: accel: bma220: make suspend state setting more robust
  2021-06-25 14:01 [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Alexandru Ardelean
@ 2021-06-25 14:01 ` Alexandru Ardelean
  2021-07-04 17:08 ` [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Jonathan Cameron
  1 sibling, 0 replies; 4+ messages in thread
From: Alexandru Ardelean @ 2021-06-25 14:01 UTC (permalink / raw)
  To: linux-iio, linux-kernel; +Cc: jic23, Alexandru Ardelean

The datasheet mentions that the suspend mode is toggled by reading the
suspend register. The reading returns value 0xFF if the system was in
suspend mode, otherwise it returns value 0x00.

The bma220_deinit() function does up to 2 reads, in case the device was in
suspend mode, which suggests a level of paranoia that makes the logic in
bma220_suspend() and bma220_resume() look insufficient.

This change implements a bma220_power() function which does up to 2 reads
of the suspend register to make sure that the chip enters a desired
(suspended or normal) mode.

If the transition fails, then -EBUSY is returned.

Since only a reference to SPI device is required, we can remove the
spi_set_drvdata() call and get the SPI device object from the base device
object in the suspend/resume routines.

Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
 drivers/iio/accel/bma220_spi.c | 41 ++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 0095931a11f8..bc4c626e454d 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -218,14 +218,33 @@ static int bma220_init(struct spi_device *spi)
 	return 0;
 }
 
-static void bma220_deinit(void *spi)
+static int bma220_power(struct spi_device *spi, bool up)
 {
-	int ret;
+	int i, ret;
+
+	/**
+	 * The chip can be suspended/woken up by a simple register read.
+	 * So, we need up to 2 register reads of the suspend register
+	 * to make sure that the device is in the desired state.
+	 */
+	for (i = 0; i < 2; i++) {
+		ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
+		if (ret < 0)
+			return ret;
 
-	/* Make sure the chip is powered off */
-	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
-	if (ret == BMA220_SUSPEND_SLEEP)
-		bma220_read_reg(spi, BMA220_REG_SUSPEND);
+		if (up && ret == BMA220_SUSPEND_SLEEP)
+			return 0;
+
+		if (!up && ret == BMA220_SUSPEND_WAKE)
+			return 0;
+	}
+
+	return -EBUSY;
+}
+
+static void bma220_deinit(void *spi)
+{
+	bma220_power(spi, false);
 }
 
 static int bma220_probe(struct spi_device *spi)
@@ -242,7 +261,6 @@ static int bma220_probe(struct spi_device *spi)
 
 	data = iio_priv(indio_dev);
 	data->spi_device = spi;
-	spi_set_drvdata(spi, indio_dev);
 	mutex_init(&data->lock);
 
 	indio_dev->info = &bma220_info;
@@ -273,17 +291,16 @@ static int bma220_probe(struct spi_device *spi)
 
 static __maybe_unused int bma220_suspend(struct device *dev)
 {
-	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
+	struct spi_device *spi = to_spi_device(dev);
 
-	/* The chip can be suspended/woken up by a simple register read. */
-	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
+	return bma220_power(spi, false);
 }
 
 static __maybe_unused int bma220_resume(struct device *dev)
 {
-	struct bma220_data *data = iio_priv(dev_get_drvdata(dev));
+	struct spi_device *spi = to_spi_device(dev);
 
-	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
+	return bma220_power(spi, true);
 }
 static SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume);
 
-- 
2.31.1


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

* Re: [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions
  2021-06-25 14:01 [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Alexandru Ardelean
  2021-06-25 14:01 ` [PATCH 2/2] iio: accel: bma220: make suspend state setting more robust Alexandru Ardelean
@ 2021-07-04 17:08 ` Jonathan Cameron
  2021-07-17 18:13   ` Jonathan Cameron
  1 sibling, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2021-07-04 17:08 UTC (permalink / raw)
  To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel

On Fri, 25 Jun 2021 17:01:36 +0300
Alexandru Ardelean <aardelean@deviqon.com> wrote:

> This change converts the driver to use devm_iio_triggered_buffer_setup()
> and devm_iio_device_register() for initializing and registering the IIO
> device.
> 
> The bma220_deinit() is converted into a callback for a
> devm_add_action_or_reset() hook, so that the device is put in stand-by when
> the driver gets uninitialized.
> The return value of the bma220_deinit() function isn't used as it does not
> add any value. On the error path of the probe function, this can just
> override the actual error with -EBUSY, or can even return 0 (no error), on
> the error path.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
Hmm. That return on the error path issue in probe is almost worth a backported
fix, but meh, it's been there for ages so unless anyone shouts, let us assume
it never bit anyone.

Otherwise, this looks fine.  I'm not going to comment on all the similar
devm patches you have on list unless I have something I want changed.
I'll pick them all up in a week or two once we are well into the new cycle.

thanks,

Jonathan

> ---
>  drivers/iio/accel/bma220_spi.c | 44 +++++++++-------------------------
>  1 file changed, 11 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index 0622c7936499..0095931a11f8 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -218,20 +218,14 @@ static int bma220_init(struct spi_device *spi)
>  	return 0;
>  }
>  
> -static int bma220_deinit(struct spi_device *spi)
> +static void bma220_deinit(void *spi)
>  {
>  	int ret;
>  
>  	/* Make sure the chip is powered off */
>  	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
>  	if (ret == BMA220_SUSPEND_SLEEP)
> -		ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> -	if (ret < 0)
> -		return ret;
> -	if (ret == BMA220_SUSPEND_SLEEP)
> -		return -EBUSY;
> -
> -	return 0;
> +		bma220_read_reg(spi, BMA220_REG_SUSPEND);
>  }
>  
>  static int bma220_probe(struct spi_device *spi)
> @@ -262,34 +256,19 @@ static int bma220_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> -					 bma220_trigger_handler, NULL);
> -	if (ret < 0) {
> -		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> -		goto err_suspend;
> -	}
> +	ret = devm_add_action_or_reset(&spi->dev, bma220_deinit, spi);
> +	if (ret)
> +		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      bma220_trigger_handler, NULL);
>  	if (ret < 0) {
> -		dev_err(&spi->dev, "iio_device_register failed\n");
> -		iio_triggered_buffer_cleanup(indio_dev);
> -		goto err_suspend;
> +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> +		return ret;
>  	}
>  
> -	return 0;
> -
> -err_suspend:
> -	return bma220_deinit(spi);
> -}
> -
> -static int bma220_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -
> -	iio_device_unregister(indio_dev);
> -	iio_triggered_buffer_cleanup(indio_dev);
> -
> -	return bma220_deinit(spi);
> +	return devm_iio_device_register(&spi->dev, indio_dev);
>  }
>  
>  static __maybe_unused int bma220_suspend(struct device *dev)
> @@ -326,7 +305,6 @@ static struct spi_driver bma220_driver = {
>  		.acpi_match_table = bma220_acpi_id,
>  	},
>  	.probe =            bma220_probe,
> -	.remove =           bma220_remove,
>  	.id_table =         bma220_spi_id,
>  };
>  module_spi_driver(bma220_driver);


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

* Re: [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions
  2021-07-04 17:08 ` [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Jonathan Cameron
@ 2021-07-17 18:13   ` Jonathan Cameron
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-07-17 18:13 UTC (permalink / raw)
  To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel

On Sun, 4 Jul 2021 18:08:17 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 25 Jun 2021 17:01:36 +0300
> Alexandru Ardelean <aardelean@deviqon.com> wrote:
> 
> > This change converts the driver to use devm_iio_triggered_buffer_setup()
> > and devm_iio_device_register() for initializing and registering the IIO
> > device.
> > 
> > The bma220_deinit() is converted into a callback for a
> > devm_add_action_or_reset() hook, so that the device is put in stand-by when
> > the driver gets uninitialized.
> > The return value of the bma220_deinit() function isn't used as it does not
> > add any value. On the error path of the probe function, this can just
> > override the actual error with -EBUSY, or can even return 0 (no error), on
> > the error path.
> > 
> > Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>  
> Hmm. That return on the error path issue in probe is almost worth a backported
> fix, but meh, it's been there for ages so unless anyone shouts, let us assume
> it never bit anyone.
> 
> Otherwise, this looks fine.  I'm not going to comment on all the similar
> devm patches you have on list unless I have something I want changed.
> I'll pick them all up in a week or two once we are well into the new cycle.
> 
> thanks,
> 
> Jonathan

Both applied to the togreg branch of iio.git and pushed out as testing for
0-day to poke at them and see if we missed anything.

Thanks,

Jonathan

> 
> > ---
> >  drivers/iio/accel/bma220_spi.c | 44 +++++++++-------------------------
> >  1 file changed, 11 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> > index 0622c7936499..0095931a11f8 100644
> > --- a/drivers/iio/accel/bma220_spi.c
> > +++ b/drivers/iio/accel/bma220_spi.c
> > @@ -218,20 +218,14 @@ static int bma220_init(struct spi_device *spi)
> >  	return 0;
> >  }
> >  
> > -static int bma220_deinit(struct spi_device *spi)
> > +static void bma220_deinit(void *spi)
> >  {
> >  	int ret;
> >  
> >  	/* Make sure the chip is powered off */
> >  	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> >  	if (ret == BMA220_SUSPEND_SLEEP)
> > -		ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> > -	if (ret < 0)
> > -		return ret;
> > -	if (ret == BMA220_SUSPEND_SLEEP)
> > -		return -EBUSY;
> > -
> > -	return 0;
> > +		bma220_read_reg(spi, BMA220_REG_SUSPEND);
> >  }
> >  
> >  static int bma220_probe(struct spi_device *spi)
> > @@ -262,34 +256,19 @@ static int bma220_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> > -					 bma220_trigger_handler, NULL);
> > -	if (ret < 0) {
> > -		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> > -		goto err_suspend;
> > -	}
> > +	ret = devm_add_action_or_reset(&spi->dev, bma220_deinit, spi);
> > +	if (ret)
> > +		return ret;
> >  
> > -	ret = iio_device_register(indio_dev);
> > +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > +					      iio_pollfunc_store_time,
> > +					      bma220_trigger_handler, NULL);
> >  	if (ret < 0) {
> > -		dev_err(&spi->dev, "iio_device_register failed\n");
> > -		iio_triggered_buffer_cleanup(indio_dev);
> > -		goto err_suspend;
> > +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> > +		return ret;
> >  	}
> >  
> > -	return 0;
> > -
> > -err_suspend:
> > -	return bma220_deinit(spi);
> > -}
> > -
> > -static int bma220_remove(struct spi_device *spi)
> > -{
> > -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > -
> > -	iio_device_unregister(indio_dev);
> > -	iio_triggered_buffer_cleanup(indio_dev);
> > -
> > -	return bma220_deinit(spi);
> > +	return devm_iio_device_register(&spi->dev, indio_dev);
> >  }
> >  
> >  static __maybe_unused int bma220_suspend(struct device *dev)
> > @@ -326,7 +305,6 @@ static struct spi_driver bma220_driver = {
> >  		.acpi_match_table = bma220_acpi_id,
> >  	},
> >  	.probe =            bma220_probe,
> > -	.remove =           bma220_remove,
> >  	.id_table =         bma220_spi_id,
> >  };
> >  module_spi_driver(bma220_driver);  
> 


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

end of thread, other threads:[~2021-07-17 18:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 14:01 [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Alexandru Ardelean
2021-06-25 14:01 ` [PATCH 2/2] iio: accel: bma220: make suspend state setting more robust Alexandru Ardelean
2021-07-04 17:08 ` [PATCH 1/2] iio: accel: bma220: convert probe to device-managed functions Jonathan Cameron
2021-07-17 18:13   ` Jonathan Cameron

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).