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

[NeilBrown]

On Wed, 19 Nov 2014 23:37:41 +0100 Hartmut Knaack <knaack.h@xxxxxx> 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@xxxxxxxxxx>
> > 
> > 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: pgprSQQj7eL7U.pgp
Description: OpenPGP digital signature