[PATCH 0/2] Two bq24190 PM improvments

[PATCH 0/2] Two bq24190 PM improvments

[Tony Lindgren]

Hi,

Here are the two bq24190 patches rebased against current power-supply
tree now that Liam's changes are applied.

I've kept Mark's acks as the changes were minor and mostly limited to
bq24190_probe() basedon Liam's changes.

Regards,

Tony


Changes from earlier set:

- Separated from Liam's changes that are now in the power-supply tree

- Moved bdi->initialized before devm_request_threaded_irq() as suggested
  by Liam

- Added missing return value check for pm_runtime_get() as suggested by
  Liam

- Updated against current power-supply tree, mostly for the probe error
  handling

Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

 drivers/power/supply/bq24190_charger.c | 180 ++++++++++++++++++++++++++-------
 1 file changed, 143 insertions(+), 37 deletions(-)

-- 
2.11.0

[PATCH 1/2] power: bq24190_charger: Check the interrupt status on resume

[Tony Lindgren]

Some SoCs like omap3 can configure GPIO irqs to use Linux generic
dedicated wakeirq support. If the dedicated wakeirq is configured,
the SoC will use a always-on interrupt controller to produce wake-up
events.

If bq24190 is configured for dedicated wakeirq, we need to check the
interrupt status on PM runtime resume. This is because the Linux
generic wakeirq will call pm_runtime_resume() on the device on a
wakeirq. And as the bq24190 interrupt is falling edge sensitive
and only active for 250 us, there will be no device interrupt seen
by the runtime SoC IRQ controller.

Note that this can cause spurious interrupts on omap3 devices with
bq24190 connected to gpio banks 2 - 5 as there's a glitch on those
pins waking from off mode as listed in "Advisory 1.45". Devices
with this issue should not configure the optional wakeirq interrupt
in the dts file.

Cc: Matt Ranostay <matt@ranostay.consulting>
Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 62 ++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -155,6 +155,8 @@ struct bq24190_dev_info {
 	kernel_ulong_t			model;
 	unsigned int			gpio_int;
 	unsigned int			irq;
+	bool				initialized;
+	bool				irq_event;
 	struct mutex			f_reg_lock;
 	u8				f_reg;
 	u8				ss_reg;
@@ -1157,9 +1159,8 @@ static const struct power_supply_desc bq24190_battery_desc = {
 	.property_is_writeable	= bq24190_battery_property_is_writeable,
 };
 
-static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
+static void bq24190_check_status(struct bq24190_dev_info *bdi)
 {
-	struct bq24190_dev_info *bdi = data;
 	const u8 battery_mask_ss = BQ24190_REG_SS_CHRG_STAT_MASK;
 	const u8 battery_mask_f = BQ24190_REG_F_BAT_FAULT_MASK
 				| BQ24190_REG_F_NTC_FAULT_MASK;
@@ -1167,12 +1168,10 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	u8 ss_reg = 0, f_reg = 0;
 	int i, ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
 	if (ret < 0) {
 		dev_err(bdi->dev, "Can't read SS reg: %d\n", ret);
-		goto out;
+		return;
 	}
 
 	i = 0;
@@ -1180,7 +1179,7 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 		ret = bq24190_read(bdi, BQ24190_REG_F, &f_reg);
 		if (ret < 0) {
 			dev_err(bdi->dev, "Can't read F reg: %d\n", ret);
-			goto out;
+			return;
 		}
 	} while (f_reg && ++i < 2);
 
@@ -1229,10 +1228,18 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	if (alert_battery)
 		power_supply_changed(bdi->battery);
 
-out:
-	pm_runtime_put_sync(bdi->dev);
-
 	dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
+}
+
+static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
+{
+	struct bq24190_dev_info *bdi = data;
+
+	bdi->irq_event = true;
+	pm_runtime_get_sync(bdi->dev);
+	bq24190_check_status(bdi);
+	pm_runtime_put_sync(bdi->dev);
+	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
 }
@@ -1391,6 +1398,8 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out3;
 	}
 
