Re: [PATCH 2/2] power: supply: sbs-battery: Add support for polling battery status

[Sebastian Reichel <sebastian.reichel@collabora.com>]

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

Hi,

On Wed, Dec 31, 2025 at 09:31:52AM +0000, LI Qingwu wrote:
> Enable periodic polling of SBS battery status on systems where the
> battery interrupt line is not connected. Polling is configured via
> the poll_interval parameter (ms, default 0/disabled) and automatically
> disabled when a working GPIO interrupt is available.
> 
> Example usage:
>   echo 5000 > /sys/module/sbs_battery/parameters/poll_interval
> 
> Tested on i.MX 8M Plus platform with SBS-compliant battery.

This is messed up. You are calling the delayed work intended to be
used when the state of the charger supplying the battery changes.
The detection should instead call sbs_supply_changed(). Also I
wonder why you need this in the first place. Isn't force_load
enough? The battery detection status should be updated when you
read any property from userspace.

If we need to add a detection poll feature, let's just use sensible
timings and enable it by default if there is no detection gpio.

Greetings,

-- Sebastian

> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/power/supply/sbs-battery.c | 84 +++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 0b9ecfc1f3f7..f4f1189fec3c 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -14,6 +14,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> +#include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/of.h>
> @@ -214,8 +215,10 @@ struct sbs_info {
>  	u32				poll_retry_count;
>  	struct delayed_work		work;
>  	struct mutex			mode_lock;
> +	u32				poll_interval;
>  	u32				flags;
>  	int				technology;
> +	struct list_head		list;
>  	char				strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1];
>  };
>  
> @@ -242,6 +245,52 @@ static void sbs_invalidate_cached_props(struct sbs_info *chip)
>  }
>  
>  static bool force_load;
> +static DEFINE_MUTEX(sbs_list_lock);
> +static LIST_HEAD(sbs_battery_devices);
> +static unsigned int poll_interval;
> +
> +static int poll_interval_param_set(const char *val,
> +				   const struct kernel_param *kp)
> +{
> +	struct sbs_info *chip;
> +	unsigned int prev_val = *(unsigned int *)kp->arg;
> +	int ret;
> +
> +	ret = param_set_uint(val, kp);
> +	if (ret < 0 || prev_val == *(unsigned int *)kp->arg)
> +		return ret;
> +
> +	mutex_lock(&sbs_list_lock);
> +	list_for_each_entry(chip, &sbs_battery_devices, list) {
> +		if (!chip->gpio_detect) {
> +			chip->poll_interval = poll_interval;
> +			if (chip->poll_interval)
> +				mod_delayed_work(system_wq, &chip->work, 0);
> +		}
> +	}
> +	mutex_unlock(&sbs_list_lock);
> +
> +	return 0;
> +}
> +
> +static const struct kernel_param_ops param_ops_poll_interval = {
> +	.set = poll_interval_param_set,
> +	.get = param_get_uint,
> +};
> +
> +static void sbs_list_remove(void *data)
> +{
> +	struct sbs_info *chip = data;
> +
> +	mutex_lock(&sbs_list_lock);
> +	list_del(&chip->list);
> +	mutex_unlock(&sbs_list_lock);
> +}
> +
> +module_param_cb(poll_interval, &param_ops_poll_interval, &poll_interval, 0644);
> +MODULE_PARM_DESC(
> +	poll_interval,
> +	"Polling interval in milliseconds for devices without GPIO interrupt (0=disabled)");
>  
>  static int sbs_read_word_data(struct i2c_client *client, u8 address);
>  static int sbs_write_word_data(struct i2c_client *client, u8 address, u16 value);
> @@ -1091,6 +1140,10 @@ static void sbs_delayed_work(struct work_struct *work)
>  	/* if the read failed, give up on this work */
>  	if (ret < 0) {
>  		chip->poll_time = 0;
> +		if (chip->poll_interval)
> +			schedule_delayed_work(
> +				&chip->work,
> +				msecs_to_jiffies(chip->poll_interval));
>  		return;
>  	}
>  
> @@ -1106,6 +1159,11 @@ static void sbs_delayed_work(struct work_struct *work)
>  	if (chip->last_state != ret) {
>  		chip->poll_time = 0;
>  		power_supply_changed(chip->power_supply);
> +	}
> +
> +	if (chip->poll_interval) {
> +		schedule_delayed_work(&chip->work,
> +				      msecs_to_jiffies(chip->poll_interval));
>  		return;
>  	}
>  	if (chip->poll_time > 0) {
> @@ -1173,6 +1231,8 @@ static int sbs_probe(struct i2c_client *client)
>  	}
>  	chip->i2c_retry_count = chip->i2c_retry_count + 1;
>  
> +	chip->poll_interval = poll_interval;
> +
>  	chip->charger_broadcasts = !device_property_read_bool(&client->dev,
>  					"sbs,disable-charger-broadcasts");
>  
> @@ -1201,12 +1261,18 @@ static int sbs_probe(struct i2c_client *client)
>  		goto skip_gpio;
>  	}
>  
> +	if (chip->poll_interval) {
> +		dev_dbg(&client->dev,
> +			"GPIO-based IRQ configured, polling disabled\n");
> +		chip->poll_interval = 0;
> +	}
> +
>  skip_gpio:
>  	/*
>  	 * Before we register, we might need to make sure we can actually talk
>  	 * to the battery.
>  	 */
> -	if (!(force_load || chip->gpio_detect)) {
> +	if (!(force_load || chip->gpio_detect || chip->poll_interval)) {
>  		union power_supply_propval val;
>  
>  		rc = sbs_get_battery_presence_and_health(
> @@ -1230,6 +1296,22 @@ static int sbs_probe(struct i2c_client *client)
>  	dev_info(&client->dev,
>  		"%s: battery gas gauge device registered\n", client->name);
>  
> +	mutex_lock(&sbs_list_lock);
> +	list_add(&chip->list, &sbs_battery_devices);
> +	mutex_unlock(&sbs_list_lock);
> +
> +	rc = devm_add_action(&client->dev, sbs_list_remove, chip);
> +	if (rc) {
> +		mutex_lock(&sbs_list_lock);
> +		list_del(&chip->list);
> +		mutex_unlock(&sbs_list_lock);
> +		return rc;
> +	}
> +
> +	if (chip->poll_interval > 0)
> +		schedule_delayed_work(&chip->work,
> +				      msecs_to_jiffies(chip->poll_interval));
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
> 
> 

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