diff mbox series

power: supply: bq256xx: Remove init ichg/vbat with max value

Message ID 20221129090112.3451501-1-chenhuiz@axis.com
State New
Headers show
Series power: supply: bq256xx: Remove init ichg/vbat with max value | expand

Commit Message

Hermes Zhang Nov. 29, 2022, 9:01 a.m. UTC
Init the ichg and vbat reg with max value is not good. First the chip
already has a default value for ichg and vbat (small value). Init these
two reg with max value will result an unsafe case (e.g. battery is over
charging in a hot environment) if no user space change them later.

Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
---
 drivers/power/supply/bq256xx_charger.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Sebastian Reichel Dec. 3, 2022, 1:03 a.m. UTC | #1
Hi,

On Wed, Nov 30, 2022 at 09:27:42AM +0000, Hermes Zhang wrote:
> Hi,
> 
> 在 2022/11/29 23:27, Sebastian Reichel 写道:
> > Hi,
> >
> > On Tue, Nov 29, 2022 at 05:01:12PM +0800, Hermes Zhang wrote:
> >> Init the ichg and vbat reg with max value is not good. First the chip
> >> already has a default value for ichg and vbat (small value). Init these
> >> two reg with max value will result an unsafe case (e.g. battery is over
> >> charging in a hot environment) if no user space change them later.
> >>
> >> Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
> >> ---
> > It's the driver's task to setup safe initial maximum values.
> > Pre-kernel values may or may not be safe if you consider things
> > like kexec. If you get unsafe values programmed, then fix the
> > values instead.
> >
> > -- Sebastian
> 
> The constant_charge_current_max_ua is either from dts or default value 
> for each chip in the code, but I guess I could ot change them because it 
> has their own meaning (it will be used to check if the setting is valid 
> or not). Do you mean I can set some other value here instead of 
> constant_xxx_max?

You can just change the DT value to something safe as it is meant to be?

-- Sebastian
Sebastian Reichel Dec. 5, 2022, 9:21 p.m. UTC | #2
Hi,

On Mon, Dec 05, 2022 at 03:23:53AM +0000, Hermes Zhang wrote:
> Init the ichg and vbat reg with max value is not good. First the chip
> already has a default value for ichg and vbat (small value). Init these
> two reg with max value will result an unsafe case (e.g. battery is over
> charging in a hot environment) if no user space change them later.
> 
> Signed-off-by: Hermes Zhang <chenhuiz@axis.com><mailto:chenhuiz@axis.com>
> ---
> 
> 
> It's the driver's task to setup safe initial maximum values.
> Pre-kernel values may or may not be safe if you consider things
> like kexec. If you get unsafe values programmed, then fix the
> values instead.
> 
> -- Sebastian
> 
> 
> 
> The constant_charge_current_max_ua is either from dts or default value
> for each chip in the code, but I guess I could ot change them because it
> has their own meaning (it will be used to check if the setting is valid
> or not). Do you mean I can set some other value here instead of
> constant_xxx_max?
> 
> 
> 
> You can just change the DT value to something safe as it is meant to be?
> 
> 
> Hi,

Please use proper quoting.

> I tried it but it doesn't work. As the property in dts (constant_charge_current_max_microamp
> and constant_charge_voltage_max_microvolt) means the max current and voltage in CC phase, if
> I change them to a safe(small) value, we could not apply any vlaue bigger than them in the
> furture:

Right, because the bigger values are not safe.

> in
> bq256xx_set_ichg_curr()
> 
> ichg = clamp(ichg, BQ256XX_ICHG_MIN_uA, ichg_max);
> 
> So the DT value are not match/sutiable for what we want to set here. Either we introduce two
> new DT property (for init purpose) to set here or we just remove the setting here as the chip
> already has the default values.

There are no chip defaults if you use kexec.

I'm trying to read between the lines. My understanding is, that you
have some kind of userspace based solution to monitor the charging
process and reduce the current if it gets dangerous. This process
should use higher charge currents then what is considered safe; when
it's not running charging should not go above the safe threshold.

This effectively means, that you want to take over the decision what
is considered safe to userspace. So I suggest adding a module parameter
to disable the safety clamp like this:

static unsigned bool no_safety = false;
module_param(no_safety, bool, 0644);
MODULE_PARM_DESC(no_safety, "allow charge currents/voltages considered unsafe by the kernel [default: disallowed]");

...

int ichg_max = bq->init_data.ichg_max;
if (no_safety)
    ichg_max = bq->init_data.ichg_chip_max;

-- Sebastian
diff mbox series

Patch

diff --git a/drivers/power/supply/bq256xx_charger.c b/drivers/power/supply/bq256xx_charger.c
index 01ad84fd147c..d1aa92c10c22 100644
--- a/drivers/power/supply/bq256xx_charger.c
+++ b/drivers/power/supply/bq256xx_charger.c
@@ -1562,21 +1562,11 @@  static int bq256xx_hw_init(struct bq256xx_device *bq)
 	if (ret)
 		return ret;
 
-	ret = bq->chip_info->bq256xx_set_ichg(bq,
-				bat_info->constant_charge_current_max_ua);
-	if (ret)
-		return ret;
-
 	ret = bq->chip_info->bq256xx_set_iprechg(bq,
 				bat_info->precharge_current_ua);
 	if (ret)
 		return ret;
 
-	ret = bq->chip_info->bq256xx_set_vbatreg(bq,
-				bat_info->constant_charge_voltage_max_uv);
-	if (ret)
-		return ret;
-
 	ret = bq->chip_info->bq256xx_set_iterm(bq,
 				bat_info->charge_term_current_ua);
 	if (ret)