+	bdi->initialized = true;
+
 	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
 			bq24190_irq_handler_thread,
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
@@ -1437,6 +1446,35 @@ static int bq24190_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int bq24190_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+
+	if (!bdi->initialized)
+		return 0;
+
+	dev_dbg(bdi->dev, "%s\n", __func__);
+
+	return 0;
+}
+
+static int bq24190_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+
+	if (!bdi->initialized)
+		return 0;
+
+	if (!bdi->irq_event) {
+		dev_dbg(bdi->dev, "checking events on possible wakeirq\n");
+		bq24190_check_status(bdi);
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int bq24190_pm_suspend(struct device *dev)
 {
@@ -1472,7 +1510,11 @@ static int bq24190_pm_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(bq24190_pm_ops, bq24190_pm_suspend, bq24190_pm_resume);
+static const struct dev_pm_ops bq24190_pm_ops = {
+	SET_RUNTIME_PM_OPS(bq24190_runtime_suspend, bq24190_runtime_resume,
+			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(bq24190_pm_suspend, bq24190_pm_resume)
+};
 
 /*
  * Only support the bq24190 right now.  The bq24192, bq24192i, and bq24193
-- 
2.11.0

Re: [PATCH 1/2] power: bq24190_charger: Check the interrupt status on resume

[Liam Breck]

On Fri, Jan 20, 2017 at 2:04 PM, Tony Lindgren <tony@atomide.com> wrote:
> Some SoCs like omap3 can configure GPIO irqs to use Linux generic
> dedicated wakeirq support. If the dedicated wakeirq is configured,
> the SoC will use a always-on interrupt controller to produce wake-up
> events.
>
> If bq24190 is configured for dedicated wakeirq, we need to check the
> interrupt status on PM runtime resume. This is because the Linux
> generic wakeirq will call pm_runtime_resume() on the device on a
> wakeirq. And as the bq24190 interrupt is falling edge sensitive
> and only active for 250 us, there will be no device interrupt seen
> by the runtime SoC IRQ controller.
>
> Note that this can cause spurious interrupts on omap3 devices with
> bq24190 connected to gpio banks 2 - 5 as there's a glitch on those
> pins waking from off mode as listed in "Advisory 1.45". Devices
> with this issue should not configure the optional wakeirq interrupt
> in the dts file.
>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 62 ++++++++++++++++++++++++++++------
>  1 file changed, 52 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -155,6 +155,8 @@ struct bq24190_dev_info {
>         kernel_ulong_t                  model;
>         unsigned int                    gpio_int;
>         unsigned int                    irq;
> +       bool                            initialized;
> +       bool                            irq_event;
>         struct mutex                    f_reg_lock;
>         u8                              f_reg;
>         u8                              ss_reg;
> @@ -1157,9 +1159,8 @@ static const struct power_supply_desc bq24190_battery_desc = {
>         .property_is_writeable  = bq24190_battery_property_is_writeable,
>  };
>
> -static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> +static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  {
> -       struct bq24190_dev_info *bdi = data;
>         const u8 battery_mask_ss = BQ24190_REG_SS_CHRG_STAT_MASK;
>         const u8 battery_mask_f = BQ24190_REG_F_BAT_FAULT_MASK
>                                 | BQ24190_REG_F_NTC_FAULT_MASK;
> @@ -1167,12 +1168,10 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         u8 ss_reg = 0, f_reg = 0;
>         int i, ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> -
>         ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
>         if (ret < 0) {
>                 dev_err(bdi->dev, "Can't read SS reg: %d\n", ret);
> -               goto out;
> +               return;
>         }
>
>         i = 0;
> @@ -1180,7 +1179,7 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>                 ret = bq24190_read(bdi, BQ24190_REG_F, &f_reg);
>                 if (ret < 0) {
>                         dev_err(bdi->dev, "Can't read F reg: %d\n", ret);
> -                       goto out;
> +                       return;
>                 }
>         } while (f_reg && ++i < 2);
>
> @@ -1229,10 +1228,18 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         if (alert_battery)
>                 power_supply_changed(bdi->battery);
>
> -out:
> -       pm_runtime_put_sync(bdi->dev);
> -
>         dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
> +}
> +
> +static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> +{
> +       struct bq24190_dev_info *bdi = data;
> +
> +       bdi->irq_event = true;
> +       pm_runtime_get_sync(bdi->dev);

Do we need to call pm_runtime_irq_safe() prior to the above?

> +       bq24190_check_status(bdi);
> +       pm_runtime_put_sync(bdi->dev);
> +       bdi->irq_event = false;
>
>         return IRQ_HANDLED;
>  }
> ...

Re: [PATCH 1/2] power: bq24190_charger: Check the interrupt status on resume

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Hi,

On Sat, Jan 21, 2017 at 12:08:02AM -0800, Liam Breck wrote:
> > +static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> > +{
> > +       struct bq24190_dev_info *bdi = data;
> > +
> > +       bdi->irq_event = true;
> > +       pm_runtime_get_sync(bdi->dev);
> 
> Do we need to call pm_runtime_irq_safe() prior to the above?

It's not needed here, since the irq handler does not run in
irq context (it's a threaded irq handler).

Btw. calling pm_runtime_irq_safe() basically means, that your
driver's power-management is broken:

http://lxr.free-electrons.com/source/drivers/base/power/runtime.c#L1288

(The relevant part is: »It also causes the parent's usage counter to
be permanently incremented«)

> > +       bq24190_check_status(bdi);
> > +       pm_runtime_put_sync(bdi->dev);
> > +       bdi->irq_event = false;
> >
> >         return IRQ_HANDLED;
> >  }

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] power: bq24190_charger: Check the interrupt status on resume

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

Hi Tony,

I think your runtime_resume is missing runtime_pm_get/put_sync, see
below:

On Fri, Jan 20, 2017 at 02:04:39PM -0800, Tony Lindgren wrote:
> [...]
> +static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> +{
> +	struct bq24190_dev_info *bdi = data;
> +
> +	bdi->irq_event = true;
> +	pm_runtime_get_sync(bdi->dev);
> +	bq24190_check_status(bdi);
> +	pm_runtime_put_sync(bdi->dev);

bq24190_check_status is called with runtime_pm enabled.

> +	bdi->irq_event = false;
>  
>  	return IRQ_HANDLED;
>  }
>
> [...]
>
> +static int bq24190_runtime_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +
> +	if (!bdi->initialized)
> +		return 0;
> +
> +	if (!bdi->irq_event) {
> +		dev_dbg(bdi->dev, "checking events on possible wakeirq\n");
> +		bq24190_check_status(bdi);

bq24190_check_status is called without runtime_pm enabled.

> +	}
> +
> +	return 0;
> +}
>
> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] power: bq24190_charger: Check the interrupt status on resume

[Tony Lindgren]

* Sebastian Reichel <sre@kernel.org> [170121 17:55]:
> Hi Tony,
> 
> I think your runtime_resume is missing runtime_pm_get/put_sync, see
> below:
> 
> On Fri, Jan 20, 2017 at 02:04:39PM -0800, Tony Lindgren wrote:
> > [...]
> > +static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> > +{
> > +	struct bq24190_dev_info *bdi = data;
> > +
> > +	bdi->irq_event = true;
> > +	pm_runtime_get_sync(bdi->dev);
> > +	bq24190_check_status(bdi);
> > +	pm_runtime_put_sync(bdi->dev);
> 
> bq24190_check_status is called with runtime_pm enabled.
> 
> > +	bdi->irq_event = false;
> >  
> >  	return IRQ_HANDLED;
> >  }
> >
> > [...]
> >
> > +static int bq24190_runtime_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +
> > +	if (!bdi->initialized)
> > +		return 0;
> > +
> > +	if (!bdi->irq_event) {
> > +		dev_dbg(bdi->dev, "checking events on possible wakeirq\n");
> > +		bq24190_check_status(bdi);
> 
> bq24190_check_status is called without runtime_pm enabled.

But it's the runtime_resume call we're calling it from :)

I don't think there is anything to configure in bq24190 at least
at this point, we just want to see if we got the wakeirq call
resume while in some deeper idle state.

Regards,

Tony

[PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Matt Ranostay <matt@ranostay.consulting>
Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 122 +++++++++++++++++++++++++--------
 1 file changed, 93 insertions(+), 29 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,7 +1286,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
@@ -1274,7 +1313,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 
 	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 out:
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1364,12 +1405,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	pm_runtime_use_autosuspend(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1425,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1434,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1451,28 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,9 +1482,13 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
 	bq24190_register_reset(bdi);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_put_sync(bdi->dev);
 
 	bq24190_sysfs_remove_group(bdi);
@@ -1480,10 +1536,14 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return 0;
 }
@@ -1492,15 +1552,19 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Fri, Jan 20, 2017 at 2:04 PM, Tony Lindgren <tony@atomide.com> wrote:
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> state.

Which device state would constantly toggle?

> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Matt Ranostay <matt@ranostay.consulting>

We can probably drop Matt from this thread; he's been busy with the
fuel gauge, etc.

> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 122 +++++++++++++++++++++++++--------
>  1 file changed, 93 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         struct power_supply *psy = dev_get_drvdata(dev);
>         struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>         struct bq24190_sysfs_field_info *info;
> +       ssize_t count;
>         int ret;
>         u8 v;
>
> @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         if (!info)
>                 return -EINVAL;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +       else
> +               count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);

Unless it violates a style guide, I prefer
  count = ret ? ret : scnprintf(buf, PAGE_SIZE, "%hhx\n", v);

> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
> -       return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> +       return count;
>  }
>
>  static ssize_t bq24190_sysfs_store(struct device *dev,
> @@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
>         if (ret < 0)
>                 return ret;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
>         return count;
>  }
> @@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> @@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_ONLINE:
> @@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>         struct bq24190_dev_info *bdi = data;
> +       int ret;
>
>         bdi->irq_event = true;
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>         bq24190_check_status(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>         bdi->irq_event = false;
>
>         return IRQ_HANDLED;
> @@ -1249,7 +1286,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>         u8 v;
>         int ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Why is pm_runtime_*() necessary in this function?
probe() already calls pm_runtime_*() around hw_init(); nothing else calls it.

>         /* First check that the device really is what its supposed to be */
>         ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
> @@ -1274,7 +1313,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>
>         ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>  out:
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1364,12 +1405,16 @@ static int bq24190_probe(struct i2c_client *client,
>         }

v1 patchset called enable_irq_wake() here.
v2 change list does not mention its removal.

>         pm_runtime_enable(dev);
> -       pm_runtime_resume(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 600);
> +       pm_runtime_use_autosuspend(dev);
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               goto out1;
>
>         ret = bq24190_hw_init(bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Hardware init failed\n");
> -               goto out1;
> +               goto out2;
>         }
>
>         charger_cfg.drv_data = bdi;
> @@ -1380,7 +1425,7 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->charger)) {
>                 dev_err(dev, "Can't register charger\n");
>                 ret = PTR_ERR(bdi->charger);
> -               goto out1;
> +               goto out2;
>         }
>
>         battery_cfg.drv_data = bdi;
> @@ -1389,13 +1434,13 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->battery)) {
>                 dev_err(dev, "Can't register battery\n");
>                 ret = PTR_ERR(bdi->battery);
> -               goto out2;
> +               goto out3;
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto out3;
> +               goto out4;
>         }
>
>         bdi->initialized = true;
> @@ -1406,21 +1451,28 @@ static int bq24190_probe(struct i2c_client *client,
>                         "bq24190-charger", bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Can't set up irq handler\n");
> -               goto out4;
> +               goto out5;
>         }
>
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +
>         return 0;
>
> -out4:
> +out5:
>         bq24190_sysfs_remove_group(bdi);
>
> -out3:
> +out4:
>         power_supply_unregister(bdi->battery);
>
> -out2:
> +out3:
>         power_supply_unregister(bdi->charger);
>
> +out2:
> +       pm_runtime_put_sync(dev);
> +

