Re: [PATCH] power: supply: bq256xx: Remove init ichg/vbat with max value

[Sebastian Reichel]

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@xxxxxxxx><mailto:chenhuiz@xxxxxxxx>
> ---
> 
> 
> 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

Attachment: signature.asc
Description: PGP signature