| Message ID | 20170404085706.32592-4-liam@networkimprov.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 04/04/2017 03:57 AM, Liam Breck wrote: > From: Liam Breck <kernel@networkimprov.net> > > Previously there was no way to configure these chips in the event that the > defaults didn't match the battery in question. > > We now call power_supply_get_battery_info(), check its values, and write > battery data to chip data memory (flash/RAM/NVM). > I'm almost convinced now this is *not* the correct thing to do, we should not be writing to NVM. If the DT states different values than in the device we need to trust the device. DT is static for a board, the battery data may not be. At most we could have an override flag as a driver param that causes us to ignore NVM and use the DT values, but always overwriting the device's NVM with the DT values is not what most want. > Signed-off-by: Matt Ranostay <matt@ranostay.consulting> > Signed-off-by: Liam Breck <kernel@networkimprov.net> > --- > drivers/power/supply/bq27xxx_battery.c | 172 ++++++++++++++++++++++++++++++++- > include/linux/power/bq27xxx_battery.h | 1 + > 2 files changed, 172 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c > index 5f692b1..41eb5b7 100644 > --- a/drivers/power/supply/bq27xxx_battery.c > +++ b/drivers/power/supply/bq27xxx_battery.c > @@ -804,6 +804,13 @@ static LIST_HEAD(bq27xxx_battery_devices); > > #define BQ27XXX_DM_SZ 32 > > +struct bq27xxx_dm_reg { > + u8 subclass_id; > + u8 offset; > + u8 bytes; > + u16 min, max; > +}; > + > /** > * struct bq27xxx_dm_buf - chip data memory buffer > * @class: data memory subclass_id > @@ -821,6 +828,33 @@ struct bq27xxx_dm_buf { > bool has_data, dirty; > }; > > +#define BQ27XXX_DM_BUF(di, i) { \ > + .class = (di)->dm_regs[i].subclass_id, \ > + .block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \ > +} > + > +static inline u16* bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf, > + struct bq27xxx_dm_reg *reg) > +{ > + if (buf->class == reg->subclass_id && > + buf->block == reg->offset / BQ27XXX_DM_SZ) > + return (u16*) (buf->data + reg->offset % BQ27XXX_DM_SZ); > + > + return NULL; > +} > + > +enum bq27xxx_dm_reg_id { > + BQ27XXX_DM_DESIGN_CAPACITY = 0, > + BQ27XXX_DM_DESIGN_ENERGY, > + BQ27XXX_DM_TERMINATE_VOLTAGE, > +}; > + > +static const char* bq27xxx_dm_reg_name[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity", > + [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy", > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", > +}; > + > > static int poll_interval_param_set(const char *val, const struct kernel_param *kp) > { > @@ -1011,6 +1045,39 @@ static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di, > return ret; > } > > +static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di, > + struct bq27xxx_dm_buf *buf, > + enum bq27xxx_dm_reg_id reg_id, > + unsigned int val) > +{ > + struct bq27xxx_dm_reg *reg = &di->dm_regs[reg_id]; > + const char *str = bq27xxx_dm_reg_name[reg_id]; > + u16 *prev = bq27xxx_dm_reg_ptr(buf, reg); > + > + if (prev == NULL) { > + dev_warn(di->dev, "buffer does not match %s dm spec\n", str); > + return; > + } > + > + if (reg->bytes != 2) { > + dev_warn(di->dev, "%s dm spec has unsupported byte size\n", str); > + return; > + } > + > + if (!buf->has_data) > + return; > + > + if (be16_to_cpup(prev) == val) { > + dev_info(di->dev, "%s has %u\n", str, val); > + return; > + } > + > + dev_info(di->dev, "update %s to %u\n", str, val); > + > + *prev = cpu_to_be16(val); > + buf->dirty = true; > +} > + > static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state) > { > const int limit = 100; > @@ -1109,6 +1176,98 @@ static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di, > return ret; > } > > +static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di, > + struct power_supply_battery_info *info) > +{ > + struct bq27xxx_dm_buf bd = BQ27XXX_DM_BUF(di, BQ27XXX_DM_DESIGN_CAPACITY); > + struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE); > + > + if (bq27xxx_battery_unseal(di) < 0) > + return; > + > + if (info->charge_full_design_uah != -EINVAL && > + info->energy_full_design_uwh != -EINVAL) { > + bq27xxx_battery_read_dm_block(di, &bd); > + /* assume design energy & capacity are in same block */ > + bq27xxx_battery_update_dm_block(di, &bd, > + BQ27XXX_DM_DESIGN_CAPACITY, > + info->charge_full_design_uah / 1000); > + bq27xxx_battery_update_dm_block(di, &bd, > + BQ27XXX_DM_DESIGN_ENERGY, > + info->energy_full_design_uwh / 1000); > + } > + > + if (info->voltage_min_design_uv != -EINVAL) { > + bool same = bd.class == bt.class && bd.block == bt.block; > + if (!same) > + bq27xxx_battery_read_dm_block(di, &bt); > + bq27xxx_battery_update_dm_block(di, same ? &bd : &bt, > + BQ27XXX_DM_TERMINATE_VOLTAGE, > + info->voltage_min_design_uv / 1000); > + } > + > + bq27xxx_battery_write_dm_block(di, &bd); > + bq27xxx_battery_write_dm_block(di, &bt); > + > + bq27xxx_battery_seal(di); > + > + if (di->chip != BQ27421) { /* not a cfgupdate chip, so reset */ > + bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false); > + BQ27XXX_MSLEEP(300); /* reset time is not documented */ > + } > + /* assume bq27xxx_battery_update() is called hereafter */ > +} > + > +void bq27xxx_battery_settings(struct bq27xxx_device_info *di) > +{ > + struct power_supply_battery_info info = {}; > + unsigned int min, max; > + > + if (!di->dm_regs) > + return; > + > + if (power_supply_get_battery_info(di->bat, &info) < 0) > + return; > + > + if (info.energy_full_design_uwh != info.charge_full_design_uah) { > + if (info.energy_full_design_uwh == -EINVAL) > + dev_warn(di->dev, "missing battery:energy-full-design-microwatt-hours\n"); > + else if (info.charge_full_design_uah == -EINVAL) > + dev_warn(di->dev, "missing battery:charge-full-design-microamp-hours\n"); > + } > + > + /* assume min == 0 */ > + max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max; > + if (info.energy_full_design_uwh > max * 1000) { > + dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n", > + info.energy_full_design_uwh); > + info.energy_full_design_uwh = -EINVAL; > + } > + > + /* assume min == 0 */ > + max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max; > + if (info.charge_full_design_uah > max * 1000) { > + dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n", > + info.charge_full_design_uah); > + info.charge_full_design_uah = -EINVAL; > + } > + > + min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min; > + max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max; > + if ((info.voltage_min_design_uv < min * 1000 || > + info.voltage_min_design_uv > max * 1000) && > + info.voltage_min_design_uv != -EINVAL) { > + dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n", > + info.voltage_min_design_uv); > + info.voltage_min_design_uv = -EINVAL; > + } > + > + if ((info.energy_full_design_uwh != -EINVAL && > + info.charge_full_design_uah != -EINVAL) || > + info.voltage_min_design_uv != -EINVAL) > + bq27xxx_battery_set_config(di, &info); > +} > + > /* > * Return the battery State-of-Charge > * Or < 0 if something fails. > @@ -1626,6 +1785,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > ret = bq27xxx_simple_value(di->charge_design_full, val); > break; > + /* > + * TODO: Implement these to make registers set from > + * power_supply_battery_info visible in sysfs. > + */ > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + return -EINVAL; > case POWER_SUPPLY_PROP_CYCLE_COUNT: > ret = bq27xxx_simple_value(di->cache.cycle_count, val); > break; > @@ -1659,7 +1825,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy) > int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > { > struct power_supply_desc *psy_desc; > - struct power_supply_config psy_cfg = { .drv_data = di, }; > + struct power_supply_config psy_cfg = { > + .of_node = di->dev->of_node, > + .drv_data = di, > + }; > > INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll); > mutex_init(&di->lock); > @@ -1684,6 +1853,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > > dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION); > > + bq27xxx_battery_settings(di); > bq27xxx_battery_update(di); > > mutex_lock(&bq27xxx_list_lock); > diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h > index b1defb8..227eb08 100644 > --- a/include/linux/power/bq27xxx_battery.h > +++ b/include/linux/power/bq27xxx_battery.h > @@ -64,6 +64,7 @@ struct bq27xxx_device_info { > int id; > enum bq27xxx_chip chip; > const char *name; > + struct bq27xxx_dm_reg *dm_regs; > u32 unseal_key; > struct bq27xxx_access_methods bus; > struct bq27xxx_reg_cache cache; >
On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote: > On 04/04/2017 03:57 AM, Liam Breck wrote: >> From: Liam Breck <kernel@networkimprov.net> >> >> Previously there was no way to configure these chips in the event that the >> defaults didn't match the battery in question. >> >> We now call power_supply_get_battery_info(), check its values, and write >> battery data to chip data memory (flash/RAM/NVM). >> > > I'm almost convinced now this is *not* the correct thing to do, we > should not be writing to NVM. If the DT states different values than in > the device we need to trust the device. DT is static for a board, the > battery data may not be. > > At most we could have an override flag as a driver param that causes us > to ignore NVM and use the DT values, but always overwriting the device's > NVM with the DT values is not what most want. Reprising my previous response, with addenda... We trust the distro/device kernel package to contain correct config for RAM-only chips, but not NVM? A battery config would not be set in mainline dts files, unless a battery is soldered to the board, or similar. I will note this in our DT binding. Re DT provisioning... Device makers and distros are extremely careful about what they put into DTs and what dtbs are included with a kernel package. They know that a misconfigured DT is a showstopper, and could be catastrophic. I've seen the latter -- a guru-level kernel maintainer whom I work with set a DT voltage level wrong on a prototype and fried its eMMC chip. If the DT has a gauge config, the default assumption is that it's correct. It is wrong to force distros and device makers to include /etc/modprobe.d/xyz.conf to fully configure the hardware. However we should indeed let users override the DT config with a module param. How about use-on-chip-config=1, type bool? >> Signed-off-by: Matt Ranostay <matt@ranostay.consulting> >> Signed-off-by: Liam Breck <kernel@networkimprov.net> >> --- >> drivers/power/supply/bq27xxx_battery.c | 172 ++++++++++++++++++++++++++++++++- >> include/linux/power/bq27xxx_battery.h | 1 + >> 2 files changed, 172 insertions(+), 1 deletion(-)
On Thu, Apr 6, 2017 at 11:23 AM, Liam Breck <liam@networkimprov.net> wrote: > On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote: >> On 04/04/2017 03:57 AM, Liam Breck wrote: >>> From: Liam Breck <kernel@networkimprov.net> >>> >>> Previously there was no way to configure these chips in the event that the >>> defaults didn't match the battery in question. >>> >>> We now call power_supply_get_battery_info(), check its values, and write >>> battery data to chip data memory (flash/RAM/NVM). >>> >> >> I'm almost convinced now this is *not* the correct thing to do, we >> should not be writing to NVM. If the DT states different values than in >> the device we need to trust the device. DT is static for a board, the >> battery data may not be. >> >> At most we could have an override flag as a driver param that causes us >> to ignore NVM and use the DT values, but always overwriting the device's >> NVM with the DT values is not what most want. > > Reprising my previous response, with addenda... > > We trust the distro/device kernel package to contain correct config > for RAM-only chips, but not NVM? A battery config would not be set in > mainline dts files, unless a battery is soldered to the board, or > similar. I will note this in our DT binding. > > Re DT provisioning... Device makers and distros are extremely careful > about what they put into DTs and what dtbs are included with a kernel > package. They know that a misconfigured DT is a showstopper, and could > be catastrophic. I've seen the latter -- a guru-level kernel > maintainer whom I work with set a DT voltage level wrong on a > prototype and fried its eMMC chip. If the DT has a gauge config, the > default assumption is that it's correct. > > It is wrong to force distros and device makers to include > /etc/modprobe.d/xyz.conf to fully configure the hardware. > > However we should indeed let users override the DT config with a > module param. How about use-on-chip-config=1, type bool? I came up with... static bool bq27xxx_dt_battery = true; module_param_named(use_dt_battery, bq27xxx_dt_battery, bool, S_IRUGO); MODULE_PARM_DESC(use_dt_battery, "Use devicetree monitored-battery config if present. Default is 1/Y."); After we check for DT monitored-battery, we test bq27xxx_dt_battery, and if false, emit a warning that we are skipping the DT config.
On 04/07/2017 01:58 PM, Liam Breck wrote: > On Thu, Apr 6, 2017 at 11:23 AM, Liam Breck <liam@networkimprov.net> wrote: >> On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote: >>> On 04/04/2017 03:57 AM, Liam Breck wrote: >>>> From: Liam Breck <kernel@networkimprov.net> >>>> >>>> Previously there was no way to configure these chips in the event that the >>>> defaults didn't match the battery in question. >>>> >>>> We now call power_supply_get_battery_info(), check its values, and write >>>> battery data to chip data memory (flash/RAM/NVM). >>>> >>> >>> I'm almost convinced now this is *not* the correct thing to do, we >>> should not be writing to NVM. If the DT states different values than in >>> the device we need to trust the device. DT is static for a board, the >>> battery data may not be. >>> >>> At most we could have an override flag as a driver param that causes us >>> to ignore NVM and use the DT values, but always overwriting the device's >>> NVM with the DT values is not what most want. >> >> Reprising my previous response, with addenda... >> >> We trust the distro/device kernel package to contain correct config >> for RAM-only chips, but not NVM? A battery config would not be set in >> mainline dts files, unless a battery is soldered to the board, or >> similar. I will note this in our DT binding. >> DT is for hardware descriptions, in cases where the hardware cannot be automatically detected. If we find we can get the battery info from the battery gauge why would we not only ignore it, but go and overwrite it with the info from DT? This is backwards. >> Re DT provisioning... Device makers and distros are extremely careful >> about what they put into DTs and what dtbs are included with a kernel >> package. They know that a misconfigured DT is a showstopper, and could >> be catastrophic. I've seen the latter -- a guru-level kernel >> maintainer whom I work with set a DT voltage level wrong on a >> prototype and fried its eMMC chip. If the DT has a gauge config, the >> default assumption is that it's correct. >> And if that eMMC chip could communicate its requested voltage we would not need to describe it in DT, and we certainly would not say "sorry eMMC, you requested 3v but DT says you need 33v, so thats what you're getting". >> It is wrong to force distros and device makers to include >> /etc/modprobe.d/xyz.conf to fully configure the hardware. >> It's either that or add a driver sysfs entry, we don't configure hardware in DT, thats rule #1. You got an ack for a binding that describes a "simple-battery"(one that cannot self report), you are misusing this binding as input configuration data to feed to batteries that *can* report their configuration. >> However we should indeed let users override the DT config with a >> module param. How about use-on-chip-config=1, type bool? > If we do add a param it needs to be default "off". It should not update saved battery data on the chip in either case, that needs to be a manually triggered through sysfs or similar. > I came up with... > > static bool bq27xxx_dt_battery = true; > module_param_named(use_dt_battery, bq27xxx_dt_battery, bool, S_IRUGO); > MODULE_PARM_DESC(use_dt_battery, > "Use devicetree monitored-battery config if present. Default is 1/Y."); > > After we check for DT monitored-battery, we test bq27xxx_dt_battery, > and if false, emit a warning that we are skipping the DT config. >
On Fri, Apr 7, 2017 at 12:19 PM, Andrew F. Davis <afd@ti.com> wrote: > On 04/07/2017 01:58 PM, Liam Breck wrote: >> On Thu, Apr 6, 2017 at 11:23 AM, Liam Breck <liam@networkimprov.net> wrote: >>> On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote: >>>> On 04/04/2017 03:57 AM, Liam Breck wrote: >>>>> From: Liam Breck <kernel@networkimprov.net> >>>>> >>>>> Previously there was no way to configure these chips in the event that the >>>>> defaults didn't match the battery in question. >>>>> >>>>> We now call power_supply_get_battery_info(), check its values, and write >>>>> battery data to chip data memory (flash/RAM/NVM). >>>>> >>>> >>>> I'm almost convinced now this is *not* the correct thing to do, we >>>> should not be writing to NVM. If the DT states different values than in >>>> the device we need to trust the device. DT is static for a board, the >>>> battery data may not be. >>>> >>>> At most we could have an override flag as a driver param that causes us >>>> to ignore NVM and use the DT values, but always overwriting the device's >>>> NVM with the DT values is not what most want. >>> >>> Reprising my previous response, with addenda... >>> >>> We trust the distro/device kernel package to contain correct config >>> for RAM-only chips, but not NVM? A battery config would not be set in >>> mainline dts files, unless a battery is soldered to the board, or >>> similar. I will note this in our DT binding. >>> > > DT is for hardware descriptions, in cases where the hardware cannot be > automatically detected. If we find we can get the battery info from the > battery gauge why would we not only ignore it, but go and overwrite it > with the info from DT? This is backwards. Can this driver manage gauges packaged in batteries? I thought it could only see those wired onto main boards. I agree we should not update battery-packaged gauges by default. Is it possible to tell the difference? On the other hand, no gauge can detect battery characteristics by its own circuitry. It has to be programmed by human hands. A dtb + this patch makes a tool for those hands. And you don't trust device vendors? If a vendor records DT properties for a gauge, it's specifically because the gauge-resident config isn't correct. >>> Re DT provisioning... Device makers and distros are extremely careful >>> about what they put into DTs and what dtbs are included with a kernel >>> package. They know that a misconfigured DT is a showstopper, and could >>> be catastrophic. I've seen the latter -- a guru-level kernel >>> maintainer whom I work with set a DT voltage level wrong on a >>> prototype and fried its eMMC chip. If the DT has a gauge config, the >>> default assumption is that it's correct. >>> > > And if that eMMC chip could communicate its requested voltage we would > not need to describe it in DT, and we certainly would not say "sorry > eMMC, you requested 3v but DT says you need 33v, so thats what you're > getting". >>> It is wrong to force distros and device makers to include >>> /etc/modprobe.d/xyz.conf to fully configure the hardware. >>> > > It's either that or add a driver sysfs entry, we don't configure > hardware in DT, thats rule #1. You got an ack for a binding that Setting regulator voltage levels is not "configuring hardware"? > describes a "simple-battery"(one that cannot self report), you are > misusing this binding as input configuration data to feed to batteries > that *can* report their configuration. >>> However we should indeed let users override the DT config with a >>> module param. How about use-on-chip-config=1, type bool? >> > > If we do add a param it needs to be default "off". It should not update > saved battery data on the chip in either case, that needs to be a > manually triggered through sysfs or similar. > >> I came up with... >> >> static bool bq27xxx_dt_battery = true; >> module_param_named(use_dt_battery, bq27xxx_dt_battery, bool, S_IRUGO); >> MODULE_PARM_DESC(use_dt_battery, >> "Use devicetree monitored-battery config if present. Default is 1/Y."); >> >> After we check for DT monitored-battery, we test bq27xxx_dt_battery, >> and if false, emit a warning that we are skipping the DT config. >>
On 04/07/2017 02:56 PM, Liam Breck wrote: > On Fri, Apr 7, 2017 at 12:19 PM, Andrew F. Davis <afd@ti.com> wrote: >> On 04/07/2017 01:58 PM, Liam Breck wrote: >>> On Thu, Apr 6, 2017 at 11:23 AM, Liam Breck <liam@networkimprov.net> wrote: >>>> On Thu, Apr 6, 2017 at 7:30 AM, Andrew F. Davis <afd@ti.com> wrote: >>>>> On 04/04/2017 03:57 AM, Liam Breck wrote: >>>>>> From: Liam Breck <kernel@networkimprov.net> >>>>>> >>>>>> Previously there was no way to configure these chips in the event that the >>>>>> defaults didn't match the battery in question. >>>>>> >>>>>> We now call power_supply_get_battery_info(), check its values, and write >>>>>> battery data to chip data memory (flash/RAM/NVM). >>>>>> >>>>> >>>>> I'm almost convinced now this is *not* the correct thing to do, we >>>>> should not be writing to NVM. If the DT states different values than in >>>>> the device we need to trust the device. DT is static for a board, the >>>>> battery data may not be. >>>>> >>>>> At most we could have an override flag as a driver param that causes us >>>>> to ignore NVM and use the DT values, but always overwriting the device's >>>>> NVM with the DT values is not what most want. >>>> >>>> Reprising my previous response, with addenda... >>>> >>>> We trust the distro/device kernel package to contain correct config >>>> for RAM-only chips, but not NVM? A battery config would not be set in >>>> mainline dts files, unless a battery is soldered to the board, or >>>> similar. I will note this in our DT binding. >>>> >> >> DT is for hardware descriptions, in cases where the hardware cannot be >> automatically detected. If we find we can get the battery info from the >> battery gauge why would we not only ignore it, but go and overwrite it >> with the info from DT? This is backwards. > > Can this driver manage gauges packaged in batteries? I thought it > could only see those wired onto main boards. I agree we should not > update battery-packaged gauges by default. Is it possible to tell the > difference? > Yes, this is actually the most common configuration. (You probably have a BQ27xxx in this config in your pocket right now if you have a cell phone). Some BQ27xxx parts have a battery detect, this is usually only needed for gauges that do not ride along with the battery, but in practice you cannot tell the difference except by looking at how it is wired. > On the other hand, no gauge can detect battery characteristics by its > own circuitry. It has to be programmed by human hands. A dtb + this > patch makes a tool for those hands. > We don't need a tool for this, we have several open source tools for this already for battery and device manufactures. This is the kernel driver we are working on, it is for the user of hardware, not producer. > And you don't trust device vendors? If a vendor records DT properties > for a gauge, it's specifically because the gauge-resident config isn't > correct. > I trust the manufacturer of the device more than the Linux port I just downloaded for the device, yes. Sometimes the manufacturer gets it wrong, but this should be the exception, not the expected case. >>>> Re DT provisioning... Device makers and distros are extremely careful >>>> about what they put into DTs and what dtbs are included with a kernel >>>> package. They know that a misconfigured DT is a showstopper, and could >>>> be catastrophic. I've seen the latter -- a guru-level kernel >>>> maintainer whom I work with set a DT voltage level wrong on a >>>> prototype and fried its eMMC chip. If the DT has a gauge config, the >>>> default assumption is that it's correct. >>>> >> >> And if that eMMC chip could communicate its requested voltage we would >> not need to describe it in DT, and we certainly would not say "sorry >> eMMC, you requested 3v but DT says you need 33v, so thats what you're >> getting". > >>>> It is wrong to force distros and device makers to include >>>> /etc/modprobe.d/xyz.conf to fully configure the hardware. >>>> >> >> It's either that or add a driver sysfs entry, we don't configure >> hardware in DT, thats rule #1. You got an ack for a binding that > > Setting regulator voltage levels is not "configuring hardware"? > No it is not, look at a regulator binding, in the regulator node we describe the device's voltage limits, these voltage limits are a physical property of the regulator. Then in the node for a device powered by this regulator we describe the voltage it is designed to operate at, this also is a physical description of a device, not something we chose. >> describes a "simple-battery"(one that cannot self report), you are >> misusing this binding as input configuration data to feed to batteries >> that *can* report their configuration. > >>>> However we should indeed let users override the DT config with a >>>> module param. How about use-on-chip-config=1, type bool? >>> >> >> If we do add a param it needs to be default "off". It should not update >> saved battery data on the chip in either case, that needs to be a >> manually triggered through sysfs or similar. >> >>> I came up with... >>> >>> static bool bq27xxx_dt_battery = true; >>> module_param_named(use_dt_battery, bq27xxx_dt_battery, bool, S_IRUGO); >>> MODULE_PARM_DESC(use_dt_battery, >>> "Use devicetree monitored-battery config if present. Default is 1/Y."); >>> >>> After we check for DT monitored-battery, we test bq27xxx_dt_battery, >>> and if false, emit a warning that we are skipping the DT config. >>>
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c index 5f692b1..41eb5b7 100644 --- a/drivers/power/supply/bq27xxx_battery.c +++ b/drivers/power/supply/bq27xxx_battery.c @@ -804,6 +804,13 @@ static LIST_HEAD(bq27xxx_battery_devices); #define BQ27XXX_DM_SZ 32 +struct bq27xxx_dm_reg { + u8 subclass_id; + u8 offset; + u8 bytes; + u16 min, max; +}; + /** * struct bq27xxx_dm_buf - chip data memory buffer * @class: data memory subclass_id @@ -821,6 +828,33 @@ struct bq27xxx_dm_buf { bool has_data, dirty; }; +#define BQ27XXX_DM_BUF(di, i) { \ + .class = (di)->dm_regs[i].subclass_id, \ + .block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \ +} + +static inline u16* bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf, + struct bq27xxx_dm_reg *reg) +{ + if (buf->class == reg->subclass_id && + buf->block == reg->offset / BQ27XXX_DM_SZ) + return (u16*) (buf->data + reg->offset % BQ27XXX_DM_SZ); + + return NULL; +} + +enum bq27xxx_dm_reg_id { + BQ27XXX_DM_DESIGN_CAPACITY = 0, + BQ27XXX_DM_DESIGN_ENERGY, + BQ27XXX_DM_TERMINATE_VOLTAGE, +}; + +static const char* bq27xxx_dm_reg_name[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity", + [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy", + [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", +}; + static int poll_interval_param_set(const char *val, const struct kernel_param *kp) { @@ -1011,6 +1045,39 @@ static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di, return ret; } +static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di, + struct bq27xxx_dm_buf *buf, + enum bq27xxx_dm_reg_id reg_id, + unsigned int val) +{ + struct bq27xxx_dm_reg *reg = &di->dm_regs[reg_id]; + const char *str = bq27xxx_dm_reg_name[reg_id]; + u16 *prev = bq27xxx_dm_reg_ptr(buf, reg); + + if (prev == NULL) { + dev_warn(di->dev, "buffer does not match %s dm spec\n", str); + return; + } + + if (reg->bytes != 2) { + dev_warn(di->dev, "%s dm spec has unsupported byte size\n", str); + return; + } + + if (!buf->has_data) + return; + + if (be16_to_cpup(prev) == val) { + dev_info(di->dev, "%s has %u\n", str, val); + return; + } + + dev_info(di->dev, "update %s to %u\n", str, val); + + *prev = cpu_to_be16(val); + buf->dirty = true; +} + static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state) { const int limit = 100; @@ -1109,6 +1176,98 @@ static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di, return ret; } +static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di, + struct power_supply_battery_info *info) +{ + struct bq27xxx_dm_buf bd = BQ27XXX_DM_BUF(di, BQ27XXX_DM_DESIGN_CAPACITY); + struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE); + + if (bq27xxx_battery_unseal(di) < 0) + return; + + if (info->charge_full_design_uah != -EINVAL && + info->energy_full_design_uwh != -EINVAL) { + bq27xxx_battery_read_dm_block(di, &bd); + /* assume design energy & capacity are in same block */ + bq27xxx_battery_update_dm_block(di, &bd, + BQ27XXX_DM_DESIGN_CAPACITY, + info->charge_full_design_uah / 1000); + bq27xxx_battery_update_dm_block(di, &bd, + BQ27XXX_DM_DESIGN_ENERGY, + info->energy_full_design_uwh / 1000); + } + + if (info->voltage_min_design_uv != -EINVAL) { + bool same = bd.class == bt.class && bd.block == bt.block; + if (!same) + bq27xxx_battery_read_dm_block(di, &bt); + bq27xxx_battery_update_dm_block(di, same ? &bd : &bt, + BQ27XXX_DM_TERMINATE_VOLTAGE, + info->voltage_min_design_uv / 1000); + } + + bq27xxx_battery_write_dm_block(di, &bd); + bq27xxx_battery_write_dm_block(di, &bt); + + bq27xxx_battery_seal(di); + + if (di->chip != BQ27421) { /* not a cfgupdate chip, so reset */ + bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false); + BQ27XXX_MSLEEP(300); /* reset time is not documented */ + } + /* assume bq27xxx_battery_update() is called hereafter */ +} + +void bq27xxx_battery_settings(struct bq27xxx_device_info *di) +{ + struct power_supply_battery_info info = {}; + unsigned int min, max; + + if (!di->dm_regs) + return; + + if (power_supply_get_battery_info(di->bat, &info) < 0) + return; + + if (info.energy_full_design_uwh != info.charge_full_design_uah) { + if (info.energy_full_design_uwh == -EINVAL) + dev_warn(di->dev, "missing battery:energy-full-design-microwatt-hours\n"); + else if (info.charge_full_design_uah == -EINVAL) + dev_warn(di->dev, "missing battery:charge-full-design-microamp-hours\n"); + } + + /* assume min == 0 */ + max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max; + if (info.energy_full_design_uwh > max * 1000) { + dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n", + info.energy_full_design_uwh); + info.energy_full_design_uwh = -EINVAL; + } + + /* assume min == 0 */ + max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max; + if (info.charge_full_design_uah > max * 1000) { + dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n", + info.charge_full_design_uah); + info.charge_full_design_uah = -EINVAL; + } + + min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min; + max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max; + if ((info.voltage_min_design_uv < min * 1000 || + info.voltage_min_design_uv > max * 1000) && + info.voltage_min_design_uv != -EINVAL) { + dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n", + info.voltage_min_design_uv); + info.voltage_min_design_uv = -EINVAL; + } + + if ((info.energy_full_design_uwh != -EINVAL && + info.charge_full_design_uah != -EINVAL) || + info.voltage_min_design_uv != -EINVAL) + bq27xxx_battery_set_config(di, &info); +} + /* * Return the battery State-of-Charge * Or < 0 if something fails. @@ -1626,6 +1785,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy, case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: ret = bq27xxx_simple_value(di->charge_design_full, val); break; + /* + * TODO: Implement these to make registers set from + * power_supply_battery_info visible in sysfs. + */ + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: + return -EINVAL; case POWER_SUPPLY_PROP_CYCLE_COUNT: ret = bq27xxx_simple_value(di->cache.cycle_count, val); break; @@ -1659,7 +1825,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy) int bq27xxx_battery_setup(struct bq27xxx_device_info *di) { struct power_supply_desc *psy_desc; - struct power_supply_config psy_cfg = { .drv_data = di, }; + struct power_supply_config psy_cfg = { + .of_node = di->dev->of_node, + .drv_data = di, + }; INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll); mutex_init(&di->lock); @@ -1684,6 +1853,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION); + bq27xxx_battery_settings(di); bq27xxx_battery_update(di); mutex_lock(&bq27xxx_list_lock); diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h index b1defb8..227eb08 100644 --- a/include/linux/power/bq27xxx_battery.h +++ b/include/linux/power/bq27xxx_battery.h @@ -64,6 +64,7 @@ struct bq27xxx_device_info { int id; enum bq27xxx_chip chip; const char *name; + struct bq27xxx_dm_reg *dm_regs; u32 unseal_key; struct bq27xxx_access_methods bus; struct bq27xxx_reg_cache cache;