v2 change list does not mention this addition

>  out1:
> +       pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
>         if (bdi->gpio_int)
>                 gpio_free(bdi->gpio_int);
> @@ -1430,9 +1482,13 @@ static int bq24190_probe(struct i2c_client *client,
>  static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
>         bq24190_register_reset(bdi);
> +       pm_runtime_dont_use_autosuspend(bdi->dev);
>         pm_runtime_put_sync(bdi->dev);

Should skip these if get_sync() failed?
Is pm_runtime_disable() needed here?

>         bq24190_sysfs_remove_group(bdi);
> @@ -1480,10 +1536,14 @@ static int bq24190_pm_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0)
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>         bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);

Should skip these if get_sync() failed?

>         return 0;
>  }
> @@ -1492,15 +1552,19 @@ static int bq24190_pm_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0)
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>         bq24190_register_reset(bdi);
>         bq24190_set_mode_host(bdi);
>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);

Should skip these if get_sync() failed?

Thanks,
Liam

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170120 16:48]:
> On Fri, Jan 20, 2017 at 2:04 PM, Tony Lindgren <tony@atomide.com> wrote:
> > We can get quite a few interrupts when the battery is trickle charging.
> > Let's enable PM runtime autosuspend to avoid constantly toggling device
> > state.
> 
> Which device state would constantly toggle?

The device driver PM runtime state. I've updated the description for that.

> > Cc: Matt Ranostay <matt@ranostay.consulting>
> 
> We can probably drop Matt from this thread; he's been busy with the
> fuel gauge, etc.

Fair enough :)

> > @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
> >         if (!info)
> >                 return -EINVAL;
> >
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> >         ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
> >         if (ret)
> > -               return ret;
> > +               count = ret;
> > +       else
> > +               count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> 
> Unless it violates a style guide, I prefer
>   count = ret ? ret : scnprintf(buf, PAGE_SIZE, "%hhx\n", v);

I don't like that, I find it hard to read.

> > @@ -1249,7 +1286,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
> >         u8 v;
> >         int ret;
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0)
> > +               return ret;
> 
> Why is pm_runtime_*() necessary in this function?
> probe() already calls pm_runtime_*() around hw_init(); nothing else calls it.

Good point, it's not needed. I've removed the pm_runtime_calls.

> > @@ -1364,12 +1405,16 @@ static int bq24190_probe(struct i2c_client *client,
> >         }
> 
> v1 patchset called enable_irq_wake() here.
> v2 change list does not mention its removal.

Oops sorry about that, that's a mismerge on the rebase. I've added that
now to after request_threaded_irq().

> > -out4:
> > +out5:
> >         bq24190_sysfs_remove_group(bdi);
> >
> > -out3:
> > +out4:
> >         power_supply_unregister(bdi->battery);
> >
> > -out2:
> > +out3:
> >         power_supply_unregister(bdi->charger);
> >
> > +out2:
> > +       pm_runtime_put_sync(dev);
> > +
> 
> v2 change list does not mention this addition

Well I mentioned the changes for "mostly for the probe error handling"
in the cover letter and that's needed as we now have the multiple out
labels.

