Re: dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation

[Liam Breck]

Hi,

On Tue, Aug 29, 2017 at 12:50 PM, H. Nikolaus Schaller
<hns@xxxxxxxxxxxxx> wrote:
> Hi Liam,
>
>> Am 29.08.2017 um 20:18 schrieb Liam Breck <liam@xxxxxxxxxxxxxxxxx>:
>>
>> Hi Nikolaus, thanks for the testing report...
>>
>> On Tue, Aug 29, 2017 at 7:24 AM, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>> Hi Liam,
>>> seems to work as expected :)
>>>
>>> First boot:
>>>
>>> [    6.096336] bq27xxx_battery_settings
>>> [    6.097987] bq27xxx_battery_set_config
>>> [    6.097990] bq27xxx_battery_unseal
>>> [    6.107987] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [    6.107992] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [    6.107997] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [    6.382048] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [    6.382054] bq27xxx_battery_seal
>>>
>>> Second boot (w/o removing battery in between):
>>>
>>> [    6.008883] bq27xxx_battery_settings
>>> [    6.013280] bq27xxx_battery_set_config
>>> [    6.018761] bq27xxx_battery_unseal
>>> [    6.050694] bq27xxx-battery 1-0055: design-capacity has 6000
>>> [    6.059913] bq27xxx-battery 1-0055: design-energy has 22200
>>> [    6.067113] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [    6.075803] bq27xxx_battery_seal
>>
>> I'd be interested to hear what happens after poweroff & boot (ie does
>> it retain config)?
>
> second boot was simple poweroff + reboot and it changes from "update design-capacity" to "design-capacity has"
> which IMHO means that the bq27421 did retain settings and was therefore not reprogrammed.

That implies that the board is keeping the battery gauge powered
during poweroff. That's not a problem per se, but could indicate that
other subsystems are also powered...

>>
>>> Third boot (with removing battery in between):
>>>
>>> [    6.161085] bq27xxx_battery_settings
>>> [    6.161162] bq27xxx_battery_set_config
>>> [    6.161165] bq27xxx_battery_unseal
>>> [    6.177904] bq27xxx-battery 1-0055: update design-capacity to 6000
>>> [    6.177909] bq27xxx-battery 1-0055: update design-energy to 22200
>>> [    6.177914] bq27xxx-battery 1-0055: terminate-voltage has 3200
>>> [    6.440592] bq27xxx-battery 1-0055: cfgupdate 0, retries 7
>>> [    6.440597] bq27xxx_battery_seal
>
> this was with removing all power which makes the bq27421 loosing the settings.
> And next reboot does another "update design-capacity".
>
>>>
>>> (I have added some printk to trace seal/unseal etc.)
>>>
>>> Values match the 6000mAh specified by DT:
>>>
>>>        bat: battery {
>>>                compatible = "simple-battery", "pyra-battery";
>>>                voltage-min-design-microvolt = <3200000>;
>>>                energy-full-design-microwatt-hours = <22200000>;
>>>                charge-full-design-microamp-hours = <6000000>;
>>>        };
>>>
>>> root@letux:~# cat /sys/class/power_supply/bq27421-0/uevent
>>> POWER_SUPPLY_NAME=bq27421-0
>>> POWER_SUPPLY_STATUS=Discharging
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_VOLTAGE_NOW=3943000
>>> POWER_SUPPLY_CURRENT_NOW=-634000
>>> POWER_SUPPLY_CAPACITY=82
>>> POWER_SUPPLY_CAPACITY_LEVEL=Normal
>>> POWER_SUPPLY_TEMP=319
>>> POWER_SUPPLY_TECHNOLOGY=Li-ion
>>> POWER_SUPPLY_CHARGE_FULL=5467000
>>> POWER_SUPPLY_CHARGE_NOW=4611000
>>> POWER_SUPPLY_CHARGE_FULL_DESIGN=6000000
>>> POWER_SUPPLY_MANUFACTURER=Texas Instruments
>>> root@letux:~#
>>>
>>> Anything else I should test?
>>
>> Could you also test voltage-min-design-microvolt at 3177 to make sure
>> that's configurable too?
>
> Yes:
>
>         bat: battery {
>                 compatible = "simple-battery", "pyra-battery";
>                 voltage-min-design-microvolt = <3177000>;
>                 energy-full-design-microwatt-hours = <22200000>;
>                 charge-full-design-microamp-hours = <6000000>;
>         };
>
> gives
>
> [    6.241009] bq27xxx_battery_settings
> [    6.241055] bq27xxx_battery_set_config
> [    6.241058] bq27xxx_battery_unseal
> [    6.257073] bq27xxx-battery 1-0055: update design-capacity to 6000
> [    6.257078] bq27xxx-battery 1-0055: update design-energy to 22200
> [    6.257084] bq27xxx-battery 1-0055: update terminate-voltage to 3177
> [    6.379230] bq27xxx_battery_seal
>
> So it looks good.

 \o/

