Message ID | 20200528140546.25260-1-r-rivera-matos@ti.com |
---|---|
Headers | show |
Series | Add JEITA properties and introduce the bq2515x charger | expand |
On 5/28/20 10:05 AM, Ricardo Rivera-Matos wrote: > From: Dan Murphy <dmurphy@ti.com> > > Add HEALTH_WARM, HEALTH_COOL and HEALTH_HOT to the health enum. > > HEALTH_WARM, HEALTH_COOL, and HEALTH_HOT properties are taken from the JEITA spec. Wouldn't hurt to list the specific version of the spec these are from, but not super important, Acked-by: Andrew F. Davis <afd@ti.com> > > Tested-by: Guru Das Srinagesh <gurus@codeaurora.org> > Signed-off-by: Dan Murphy <dmurphy@ti.com> > --- > Documentation/ABI/testing/sysfs-class-power | 2 +- > drivers/power/supply/power_supply_sysfs.c | 2 +- > include/linux/power_supply.h | 3 +++ > 3 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power > index bf3b48f022dc..9f3fd01a9373 100644 > --- a/Documentation/ABI/testing/sysfs-class-power > +++ b/Documentation/ABI/testing/sysfs-class-power > @@ -190,7 +190,7 @@ Description: > Valid values: "Unknown", "Good", "Overheat", "Dead", > "Over voltage", "Unspecified failure", "Cold", > "Watchdog timer expire", "Safety timer expire", > - "Over current" > + "Over current", "Warm", "Cool", "Hot" > > What: /sys/class/power_supply/<supply_name>/precharge_current > Date: June 2017 > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c > index f37ad4eae60b..d0d549611794 100644 > --- a/drivers/power/supply/power_supply_sysfs.c > +++ b/drivers/power/supply/power_supply_sysfs.c > @@ -61,7 +61,7 @@ static const char * const power_supply_charge_type_text[] = { > static const char * const power_supply_health_text[] = { > "Unknown", "Good", "Overheat", "Dead", "Over voltage", > "Unspecified failure", "Cold", "Watchdog timer expire", > - "Safety timer expire", "Over current" > + "Safety timer expire", "Over current", "Warm", "Cool", "Hot" > }; > > static const char * const power_supply_technology_text[] = { > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h > index dcd5a71e6c67..8670e90c1d51 100644 > --- a/include/linux/power_supply.h > +++ b/include/linux/power_supply.h > @@ -61,6 +61,9 @@ enum { > POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE, > POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE, > POWER_SUPPLY_HEALTH_OVERCURRENT, > + POWER_SUPPLY_HEALTH_WARM, > + POWER_SUPPLY_HEALTH_COOL, > + POWER_SUPPLY_HEALTH_HOT, > }; > > enum { >
On 5/28/20 10:05 AM, Ricardo Rivera-Matos wrote: > +static int bq2515x_set_precharge_current(struct bq2515x_device *bq2515x, > + int val) > +{ > + int ret; > + unsigned int pchrgctrl; > + unsigned int icharge_range; > + unsigned int precharge_reg_code; > + u16 precharge_multiplier = BQ2515X_ICHG_RNG_1B0_UA; > + u16 precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_1875_UA; Why u16? looks like it gets promoted everywhere it's used anyway. > + > + ret = regmap_read(bq2515x->regmap, BQ2515X_PCHRGCTRL, &pchrgctrl); > + if (ret) > + return ret; > + > + icharge_range = pchrgctrl & BQ2515X_ICHARGE_RANGE; > + > + if (icharge_range) { > + precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_3750_UA; > + precharge_multiplier = BQ2515X_ICHG_RNG_1B1_UA; This is a little hard to read when we have a default value overwritten in an if, it basically hides the else logic, suggest: if (icharge_range) { precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_3750_UA; precharge_multiplier = BQ2515X_ICHG_RNG_1B1_UA; } else { precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_1875_UA; precharge_multiplier = BQ2515X_ICHG_RNG_1B0_UA; } > + } > + if (val > precharge_max_ua || val < BQ2515X_ICHG_MIN_UA) > + return -EINVAL; > + > + precharge_reg_code = val / precharge_multiplier; > + > + ret = bq2515x_set_charge_disable(bq2515x, 1); > + if (ret) > + return ret; > + > + ret = regmap_update_bits(bq2515x->regmap, BQ2515X_PCHRGCTRL, > + BQ2515X_PRECHARGE_MASK, precharge_reg_code); > + if (ret) > + return ret; > + > + return bq2515x_set_charge_disable(bq2515x, 0); > +} [snip] > + > +static int bq2515x_set_ilim_lvl(struct bq2515x_device *bq2515x, int val) > +{ > + int i = 0; > + unsigned int array_size = ARRAY_SIZE(bq2515x_ilim_lvl_values); > + > + if (val >= bq2515x_ilim_lvl_values[array_size - 1]) { Isn't this check the same as is done in first iteration of the below loop? Andrew > + i = array_size - 1; > + } else { > + for (i = array_size - 1; i > 0; i--) { > + if (val >= bq2515x_ilim_lvl_values[i]) > + break; > + } > + } > + return regmap_write(bq2515x->regmap, BQ2515X_ILIMCTRL, i); > +} > +
On 5/28/20 9:43 AM, Andrew F. Davis wrote: > On 5/28/20 10:05 AM, Ricardo Rivera-Matos wrote: >> +static int bq2515x_set_precharge_current(struct bq2515x_device *bq2515x, >> + int val) >> +{ >> + int ret; >> + unsigned int pchrgctrl; >> + unsigned int icharge_range; >> + unsigned int precharge_reg_code; >> + u16 precharge_multiplier = BQ2515X_ICHG_RNG_1B0_UA; >> + u16 precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_1875_UA; > > Why u16? looks like it gets promoted everywhere it's used anyway. ACK > > >> + >> + ret = regmap_read(bq2515x->regmap, BQ2515X_PCHRGCTRL, &pchrgctrl); >> + if (ret) >> + return ret; >> + >> + icharge_range = pchrgctrl & BQ2515X_ICHARGE_RANGE; >> + >> + if (icharge_range) { >> + precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_3750_UA; >> + precharge_multiplier = BQ2515X_ICHG_RNG_1B1_UA; > This is a little hard to read when we have a default value overwritten > in an if, it basically hides the else logic, suggest: > > > if (icharge_range) { > precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_3750_UA; > precharge_multiplier = BQ2515X_ICHG_RNG_1B1_UA; > } else { > precharge_max_ua = BQ2515X_PRECHRG_ICHRG_RNGE_1875_UA; > precharge_multiplier = BQ2515X_ICHG_RNG_1B0_UA; > } ACK. I originally had it as an if/else deal, but I got feedback it was too verbose. It will stay verbose. > > >> + } >> + if (val > precharge_max_ua || val < BQ2515X_ICHG_MIN_UA) >> + return -EINVAL; >> + >> + precharge_reg_code = val / precharge_multiplier; >> + >> + ret = bq2515x_set_charge_disable(bq2515x, 1); >> + if (ret) >> + return ret; >> + >> + ret = regmap_update_bits(bq2515x->regmap, BQ2515X_PCHRGCTRL, >> + BQ2515X_PRECHARGE_MASK, precharge_reg_code); >> + if (ret) >> + return ret; >> + >> + return bq2515x_set_charge_disable(bq2515x, 0); >> +} > [snip] > >> + >> +static int bq2515x_set_ilim_lvl(struct bq2515x_device *bq2515x, int val) >> +{ >> + int i = 0; >> + unsigned int array_size = ARRAY_SIZE(bq2515x_ilim_lvl_values); >> + >> + if (val >= bq2515x_ilim_lvl_values[array_size - 1]) { > > Isn't this check the same as is done in first iteration of the below loop? > > Andrew ACK > > >> + i = array_size - 1; >> + } else { >> + for (i = array_size - 1; i > 0; i--) { >> + if (val >= bq2515x_ilim_lvl_values[i]) >> + break; >> + } >> + } >> + return regmap_write(bq2515x->regmap, BQ2515X_ILIMCTRL, i); >> +} >> +
On 5/28/20 9:16 AM, Andrew F. Davis wrote: > On 5/28/20 10:05 AM, Ricardo Rivera-Matos wrote: >> From: Dan Murphy <dmurphy@ti.com> >> >> Add HEALTH_WARM, HEALTH_COOL and HEALTH_HOT to the health enum. >> >> HEALTH_WARM, HEALTH_COOL, and HEALTH_HOT properties are taken from the JEITA spec. > > Wouldn't hurt to list the specific version of the spec these are from, > but not super important, > > Acked-by: Andrew F. Davis <afd@ti.com> ACK. This originates from JISC8712:2015, but is more succinctly explained in "A Guide to the Safe Use of Secondary Lithium Ion Batteries in Notebook-type Personal Computer" > > >> Tested-by: Guru Das Srinagesh <gurus@codeaurora.org> >> Signed-off-by: Dan Murphy <dmurphy@ti.com> >> --- >> Documentation/ABI/testing/sysfs-class-power | 2 +- >> drivers/power/supply/power_supply_sysfs.c | 2 +- >> include/linux/power_supply.h | 3 +++ >> 3 files changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power >> index bf3b48f022dc..9f3fd01a9373 100644 >> --- a/Documentation/ABI/testing/sysfs-class-power >> +++ b/Documentation/ABI/testing/sysfs-class-power >> @@ -190,7 +190,7 @@ Description: >> Valid values: "Unknown", "Good", "Overheat", "Dead", >> "Over voltage", "Unspecified failure", "Cold", >> "Watchdog timer expire", "Safety timer expire", >> - "Over current" >> + "Over current", "Warm", "Cool", "Hot" >> >> What: /sys/class/power_supply/<supply_name>/precharge_current >> Date: June 2017 >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c >> index f37ad4eae60b..d0d549611794 100644 >> --- a/drivers/power/supply/power_supply_sysfs.c >> +++ b/drivers/power/supply/power_supply_sysfs.c >> @@ -61,7 +61,7 @@ static const char * const power_supply_charge_type_text[] = { >> static const char * const power_supply_health_text[] = { >> "Unknown", "Good", "Overheat", "Dead", "Over voltage", >> "Unspecified failure", "Cold", "Watchdog timer expire", >> - "Safety timer expire", "Over current" >> + "Safety timer expire", "Over current", "Warm", "Cool", "Hot" >> }; >> >> static const char * const power_supply_technology_text[] = { >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h >> index dcd5a71e6c67..8670e90c1d51 100644 >> --- a/include/linux/power_supply.h >> +++ b/include/linux/power_supply.h >> @@ -61,6 +61,9 @@ enum { >> POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE, >> POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE, >> POWER_SUPPLY_HEALTH_OVERCURRENT, >> + POWER_SUPPLY_HEALTH_WARM, >> + POWER_SUPPLY_HEALTH_COOL, >> + POWER_SUPPLY_HEALTH_HOT, >> }; >> >> enum { >>