> > @@ -1430,9 +1482,13 @@ static int bq24190_probe(struct i2c_client *client,
> >  static int bq24190_remove(struct i2c_client *client)
> >  {
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int ret;
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0)
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
> >         bq24190_register_reset(bdi);
> > +       pm_runtime_dont_use_autosuspend(bdi->dev);
> >         pm_runtime_put_sync(bdi->dev);
> 
> Should skip these if get_sync() failed?

Yes, I've fixed it.

> Is pm_runtime_disable() needed here?

Yes it's best to keep the pm_runtime calls always paired.

> > @@ -1480,10 +1536,14 @@ static int bq24190_pm_suspend(struct device *dev)
> >  {
> >         struct i2c_client *client = to_i2c_client(dev);
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0)
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> >         bq24190_register_reset(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       pm_runtime_mark_last_busy(bdi->dev);
> > +       pm_runtime_put_autosuspend(bdi->dev);
> 
> Should skip these if get_sync() failed?

Yes fixed.

> > @@ -1492,15 +1552,19 @@ static int bq24190_pm_resume(struct device *dev)
> >  {
> >         struct i2c_client *client = to_i2c_client(dev);
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> >
> >         bdi->f_reg = 0;
> >         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0)
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> >         bq24190_register_reset(bdi);
> >         bq24190_set_mode_host(bdi);
> >         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       pm_runtime_mark_last_busy(bdi->dev);
> > +       pm_runtime_put_autosuspend(bdi->dev);
> 
> Should skip these if get_sync() failed?

Here too.

Updated patch below with fixes for the comments above. I've dropped Mark's
ack as there are several changes so please re-review.

Regards,

Tony

8< ---------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 20 Jan 2017 09:58:51 -0800
Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 147 ++++++++++++++++++++++-----------
 1 file changed, 101 insertions(+), 46 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	pm_runtime_use_autosuspend(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1431,13 +1474,17 @@ static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	if (!pm_runtime_get_sync(bdi->dev)) {
+		bq24190_register_reset(bdi);
+		pm_runtime_put_sync(bdi->dev);
+	} else {
+		dev_warn(bdi->dev, "pm_runtime_get failed\n");
+	}
 
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1481,9 +1528,13 @@ static int bq24190_pm_suspend(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	if (!pm_runtime_get_sync(bdi->dev)) {
+		bq24190_register_reset(bdi);
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	} else {
+		dev_warn(bdi->dev, "pm_runtime_get failed\n");
+	}
 
 	return 0;
 }
@@ -1496,11 +1547,15 @@ static int bq24190_pm_resume(struct device *dev)
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	bq24190_set_mode_host(bdi);
-	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+	if (!pm_runtime_get_sync(bdi->dev)) {
+		bq24190_register_reset(bdi);
+		bq24190_set_mode_host(bdi);
+		bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	} else {
+		dev_warn(bdi->dev, "pm_runtime_get failed\n");
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 20 Jan 2017 09:58:51 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 147 ++++++++++++++++++++++-----------
>  1 file changed, 101 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
...
> @@ -1431,13 +1474,17 @@ static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       if (!pm_runtime_get_sync(bdi->dev)) {
> +               bq24190_register_reset(bdi);
> +               pm_runtime_put_sync(bdi->dev);
> +       } else {
> +               dev_warn(bdi->dev, "pm_runtime_get failed\n");
> +       }

If we skip the register_reset, things may break.
Only the other pm_runtime_* calls should be skipped if get_sync() fails.


>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> +       pm_runtime_dont_use_autosuspend(bdi->dev);
>         pm_runtime_disable(bdi->dev);
>
>         if (bdi->gpio_int)
> @@ -1481,9 +1528,13 @@ static int bq24190_pm_suspend(struct device *dev)
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       if (!pm_runtime_get_sync(bdi->dev)) {
> +               bq24190_register_reset(bdi);
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       } else {
> +               dev_warn(bdi->dev, "pm_runtime_get failed\n");
> +       }

Only the other pm_runtime_* calls should be skipped.


>         return 0;
>  }
> @@ -1496,11 +1547,15 @@ static int bq24190_pm_resume(struct device *dev)
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       bq24190_set_mode_host(bdi);
> -       bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -       pm_runtime_put_sync(bdi->dev);
> +       if (!pm_runtime_get_sync(bdi->dev)) {
> +               bq24190_register_reset(bdi);
> +               bq24190_set_mode_host(bdi);
> +               bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       } else {
> +               dev_warn(bdi->dev, "pm_runtime_get failed\n");
> +       }

Only the other pm_runtime_* calls should be skipped.


>         /* Things may have changed while suspended so alert upper layer */
>         power_supply_changed(bdi->charger);
> --
> 2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170123 19:35]:
> > From: Tony Lindgren <tony@atomide.com>
> > Date: Fri, 20 Jan 2017 09:58:51 -0800
> > Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
> >
> > We can get quite a few interrupts when the battery is trickle charging.
> > Let's enable PM runtime autosuspend to avoid constantly toggling device
> > driver PM runtime state.
> >
> > Let's use a 600 ms timeout as that's how long the USB chager detection
> > might take.
> >
> > Cc: Liam Breck <kernel@networkimprov.net>
> > Cc: Mark Greer <mgreer@animalcreek.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/power/supply/bq24190_charger.c | 147 ++++++++++++++++++++++-----------
> >  1 file changed, 101 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> ...
> > @@ -1431,13 +1474,17 @@ static int bq24190_remove(struct i2c_client *client)
> >  {
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > -       bq24190_register_reset(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       if (!pm_runtime_get_sync(bdi->dev)) {
> > +               bq24190_register_reset(bdi);
> > +               pm_runtime_put_sync(bdi->dev);
> > +       } else {
> > +               dev_warn(bdi->dev, "pm_runtime_get failed\n");
> > +       }
> 
> If we skip the register_reset, things may break.
> Only the other pm_runtime_* calls should be skipped if get_sync() fails.

Hmm yeah I guess the pm_runtime_get() could fail and yet we still could
have i2c access working. I've added handling with pm_runtime_put_noidle()
on error. Also replaced int ret with int error as we're not returning
anything. Updated patch below.

Regards,

Tony

8< -----------------------------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Fri, 20 Jan 2017 09:58:51 -0800
Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 155 ++++++++++++++++++++++++---------
 1 file changed, 114 insertions(+), 41 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	pm_runtime_use_autosuspend(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,10 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_dont_use_autosuspend(bdi->dev);
+		pm_runtime_put_sync(bdi->dev);
+	}
 
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
@@ -1480,10 +1533,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1555,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(bdi->dev);
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Mark Greer]

On Tue, Jan 24, 2017 at 10:29:26AM -0800, Tony Lindgren wrote:

> 8< -----------------------------------------
> >From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 20 Jan 2017 09:58:51 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
> 
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
> 
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
> 
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Acked-by: Mark Greer <mgreer@animalcreek.com>

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Mark Greer <mgreer@animalcreek.com> [170124 15:06]:
> On Tue, Jan 24, 2017 at 10:29:26AM -0800, Tony Lindgren wrote:
> 
> > 8< -----------------------------------------
> > >From tony Mon Sep 17 00:00:00 2001
> > From: Tony Lindgren <tony@atomide.com>
> > Date: Fri, 20 Jan 2017 09:58:51 -0800
> > Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
> > 
> > We can get quite a few interrupts when the battery is trickle charging.
> > Let's enable PM runtime autosuspend to avoid constantly toggling device
> > driver PM runtime state.
> > 
> > Let's use a 600 ms timeout as that's how long the USB chager detection
> > might take.
> > 
> > Cc: Liam Breck <kernel@networkimprov.net>
> > Cc: Mark Greer <mgreer@animalcreek.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> 
> Acked-by: Mark Greer <mgreer@animalcreek.com>

OK thanks will repost the two patches. Liam, got any more comments
or care to ack?

Regards,

Tony

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Tue, Jan 24, 2017 at 10:29 AM, Tony Lindgren <tony@atomide.com> wrote:

>> If we skip the register_reset, things may break.
>> Only the other pm_runtime_* calls should be skipped if get_sync() fails.
>
> Hmm yeah I guess the pm_runtime_get() could fail and yet we still could
> have i2c access working. I've added handling with pm_runtime_put_noidle()
> on error. Also replaced int ret with int error as we're not returning
> anything. Updated patch below.
>
> Regards,
>
> Tony
>
> 8< -----------------------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 20 Jan 2017 09:58:51 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Assuming resolution of my 2 comments below,

Acked-by: Liam Breck <kernel@networkimprov.net>

> ---
>  drivers/power/supply/bq24190_charger.c | 155 ++++++++++++++++++++++++---------
>  1 file changed, 114 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         struct power_supply *psy = dev_get_drvdata(dev);
>         struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>         struct bq24190_sysfs_field_info *info;
> +       ssize_t count;
>         int ret;
>         u8 v;
>
> @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         if (!info)
>                 return -EINVAL;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +       else
> +               count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
>
> -       return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
> +       return count;
>  }
>
>  static ssize_t bq24190_sysfs_store(struct device *dev,
> @@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
>         if (ret < 0)
>                 return ret;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
>         return count;
>  }
> @@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> @@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_ONLINE:
> @@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>         struct bq24190_dev_info *bdi = data;
> +       int ret;
>
>         bdi->irq_event = true;
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>         bq24190_check_status(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>         bdi->irq_event = false;
>
>         return IRQ_HANDLED;
> @@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>         u8 v;
>         int ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> -
>         /* First check that the device really is what its supposed to be */
>         ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
>                         BQ24190_REG_VPRS_PN_MASK,
>                         BQ24190_REG_VPRS_PN_SHIFT,
>                         &v);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
> -       if (v != bdi->model) {
> -               ret = -ENODEV;
> -               goto out;
> -       }
> +       if (v != bdi->model)
> +               return -ENODEV;
>
>         ret = bq24190_register_reset(bdi);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
>         ret = bq24190_set_mode_host(bdi);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
> -       ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -out:
> -       pm_runtime_put_sync(bdi->dev);
> -       return ret;
> +       return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>  }
>
>  #ifdef CONFIG_OF
> @@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
>         }
>
>         pm_runtime_enable(dev);
> -       pm_runtime_resume(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 600);
> +       pm_runtime_use_autosuspend(dev);
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               goto out1;
>
>         ret = bq24190_hw_init(bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Hardware init failed\n");
> -               goto out1;
> +               goto out2;
>         }
>
>         charger_cfg.drv_data = bdi;
> @@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->charger)) {
>                 dev_err(dev, "Can't register charger\n");
>                 ret = PTR_ERR(bdi->charger);
> -               goto out1;
> +               goto out2;
>         }
>
>         battery_cfg.drv_data = bdi;
> @@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->battery)) {
>                 dev_err(dev, "Can't register battery\n");
>                 ret = PTR_ERR(bdi->battery);
> -               goto out2;
> +               goto out3;
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto out3;
> +               goto out4;
>         }
>
>         bdi->initialized = true;
> @@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
>                         "bq24190-charger", bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Can't set up irq handler\n");
> -               goto out4;
> +               goto out5;
>         }
>
> +       enable_irq_wake(bdi->irq);
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +
>         return 0;
>
> -out4:
> +out5:
>         bq24190_sysfs_remove_group(bdi);
>
> -out3:
> +out4:
>         power_supply_unregister(bdi->battery);
>
> -out2:
> +out3:
>         power_supply_unregister(bdi->charger);
>
> +out2:
> +       pm_runtime_put_sync(dev);
> +
>  out1:
> +       pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
>         if (bdi->gpio_int)
>                 gpio_free(bdi->gpio_int);
> @@ -1430,10 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
>  static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
> +
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> -       pm_runtime_get_sync(bdi->dev);
>         bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_dont_use_autosuspend(bdi->dev);
> +               pm_runtime_put_sync(bdi->dev);
> +       }

Should put_sync() be called before dont_use_autosuspend(), as in probe()?

Should the pm_runtime_disable() further on in remove() go inside the
above block?

