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@xxxxxx>
> > ---
> > 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
-- --
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
