[PATCH v2 0/2] power: supply: bq25630: Improve hardware setup

[PATCH v2 0/2] power: supply: bq25630: Improve hardware setup

[Linmao Li]

This revision addresses the review comments from Waqar Hameed and
Sebastian Reichel on v1.

Patch 1 scopes the battery information to bq25630_setup(), its only
user. Patch 2 uses the new power_supply_desc::init callback, so that the
charger registers are programmed before the device is exposed to the
system.

Patch 2 depends on commit c1eb5905fdce ("power: supply: Add registration
init callback"), which is in the power-supply tree but not in linux-next
yet. The series is therefore based on power-supply/for-next.

Changes in v2:
- Move the battery information get/put pair into bq25630_setup().
- Remove the now-unused batinfo member from the driver data.
- Use power_supply_desc::init to complete the hardware setup before the
  power supply is exposed.
- Split the changes into two patches.

Link to v1:
https://lore.kernel.org/linux-pm/20260727093738.2611185-1-lilinmao@kylinos.cn/

Linmao Li (2):
  power: supply: bq25630: Scope battery information to bq25630_setup()
  power: supply: bq25630: Initialize hardware before exposing the power
    supply

 drivers/power/supply/bq25630_charger.c | 55 +++++++++++++-------------
 1 file changed, 28 insertions(+), 27 deletions(-)


base-commit: c84ccde22d9d3070a9ee144e825270a7a784c570
-- 
2.25.1

[PATCH v2 1/2] power: supply: bq25630: Scope battery information to bq25630_setup()

[Linmao Li]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 4566 bytes --]

data->batinfo is only used by bq25630_setup() to program the initial
charge limits, but power_supply_get_battery_info() allocates it on
psy->dev, so it stays around for the lifetime of the device. Nothing
else in the driver uses it.