>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
> @@ -1480,10 +1533,20 @@ static int bq24190_pm_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
> +
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> -       pm_runtime_get_sync(bdi->dev);
>         bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       }
>
>         return 0;
>  }
> @@ -1492,15 +1555,25 @@ static int bq24190_pm_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               pm_runtime_put_noidle(bdi->dev);
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +       }
> +
>         bq24190_register_reset(bdi);
>         bq24190_set_mode_host(bdi);
>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       }
>
>         /* Things may have changed while suspended so alert upper layer */
>         power_supply_changed(bdi->charger);
> --
> 2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170126 15:57]:
> On Tue, Jan 24, 2017 at 10:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> > @@ -1430,10 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
> >  static int bq24190_remove(struct i2c_client *client)
> >  {
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> > +
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0) {
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> > +               pm_runtime_put_noidle(bdi->dev);
> > +       }
> >
> > -       pm_runtime_get_sync(bdi->dev);
> >         bq24190_register_reset(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +
> > +       if (!error) {
> > +               pm_runtime_dont_use_autosuspend(bdi->dev);
> > +               pm_runtime_put_sync(bdi->dev);
> > +       }
> 
> Should put_sync() be called before dont_use_autosuspend(), as in probe()?

Yeah good to have them paired. With pm_runtime_put_noidle() we're
active on error, so we can just call pm_runtime_dont_use_autosuspend()
and pm_runtime_put_sync() unconditionally.

> Should the pm_runtime_disable() further on in remove() go inside the
> above block?

No need to because of the pm_runtime_put_noidle(). I'll just move the
pm_runtime_dont_use_autosuspend() and pm_runtime_put_sync() to the
end.

I'll repost both patches one more time with acks.

Regards,

Tony

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Mon, Jan 30, 2017 at 3:55 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Liam Breck <liam@networkimprov.net> [170126 15:57]:
>> On Tue, Jan 24, 2017 at 10:29 AM, Tony Lindgren <tony@atomide.com> wrote:
>> > @@ -1430,10 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
>> >  static int bq24190_remove(struct i2c_client *client)
>> >  {
>> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>> > +       int error;
>> > +
>> > +       error = pm_runtime_get_sync(bdi->dev);
>> > +       if (error < 0) {
>> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> > +               pm_runtime_put_noidle(bdi->dev);
>> > +       }
>> >
>> > -       pm_runtime_get_sync(bdi->dev);
>> >         bq24190_register_reset(bdi);
>> > -       pm_runtime_put_sync(bdi->dev);
>> > +
>> > +       if (!error) {
>> > +               pm_runtime_dont_use_autosuspend(bdi->dev);
>> > +               pm_runtime_put_sync(bdi->dev);
>> > +       }
>>
>> Should put_sync() be called before dont_use_autosuspend(), as in probe()?
>
> Yeah good to have them paired. With pm_runtime_put_noidle() we're
> active on error, so we can just call pm_runtime_dont_use_autosuspend()
> and pm_runtime_put_sync() unconditionally.

I meant should the two pm_runtime_* calls above be flipped, so
put_sync() is first. That's the order in probe()

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170130 16:02]:
> On Mon, Jan 30, 2017 at 3:55 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Liam Breck <liam@networkimprov.net> [170126 15:57]:
> >> On Tue, Jan 24, 2017 at 10:29 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> > @@ -1430,10 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
> >> >  static int bq24190_remove(struct i2c_client *client)
> >> >  {
> >> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> >> > +       int error;
> >> > +
> >> > +       error = pm_runtime_get_sync(bdi->dev);
> >> > +       if (error < 0) {
> >> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> >> > +               pm_runtime_put_noidle(bdi->dev);
> >> > +       }
> >> >
> >> > -       pm_runtime_get_sync(bdi->dev);
> >> >         bq24190_register_reset(bdi);
> >> > -       pm_runtime_put_sync(bdi->dev);
> >> > +
> >> > +       if (!error) {
> >> > +               pm_runtime_dont_use_autosuspend(bdi->dev);
> >> > +               pm_runtime_put_sync(bdi->dev);
> >> > +       }
> >>
> >> Should put_sync() be called before dont_use_autosuspend(), as in probe()?
> >
> > Yeah good to have them paired. With pm_runtime_put_noidle() we're
> > active on error, so we can just call pm_runtime_dont_use_autosuspend()
> > and pm_runtime_put_sync() unconditionally.
> 
> I meant should the two pm_runtime_* calls above be flipped, so
> put_sync() is first. That's the order in probe()

Oh I see yeah ideally yes.

Regards,

Tony

[PATCHv2 0/2] Two bq24190 PM improvments

[Tony Lindgren]

Hi,

Here's a repost of the two bq24190 patches rebased against current
power-supply tree.

Regards,

Tony

Changes from v1:

- Call pm_runtime_dont_use_autosuspend() and pm_runtime_put_sync()
  unconditionally in bq24190_remove() as suggested by Liam

Changes from earlier set:

- Separated from Liam's changes that are now in the power-supply tree

- Moved bdi->initialized before devm_request_threaded_irq() as suggested
  by Liam

  - Added missing return value check for pm_runtime_get() as suggested by
    Liam

    - Updated against current power-supply tree, mostly for the probe error
      handling

Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

 drivers/power/supply/bq24190_charger.c | 211 +++++++++++++++++++++++++--------
 1 file changed, 161 insertions(+), 50 deletions(-)

-- 
2.11.0

[PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 153 ++++++++++++++++++++++++---------
 1 file changed, 111 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	pm_runtime_use_autosuspend(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
+	pm_runtime_put_sync(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1529,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1551,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(bdi->dev);
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

Hi, these emails are missing v2 in subject line.


On Mon, Jan 30, 2017 at 4:02 PM, Tony Lindgren <tony@atomide.com> wrote:
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Acked-by: Liam Breck <kernel@networkimprov.net>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 153 ++++++++++++++++++++++++---------
>  1 file changed, 111 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> ...
> @@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
>         }
>
>         pm_runtime_enable(dev);
> -       pm_runtime_resume(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 600);
> +       pm_runtime_use_autosuspend(dev);
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               goto out1;

Call get_sync() before set_autosuspend() ?

>         ret = bq24190_hw_init(bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Hardware init failed\n");
> -               goto out1;
> +               goto out2;
>         }
>
>         charger_cfg.drv_data = bdi;
> @@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->charger)) {
>                 dev_err(dev, "Can't register charger\n");
>                 ret = PTR_ERR(bdi->charger);
> -               goto out1;
> +               goto out2;
>         }
>
>         battery_cfg.drv_data = bdi;
> @@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->battery)) {
>                 dev_err(dev, "Can't register battery\n");
>                 ret = PTR_ERR(bdi->battery);
> -               goto out2;
> +               goto out3;
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto out3;
> +               goto out4;
>         }
>
>         bdi->initialized = true;
> @@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
>                         "bq24190-charger", bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Can't set up irq handler\n");
> -               goto out4;
> +               goto out5;
>         }
>
> +       enable_irq_wake(bdi->irq);
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +
>         return 0;
>
> -out4:
> +out5:
>         bq24190_sysfs_remove_group(bdi);
>
> -out3:
> +out4:
>         power_supply_unregister(bdi->battery);
>
> -out2:
> +out3:
>         power_supply_unregister(bdi->charger);
>
> +out2:
> +       pm_runtime_put_sync(dev);
> +
>  out1:
> +       pm_runtime_dont_use_autosuspend(dev);

Change order of above two if my prev comment is right.

>         pm_runtime_disable(dev);
>         if (bdi->gpio_int)
>                 gpio_free(bdi->gpio_int);
> @@ -1430,14 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
>  static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> +       bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> +       pm_runtime_dont_use_autosuspend(bdi->dev);
> +       pm_runtime_put_sync(bdi->dev);

Alternatively, switch order of above two.

>         pm_runtime_disable(bdi->dev);
>
>         if (bdi->gpio_int)

Apologies if I'm nitpicking!

~.~

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170130 16:15]:
> Hi, these emails are missing v2 in subject line.

Oh sorry it's in the cover letter. Looks like you did not get Cc:ed
on the cover letter, I need to update my scripts to properly collect
all the acks to the cover letter.

> On Mon, Jan 30, 2017 at 4:02 PM, Tony Lindgren <tony@atomide.com> wrote:
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> > ...
> > @@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
> >         }
> >
> >         pm_runtime_enable(dev);
> > -       pm_runtime_resume(dev);
> > +       pm_runtime_set_autosuspend_delay(dev, 600);
> > +       pm_runtime_use_autosuspend(dev);
> > +       ret = pm_runtime_get_sync(dev);
> > +       if (ret < 0)
> > +               goto out1;
> 
> Call get_sync() before set_autosuspend() ?

