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

  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(-)
  

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

>  drivers/power/supply/bq256xx_charger.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> 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)
> -- 
> 2.30.2
>
  

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?

Best Regards,
Hermes


>>   drivers/power/supply/bq256xx_charger.c | 10 ----------
>>   1 file changed, 10 deletions(-)
>>
>> 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)
>> -- 
>> 2.30.2
>>
  

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
  

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
  

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