[PATCH] iio: humidity: hts221: add power management support

[PATCH] iio: humidity: hts221: add power management support

[Lorenzo Bianconi]

Add system sleep power management support to hts221 driver

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
---
 drivers/iio/humidity/hts221.h      |  3 ++
 drivers/iio/humidity/hts221_core.c | 56 +++++++++++++++++++++++++++++++++++---
 drivers/iio/humidity/hts221_i2c.c  |  1 +
 drivers/iio/humidity/hts221_spi.c  |  1 +
 4 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
index c7154665512e..94510266e0a5 100644
--- a/drivers/iio/humidity/hts221.h
+++ b/drivers/iio/humidity/hts221.h
@@ -57,12 +57,15 @@ struct hts221_hw {
 
 	struct hts221_sensor sensors[HTS221_SENSOR_MAX];
 
+	bool enabled;
 	u8 odr;
 
 	const struct hts221_transfer_function *tf;
 	struct hts221_transfer_buffer tb;
 };
 
+extern const struct dev_pm_ops hts221_pm_ops;
+
 int hts221_config_drdy(struct hts221_hw *hw, bool enable);
 int hts221_probe(struct iio_dev *iio_dev);
 int hts221_power_on(struct hts221_hw *hw);
diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c
index 3f3ef4a1a474..ccb07e7f993f 100644
--- a/drivers/iio/humidity/hts221_core.c
+++ b/drivers/iio/humidity/hts221_core.c
@@ -13,6 +13,7 @@
 #include <linux/device.h>
 #include <linux/iio/sysfs.h>
 #include <linux/delay.h>
+#include <linux/pm.h>
 #include <asm/unaligned.h>
 
 #include "hts221.h"
@@ -307,15 +308,30 @@ hts221_sysfs_temp_oversampling_avail(struct device *dev,
 
 int hts221_power_on(struct hts221_hw *hw)
 {
-	return hts221_update_odr(hw, hw->odr);
+	int err;
+
+	err = hts221_update_odr(hw, hw->odr);
+	if (err < 0)
+		return err;
+
+	hw->enabled = true;
+
+	return 0;
 }
 
 int hts221_power_off(struct hts221_hw *hw)
 {
-	u8 data[] = {0x00, 0x00};
+	__le16 data = 0;
+	int err;
 
-	return hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
-			     data);
+	err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
+			    (u8 *)&data);
+	if (err < 0)
+		return err;
+
+	hw->enabled = false;
+
+	return 0;
 }
 
 static int hts221_parse_temp_caldata(struct hts221_hw *hw)
@@ -682,6 +698,38 @@ int hts221_probe(struct iio_dev *iio_dev)
 }
 EXPORT_SYMBOL(hts221_probe);
 
