On Sat, Jan 21, 2017 at 02:58:12PM +0000, Jonathan Cameron wrote:
> On 17/01/17 09:25, Brian Masney wrote:
> > This patch adds runtime power management support to the isl29028 driver.
> > It defaults to powering off the device after two seconds of inactivity.
> >
> > isl29028_chip_init_and_power_on() currently only zeros the CONFIGURE
> > register on the chip, which will cause the chip to turn off. This patch
> > also renames that function to isl29028_clear_configure_reg() since it is
> > now used in several places.
> >
> > Signed-off-by: Brian Masney <masneyb@xxxxxxxxxxxxx>
> Whilst I'm not against it by an means, runtime PM is hardly a requirement
> for moving out of staging! Good stuff though so I'm not going to turn it
> down ;)
Thanks. I know it wasn't a requirement for a staging graduation but I
did it just for the fun of it to learn more about power management in
the kernel.
> I'm not 100% sure about the one comment I made at the end.
> It's a very unlikely to occur condition anyway, but if you want to follow up
> with a patch on that then feel free.
See below.
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
>
> thanks,
>
> Jonathan
> > ---
> > drivers/staging/iio/light/isl29028.c | 118 ++++++++++++++++++++++++++++++++---
> > 1 file changed, 110 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> > index 598a5a5..a3264f7 100644
> > --- a/drivers/staging/iio/light/isl29028.c
> > +++ b/drivers/staging/iio/light/isl29028.c
> > @@ -26,6 +26,7 @@
> > #include <linux/regmap.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > +#include <linux/pm_runtime.h>
> >
> > #define ISL29028_CONV_TIME_MS 100
> >
> > @@ -60,6 +61,8 @@
> >
> > #define ISL29028_NUM_REGS (ISL29028_REG_TEST2_MODE + 1)
> >
> > +#define ISL29028_POWER_OFF_DELAY_MS 2000
> > +
> > enum isl29028_als_ir_mode {
> > ISL29028_MODE_NONE = 0,
> > ISL29028_MODE_ALS,
> > @@ -297,6 +300,23 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
> > return isl29028_read_als_ir(chip, ir_data);
> > }
> >
> > +static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
> > +{
> > + struct device *dev = regmap_get_device(chip->regmap);
> > + int ret;
> > +
> > + if (on) {
> > + ret = pm_runtime_get_sync(dev);
> > + if (ret < 0)
> > + pm_runtime_put_noidle(dev);
> > + } else {
> > + pm_runtime_mark_last_busy(dev);
> > + ret = pm_runtime_put_autosuspend(dev);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /* Channel IO */
> > static int isl29028_write_raw(struct iio_dev *indio_dev,
> > struct iio_chan_spec const *chan,
> > @@ -304,9 +324,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
> > {
> > struct isl29028_chip *chip = iio_priv(indio_dev);
> > struct device *dev = regmap_get_device(chip->regmap);
> > - int ret = -EINVAL;
> > + int ret;
> > +
> > + ret = isl29028_set_pm_runtime_busy(chip, true);
> > + if (ret < 0)
> > + return ret;
> >
> > mutex_lock(&chip->lock);
> > +
> > + ret = -EINVAL;
> > switch (chan->type) {
> > case IIO_PROXIMITY:
> > if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
> > @@ -350,6 +376,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
> >
> > mutex_unlock(&chip->lock);
> >
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = isl29028_set_pm_runtime_busy(chip, false);
> > + if (ret < 0)
> > + return ret;
> > +
> > return ret;
> > }
> >
> > @@ -359,9 +392,15 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
> > {
> > struct isl29028_chip *chip = iio_priv(indio_dev);
> > struct device *dev = regmap_get_device(chip->regmap);
> > - int ret = -EINVAL;
> > + int ret, pm_ret;
> > +
> > + ret = isl29028_set_pm_runtime_busy(chip, true);
> > + if (ret < 0)
> > + return ret;
> >
> > mutex_lock(&chip->lock);
> > +
> > + ret = -EINVAL;
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > case IIO_CHAN_INFO_PROCESSED:
> > @@ -405,6 +444,18 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
> >
> > mutex_unlock(&chip->lock);
> >
> > + if (ret < 0)
> > + return ret;
> > +
> > + /**
> > + * Preserve the ret variable if the call to
> > + * isl29028_set_pm_runtime_busy() is successful so the reading
> > + * (if applicable) is returned to user space.
> > + */
> > + pm_ret = isl29028_set_pm_runtime_busy(chip, false);
> > + if (pm_ret < 0)
> > + return pm_ret;
> > +
> > return ret;
> > }
> >
> > @@ -445,17 +496,18 @@ static const struct iio_info isl29028_info = {
> > .write_raw = isl29028_write_raw,
> > };
> >
> > -static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
> > +static int isl29028_clear_configure_reg(struct isl29028_chip *chip)
> > {
> > struct device *dev = regmap_get_device(chip->regmap);
> > int ret;
> >
> > ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
> > __func__, ret);
> > - return ret;
> > - }
> > +
> > + chip->als_ir_mode = ISL29028_MODE_NONE;
> > + chip->enable_prox = false;
> >
> > return ret;
> > }
> > @@ -509,7 +561,6 @@ static int isl29028_probe(struct i2c_client *client,
> > chip->enable_prox = false;
> > chip->prox_sampling = 20;
> > chip->lux_scale = 2000;
> > - chip->als_ir_mode = ISL29028_MODE_NONE;
> >
> > ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
> > if (ret < 0) {
> > @@ -527,7 +578,7 @@ static int isl29028_probe(struct i2c_client *client,
> > return ret;
> > }
> >
> > - ret = isl29028_chip_init_and_power_on(chip);
> > + ret = isl29028_clear_configure_reg(chip);
> > if (ret < 0)
> > return ret;
> >
> > @@ -538,6 +589,11 @@ static int isl29028_probe(struct i2c_client *client,
> > indio_dev->dev.parent = &client->dev;
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >
> > + pm_runtime_enable(&client->dev);
> > + pm_runtime_set_autosuspend_delay(&client->dev,
> > + ISL29028_POWER_OFF_DELAY_MS);
> > + pm_runtime_use_autosuspend(&client->dev);
> > +
> > ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > if (ret < 0) {
> > dev_err(&client->dev,
> > @@ -549,6 +605,50 @@ static int isl29028_probe(struct i2c_client *client,
> > return 0;
> > }
> >
> > +static int isl29028_remove(struct i2c_client *client)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > + struct isl29028_chip *chip = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + pm_runtime_disable(&client->dev);
> > + pm_runtime_set_suspended(&client->dev);
> > + pm_runtime_put_noidle(&client->dev);
> > +
> > + return isl29028_clear_configure_reg(chip);
> > +}
> > +
> > +static int __maybe_unused isl29028_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > + struct isl29028_chip *chip = iio_priv(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(&chip->lock);
> > +
> > + ret = isl29028_clear_configure_reg(chip);
> I'm stretching a bit, but there might be a race here.
> If we get a suspend and resume in under the time it takes for
> the auto suspend to conclude it is good to suspend, it might think
> the device is still ready to go when it isn't.
I don't believe that there is a race condition in that situation since
all of the i2c calls outside of the probe and remove functions come from
either isl29028_write_raw(), isl29028_read_raw(), or isl29028_suspend().
All of three of those functions lock a common mutex.
I just noticed that isl29028_remove() doesn't lock that shared mutex
when the device is removed and the chip is powered off. This is
unlikely to be an issue but I'll send a follow up patch in my next
series fixing that just to be sure.
Brian
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
