Re: [PATCH 5/5] bq27x00 - don't report power-supply change sooften.

[NeilBrown]

On Sat, 31 Dec 2011 12:27:43 +0100 Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:

> On 12/30/2011 12:13 PM, Grazvydas Ignotas wrote:
> > CCing Lars who added this. I vaguely recall something about generating
> > events to make some battery monitors update but I forget the details
> > now, maybe it was about something else. Also CCing Anton (the
> > maintainer).
> > 
> > On Fri, Dec 30, 2011 at 2:58 AM, NeilBrown <neilb@xxxxxxx> wrote:
> >> A power_supply_changed should only be reported on significant changes
> >> such as transition between charging and not.  Incremental changes
> >> such as charge increasing should not be reported - that can easily
> >> be polled for.
> 
> Well, you can also poll for those "significant" changes, too. The point of
> adding this was to have a centralized point where polling takes place instead
> of letting each battery monitor do this on its own. Though if, as you wrote in
> the cover letter, the some properties change every time the values are read it
> might makes sense to exclude these from the comparison. On the other hand one
> event every 6 minutes doesn't really sound harmful and I would suspect that
> battery monitors will use a similar interval when manually polling the device.

Hi,

 That is a very good point.  user-space could poll for all of the changes and
 polling the kernel provides no real benefit.

 I don't really see the problem with each battery monitor doing their own
 polling.  At least that way they would have control over when polling
 happens.

 Polling every 6 minutes does not really seem like a lot - except that
 polling at all in the kernel should be avoided.  If the system is idle and
 has managed to get into a lower power state, waking up just to check on the
 battery would be the wrong thing to do.

 A battery monitor could notice that no-one cared (e.g. A X11 client not
 getting any 'expose' events) and could stop polling.  The kernel cannot do
 that.

 However the part of the current code that really bothered me was that a
 'change' event is generated every time anyone polls the battery status - but
 at most every 5 seconds.  These extra change events really aren't wanted.

 I think we should always be cautious of adding change events.  They are
 not there just to report when any detail changed, else a 'keyboard' device
 would report a 'change' event on every keystroke.
 The main purpose of uevents are to report 'add' and 'remove' of devices.
 'change' is for situations where a device changes in a way that is very
 similar to an 'add' or a 'remove' but isn't implemented as a new device.
 A simple example is inserting a CD into a CD drive.  Before the device
 couldn't do anything useful, now it can.  It is a new medium, which is
 almost like a new device but just not implemented that way.  So it deserves a
 change event.
 Or when a power supply gets plugged in.  We really have a new thing here -
 we have added power.  But as the power isn't a device we cannot have an
 'add' uevent, so we have a 'change' uevent on the power supply.

 So I would be in favour of removing the in-kernel polling altogether.
 That puts complete control where it belongs: is the app that is monitoring
 the battery.

 Thoughts??

Thanks,
NeilBrown


> 
> - Lars
> 
> >>
> >> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> >> ---
> >>
> >>  drivers/power/bq27x00_battery.c |   15 ++++++++++++---
> >>  1 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
> >> index bb16f5b..7993a17 100644
> >> --- a/drivers/power/bq27x00_battery.c
> >> +++ b/drivers/power/bq27x00_battery.c
> >> @@ -57,11 +57,15 @@
> >>  #define BQ27000_FLAG_CHGS              BIT(7)
> >>  #define BQ27000_FLAG_FC                        BIT(5)
> >>
> >> +#define BQ27000_FLAGS_IMPORTANT                (BQ27000_FLAG_FC|BQ27000_FLAG_CHGS|BIT(31))
> >> +
> >>  #define BQ27500_REG_SOC                        0x2C
> >>  #define BQ27500_REG_DCAP               0x3C /* Design capacity */
> >>  #define BQ27500_FLAG_DSC               BIT(0)
> >>  #define BQ27500_FLAG_FC                        BIT(9)
> >>
> >> +#define BQ27500_FLAGS_IMPORTANT                (BQ27500_FLAG_FC|BQ27500_FLAG_DSC|BIT(31))
> >> +
> >>  #define BQ27000_RS                     20 /* Resistor sense */
> >>
> >>  struct bq27x00_device_info;
> >> @@ -259,6 +263,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >>  {
> >>        struct bq27x00_reg_cache cache = {0, };
> >>        bool is_bq27500 = di->chip == BQ27500;
> >> +       int flags_changed;
> >>
> >>        cache.flags = bq27x00_read(di, BQ27x00_REG_FLAGS, is_bq27500);
> >>        if (cache.flags >= 0) {
> >> @@ -280,10 +285,14 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> >>
> >>        /* Ignore current_now which is a snapshot of the current battery state
> >>         * and is likely to be different even between two consecutive reads */
> >> -       if (memcmp(&di->cache, &cache, sizeof(cache) - sizeof(int)) != 0) {
> >> -               di->cache = cache;
> >> +       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);
> >> -       }
> >>
> >>        di->last_update = jiffies;
> >>  }

Attachment: signature.asc
Description: PGP signature