[Gta04-owner] bq27x00 driver for Replicant

Dr. H. Nikolaus Schaller hns at goldelico.com
Tue Apr 1 08:39:08 CEST 2014


Hi Neil,

Am 01.04.2014 um 00:37 schrieb NeilBrown:

> On Mon, 31 Mar 2014 21:41:47 +0200 "Dr. H. Nikolaus Schaller"
> <hns at goldelico.com> wrote:
> 
>> Hi,
>> 
>> Am 31.03.2014 um 20:57 schrieb Dr. H. Nikolaus Schaller:
>> 
>>> 
>>> Am 31.03.2014 um 17:02 schrieb Lukas Maerdian:
>>> 
>>>> Hi!
>>>> 
>>>> 2014-03-27 19:07 GMT+01:00 Dr. H. Nikolaus Schaller <hns at goldelico.com>:
>>>>>>> And, the battery poll interval can be set from user space when modprobing the module.
>>>>>>> Simply give the module a parameter "poll_interval=60" but leave the kernel driver as it is.
>>>>>> 
>>>>>> I agree that it possible to do as module parameter.
>>>>> 
>>>>> That is the way it should be done. Otherwise it will boomerang back to us as soon as we
>>>>> want to get something upstream.
> 
> Probably.  Though I would argue that the kernel should avoid polling if at
> all possible, as that can trigger wakeups and thus cost power.
> Polling, should be done by user-space where it is more easily controlled, and
> then only when it  actually needs the information.
> 
> But pushing that argument is best done from a position of strength and if we
> would rather  spend our energies elsewhere, then if we can adjust the timer
> from user-space, then that is enough.
> 
> 
>>>> 
>>>> I agree, too. It can also be done through sysfs, by writing the interval to:
>>>> /sys/module/bq27x00_battery/parameters/poll_interval
>>>> 
>>>> Maybe that can be done in the init.gta04.rc file?
>>>> 
>>>>>>> b) you disable the caching mechanism (which checks for relevant bits) and always notify power_supply_changed()
>>>>>>> 
>>>>>>> I doubt this is acceptable because I don't see what this has to do with battery status in real time.
>>>>>>> If there is no change reported from the battery there is no change to be reported to user space...
>>>>>>> 
>>>>>>> So if you are missing updates, there might be something wrong with the bits reported by the bq27000
>>>>>>> and that should be fixed.
>>>>>> 
>>>>>> I think so, but I repeat that battery technical detail is new for me and I would like that
>>>>>> somebody, who knows technical well give explanation.
>>>>> 
>>>>> I assume that the battery driver has gone through a lot of reviews since it is part of the
>>>>> official kernel.org tree. Therefore it must be quite correct and has not been changed recently.
> 
> Your faith in the process is ... heartwarming.
> 
>>>>> 
>>>>> We can also look into the data sheet to better understand how it works.
>>>>> 
>>>>> BTW: git blame drivers//power/bq27x00_battery.c
>>>>> 
>>>>> tells that Neil Brown has added this caching. I hope he is still reading this list
>>>>> and can comment.
> 
> I didn't add the caching.  I did add the test so that it only triggers a
> uevent when something important happens.
> Each uevent triggers a non-trivial amount of work in udev and elsewhere.
> Continuously generating them is a waste.
> 
>>>>> 
>>>>>> I only can tell, that according my testing only both changes together give a result
>>>>>> ( could see battery status in real time ).
> 
> What does "real time" mean?  And exactly how important is it?
> 
> bq27x00_battery_get_property() is called multiple times to process e.g.
>  cat /proc/class/power_supply/bq27x00_battery/uevent
> (or whatever the path is), so poking at the i2c bus every time is a bit of a
> waste.
> So it currently (in mainline) uses the old cached data (for some values) if
> it is less that 5 seconds old, other wise gets new data.
> 
> This makes user-space polling more often that 5 seconds pointless.
> If you *really* think that polling more often is important, then it is
> probably reasonable to reduce that window to 1 second.
> 
> The bq27000 only updates the voltage register ever 2.56 seconds.
> The "Standby Current" (not sure what that is) is updated every 5.12 seconds.
> 
> So probably returning the caches values for 2 seconds before refreshing them
> would make sense.
> 
> 
> 
>>>>> 
>>>>> I don't doubt that it solves the problem.
>>>>> 
>>>>> The question is if it is the right way to solve it or if it just interacts with some other bug
>>>>> in a positive way.
>>>>> 
>>>>> But I think now since we know that disabling the battery driver caching solves the
>>>>> problem it may help to understand why the user space is not getting regular updates
>>>>> without the patch.
>>>> 
>>>> I can confirm that there are no updates coming from the
>>>> bq27x00_battery.c driver. My assumption even is that the
>>>> charging/not-charging/full events (which are recognized) are coming
>>>> directly from the TWL4030
>>> 
>>> yes, charging/not charging status comes from the usb charger driver and not the hdq battery.
> 
> The usb changer can report if it is passing current from VBUS through to VBAT
> (i.e. supplying the device).
> The hdq interface to the bq27000 can report if the battery is receiving
> charge and allowing it in to the battery (which it won't if the batter is
> full).
> It is quite possible for these readings to be inconsistent, though usually
> only for a short period of time.
> 
>>> 
>>> how does replicant wait/look for events? Are there udev rules/triggers for battery monitoring?
>>> 
>>>> and not from the BQ27000. In this case the
>>>> BQ27000 wouldn't send any updates.
>>> 
>>>> 
>>>> I've CCed Neil Brown, maybe he knows what's going wrong in the
>>>> bq27x00_battery.c code in our 3.12-replicant4 kernel?
>> 
>> Yes, there *is* something wrong.
>> 
>> While looking for bugs in the 3.14 kernel I reviewed some diffs of our kernel vs. linus/v3.14
>> 
>> And found that there is a slightly (or significantly) different rule when to report power_supply_changed()
>> 
>> We have:
>> 
>> 	flags_changed = di->cache.flags ^ cache.flags;
>> 	di->cache = cache;
>> 	if (is_bq27500)
>> 		flags_changed &= BQ27500_FLAGS_IMPORTANT;
>> 	else
>> 		flags_changed &= BQ27000_FLAGS_IMPORTANT;
>> 	if (flags_changed)
>> 		power_supply_changed(&di->bat);
>> 
>> Linus/master has:
>> 
>> 	if (memcmp(&di->cache, &cache, sizeof(cache)) != 0) {
>> 		di->cache = cache;
>> 		power_supply_changed(&di->bat);
>> 	}
> 
> That was one of my changes.  With the mainline code the bq27000 is every
> noisy with uevents.
> 
>> 
>> So our driver reports only changes in *important* flags. And nothing else. And *important*
>> is mainly the charging status as seen by the bq27x00 chip.
>> 
>> Contrarily, the mainline driver reports *any* change in the cached data. Being charging status
>> or level of charge or whatever is stored in di->cache (which is a struct with the values you
>> can read from the /sys node).
>> 
>> Neil apparently has added a mask to reduce the reports to charging status.
>> 
>> Reasons are not clear to me. I don#t know if the commit message in the 3.7 kernel tells something.
>> 
>> IMHO it is no problem reporting almost any change, since it is polled already very rarely as defined by the poll interval.
>> So it should neither influence system load nor power demand.
>> 
>> So I think we should revert neil's extension here (but keep two others).
>> 
>> If you don't disagree, I can prepare it for the 3.14 kernel and Lukas can backport it to the
>> 3.12-replicant kernel.
>> 
>> BR,
>> Nikolaus
> 
> Please do feel free to discard any of my changes that seem to be a problem.
> They seemed like a good idea at the time, but maybe they aren't.
> 
> NeilBrown

thanks for these explanations. I didn't think about the driver potentially flooding udevd with requests.
And thanks to question what "real time" means in this case.

IMHO that flooding not pose a problem if the battery is kernel polled only once every 360 seconds. And although this introduces a neglectable delay. If we consider that it takes 3 hours to drain a battery this gives around 30 events. This should make the charging level go down in steps of 3%. If this is not fine grained enough, the poll_interval can be modified from user space.

So the mainline caching mechanism appears to be right.

Replicant should be able to live with poll_interval=360.

BR and thanks for all these insights making the GTA04 that well supported!
Nikolaus




More information about the Gta04-owner mailing list