Messages in this thread | | | From | Hermes Zhang <> | Subject | Re: [PATCH] power: supply: bq256xx: Remove init ichg/vbat with max value | Date | Wed, 7 Dec 2022 02:53:15 +0000 |
| |
Hi,
On 2022/12/6 5:21, Sebastian Reichel wrote: > 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.
OK.
> >> 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. >
Yes and thanks for the review. I also go through the whole idea and I think the problem is: when I pass constant_charge_xx_max values in dts, I intend to set the limit of the charging current/voltage which is correct to map to ichg_max/vbatreg_max, but the driver here do extra step to write the max value to ichg/vbatreg regs which I think is not good/safe. So if we want to keep the init setting in driver, I suggest we change the values to the default one: bq->chip_info->bq256xx_def_ichg and bq256xx_def_vbatreg.
> 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; >
Yes, this update will work and solve my problem, the only thing is I need set a safe value in dts instead of a real max value to constant_charge_voltage_max_microvolt, the property name and value is not match. I have another idea as described above, please check. Thanks.
Best Regards, Hermes
| |