+#ifdef CONFIG_PM
+static int hts221_suspend(struct device *dev)
+{
+	struct iio_dev *iio_dev = dev_get_drvdata(dev);
+	struct hts221_hw *hw = iio_priv(iio_dev);
+	__le16 data = 0;
+	int err;
+
+	err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
+			    (u8 *)&data);
+
+	return err < 0 ? err : 0;
+}
+
+static int hts221_resume(struct device *dev)
+{
+	struct iio_dev *iio_dev = dev_get_drvdata(dev);
+	struct hts221_hw *hw = iio_priv(iio_dev);
+	int err = 0;
+
+	if (hw->enabled)
+		err = hts221_update_odr(hw, hw->odr);
+
+	return err;
+}
+#endif /* CONFIG_PM */
+
+const struct dev_pm_ops hts221_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(hts221_suspend, hts221_resume)
+};
+EXPORT_SYMBOL(hts221_pm_ops);
+
 MODULE_AUTHOR("Lorenzo Bianconi <lorenzo.bianconi@st.com>");
 MODULE_DESCRIPTION("STMicroelectronics hts221 sensor driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/humidity/hts221_i2c.c b/drivers/iio/humidity/hts221_i2c.c
index 8333c0296c0e..f38e4b7e0160 100644
--- a/drivers/iio/humidity/hts221_i2c.c
+++ b/drivers/iio/humidity/hts221_i2c.c
@@ -105,6 +105,7 @@ MODULE_DEVICE_TABLE(i2c, hts221_i2c_id_table);
 static struct i2c_driver hts221_driver = {
 	.driver = {
 		.name = "hts221_i2c",
+		.pm = &hts221_pm_ops,
 		.of_match_table = of_match_ptr(hts221_i2c_of_match),
 		.acpi_match_table = ACPI_PTR(hts221_acpi_match),
 	},
diff --git a/drivers/iio/humidity/hts221_spi.c b/drivers/iio/humidity/hts221_spi.c
index 70df5e7150c1..57cbc256771b 100644
--- a/drivers/iio/humidity/hts221_spi.c
+++ b/drivers/iio/humidity/hts221_spi.c
@@ -113,6 +113,7 @@ MODULE_DEVICE_TABLE(spi, hts221_spi_id_table);
 static struct spi_driver hts221_driver = {
 	.driver = {
 		.name = "hts221_spi",
+		.pm = &hts221_pm_ops,
 		.of_match_table = of_match_ptr(hts221_spi_of_match),
 	},
 	.probe = hts221_spi_probe,
-- 
2.12.2

Re: [PATCH] iio: humidity: hts221: add power management support

[Brian Masney]

On Wed, May 10, 2017 at 10:40:05PM +0200, Lorenzo Bianconi wrote:
> Add system sleep power management support to hts221 driver
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> ---
>  drivers/iio/humidity/hts221.h      |  3 ++
>  drivers/iio/humidity/hts221_core.c | 56 +++++++++++++++++++++++++++++++++++---
>  drivers/iio/humidity/hts221_i2c.c  |  1 +
>  drivers/iio/humidity/hts221_spi.c  |  1 +
>  4 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
> index c7154665512e..94510266e0a5 100644
> --- a/drivers/iio/humidity/hts221.h
> +++ b/drivers/iio/humidity/hts221.h
> @@ -57,12 +57,15 @@ struct hts221_hw {
>  
>  	struct hts221_sensor sensors[HTS221_SENSOR_MAX];
>  
> +	bool enabled;
>  	u8 odr;
>  
>  	const struct hts221_transfer_function *tf;
>  	struct hts221_transfer_buffer tb;
>  };
>  
> +extern const struct dev_pm_ops hts221_pm_ops;
> +
>  int hts221_config_drdy(struct hts221_hw *hw, bool enable);
>  int hts221_probe(struct iio_dev *iio_dev);
>  int hts221_power_on(struct hts221_hw *hw);
> diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c
> index 3f3ef4a1a474..ccb07e7f993f 100644
> --- a/drivers/iio/humidity/hts221_core.c
> +++ b/drivers/iio/humidity/hts221_core.c
> @@ -13,6 +13,7 @@
>  #include <linux/device.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/delay.h>
> +#include <linux/pm.h>
>  #include <asm/unaligned.h>
>  
>  #include "hts221.h"
> @@ -307,15 +308,30 @@ hts221_sysfs_temp_oversampling_avail(struct device *dev,
>  
>  int hts221_power_on(struct hts221_hw *hw)
>  {
> -	return hts221_update_odr(hw, hw->odr);
> +	int err;
> +
> +	err = hts221_update_odr(hw, hw->odr);
> +	if (err < 0)
> +		return err;
> +
> +	hw->enabled = true;
> +
> +	return 0;
>  }
>  
>  int hts221_power_off(struct hts221_hw *hw)
>  {
> -	u8 data[] = {0x00, 0x00};
> +	__le16 data = 0;
> +	int err;
>  
> -	return hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
> -			     data);
> +	err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
> +			    (u8 *)&data);
> +	if (err < 0)
> +		return err;
> +
> +	hw->enabled = false;
> +
> +	return 0;
>  }
>  
>  static int hts221_parse_temp_caldata(struct hts221_hw *hw)
> @@ -682,6 +698,38 @@ int hts221_probe(struct iio_dev *iio_dev)
>  }
>  EXPORT_SYMBOL(hts221_probe);
>  
> +#ifdef CONFIG_PM
> +static int hts221_suspend(struct device *dev)

You can get rid of this #ifdef by adding __maybe_unused to the
_suspend and _resume function declarations. This will allow for
additional compile-time code checking if power management is
disabled.

If desired, one way to get rid of the enabled flag would be to
add support for runtime power management to automatically shutdown
the chip after a period of inactivity. See
https://lkml.org/lkml/2017/4/25/101 for an example.

Brian

Re: [PATCH] iio: humidity: hts221: add power management support

[Lorenzo BIANCONI]

Hi Brian,

Thanks for the review. Comments inline.

Regards,
Lorenzo

> On Wed, May 10, 2017 at 10:40:05PM +0200, Lorenzo Bianconi wrote:
> > Add system sleep power management support to hts221 driver
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> > ---
> >  drivers/iio/humidity/hts221.h      |  3 ++
> >  drivers/iio/humidity/hts221_core.c | 56 +++++++++++++++++++++++++++++++++++---
> >  drivers/iio/humidity/hts221_i2c.c  |  1 +
> >  drivers/iio/humidity/hts221_spi.c  |  1 +
> >  4 files changed, 57 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
> > index c7154665512e..94510266e0a5 100644
> > --- a/drivers/iio/humidity/hts221.h
> > +++ b/drivers/iio/humidity/hts221.h
> > @@ -57,12 +57,15 @@ struct hts221_hw {
> >  
> >  	struct hts221_sensor sensors[HTS221_SENSOR_MAX];
> >  
> > +	bool enabled;
> >  	u8 odr;
> >  
> >  	const struct hts221_transfer_function *tf;
> >  	struct hts221_transfer_buffer tb;
> >  };
> >  
> > +extern const struct dev_pm_ops hts221_pm_ops;
> > +
> >  int hts221_config_drdy(struct hts221_hw *hw, bool enable);
> >  int hts221_probe(struct iio_dev *iio_dev);
> >  int hts221_power_on(struct hts221_hw *hw);
> > diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c
> > index 3f3ef4a1a474..ccb07e7f993f 100644
> > --- a/drivers/iio/humidity/hts221_core.c
> > +++ b/drivers/iio/humidity/hts221_core.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/device.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/delay.h>
> > +#include <linux/pm.h>
> >  #include <asm/unaligned.h>
> >  
> >  #include "hts221.h"
> > @@ -307,15 +308,30 @@ hts221_sysfs_temp_oversampling_avail(struct device *dev,
> >  
> >  int hts221_power_on(struct hts221_hw *hw)
> >  {
> > -	return hts221_update_odr(hw, hw->odr);
> > +	int err;
> > +
> > +	err = hts221_update_odr(hw, hw->odr);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	hw->enabled = true;
> > +
> > +	return 0;
> >  }
> >  
> >  int hts221_power_off(struct hts221_hw *hw)
> >  {
> > -	u8 data[] = {0x00, 0x00};
> > +	__le16 data = 0;
> > +	int err;
> >  
> > -	return hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
> > -			     data);
> > +	err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
> > +			    (u8 *)&data);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	hw->enabled = false;
> > +
> > +	return 0;
> >  }
> >  
> >  static int hts221_parse_temp_caldata(struct hts221_hw *hw)
> > @@ -682,6 +698,38 @@ int hts221_probe(struct iio_dev *iio_dev)
> >  }
> >  EXPORT_SYMBOL(hts221_probe);
> >  
> > +#ifdef CONFIG_PM
> > +static int hts221_suspend(struct device *dev)
> 
> You can get rid of this #ifdef by adding __maybe_unused to the
> _suspend and _resume function declarations. This will allow for
> additional compile-time code checking if power management is
> disabled.

