Re: [PATCH 1/2] power: supply: bq27xxx_battery: Notify about all battery changes

[Krzysztof Kozlowski <krzk@kernel.org>]

On Tue, May 26, 2020 at 09:24:39PM -0400, Andrew F. Davis wrote:
> On 5/25/20 10:11 AM, Krzysztof Kozlowski wrote:
> > All battery related data could be important for user-space.  For example
> > time-to-full could be shown to user on the screen or health could be
> > monitored for any issues.  Instead of comparing few selected old/new
> > values, just check if anything changed in the cache.
> > 
> 
> 
> At least some value will change every time we poll the battery, are we
> okay with having power_supply_changed() called every time?

Hi,

Let me give few arguments:
1. "Every time" means still once per poll interval or in case of many
   get_property() calls, once per 5 seconds. In first case, if users
   sets polling every 1 second, I expect he knows what he wants. I2C
   will be busy anyway so uevents should not matter that much.
   In second case, called through get_property(), once per 5 seconds is
   not that frequent.

2. Different drivers do it differently. Many chargers notify about
   everything. Most fuel gauges only on status or capacity change (although
   I am not sure if they measure more) but few FG send uevents about
   everything (max17042_battery, sbs-battery, s3c_adc_battery).

3. If drivers does not send notifications on changed properties of
   battery, then basically the user-space has to poll every time for all
   data which is not being a trigger.  The overhead for system would be
   the same, I guess.

Best regards,
Krzysztof


> Andrew
> 
> 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> > index 942c92127b6d..33c26d42cd02 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1612,12 +1612,10 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
> >  			di->charge_design_full = bq27xxx_battery_read_dcap(di);
> >  	}
> >  
> > -	if ((di->cache.capacity != cache.capacity) ||
> > -	    (di->cache.flags != cache.flags))
> > +	if (memcmp(&di->cache, &cache, sizeof(cache)) != 0) {
> >  		power_supply_changed(di->bat);
> > -
> > -	if (memcmp(&di->cache, &cache, sizeof(cache)) != 0)
> >  		di->cache = cache;
> > +	}
> >  
> >  	di->last_update = jiffies;
> >  }
> >