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

[Hermes Zhang]

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@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.

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