No need to, but I'll move pm_runtime_set_autosuspend_delay() after
pm_runtime_use_autosuspend() :)

> >  out1:
> > +       pm_runtime_dont_use_autosuspend(dev);
> 
> Change order of above two if my prev comment is right.

No need to.

> > @@ -1430,14 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
> >  static int bq24190_remove(struct i2c_client *client)
> >  {
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > -       bq24190_register_reset(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0) {
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> > +               pm_runtime_put_noidle(bdi->dev);
> > +       }
> >
> > +       bq24190_register_reset(bdi);
> >         bq24190_sysfs_remove_group(bdi);
> >         power_supply_unregister(bdi->battery);
> >         power_supply_unregister(bdi->charger);
> > +       pm_runtime_dont_use_autosuspend(bdi->dev);
> > +       pm_runtime_put_sync(bdi->dev);
> 
> Alternatively, switch order of above two.

Yeah will do.

> Apologies if I'm nitpicking!

No worries, this should be a boringly trivial patch so it's good to have
somebody look through it to avoid bad changes. Updated patch below, please
check and re-ack if it now looks OK to you.

Regards,

Tony

8< ---------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 30 Jan 2017 07:10:31 -0800
Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 153 ++++++++++++++++++++++++---------
 1 file changed, 111 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1529,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1551,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(bdi->dev);
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Mon, Jan 30, 2017 at 4:36 PM, Tony Lindgren <tony@atomide.com> wrote:

> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 30 Jan 2017 07:10:31 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 153 ++++++++++++++++++++++++---------
>  1 file changed, 111 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         struct power_supply *psy = dev_get_drvdata(dev);
>         struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>         struct bq24190_sysfs_field_info *info;
> +       ssize_t count;
>         int ret;
>         u8 v;
>
> @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         if (!info)
>                 return -EINVAL;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Let's not stop here if get_sync fails.

>         ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +       else
> +               count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
>
> -       return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
> +       return count;
>  }
>
>  static ssize_t bq24190_sysfs_store(struct device *dev,
> @@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
>         if (ret < 0)
>                 return ret;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 2.

>         ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
>         return count;
>  }
> @@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 3.

>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 4.

>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 5.

>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> @@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 6.

>         switch (psp) {
>         case POWER_SUPPLY_PROP_ONLINE:
> @@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>         struct bq24190_dev_info *bdi = data;
> +       int ret;
>
>         bdi->irq_event = true;
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;

Nor here 7.

>         bq24190_check_status(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>         bdi->irq_event = false;
>
>         return IRQ_HANDLED;
> @@ -1249,33 +1286,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>         u8 v;
>         int ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> -
>         /* First check that the device really is what its supposed to be */
>         ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
>                         BQ24190_REG_VPRS_PN_MASK,
>                         BQ24190_REG_VPRS_PN_SHIFT,
>                         &v);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
> -       if (v != bdi->model) {
> -               ret = -ENODEV;
> -               goto out;
> -       }
> +       if (v != bdi->model)
> +               return -ENODEV;
>
>         ret = bq24190_register_reset(bdi);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
>         ret = bq24190_set_mode_host(bdi);
>         if (ret < 0)
> -               goto out;
> +               return ret;
>
> -       ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -out:
> -       pm_runtime_put_sync(bdi->dev);
> -       return ret;
> +       return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>  }
>
>  #ifdef CONFIG_OF
> @@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
>         }
>
>         pm_runtime_enable(dev);
> -       pm_runtime_resume(dev);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 600);
> +       ret = pm_runtime_get_sync(dev);
> +       if (ret < 0)
> +               goto out1;

Nor here 8.

>         ret = bq24190_hw_init(bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Hardware init failed\n");
> -               goto out1;
> +               goto out2;
>         }
>
>         charger_cfg.drv_data = bdi;
> @@ -1380,7 +1414,7 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->charger)) {
>                 dev_err(dev, "Can't register charger\n");
>                 ret = PTR_ERR(bdi->charger);
> -               goto out1;
> +               goto out2;
>         }
>
>         battery_cfg.drv_data = bdi;
> @@ -1389,13 +1423,13 @@ static int bq24190_probe(struct i2c_client *client,
>         if (IS_ERR(bdi->battery)) {
>                 dev_err(dev, "Can't register battery\n");
>                 ret = PTR_ERR(bdi->battery);
> -               goto out2;
> +               goto out3;
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto out3;
> +               goto out4;
>         }
>
>         bdi->initialized = true;
> @@ -1406,21 +1440,30 @@ static int bq24190_probe(struct i2c_client *client,
>                         "bq24190-charger", bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Can't set up irq handler\n");
> -               goto out4;
> +               goto out5;
>         }
>
> +       enable_irq_wake(bdi->irq);
> +
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +
>         return 0;
>
> -out4:
> +out5:
>         bq24190_sysfs_remove_group(bdi);
>
> -out3:
> +out4:
>         power_supply_unregister(bdi->battery);
>
> -out2:
> +out3:
>         power_supply_unregister(bdi->charger);
>
> +out2:
> +       pm_runtime_put_sync(dev);
> +
>  out1:
> +       pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
>         if (bdi->gpio_int)
>                 gpio_free(bdi->gpio_int);
> @@ -1430,14 +1473,20 @@ static int bq24190_probe(struct i2c_client *client,
>  static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> +       bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> +       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_dont_use_autosuspend(bdi->dev);
>         pm_runtime_disable(bdi->dev);
>
>         if (bdi->gpio_int)
> @@ -1480,10 +1529,20 @@ static int bq24190_pm_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
> +
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> -       pm_runtime_get_sync(bdi->dev);
>         bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       }
>
>         return 0;
>  }
> @@ -1492,15 +1551,25 @@ static int bq24190_pm_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               pm_runtime_put_noidle(bdi->dev);
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +       }
> +
>         bq24190_register_reset(bdi);
>         bq24190_set_mode_host(bdi);
>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       }

We need helper funcs...

static inline int bq24190_pmrt_get(struct device* dev) {
  int err = pm_runtime_get_sync(dev);
  if (err < 0) {
    dev_warn(dev, "pm_runtime_get failed: %i\n", err);
    pm_runtime_put_noidle(dev);
  }
  return err;
}

static inline void bq24190_pmrt_put(struct device* dev, int err) {
  if (!err) {
    pm_runtime_mark_last_busy(dev);
    pm_runtime_put_autosuspend(dev);
  }
}

Apologies, should have raised this on last version!

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170202 17:44]:
> > @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
> >         if (!info)
> >                 return -EINVAL;
> >
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0)
> > +               return ret;
> 
> Let's not stop here if get_sync fails.

Hmm care to describe why we would want to ignore errors on pm_runtime_get()?
I don't think that's a good idea.. And for sysfs_show() output?

If these calls fail it's a sign of a serious problem somewhere
and it should be fixed instead of ignoring the errors and attempting to
continue. Ignoring errors with these calls can lead into problems that
are hard to debug.

> > @@ -1364,12 +1394,16 @@ static int bq24190_probe(struct i2c_client *client,
> >         }
> >
> >         pm_runtime_enable(dev);
> > -       pm_runtime_resume(dev);
> > +       pm_runtime_use_autosuspend(dev);
> > +       pm_runtime_set_autosuspend_delay(dev, 600);
> > +       ret = pm_runtime_get_sync(dev);
> > +       if (ret < 0)
> > +               goto out1;
> 
> Nor here 8.

Eek, let's not try to continue the probe if pm_runtime_get_fails. Let's
just assume it's either implemented and must be working, or a nop operation
that will not do anything. Depending on the implementation, this could
affect something in the hardware like pin multiplexing, parent device
clocking.. On the module exit path it might make sense as we know the
device was working to get there so it might be possible to do something :)

> We need helper funcs...
> 
> static inline int bq24190_pmrt_get(struct device* dev) {
>   int err = pm_runtime_get_sync(dev);
>   if (err < 0) {
>     dev_warn(dev, "pm_runtime_get failed: %i\n", err);
>     pm_runtime_put_noidle(dev);
>   }
>   return err;
> }
> 
> static inline void bq24190_pmrt_put(struct device* dev, int err) {
>   if (!err) {
>     pm_runtime_mark_last_busy(dev);
>     pm_runtime_put_autosuspend(dev);
>   }
> }