>> Sebastian just queued my series to -next. Would you be inclined to
>> submit a new patch enabling bq27421? If so, the sooner the better
>> since merge window is imminent :-)
>
> I already have a patch for that...

Peering eagerly at linux-pm for said patch...


>>
>>
>>>
>>>
>>>> Am 29.08.2017 um 12:40 schrieb Liam Breck <liam@xxxxxxxxxxxxxxxxx>:
>>>>
>>>> Hi Nikolaus,
>>>>
>>>> You need the patchset which enables this feature, which was delayed
>>>> for further work. It's now ready...
>>>>
>>>> https://patchwork.kernel.org/patch/9918947/
>>>> https://patchwork.kernel.org/patch/9918949/
>>>> https://patchwork.kernel.org/patch/9918951/
>>>> https://patchwork.kernel.org/patch/9918953/
>>>> https://patchwork.kernel.org/patch/9918955/
>>>>
>>>> And flip the #if 0 protecting bq27421_dm_regs
>>>>
>>>> I'd love to support that chip in this patchset if you can test
>>>> non-default settings for all 3 options this week?
>>>>
>>>> Thanks,
>>>> Liam
>>>>
>>>>
>>>> On Tue, Aug 29, 2017 at 2:43 AM, H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>>>>> Hi,
>>>>> I am trying to get this working on our bq27421.
>>>>>
>>>>> But the only message I get is:
>>>>>
>>>>> [    6.086407] bq27xxx-battery 1-0055: data memory update not supported for chip
>>>>>
>>>>> A little research shows that this message comes from
>>>>>
>>>>>       http://elixir.free-electrons.com/linux/v4.13-rc7/source/drivers/power/supply/bq27xxx_battery.c#L1279
>>>>>
>>>>> So di->dm_regs is NULL.
>>>>>
>>>>> But doing an fgrep for dm_regs shows no line of code where the pointer is set to a non-null value:
>>>>>
>>>>> master hns$ fgrep -R dm_regs *
>>>>> drivers/power/supply/bq27xxx_battery.c: .class = (di)->dm_regs[i].subclass_id, \
>>>>> drivers/power/supply/bq27xxx_battery.c: .block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \
>>>>> drivers/power/supply/bq27xxx_battery.c: struct bq27xxx_dm_reg *reg = &di->dm_regs[reg_id];
>>>>> drivers/power/supply/bq27xxx_battery.c: if (!di->dm_regs) {
>>>>> drivers/power/supply/bq27xxx_battery.c: max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
>>>>> drivers/power/supply/bq27xxx_battery.c: max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
>>>>> drivers/power/supply/bq27xxx_battery.c: min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
>>>>> drivers/power/supply/bq27xxx_battery.c: max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
>>>>> Binary file drivers/power/supply/bq27xxx_battery.ko matches
>>>>> Binary file drivers/power/supply/bq27xxx_battery.o matches
>>>>> Binary file drivers/power/supply/bq27xxx_battery_i2c.ko matches
>>>>> Binary file drivers/power/supply/bq27xxx_battery_i2c.o matches
>>>>> include/linux/power/bq27xxx_battery.h:  struct bq27xxx_dm_reg *dm_regs;
>>>>> master hns$
>>>>>
>>>>> What am I doing wrong here?
>>>>>
>>>>> BR and thanks,
>>>>> Nikolaus
>>>>>
>>>
>