Fair enough :). I did in that way to maintain consistency with other drivers.

> 
> If desired, one way to get rid of the enabled flag would be to
> add support for runtime power management to automatically shutdown
> the chip after a period of inactivity. See
> https://lkml.org/lkml/2017/4/25/101 for an example.
> 

I am not a pm_runtime expert but according to the documentation runtime_suspend
callback is called when device's usage counter and counter of 'active' children
of the device are equal to 0. Moreover device possible states are 'disabled'
(HTS221_REG_CNTRL1_ADDR register set to 0) or 'active' (HTS221_REG_CNTRL1_ADDR
register configured with a given sample rate). In the first condition
runtime_suspend() will not take any effect whereas in the latter one the
callback will not be called since device's usage counter is grater than 0.
Moreover implement system-wide pm support through runtime pm support just to
avoid a boolean flag seems a little bit overkill to me. What do you think?
Is my understanding correct?

> Brian

-- 

Re: [PATCH] iio: humidity: hts221: add power management support

[Brian Masney]

On Thu, May 11, 2017 at 09:35:12AM +0000, Lorenzo BIANCONI wrote:
> > If desired, one way to get rid of the enabled flag would be to
> > add support for runtime power management to automatically shutdown
> > the chip after a period of inactivity. See
> > https://lkml.org/lkml/2017/4/25/101 for an example.
> > 
> 
> I am not a pm_runtime expert but according to the documentation runtime_suspend
> callback is called when device's usage counter and counter of 'active' children
> of the device are equal to 0. Moreover device possible states are 'disabled'
> (HTS221_REG_CNTRL1_ADDR register set to 0) or 'active' (HTS221_REG_CNTRL1_ADDR
> register configured with a given sample rate). In the first condition
> runtime_suspend() will not take any effect whereas in the latter one the
> callback will not be called since device's usage counter is grater than 0.
> Moreover implement system-wide pm support through runtime pm support just to
> avoid a boolean flag seems a little bit overkill to me. What do you think?
> Is my understanding correct?

