[PATCH] iio: gyro: itg3200: add suspend/resume support.

[PATCH] iio: gyro: itg3200: add suspend/resume support.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]



Unless we put the device to sleep when not it use, it wastes
6mA.

If the device is asleep on probe, the 'id' register
sometimes mis-reads - so reset first.  If the device responds
at all a command sent to the address, it is almost certainly
the correct device already.

Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
index 6a8020d48140..394667fb23f9 100644
--- a/drivers/iio/gyro/itg3200_core.c
+++ b/drivers/iio/gyro/itg3200_core.c
@@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
 	int ret;
 	u8 val;
 
+	ret = itg3200_reset(indio_dev);
 	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
 	if (ret)
 		goto err_ret;
@@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int itg3200_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct itg3200 *st = iio_priv(indio_dev);
+	int ret;
+
+	dev_dbg(&st->i2c->dev, "suspend device");
+
+	ret = itg3200_write_reg_8(indio_dev,
+			ITG3200_REG_POWER_MANAGEMENT,
+			ITG3200_SLEEP);
+	return ret;
+}
+
+static int itg3200_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+
+	itg3200_reset(indio_dev);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(itg3200_pm_ops, itg3200_suspend, itg3200_resume);
+#define ITG3200_PM_OPS (&itg3200_pm_ops)
+#else
+#define ITG3200_PM_OPS NULL
+#endif
+
 static const struct i2c_device_id itg3200_id[] = {
 	{ "itg3200", 0 },
 	{ }
@@ -361,6 +391,7 @@ static struct i2c_driver itg3200_driver = {
 	.driver = {
 		.owner  = THIS_MODULE,
 		.name	= "itg3200",
+		.pm	= ITG3200_PM_OPS,
 	},
 	.id_table	= itg3200_id,
 	.probe		= itg3200_probe,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.

[Jonathan Cameron]

On 08/11/14 00:18, NeilBrown wrote:
> 
> 
> Unless we put the device to sleep when not it use, it wastes
> 6mA.
> 
> If the device is asleep on probe, the 'id' register
> sometimes mis-reads - so reset first.  If the device responds
> at all a command sent to the address, it is almost certainly
> the correct device already.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
Hi Neil,

Looks fine to me - but you should have cc'd a few people from
the listed driver authors.  Will give them a few days to respond...

Jonathan
> 
> diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> index 6a8020d48140..394667fb23f9 100644
> --- a/drivers/iio/gyro/itg3200_core.c
> +++ b/drivers/iio/gyro/itg3200_core.c
> @@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
>  	int ret;
>  	u8 val;
>  
> +	ret = itg3200_reset(indio_dev);
>  	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
>  	if (ret)
>  		goto err_ret;
> @@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int itg3200_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct itg3200 *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	dev_dbg(&st->i2c->dev, "suspend device");
> +
> +	ret = itg3200_write_reg_8(indio_dev,
> +			ITG3200_REG_POWER_MANAGEMENT,
> +			ITG3200_SLEEP);
> +	return ret;
> +}
> +
> +static int itg3200_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +
> +	itg3200_reset(indio_dev);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(itg3200_pm_ops, itg3200_suspend, itg3200_resume);
> +#define ITG3200_PM_OPS (&itg3200_pm_ops)
> +#else
> +#define ITG3200_PM_OPS NULL
> +#endif
> +
>  static const struct i2c_device_id itg3200_id[] = {
>  	{ "itg3200", 0 },
>  	{ }
> @@ -361,6 +391,7 @@ static struct i2c_driver itg3200_driver = {
>  	.driver = {
>  		.owner  = THIS_MODULE,
>  		.name	= "itg3200",
> +		.pm	= ITG3200_PM_OPS,
>  	},
>  	.id_table	= itg3200_id,
>  	.probe		= itg3200_probe,
> 

Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2492 bytes --]

On Sat, 08 Nov 2014 11:52:33 +0000 Jonathan Cameron <jic23@kernel.org> wrote:

> On 08/11/14 00:18, NeilBrown wrote:
> > 
> > 
> > Unless we put the device to sleep when not it use, it wastes
> > 6mA.
> > 
> > If the device is asleep on probe, the 'id' register
> > sometimes mis-reads - so reset first.  If the device responds
> > at all a command sent to the address, it is almost certainly
> > the correct device already.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> Hi Neil,
> 
> Looks fine to me - but you should have cc'd a few people from
> the listed driver authors.  Will give them a few days to respond...

Over-dependence on get_maintainers.pl I guess. :-(

Thanks,
NeilBrown


> 
> Jonathan
> > 
> > diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> > index 6a8020d48140..394667fb23f9 100644
> > --- a/drivers/iio/gyro/itg3200_core.c
> > +++ b/drivers/iio/gyro/itg3200_core.c
> > @@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
> >  	int ret;
> >  	u8 val;
> >  
> > +	ret = itg3200_reset(indio_dev);
> >  	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
> >  	if (ret)
> >  		goto err_ret;
> > @@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int itg3200_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct itg3200 *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	dev_dbg(&st->i2c->dev, "suspend device");
> > +
> > +	ret = itg3200_write_reg_8(indio_dev,
> > +			ITG3200_REG_POWER_MANAGEMENT,
> > +			ITG3200_SLEEP);
> > +	return ret;
> > +}
> > +
> > +static int itg3200_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +
> > +	itg3200_reset(indio_dev);
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(itg3200_pm_ops, itg3200_suspend, itg3200_resume);
> > +#define ITG3200_PM_OPS (&itg3200_pm_ops)
> > +#else
> > +#define ITG3200_PM_OPS NULL
> > +#endif
> > +
> >  static const struct i2c_device_id itg3200_id[] = {
> >  	{ "itg3200", 0 },
> >  	{ }
> > @@ -361,6 +391,7 @@ static struct i2c_driver itg3200_driver = {
> >  	.driver = {
> >  		.owner  = THIS_MODULE,
> >  		.name	= "itg3200",
> > +		.pm	= ITG3200_PM_OPS,
> >  	},
> >  	.id_table	= itg3200_id,
> >  	.probe		= itg3200_probe,
> > 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.

[Manuel Stahl]

Looks good to me.

Regards,
Manuel Stahl

Am Samstag, 8. November 2014, 12:52:33 schrieb Jonathan Cameron:
> On 08/11/14 00:18, NeilBrown wrote:
> > 
> > 
> > Unless we put the device to sleep when not it use, it wastes
> > 6mA.
> > 
> > If the device is asleep on probe, the 'id' register
> > sometimes mis-reads - so reset first.  If the device responds
> > at all a command sent to the address, it is almost certainly
> > the correct device already.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> Hi Neil,
> 
> Looks fine to me - but you should have cc'd a few people from
> the listed driver authors.  Will give them a few days to respond...
> 
> Jonathan
> > 
> > diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> > index 6a8020d48140..394667fb23f9 100644
> > --- a/drivers/iio/gyro/itg3200_core.c
> > +++ b/drivers/iio/gyro/itg3200_core.c
> > @@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
> >  	int ret;
> >  	u8 val;
> >  
> > +	ret = itg3200_reset(indio_dev);
> >  	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
> >  	if (ret)
> >  		goto err_ret;
> > @@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int itg3200_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct itg3200 *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	dev_dbg(&st->i2c->dev, "suspend device");
> > +
> > +	ret = itg3200_write_reg_8(indio_dev,
> > +			ITG3200_REG_POWER_MANAGEMENT,
> > +			ITG3200_SLEEP);
> > +	return ret;
> > +}
> > +
> > +static int itg3200_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +
> > +	itg3200_reset(indio_dev);
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(itg3200_pm_ops, itg3200_suspend, itg3200_resume);
> > +#define ITG3200_PM_OPS (&itg3200_pm_ops)
> > +#else
> > +#define ITG3200_PM_OPS NULL
> > +#endif
> > +
> >  static const struct i2c_device_id itg3200_id[] = {
> >  	{ "itg3200", 0 },
> >  	{ }
> > @@ -361,6 +391,7 @@ static struct i2c_driver itg3200_driver = {
> >  	.driver = {
> >  		.owner  = THIS_MODULE,
> >  		.name	= "itg3200",
> > +		.pm	= ITG3200_PM_OPS,
> >  	},
> >  	.id_table	= itg3200_id,
> >  	.probe		= itg3200_probe,
> > 
> 
> 

Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.

[Hartmut Knaack]

NeilBrown schrieb am 08.11.2014 01:18:
> 
> 
> Unless we put the device to sleep when not it use, it wastes
> 6mA.
> 
> If the device is asleep on probe, the 'id' register
> sometimes mis-reads - so reset first.  If the device responds
> at all a command sent to the address, it is almost certainly
> the correct device already.
> 
Hi Neil,
I still have some question and issues, see inline.
> Signed-off-by: NeilBrown <neil@brown.name>
> 
> diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> index 6a8020d48140..394667fb23f9 100644
> --- a/drivers/iio/gyro/itg3200_core.c
> +++ b/drivers/iio/gyro/itg3200_core.c
> @@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
>  	int ret;
>  	u8 val;
>  
> +	ret = itg3200_reset(indio_dev);
You should check possible error codes here. Also, there is still another reset issued some lines further down, although in between, there is only the register-read performed, which we see right below here - I would assume this wouldn't change anything in the device to require another reset. So, in conclusion, wouldn't it be sufficient to just move the reset part from further down up here?
>  	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
>  	if (ret)
>  		goto err_ret;
> @@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int itg3200_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct itg3200 *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	dev_dbg(&st->i2c->dev, "suspend device");
> +
> +	ret = itg3200_write_reg_8(indio_dev,
> +			ITG3200_REG_POWER_MANAGEMENT,
> +			ITG3200_SLEEP);
> +	return ret;
No need for ret, if you do it like this (fixing also some indentation issue):
	return itg3200_write_reg_8(indio_dev, ITG3200_REG_POWER_MANAGEMENT,
				   ITG3200_SLEEP);
> +}
> +
> +static int itg3200_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +
> +	itg3200_reset(indio_dev);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(itg3200_pm_ops, itg3200_suspend, itg3200_resume);
> +#define ITG3200_PM_OPS (&itg3200_pm_ops)
> +#else
> +#define ITG3200_PM_OPS NULL
> +#endif
> +
>  static const struct i2c_device_id itg3200_id[] = {
>  	{ "itg3200", 0 },
>  	{ }
> @@ -361,6 +391,7 @@ static struct i2c_driver itg3200_driver = {
>  	.driver = {
>  		.owner  = THIS_MODULE,
>  		.name	= "itg3200",
> +		.pm	= ITG3200_PM_OPS,
>  	},
>  	.id_table	= itg3200_id,
>  	.probe		= itg3200_probe,
> 

Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

On Wed, 19 Nov 2014 23:37:41 +0100 Hartmut Knaack <knaack.h@gmx.de> wrote:

> NeilBrown schrieb am 08.11.2014 01:18:
> > 
> > 
> > Unless we put the device to sleep when not it use, it wastes
> > 6mA.
> > 
> > If the device is asleep on probe, the 'id' register
> > sometimes mis-reads - so reset first.  If the device responds
> > at all a command sent to the address, it is almost certainly
> > the correct device already.
> > 
> Hi Neil,
> I still have some question and issues, see inline.
> > Signed-off-by: NeilBrown <neil@brown.name>
> > 
> > diff --git a/drivers/iio/gyro/itg3200_core.c b/drivers/iio/gyro/itg3200_core.c
> > index 6a8020d48140..394667fb23f9 100644
> > --- a/drivers/iio/gyro/itg3200_core.c
> > +++ b/drivers/iio/gyro/itg3200_core.c
> > @@ -223,6 +223,7 @@ static int itg3200_initial_setup(struct iio_dev *indio_dev)
> >  	int ret;
> >  	u8 val;
> >  
> > +	ret = itg3200_reset(indio_dev);
> You should check possible error codes here. Also, there is still another reset issued some lines further down, although in between, there is only the register-read performed, which we see right below here - I would assume this wouldn't change anything in the device to require another reset. So, in conclusion, wouldn't it be sufficient to just move the reset part from further down up here?
> >  	ret = itg3200_read_reg_8(indio_dev, ITG3200_REG_ADDRESS, &val);
> >  	if (ret)
> >  		goto err_ret;
> > @@ -351,6 +352,35 @@ static int itg3200_remove(struct i2c_client *client)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM_SLEEP
> > +static int itg3200_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct itg3200 *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	dev_dbg(&st->i2c->dev, "suspend device");
> > +
> > +	ret = itg3200_write_reg_8(indio_dev,
> > +			ITG3200_REG_POWER_MANAGEMENT,
> > +			ITG3200_SLEEP);
> > +	return ret;
> No need for ret, if you do it like this (fixing also some indentation issue):
> 	return itg3200_write_reg_8(indio_dev, ITG3200_REG_POWER_MANAGEMENT,
> 				   ITG3200_SLEEP);


hi Hartmut,
 thanks for these suggestions.  I made the changes you suggested to my code,
 but it appears that I never replied or reposted.  Sorry about that.

 I'll resubmit in a moment...

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]