| Message ID | 20221129090112.3451501-1-chenhuiz@axis.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: bq256xx: Remove init ichg/vbat with max value | expand |
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
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)
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(-)