[Gta04-owner] bq27x00 driver for Replicant

[Dr. H. Nikolaus Schaller]

Am 01.04.2014 um 11:01 schrieb Lukas Maerdian:

> Hi all!
> 
> 2014-04-01 8:39 GMT+02:00 Dr. H. Nikolaus Schaller <hns at goldelico.com>:
>>>> 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.
> 
> Thanks to everybody involved with solving this issue. I've reverted
> commit ca8f1c8e2cdbb8a475a397e37c066972dfaeae7c to go back to the
> mainline caching mechanism in 3.12-replicant4.
> 
> Nikolaus, you can integrate it in 3.14 with:
> git cherry-pick 76c42635c60fd6a2380fec2fba448cffe636ce66

I already have done the same (but not yet pushed) so that we now have two competing reverts...

But yours looks better :)

We now have both...

BR,
Nikolaus