Re: [PATCH] power: supply: bq27xxx: Stop and start delayed work in suspend and resume

[Hans de Goede <hdegoede@redhat.com>]

Hi Marek,

On 11/4/23 16:49, Marek Vasut wrote:
> This driver uses delayed work to perform periodic battery state read out.
> This delayed work is not stopped across suspend and resume cycle. The
> read out can occur early in the resume cycle. In case of an I2C variant
> of this hardware, that read out triggers I2C transfer. That I2C transfer
> may happen while the I2C controller is still suspended, which produces a
> WARNING in the kernel log.
> 
> Fix this by introducing trivial PM ops, which stop the delayed work before
> the system enters suspend, and schedule the delayed work right after the
> system resumes.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: linux-pm@vger.kernel.org
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 22 ++++++++++++++++++++++
>  drivers/power/supply/bq27xxx_battery_i2c.c |  1 +
>  include/linux/power/bq27xxx_battery.h      |  1 +
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 4296600e8912a..1c4a9d1377442 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -2162,6 +2162,28 @@ void bq27xxx_battery_teardown(struct bq27xxx_device_info *di)
>  }
>  EXPORT_SYMBOL_GPL(bq27xxx_battery_teardown);
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int bq27xxx_battery_suspend(struct device *dev)
> +{
> +	struct bq27xxx_device_info *di = dev_get_drvdata(dev);
> +
> +	cancel_delayed_work(&di->work);
> +	return 0;
> +}
> +
> +static int bq27xxx_battery_resume(struct device *dev)
> +{
> +	struct bq27xxx_device_info *di = dev_get_drvdata(dev);
> +
> +	schedule_delayed_work(&di->work, 0);
> +	return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +SIMPLE_DEV_PM_OPS(bq27xxx_battery_battery_pm_ops,
> +		  bq27xxx_battery_suspend, bq27xxx_battery_resume);
> +EXPORT_SYMBOL_GPL(bq27xxx_battery_battery_pm_ops);
> +
>  MODULE_AUTHOR("Rodolfo Giometti <giometti@linux.it>");
>  MODULE_DESCRIPTION("BQ27xxx battery monitor driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index 9b5475590518f..3a1798b0c1a79 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -295,6 +295,7 @@ static struct i2c_driver bq27xxx_battery_i2c_driver = {
>  	.driver = {
>  		.name = "bq27xxx-battery",
>  		.of_match_table = of_match_ptr(bq27xxx_battery_i2c_of_match_table),
> +		.pm = &bq27xxx_battery_battery_pm_ops,
>  	},
>  	.probe = bq27xxx_battery_i2c_probe,
>  	.remove = bq27xxx_battery_i2c_remove,
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 7c8d65414a70a..7d8025fb74b70 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -83,5 +83,6 @@ struct bq27xxx_device_info {
>  void bq27xxx_battery_update(struct bq27xxx_device_info *di);
>  int bq27xxx_battery_setup(struct bq27xxx_device_info *di);
>  void bq27xxx_battery_teardown(struct bq27xxx_device_info *di);
> +extern const struct dev_pm_ops bq27xxx_battery_battery_pm_ops;
>  
>  #endif