Message-ID: <546D1BB5.8010308@gmx.de>
Date: Wed, 19 Nov 2014 23:37:41 +0100
From: Hartmut Knaack <knaack.h@gmx.de>
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:32.0) Gecko/20100101 Firefox/32.0 SeaMonkey/2.29
MIME-Version: 1.0
To: NeilBrown <neilb@suse.de>, Jonathan Cameron <jic23@kernel.org>
CC: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
        GTA04 owners <gta04-owner@goldelico.com>
Subject: Re: [PATCH] iio: gyro: itg3200: add suspend/resume support.
References: <20141108111833.3829480c@notabene.brown>
In-Reply-To: <20141108111833.3829480c@notabene.brown>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7BIT
Sender: linux-kernel-owner@vger.kernel.org

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,
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/