Sorry, I should have also said that I didn't think that runtime PM was
absolutely required. I also agree adding runtime PM just to remove the
flag is overkill. Runtime PM is nice to have if this sensor is hooked
up to something that may run off a battery (such as a weather station)
to help conserve power. The flag removal is a by product of this. :)

As for the runtime PM reference count, when your driver is initialized,
keep the device off so that the device usage count is initially zero.
Wrap your _write_raw() and _read_raw() functions with the runtime PM
calls and the device will power on and off as needed.

Brian

Re: [PATCH] iio: humidity: hts221: add power management support

[Lorenzo BIANCONI]

On May 11, Brian Masney wrote:
> On Thu, May 11, 2017 at 09:35:12AM +0000, Lorenzo BIANCONI wrote:
> > > If desired, one way to get rid of the enabled flag would be to
> > > add support for runtime power management to automatically shutdown
> > > the chip after a period of inactivity. See
> > > https://lkml.org/lkml/2017/4/25/101 for an example.
> > > 
> > 
> > I am not a pm_runtime expert but according to the documentation runtime_suspend
> > callback is called when device's usage counter and counter of 'active' children
> > of the device are equal to 0. Moreover device possible states are 'disabled'
> > (HTS221_REG_CNTRL1_ADDR register set to 0) or 'active' (HTS221_REG_CNTRL1_ADDR
> > register configured with a given sample rate). In the first condition
> > runtime_suspend() will not take any effect whereas in the latter one the
> > callback will not be called since device's usage counter is grater than 0.
> > Moreover implement system-wide pm support through runtime pm support just to
> > avoid a boolean flag seems a little bit overkill to me. What do you think?
> > Is my understanding correct?

Hi Brian,

> 
> Sorry, I should have also said that I didn't think that runtime PM was
> absolutely required. I also agree adding runtime PM just to remove the
> flag is overkill. Runtime PM is nice to have if this sensor is hooked
> up to something that may run off a battery (such as a weather station)
> to help conserve power. The flag removal is a by product of this. :)
> 
> As for the runtime PM reference count, when your driver is initialized,
> keep the device off so that the device usage count is initially zero.
> Wrap your _write_raw() and _read_raw() functions with the runtime PM
> calls and the device will power on and off as needed.

I mean ->runtime_suspend()/->runtime_resume() are never called by the PM core
when the sensor is active since usage count is always grater than 0.
On the contrary when usage count is equal to 0 the sensor is already disabled,
so the ODR configuration does not make any difference.

Regards,
Lorenzo

> 
> Brian

-- 

Re: [PATCH] iio: humidity: hts221: add power management support

[Brian Masney]

