* [PATCH] iio: gyro: itg3200: add suspend/resume support.
@ 2014-11-08 0:18 NeilBrown
2014-11-08 11:52 ` Jonathan Cameron
2014-11-19 22:37 ` Hartmut Knaack
0 siblings, 2 replies; 6+ messages in thread
From: NeilBrown @ 2014-11-08 0:18 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, GTA04 owners
[-- 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 --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
2014-11-08 0:18 [PATCH] iio: gyro: itg3200: add suspend/resume support NeilBrown
@ 2014-11-08 11:52 ` Jonathan Cameron
2014-11-09 22:57 ` NeilBrown
2014-11-10 9:57 ` Manuel Stahl
2014-11-19 22:37 ` Hartmut Knaack
1 sibling, 2 replies; 6+ messages in thread
From: Jonathan Cameron @ 2014-11-08 11:52 UTC (permalink / raw)
To: NeilBrown
Cc: linux-iio, linux-kernel, GTA04 owners, chr >> Doug Anderson,
Thorsten Nowak, Manuel Stahl,
christian.strobel@iis.fraunhofer.de >> Christian Strobel
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
2014-11-08 11:52 ` Jonathan Cameron
@ 2014-11-09 22:57 ` NeilBrown
2014-11-10 9:57 ` Manuel Stahl
1 sibling, 0 replies; 6+ messages in thread
From: NeilBrown @ 2014-11-09 22:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, linux-kernel, GTA04 owners, chr >> Doug Anderson,
Thorsten Nowak, Manuel Stahl,
christian.strobel@iis.fraunhofer.de >> Christian Strobel
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
2014-11-08 11:52 ` Jonathan Cameron
2014-11-09 22:57 ` NeilBrown
@ 2014-11-10 9:57 ` Manuel Stahl
1 sibling, 0 replies; 6+ messages in thread
From: Manuel Stahl @ 2014-11-10 9:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: NeilBrown, linux-iio, linux-kernel, GTA04 owners,
chr >> Doug Anderson, Thorsten Nowak,
christian.strobel@iis.fraunhofer.de >> Christian Strobel
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,
> >
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
2014-11-08 0:18 [PATCH] iio: gyro: itg3200: add suspend/resume support NeilBrown
2014-11-08 11:52 ` Jonathan Cameron
@ 2014-11-19 22:37 ` Hartmut Knaack
2015-02-24 2:21 ` NeilBrown
1 sibling, 1 reply; 6+ messages in thread
From: Hartmut Knaack @ 2014-11-19 22:37 UTC (permalink / raw)
To: NeilBrown, Jonathan Cameron; +Cc: linux-iio, linux-kernel, GTA04 owners
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
2014-11-19 22:37 ` Hartmut Knaack
@ 2015-02-24 2:21 ` NeilBrown
0 siblings, 0 replies; 6+ messages in thread
From: NeilBrown @ 2015-02-24 2:21 UTC (permalink / raw)
To: Hartmut Knaack; +Cc: Jonathan Cameron, linux-iio, linux-kernel, GTA04 owners
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-24 2:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-08 0:18 [PATCH] iio: gyro: itg3200: add suspend/resume support NeilBrown
2014-11-08 11:52 ` Jonathan Cameron
2014-11-09 22:57 ` NeilBrown
2014-11-10 9:57 ` Manuel Stahl
2014-11-19 22:37 ` Hartmut Knaack
2015-02-24 2:21 ` NeilBrown
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).