[Gta04-owner] bq27x00 driver for Replicant

NeilBrown neilb at suse.de
Tue Apr 1 00:37:01 CEST 2014


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 828 bytes
Desc: not available
URL: <http://lists.goldelico.com/pipermail/gta04-owner/attachments/20140401/45b00f7c/attachment-0001.bin>


More information about the Gta04-owner mailing list