On Thu, May 11, 2017 at 11:10:31AM +0000, Lorenzo BIANCONI wrote:
> On May 11, Brian Masney wrote:
> > On Thu, May 11, 2017 at 09:35:12AM +0000, Lorenzo BIANCONI wrote:
> > > > If desired, one way to get rid of the enabled flag would be to
> > > > add support for runtime power management to automatically shutdown
> > > > the chip after a period of inactivity. See
> > > > https://lkml.org/lkml/2017/4/25/101 for an example.
> > > > 
> > > 
> > > I am not a pm_runtime expert but according to the documentation runtime_suspend
> > > callback is called when device's usage counter and counter of 'active' children
> > > of the device are equal to 0. Moreover device possible states are 'disabled'
> > > (HTS221_REG_CNTRL1_ADDR register set to 0) or 'active' (HTS221_REG_CNTRL1_ADDR
> > > register configured with a given sample rate). In the first condition
> > > runtime_suspend() will not take any effect whereas in the latter one the
> > > callback will not be called since device's usage counter is grater than 0.
> > > Moreover implement system-wide pm support through runtime pm support just to
> > > avoid a boolean flag seems a little bit overkill to me. What do you think?
> > > Is my understanding correct?
> 
> Hi Brian,
> 
> > 
> > Sorry, I should have also said that I didn't think that runtime PM was
> > absolutely required. I also agree adding runtime PM just to remove the
> > flag is overkill. Runtime PM is nice to have if this sensor is hooked
> > up to something that may run off a battery (such as a weather station)
> > to help conserve power. The flag removal is a by product of this. :)
> > 
> > As for the runtime PM reference count, when your driver is initialized,
> > keep the device off so that the device usage count is initially zero.
> > Wrap your _write_raw() and _read_raw() functions with the runtime PM
> > calls and the device will power on and off as needed.
> 
> I mean ->runtime_suspend()/->runtime_resume() are never called by the PM core
> when the sensor is active since usage count is always grater than 0.
> On the contrary when usage count is equal to 0 the sensor is already disabled,
> so the ODR configuration does not make any difference.

Ohh, I see. That makes sense. Thanks for clarifying.

Brian

Re: [PATCH] iio: humidity: hts221: add power management support

[Jonathan Cameron]

On 11/05/17 10:35, Lorenzo BIANCONI wrote:
> Hi Brian,
> 
> Thanks for the review. Comments inline.
> 
> Regards,
> Lorenzo
> 
>> On Wed, May 10, 2017 at 10:40:05PM +0200, Lorenzo Bianconi wrote:
>>> Add system sleep power management support to hts221 driver
>>>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>> ---
>>>   drivers/iio/humidity/hts221.h      |  3 ++
>>>   drivers/iio/humidity/hts221_core.c | 56 +++++++++++++++++++++++++++++++++++---
>>>   drivers/iio/humidity/hts221_i2c.c  |  1 +
>>>   drivers/iio/humidity/hts221_spi.c  |  1 +
>>>   4 files changed, 57 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iio/humidity/hts221.h b/drivers/iio/humidity/hts221.h
>>> index c7154665512e..94510266e0a5 100644
>>> --- a/drivers/iio/humidity/hts221.h
>>> +++ b/drivers/iio/humidity/hts221.h
>>> @@ -57,12 +57,15 @@ struct hts221_hw {
>>>   
>>>   	struct hts221_sensor sensors[HTS221_SENSOR_MAX];
>>>   
>>> +	bool enabled;
>>>   	u8 odr;
>>>   
>>>   	const struct hts221_transfer_function *tf;
>>>   	struct hts221_transfer_buffer tb;
>>>   };
>>>   
>>> +extern const struct dev_pm_ops hts221_pm_ops;
>>> +
>>>   int hts221_config_drdy(struct hts221_hw *hw, bool enable);
>>>   int hts221_probe(struct iio_dev *iio_dev);
>>>   int hts221_power_on(struct hts221_hw *hw);
>>> diff --git a/drivers/iio/humidity/hts221_core.c b/drivers/iio/humidity/hts221_core.c
>>> index 3f3ef4a1a474..ccb07e7f993f 100644
>>> --- a/drivers/iio/humidity/hts221_core.c
>>> +++ b/drivers/iio/humidity/hts221_core.c
>>> @@ -13,6 +13,7 @@
>>>   #include <linux/device.h>
>>>   #include <linux/iio/sysfs.h>
>>>   #include <linux/delay.h>
>>> +#include <linux/pm.h>
>>>   #include <asm/unaligned.h>
>>>   
>>>   #include "hts221.h"
>>> @@ -307,15 +308,30 @@ hts221_sysfs_temp_oversampling_avail(struct device *dev,
>>>   
>>>   int hts221_power_on(struct hts221_hw *hw)
>>>   {
>>> -	return hts221_update_odr(hw, hw->odr);
>>> +	int err;
>>> +
>>> +	err = hts221_update_odr(hw, hw->odr);
>>> +	if (err < 0)
>>> +		return err;
>>> +
>>> +	hw->enabled = true;
>>> +
>>> +	return 0;
>>>   }
>>>   
>>>   int hts221_power_off(struct hts221_hw *hw)
>>>   {
>>> -	u8 data[] = {0x00, 0x00};
>>> +	__le16 data = 0;
>>> +	int err;
>>>   
>>> -	return hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
>>> -			     data);
>>> +	err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR, sizeof(data),
>>> +			    (u8 *)&data);
>>> +	if (err < 0)
>>> +		return err;
>>> +
>>> +	hw->enabled = false;
>>> +
>>> +	return 0;
>>>   }
>>>   
>>>   static int hts221_parse_temp_caldata(struct hts221_hw *hw)
>>> @@ -682,6 +698,38 @@ int hts221_probe(struct iio_dev *iio_dev)
>>>   }
>>>   EXPORT_SYMBOL(hts221_probe);
>>>   
>>> +#ifdef CONFIG_PM
>>> +static int hts221_suspend(struct device *dev)
>>
>> You can get rid of this #ifdef by adding __maybe_unused to the
>> _suspend and _resume function declarations. This will allow for
>> additional compile-time code checking if power management is
>> disabled.
> 
> Fair enough :). I did in that way to maintain consistency with other drivers.
More recent thinking is that the ifdef route is a mess hence
the __maybe_unused option is becoming more common and is the
preferred choice now.  People are slowly working their way through
older drivers tidying this up but lots still to do.