Get the battery information in bq25630_setup(), just before it is read,
and release it on every path out of that function. The driver data no
longer has to carry the pointer.

Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/power/supply/bq25630_charger.c | 47 ++++++++++++++------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/power/supply/bq25630_charger.c b/drivers/power/supply/bq25630_charger.c
index 165f8c67b489..9b5f524505d3 100644
--- a/drivers/power/supply/bq25630_charger.c
+++ b/drivers/power/supply/bq25630_charger.c
@@ -356,7 +356,6 @@ struct bq25630_data {
 	struct regmap_field *regfields[BQ25630_REGF_MAX];
 
 	struct power_supply *psy;
-	struct power_supply_battery_info *batinfo;
 
 	/* State status from IRQs. */
 	u8 statregs[BQ25630_NR_STAT_REGS];
@@ -668,6 +667,7 @@ static int bq25630_reset(struct bq25630_data *data)
 
 static int bq25630_setup(struct bq25630_data *data)
 {
+	struct power_supply_battery_info *batinfo;
 	int ret;
 
 	ret = bq25630_reset(data);
@@ -684,69 +684,77 @@ static int bq25630_setup(struct bq25630_data *data)
 		return ret;
 	}
 
+	ret = power_supply_get_battery_info(data->psy, &batinfo);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Could not get battery info\n");
+
 	/*
 	 * Set values according to battery info. Warn on missing "dangerous"
 	 * properties.
 	 */
-	if (data->batinfo->voltage_min_design_uv >= 0) {
+	if (batinfo->voltage_min_design_uv >= 0) {
 		ret = bq25630_write_limit(data, BQ25630_REGF_VSYSMIN,
 					  BQ25630_VSYSMIN_MIN,
 					  BQ25630_VSYSMIN_MAX,
 					  BQ25630_VSYSMIN_STEP,
 					  BQ25630_VSYSMIN_MIN_REGVAL,
-					  data->batinfo->voltage_min_design_uv);
+					  batinfo->voltage_min_design_uv);
 		if (ret)
-			return ret;
+			goto out_put_batinfo;
 	} else
 		dev_warn(data->dev,
 			 "Using default value for minimum voltage\n");
 
-	if (data->batinfo->constant_charge_voltage_max_uv >= 0) {
+	if (batinfo->constant_charge_voltage_max_uv >= 0) {
 		ret = bq25630_write_limit(
 			data, BQ25630_REGF_VREG, BQ25630_VREG_MIN,
 			BQ25630_VREG_MAX, BQ25630_VREG_STEP,
 			BQ25630_VREG_MIN_REGVAL,
-			data->batinfo->constant_charge_voltage_max_uv);
+			batinfo->constant_charge_voltage_max_uv);
 		if (ret)
-			return ret;
+			goto out_put_batinfo;
 	} else
 		dev_warn(data->dev,
 			 "Using default value for maximum constant charge voltage\n");
 
-	if (data->batinfo->constant_charge_current_max_ua >= 0) {
+	if (batinfo->constant_charge_current_max_ua >= 0) {
 		ret = bq25630_write_limit(
 			data, BQ25630_REGF_ICHG, BQ25630_ICHG_MIN,
 			BQ25630_ICHG_MAX, BQ25630_ICHG_STEP,
 			BQ25630_ICHG_MIN_REGVAL,
-			data->batinfo->constant_charge_current_max_ua);
+			batinfo->constant_charge_current_max_ua);
 		if (ret)
-			return ret;
+			goto out_put_batinfo;
 	} else
 		dev_warn(data->dev,
 			 "Using default value for maximum constant charge current\n");
 
-	if (data->batinfo->charge_term_current_ua >= 0) {
+	if (batinfo->charge_term_current_ua >= 0) {
 		ret = bq25630_write_limit(
 			data, BQ25630_REGF_ITERM, BQ25630_ITERM_MIN,
 			BQ25630_ITERM_MAX, BQ25630_ITERM_STEP,
 			BQ25630_ITERM_MIN_REGVAL,
-			data->batinfo->charge_term_current_ua);
+			batinfo->charge_term_current_ua);
 		if (ret)
-			return ret;
+			goto out_put_batinfo;
 	}
 
-	if (data->batinfo->precharge_current_ua >= 0) {
+	if (batinfo->precharge_current_ua >= 0) {
 		ret = bq25630_write_limit(data, BQ25630_REGF_IPRECHG,
 					  BQ25630_IPRECHG_MIN,
 					  BQ25630_IPRECHG_MAX,
 					  BQ25630_IPRECHG_STEP,
 					  BQ25630_IPRECHG_MIN_REGVAL,
-					  data->batinfo->precharge_current_ua);
+					  batinfo->precharge_current_ua);
 		if (ret)
-			return ret;
+			goto out_put_batinfo;
 	}
 
-	return 0;
+out_put_batinfo:
+	power_supply_put_battery_info(data->psy, batinfo);
+
+	return ret;
 }
 
 static int bq25630_charger_get_property(struct power_supply *psy,
@@ -1024,11 +1032,6 @@ static int bq25630_probe(struct i2c_client *client)
 		return dev_err_probe(data->dev, PTR_ERR(data->psy),
 			      "Could not register power supply\n");
 
-	ret = power_supply_get_battery_info(data->psy, &data->batinfo);
-	if (ret)
-		return dev_err_probe(data->dev, ret,
-				     "Could not get battery info\n");
-
 	/*
 	 * Device sends active low 256 µs pulse to report status and fault.
 	 *
-- 
2.25.1

[PATCH v2 2/2] power: supply: bq25630: Initialize hardware before exposing the power supply

[Linmao Li]

bq25630_setup() resets the device, disables the watchdog and programs
the charge limits from the battery information. It runs at the end of
bq25630_probe(), that is after the power supply has been registered, so
the device is already exposed to the system while the hardware still
holds its power-on defaults.

power_supply_desc::init runs during registration, after the driver data
and the fwnode are available and before the device is added. Use it for
bq25630_setup() and drop the explicit call from bq25630_probe().

The callback is passed the power supply, so take the driver data from it
and use it for the battery information as well: data->psy is only
assigned once devm_power_supply_register() returns, which is after the
callback has run.

Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/power/supply/bq25630_charger.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/bq25630_charger.c b/drivers/power/supply/bq25630_charger.c
index 9b5f524505d3..2f6878ad0033 100644
--- a/drivers/power/supply/bq25630_charger.c
+++ b/drivers/power/supply/bq25630_charger.c
@@ -665,8 +665,9 @@ static int bq25630_reset(struct bq25630_data *data)
 	return 0;
 }
 
-static int bq25630_setup(struct bq25630_data *data)
+static int bq25630_setup(struct power_supply *psy)
 {
+	struct bq25630_data *data = power_supply_get_drvdata(psy);
 	struct power_supply_battery_info *batinfo;
 	int ret;
 
@@ -684,7 +685,7 @@ static int bq25630_setup(struct bq25630_data *data)
 		return ret;
 	}
 
-	ret = power_supply_get_battery_info(data->psy, &batinfo);
+	ret = power_supply_get_battery_info(psy, &batinfo);
 	if (ret)
 		return dev_err_probe(data->dev, ret,
 				     "Could not get battery info\n");
@@ -752,7 +753,7 @@ static int bq25630_setup(struct bq25630_data *data)
 	}
 
 out_put_batinfo:
-	power_supply_put_battery_info(data->psy, batinfo);
+	power_supply_put_battery_info(psy, batinfo);
 
 	return ret;
 }
@@ -975,6 +976,7 @@ static const struct power_supply_desc bq25630_charger_psy_desc = {
 	.get_property = bq25630_charger_get_property,
 	.set_property = bq25630_charger_set_property,
 	.property_is_writeable = bq25630_charger_property_is_writeable,
+	.init = bq25630_setup,
 };
 
 static int bq25630_probe(struct i2c_client *client)
@@ -1047,10 +1049,6 @@ static int bq25630_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
 
-	ret = bq25630_setup(data);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/2] power: supply: bq25630: Scope battery information to bq25630_setup()

[Waqar Hameed]

On Tue, Jul 28, 2026 at 10:45 +0800 Linmao Li <lilinmao@kylinos.cn> wrote:

> data->batinfo is only used by bq25630_setup() to program the initial
> charge limits, but power_supply_get_battery_info() allocates it on
> psy->dev, so it stays around for the lifetime of the device. Nothing
> else in the driver uses it.
>
> Get the battery information in bq25630_setup(), just before it is read,
> and release it on every path out of that function. The driver data no
> longer has to carry the pointer.
>
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

Much better! However, `git` complains:

  $ git am /path/to/patch
  error: cannot convert from y to UTF-8
  fatal: could not parse patch

LKML also warns about this [1]. I was able to force UTF-8 and workaround
this though... You might want check your setup.

> ---
>  drivers/power/supply/bq25630_charger.c | 47 ++++++++++++++------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/power/supply/bq25630_charger.c b/drivers/power/supply/bq25630_charger.c
> index 165f8c67b489..9b5f524505d3 100644
> --- a/drivers/power/supply/bq25630_charger.c
> +++ b/drivers/power/supply/bq25630_charger.c
> @@ -356,7 +356,6 @@ struct bq25630_data {
>  	struct regmap_field *regfields[BQ25630_REGF_MAX];
>  
>  	struct power_supply *psy;
> -	struct power_supply_battery_info *batinfo;
>  
>  	/* State status from IRQs. */
>  	u8 statregs[BQ25630_NR_STAT_REGS];
> @@ -668,6 +667,7 @@ static int bq25630_reset(struct bq25630_data *data)
>  
>  static int bq25630_setup(struct bq25630_data *data)
>  {
> +	struct power_supply_battery_info *batinfo;
>  	int ret;
>  
>  	ret = bq25630_reset(data);
> @@ -684,69 +684,77 @@ static int bq25630_setup(struct bq25630_data *data)
>  		return ret;
>  	}
>  
> +	ret = power_supply_get_battery_info(data->psy, &batinfo);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Could not get battery info\n");
> +

`dev_err_probe()` shouldn't be used here. Yes, it is _currently_ only
used from `probe()`, but that shouldn't be an assumption/"policy".
Better to let caller decide on that. I understand that it was just a
pure copy-paste from `probe()`, but we should change it do `dev_error()`
to match the rest of this function.

[...]

[1] https://lore.kernel.org/lkml/20260728024558.3611522-2-lilinmao@kylinos.cn/

Re: [PATCH v2 2/2] power: supply: bq25630: Initialize hardware before exposing the power supply

[Waqar Hameed]

On Tue, Jul 28, 2026 at 10:45 +0800 Linmao Li <lilinmao@kylinos.cn> wrote:

> bq25630_setup() resets the device, disables the watchdog and programs
> the charge limits from the battery information. It runs at the end of
> bq25630_probe(), that is after the power supply has been registered, so
> the device is already exposed to the system while the hardware still
> holds its power-on defaults.
>
> power_supply_desc::init runs during registration, after the driver data
> and the fwnode are available and before the device is added. Use it for
> bq25630_setup() and drop the explicit call from bq25630_probe().
>
> The callback is passed the power supply, so take the driver data from it
> and use it for the battery information as well: data->psy is only
> assigned once devm_power_supply_register() returns, which is after the
> callback has run.
>
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

Looks good to me!

Reviewed-by: Waqar Hameed <waqar.hameed@axis.com>

[...]