[PATCH 1/2] power: supply: sbs-battery: Reject all-zero readings as battery absent

[PATCH 1/2] power: supply: sbs-battery: Reject all-zero readings as battery absent

[LI Qingwu]

The driver reports battery present when status register read succeeds,
without checking the actual register values. Some systems return all
zeros when no battery is connected, causing false presence detection.

Add validation: when status reads zero, cross-check voltage and capacity.
Report battery absent only if all three registers return zero.

Tested on i.MX 8M Plus platform with SBS-compliant battery.

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 drivers/power/supply/sbs-battery.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 943c82ee978f..0b9ecfc1f3f7 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -594,9 +594,19 @@ static int sbs_get_battery_presence_and_health(
 		return ret;
 	}
 
-	if (psp == POWER_SUPPLY_PROP_PRESENT)
+	if (psp == POWER_SUPPLY_PROP_PRESENT) {
 		val->intval = 1; /* battery present */
-	else { /* POWER_SUPPLY_PROP_HEALTH */
+		if (ret == 0) {
+			int voltage, capacity;
+
+			voltage = sbs_read_word_data(
+				client, sbs_data[REG_VOLTAGE].addr);
+			capacity = sbs_read_word_data(
+				client, sbs_data[REG_CAPACITY].addr);
+			if (voltage == 0 && capacity == 0)
+				val->intval = 0;
+		}
+	} else { /* POWER_SUPPLY_PROP_HEALTH */
 		if (sbs_bat_needs_calibration(client)) {
 			val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
 		} else {
-- 
2.43.0

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

[LI Qingwu]

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.

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

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

[Sebastian Reichel]

[-- 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 --]

Re: [PATCH 1/2] power: supply: sbs-battery: Reject all-zero readings as battery absent

[Sebastian Reichel]

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

Hi,

On Wed, Dec 31, 2025 at 09:31:51AM +0000, LI Qingwu wrote:
> The driver reports battery present when status register read succeeds,
> without checking the actual register values. Some systems return all
> zeros when no battery is connected, causing false presence detection.
> 
> Add validation: when status reads zero, cross-check voltage and capacity.
> Report battery absent only if all three registers return zero.
> 
> Tested on i.MX 8M Plus platform with SBS-compliant battery.
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---

Can you provide some more details about the platform / SBS compliant
battery?

The SBS battery chip is supposed to be part of the battery [*], so
by removing the battery you are removing the I2C device. Accessing a
non-existing device surely should not give you 0, but an error. That
suggests either your hardware design is broken and you disconnect
the battery cells from the IC or your I2C/SMbus driver is buggy and
should be fixed.

[*] This is the whole point of calling it a 'smart battery'. The
    battery cells and the chip are supposed to be one unit. This
    allows the chip to use a coloumb counter to measure energy
    stored/taken out of the battery and learn how the battery
    behaves.

Greetings,

-- Sebastian

>  drivers/power/supply/sbs-battery.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 943c82ee978f..0b9ecfc1f3f7 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -594,9 +594,19 @@ static int sbs_get_battery_presence_and_health(
>  		return ret;
>  	}
>  
> -	if (psp == POWER_SUPPLY_PROP_PRESENT)
> +	if (psp == POWER_SUPPLY_PROP_PRESENT) {
>  		val->intval = 1; /* battery present */
> -	else { /* POWER_SUPPLY_PROP_HEALTH */
> +		if (ret == 0) {
> +			int voltage, capacity;
> +
> +			voltage = sbs_read_word_data(
> +				client, sbs_data[REG_VOLTAGE].addr);
> +			capacity = sbs_read_word_data(
> +				client, sbs_data[REG_CAPACITY].addr);
> +			if (voltage == 0 && capacity == 0)
> +				val->intval = 0;
> +		}
> +	} else { /* POWER_SUPPLY_PROP_HEALTH */
>  		if (sbs_bat_needs_calibration(client)) {
>  			val->intval = POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
>  		} else {
> -- 
> 2.43.0
> 

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