Jonathan
> 
>>
>> If desired, one way to get rid of the enabled flag would be to
>> add support for runtime power management to automatically shutdown
>> the chip after a period of inactivity. See
>> https://lkml.org/lkml/2017/4/25/101 for an example.
>>
> 
> I am not a pm_runtime expert but according to the documentation runtime_suspend
> callback is called when device's usage counter and counter of 'active' children
> of the device are equal to 0. Moreover device possible states are 'disabled'
> (HTS221_REG_CNTRL1_ADDR register set to 0) or 'active' (HTS221_REG_CNTRL1_ADDR
> register configured with a given sample rate). In the first condition
> runtime_suspend() will not take any effect whereas in the latter one the
> callback will not be called since device's usage counter is grater than 0.
> Moreover implement system-wide pm support through runtime pm support just to
> avoid a boolean flag seems a little bit overkill to me. What do you think?
> Is my understanding correct?
> 
>> Brian
> 
> -- --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: humidity: hts221: add power management support

[Lorenzo Bianconi]

> On 11/05/17 10:35, Lorenzo BIANCONI wrote:
>>
>> Hi Brian,
>>
>> Thanks for the review. Comments inline.
>>
>> Regards,
>> Lorenzo
>>
>>> On Wed, May 10, 2017 at 10:40:05PM +0200, Lorenzo Bianconi wrote:
>>>>
>>>> Add system sleep power management support to hts221 driver
>>>>
>>>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
>>>> ---
>>>>   drivers/iio/humidity/hts221.h      |  3 ++
>>>>   drivers/iio/humidity/hts221_core.c | 56
>>>> +++++++++++++++++++++++++++++++++++---
>>>>   drivers/iio/humidity/hts221_i2c.c  |  1 +
>>>>   drivers/iio/humidity/hts221_spi.c  |  1 +
>>>>   4 files changed, 57 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/humidity/hts221.h
>>>> b/drivers/iio/humidity/hts221.h
>>>> index c7154665512e..94510266e0a5 100644
>>>> --- a/drivers/iio/humidity/hts221.h
>>>> +++ b/drivers/iio/humidity/hts221.h
>>>> @@ -57,12 +57,15 @@ struct hts221_hw {
>>>>         struct hts221_sensor sensors[HTS221_SENSOR_MAX];
>>>>   +     bool enabled;
>>>>         u8 odr;
>>>>         const struct hts221_transfer_function *tf;
>>>>         struct hts221_transfer_buffer tb;
>>>>   };
>>>>   +extern const struct dev_pm_ops hts221_pm_ops;
>>>> +
>>>>   int hts221_config_drdy(struct hts221_hw *hw, bool enable);
>>>>   int hts221_probe(struct iio_dev *iio_dev);
>>>>   int hts221_power_on(struct hts221_hw *hw);
>>>> diff --git a/drivers/iio/humidity/hts221_core.c
>>>> b/drivers/iio/humidity/hts221_core.c
>>>> index 3f3ef4a1a474..ccb07e7f993f 100644
>>>> --- a/drivers/iio/humidity/hts221_core.c
>>>> +++ b/drivers/iio/humidity/hts221_core.c
>>>> @@ -13,6 +13,7 @@
>>>>   #include <linux/device.h>
>>>>   #include <linux/iio/sysfs.h>
>>>>   #include <linux/delay.h>
>>>> +#include <linux/pm.h>
>>>>   #include <asm/unaligned.h>
>>>>     #include "hts221.h"
>>>> @@ -307,15 +308,30 @@ hts221_sysfs_temp_oversampling_avail(struct device
>>>> *dev,
>>>>     int hts221_power_on(struct hts221_hw *hw)
>>>>   {
>>>> -       return hts221_update_odr(hw, hw->odr);
>>>> +       int err;
>>>> +
>>>> +       err = hts221_update_odr(hw, hw->odr);
>>>> +       if (err < 0)
>>>> +               return err;
>>>> +
>>>> +       hw->enabled = true;
>>>> +
>>>> +       return 0;
>>>>   }
>>>>     int hts221_power_off(struct hts221_hw *hw)
>>>>   {
>>>> -       u8 data[] = {0x00, 0x00};
>>>> +       __le16 data = 0;
>>>> +       int err;
>>>>   -     return hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR,
>>>> sizeof(data),
>>>> -                            data);
>>>> +       err = hw->tf->write(hw->dev, HTS221_REG_CNTRL1_ADDR,
>>>> sizeof(data),
>>>> +                           (u8 *)&data);
>>>> +       if (err < 0)
>>>> +               return err;
>>>> +
>>>> +       hw->enabled = false;
>>>> +
>>>> +       return 0;
>>>>   }
>>>>     static int hts221_parse_temp_caldata(struct hts221_hw *hw)
>>>> @@ -682,6 +698,38 @@ int hts221_probe(struct iio_dev *iio_dev)
>>>>   }
>>>>   EXPORT_SYMBOL(hts221_probe);
>>>>   +#ifdef CONFIG_PM
>>>> +static int hts221_suspend(struct device *dev)
>>>
>>>
>>> You can get rid of this #ifdef by adding __maybe_unused to the
>>> _suspend and _resume function declarations. This will allow for
>>> additional compile-time code checking if power management is
>>> disabled.
>>
>>
>> Fair enough :). I did in that way to maintain consistency with other
>> drivers.
>
> More recent thinking is that the ifdef route is a mess hence
> the __maybe_unused option is becoming more common and is the
> preferred choice now.  People are slowly working their way through
> older drivers tidying this up but lots still to do.
>
> Jonathan

Fine, v2 on the way.

Regards,
Lorenzo

>>
>>
>>>
>>> If desired, one way to get rid of the enabled flag would be to
>>> add support for runtime power management to automatically shutdown
>>> the chip after a period of inactivity. See
>>> https://lkml.org/lkml/2017/4/25/101 for an example.
>>>
>>
>> I am not a pm_runtime expert but according to the documentation
>> runtime_suspend
>> callback is called when device's usage counter and counter of 'active'
>> children
>> of the device are equal to 0. Moreover device possible states are
>> 'disabled'
>> (HTS221_REG_CNTRL1_ADDR register set to 0) or 'active'
>> (HTS221_REG_CNTRL1_ADDR
>> register configured with a given sample rate). In the first condition
>> runtime_suspend() will not take any effect whereas in the latter one the
>> callback will not be called since device's usage counter is grater than 0.
>> Moreover implement system-wide pm support through runtime pm support just
>> to
>> avoid a boolean flag seems a little bit overkill to me. What do you think?
>> Is my understanding correct?
>>
>>> Brian
>>
>>
>> -- --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>



-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep