[Gta04-owner] bq27x00 driver for Replicant

Dr. H. Nikolaus Schaller hns at goldelico.com
Mon Mar 31 21:41:47 CEST 2014


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.
>> 
>> 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.
>>> 
>>> 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 only can tell, that according my testing only both changes together give a result
>>>> ( could see battery status in real time ).
>>> 
>>> 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.
> 
> 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);
	}

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



More information about the Gta04-owner mailing list