If you want to implement something like that it should probably be
done in a Linux generic way. There's still quite a bit boilerplate
code needed for the autosuspend usage.

Regards,

Tony

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Mon, Jan 30, 2017 at 4:36 PM, Tony Lindgren <tony@atomide.com> wrote:

> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 30 Jan 2017 07:10:31 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 153 ++++++++++++++++++++++++---------
>  1 file changed, 111 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> ...
> @@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>         struct bq24190_dev_info *bdi = data;
> +       int ret;
>
>         bdi->irq_event = true;
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>         bq24190_check_status(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>         bdi->irq_event = false;
>
>         return IRQ_HANDLED;

OK get_sync() failure should return error to userspace callers, and
abort probe(), makes sense.

But should the interrupt handler (above) emit a warning and continue
as in resume() and suspend()?

If not is ret the right value to return in lieu of irq_handled?

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170203 02:40]:
> On Mon, Jan 30, 2017 at 4:36 PM, Tony Lindgren <tony@atomide.com> wrote:
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> > ...
> > @@ -1234,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
> >  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> >  {
> >         struct bq24190_dev_info *bdi = data;
> > +       int ret;
> >
> >         bdi->irq_event = true;
> > -       pm_runtime_get_sync(bdi->dev);
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0)
> > +               return ret;
> >         bq24190_check_status(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       pm_runtime_mark_last_busy(bdi->dev);
> > +       pm_runtime_put_autosuspend(bdi->dev);
> >         bdi->irq_event = false;
> >
> >         return IRQ_HANDLED;
> 
> OK get_sync() failure should return error to userspace callers, and
> abort probe(), makes sense.

OK

> But should the interrupt handler (above) emit a warning and continue
> as in resume() and suspend()?

The interrupt subsystem will retry and eventually just give up with
errors. No need to do anything special here to disable the irq, let's
just add dev_err().

> If not is ret the right value to return in lieu of irq_handled?

Oh you're right, we should return IRQ_NONE here.

Updated patch below doing dev_err() + IRQ_NONE.

Regards,

Tony

8< ----------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 30 Jan 2017 07:10:31 -0800
Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 156 ++++++++++++++++++++++++---------
 1 file changed, 114 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,18 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_err(bdi->dev, "Runtime PM failed in %s: %i\n",
+			__func__, ret);
+		return IRQ_NONE;
+	}
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1289,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1397,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1417,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1426,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1443,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1476,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1532,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1554,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(bdi->dev);
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Fri, Feb 3, 2017 at 10:54 AM, Tony Lindgren <tony@atomide.com> wrote:
> ...
> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 30 Jan 2017 07:10:31 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 156 ++++++++++++++++++++++++---------
>  1 file changed, 114 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
>
> @@ -1234,11 +1267,18 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>  {
>         struct bq24190_dev_info *bdi = data;
> +       int ret;
>
>         bdi->irq_event = true;
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0) {
> +               dev_err(bdi->dev, "Runtime PM failed in %s: %i\n",
> +                       __func__, ret);
> +               return IRQ_NONE;
> +       }

Now this is a nitpick... Error msg should be consistent with suspend/resume:

> @@ -1480,10 +1532,20 @@ static int bq24190_pm_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
> +
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }

Like here.

> @@ -1492,15 +1554,25 @@ static int bq24190_pm_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               pm_runtime_put_noidle(bdi->dev);
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +       }

And here.

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170203 13:02]:
> On Fri, Feb 3, 2017 at 10:54 AM, Tony Lindgren <tony@atomide.com> wrote:
> > --- a/drivers/power/supply/bq24190_charger.c
> > +++ b/drivers/power/supply/bq24190_charger.c
> >
> > @@ -1234,11 +1267,18 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
> >  static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> >  {
> >         struct bq24190_dev_info *bdi = data;
> > +       int ret;
> >
> >         bdi->irq_event = true;
> > -       pm_runtime_get_sync(bdi->dev);
> > +       ret = pm_runtime_get_sync(bdi->dev);
> > +       if (ret < 0) {
> > +               dev_err(bdi->dev, "Runtime PM failed in %s: %i\n",
> > +                       __func__, ret);
> > +               return IRQ_NONE;
> > +       }
> 
> Now this is a nitpick... Error msg should be consistent with suspend/resume:

Sure, updated below.

Regards,

Tony

8< ----------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Mon, 30 Jan 2017 07:10:31 -0800
Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 155 ++++++++++++++++++++++++---------
 1 file changed, 113 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		return IRQ_NONE;
+	}
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1288,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1396,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1416,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1425,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1442,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1475,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1531,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1553,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		pm_runtime_put_noidle(bdi->dev);
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Fri, Feb 3, 2017 at 1:17 PM, Tony Lindgren <tony@atomide.com> wrote:

> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 30 Jan 2017 07:10:31 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Liam Breck <kernel@networkimprov.net>


> @@ -1480,10 +1531,20 @@ static int bq24190_pm_suspend(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
> +
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> -       pm_runtime_get_sync(bdi->dev);
>         bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +
> +       if (!error) {
> +               pm_runtime_mark_last_busy(bdi->dev);
> +               pm_runtime_put_autosuspend(bdi->dev);
> +       }
>
>         return 0;
>  }
> @@ -1492,15 +1553,25 @@ static int bq24190_pm_resume(struct device *dev)
>  {
>         struct i2c_client *client = to_i2c_client(dev);
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
>         bdi->f_reg = 0;
>         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
>
> -       pm_runtime_get_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               pm_runtime_put_noidle(bdi->dev);
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +       }

Last nitpick: call put_noidle after dev_warn, as in suspend :-)

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170203 13:46]:
> On Fri, Feb 3, 2017 at 1:17 PM, Tony Lindgren <tony@atomide.com> wrote:
> > Cc: Liam Breck <kernel@networkimprov.net>
> > Acked-by: Mark Greer <mgreer@animalcreek.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Acked-by: Liam Breck <kernel@networkimprov.net>

Thanks for looking though it.

> > @@ -1492,15 +1553,25 @@ static int bq24190_pm_resume(struct device *dev)
> >  {
> >         struct i2c_client *client = to_i2c_client(dev);
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> >
> >         bdi->f_reg = 0;
> >         bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0) {
> > +               pm_runtime_put_noidle(bdi->dev);
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> > +       }
> 
> Last nitpick: call put_noidle after dev_warn, as in suspend :-)

Sure, will do that and repost v3 of both patches shortly.

Regards,

Tony

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Fri, Feb 3, 2017 at 1:17 PM, Tony Lindgren <tony@atomide.com> wrote:

> From: Tony Lindgren <tony@atomide.com>
> Date: Mon, 30 Jan 2017 07:10:31 -0800
> Subject: [PATCH] power: bq24190_charger: Use PM runtime autosuspend
>
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Acked-by: Mark Greer <mgreer@animalcreek.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 155 ++++++++++++++++++++++++---------
>  1 file changed, 113 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
>
> @@ -1430,14 +1475,20 @@ static int bq24190_probe(struct i2c_client *client,
>  static int bq24190_remove(struct i2c_client *client)
>  {
>         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> +       int error;
>
> -       pm_runtime_get_sync(bdi->dev);
> -       bq24190_register_reset(bdi);
> -       pm_runtime_put_sync(bdi->dev);
> +       error = pm_runtime_get_sync(bdi->dev);
> +       if (error < 0) {
> +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> +               pm_runtime_put_noidle(bdi->dev);
> +       }
>
> +       bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> +       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_dont_use_autosuspend(bdi->dev);
>         pm_runtime_disable(bdi->dev);

I think you addressed this, but should the above be

if (!error)
  pm_runtime_put_sync(bdi->dev);

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170203 16:25]:
> On Fri, Feb 3, 2017 at 1:17 PM, Tony Lindgren <tony@atomide.com> wrote:
> >  static int bq24190_remove(struct i2c_client *client)
> >  {
> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
> > +       int error;
> >
> > -       pm_runtime_get_sync(bdi->dev);
> > -       bq24190_register_reset(bdi);
> > -       pm_runtime_put_sync(bdi->dev);
> > +       error = pm_runtime_get_sync(bdi->dev);
> > +       if (error < 0) {
> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
> > +               pm_runtime_put_noidle(bdi->dev);
> > +       }
> >
> > +       bq24190_register_reset(bdi);
> >         bq24190_sysfs_remove_group(bdi);
> >         power_supply_unregister(bdi->battery);
> >         power_supply_unregister(bdi->charger);
> > +       pm_runtime_put_sync(bdi->dev);
> > +       pm_runtime_dont_use_autosuspend(bdi->dev);
> >         pm_runtime_disable(bdi->dev);
> 
> I think you addressed this, but should the above be
> 
> if (!error)
>   pm_runtime_put_sync(bdi->dev);

Hmm yeah.. But we need to check for if (error >= 0), also in the other
places see below.

Regards,

Tony

8< --------------------
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1487,7 +1487,8 @@ static int bq24190_remove(struct i2c_client *client)
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
-	pm_runtime_put_sync(bdi->dev);
+	if (error >= 0)
+		pm_runtime_put_sync(bdi->dev);
 	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
@@ -1541,7 +1542,7 @@ static int bq24190_pm_suspend(struct device *dev)
 
 	bq24190_register_reset(bdi);
 
-	if (!error) {
+	if (error >= 0) {
 		pm_runtime_mark_last_busy(bdi->dev);
 		pm_runtime_put_autosuspend(bdi->dev);
 	}
@@ -1568,7 +1569,7 @@ static int bq24190_pm_resume(struct device *dev)
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 
-	if (!error) {
+	if (error >= 0) {
 		pm_runtime_mark_last_busy(bdi->dev);
 		pm_runtime_put_autosuspend(bdi->dev);
 	}
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Tue, Feb 7, 2017 at 8:01 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Liam Breck <liam@networkimprov.net> [170203 16:25]:
>> On Fri, Feb 3, 2017 at 1:17 PM, Tony Lindgren <tony@atomide.com> wrote:
>> >  static int bq24190_remove(struct i2c_client *client)
>> >  {
>> >         struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
>> > +       int error;
>> >
>> > -       pm_runtime_get_sync(bdi->dev);
>> > -       bq24190_register_reset(bdi);
>> > -       pm_runtime_put_sync(bdi->dev);
>> > +       error = pm_runtime_get_sync(bdi->dev);
>> > +       if (error < 0) {
>> > +               dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
>> > +               pm_runtime_put_noidle(bdi->dev);
>> > +       }
>> >
>> > +       bq24190_register_reset(bdi);
>> >         bq24190_sysfs_remove_group(bdi);
>> >         power_supply_unregister(bdi->battery);
>> >         power_supply_unregister(bdi->charger);
>> > +       pm_runtime_put_sync(bdi->dev);
>> > +       pm_runtime_dont_use_autosuspend(bdi->dev);
>> >         pm_runtime_disable(bdi->dev);
>>
>> I think you addressed this, but should the above be
>>
>> if (!error)
>>   pm_runtime_put_sync(bdi->dev);
>
> Hmm yeah.. But we need to check for if (error >= 0), also in the other
> places see below.

OK, let's roll v4. it is

Acked-by: Liam Breck <kernel@networkimprov.net>

(and pls include the v4 in all subjects :-)

I have a patchset for DT support in the works which depends on this one.

[PATCHv3 0/2] Two bq24190 PM improvments

[Tony Lindgren]

Hi all,

Here's a repost of the two bq24190 patches against current power-supply
tree.

Regards,

Tony

Changes from v2:
- Several improvments for error handling and cosmetic issues based
  on comments by Liam Breck

Changes from v1:

- Call pm_runtime_dont_use_autosuspend() and pm_runtime_put_sync()
  unconditionally in bq24190_remove() as suggested by Liam

Changes from earlier set:

- Separated from Liam's changes that are now in the power-supply tree

- Moved bdi->initialized before devm_request_threaded_irq() as suggested
  by Liam

- Added missing return value check for pm_runtime_get() as suggested by
  Liam

- Updated against current power-supply tree, mostly for the probe error
  handling

Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

 drivers/power/supply/bq24190_charger.c | 213 +++++++++++++++++++++++++--------
 1 file changed, 163 insertions(+), 50 deletions(-)

-- 
2.11.0

[PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 155 ++++++++++++++++++++++++---------
 1 file changed, 113 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		return IRQ_NONE;
+	}
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1288,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1396,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1416,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1425,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1442,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1475,20 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1531,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1553,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (!error) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

Re: [PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170203 15:11]:
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> driver PM runtime state.

This patch still has some funky runtime PM error handling as discussed
in the v3 patches, will post v4 later today.

Regards,

Tony

[PATCHv4 0/2] Two bq24190 PM improvments

[Tony Lindgren]

Hi all,

Here's v4 repost of the two bq24190 patches against current power-supply
tree.

Regards,

Tony

Changes from v3:
- More runtime PM error handling fixes

Changes from v2:
- Several improvments for error handling and cosmetic issues based
  on comments by Liam Breck

Changes from v1:

- Call pm_runtime_dont_use_autosuspend() and pm_runtime_put_sync()
  unconditionally in bq24190_remove() as suggested by Liam

Changes from earlier set:

- Separated from Liam's changes that are now in the power-supply tree

- Moved bdi->initialized before devm_request_threaded_irq() as suggested
  by Liam

- Added missing return value check for pm_runtime_get() as suggested by
  Liam

- Updated against current power-supply tree, mostly for the probe error
  handling


Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

 drivers/power/supply/bq24190_charger.c | 214 +++++++++++++++++++++++++--------
 1 file changed, 164 insertions(+), 50 deletions(-)

-- 
2.11.1

[PATCH 2/2] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
driver PM runtime state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 156 ++++++++++++++++++++++++---------
 1 file changed, 114 insertions(+), 42 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1106,7 +1135,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -1119,7 +1150,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1234,11 +1267,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+		return IRQ_NONE;
+	}
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 
 	return IRQ_HANDLED;
@@ -1249,33 +1288,26 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
 			BQ24190_REG_VPRS_PN_MASK,
 			BQ24190_REG_VPRS_PN_SHIFT,
 			&v);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	if (v != bdi->model) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (v != bdi->model)
+		return -ENODEV;
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
-		goto out;
+		return ret;
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-out:
-	pm_runtime_put_sync(bdi->dev);
-	return ret;
+	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
 #ifdef CONFIG_OF
@@ -1364,12 +1396,16 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out1;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out1;
+		goto out2;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1380,7 +1416,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out1;
+		goto out2;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1389,13 +1425,13 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out2;
+		goto out3;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out3;
+		goto out4;
 	}
 
 	bdi->initialized = true;
@@ -1406,21 +1442,30 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out4;
+		goto out5;
 	}
 
+	enable_irq_wake(bdi->irq);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
-out4:
+out5:
 	bq24190_sysfs_remove_group(bdi);
 
-out3:
+out4:
 	power_supply_unregister(bdi->battery);
 
-out2:
+out3:
 	power_supply_unregister(bdi->charger);
 
+out2:
+	pm_runtime_put_sync(dev);
+
 out1:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
@@ -1430,14 +1475,21 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
-	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
+	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
+	if (error >= 0)
+		pm_runtime_put_sync(bdi->dev);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
 	if (bdi->gpio_int)
@@ -1480,10 +1532,20 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
+
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (error >= 0) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	return 0;
 }
@@ -1492,15 +1554,25 @@ static int bq24190_pm_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
+	}
+
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+
+	if (error >= 0) {
+		pm_runtime_mark_last_busy(bdi->dev);
+		pm_runtime_put_autosuspend(bdi->dev);
+	}
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.1