Re: [PATCH 3/4] power:bq27x00: don't fill system log by missing battery

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

On Thu, Oct 08, 2015 at 09:50:26AM +1100, Neil Brown wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
> >> From: NeilBrown <neilb@suse.de>
> >> 
> >> Print message that battery is not calibrated only during debug build
> >> but not during normal operation.
> >> 
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >>  drivers/power/bq27x00_battery.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/power/bq27x00_battery.c
> >> b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
> >> --- a/drivers/power/bq27x00_battery.c
> >> +++ b/drivers/power/bq27x00_battery.c
> >> @@ -491,7 +491,7 @@ static void bq27x00_update(struct
> >> bq27x00_device_info *di) if (cache.flags >= 0) {
> >>  		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
> >>  				&& (cache.flags & BQ27000_FLAG_CI)) {
> >> -			dev_info(di->dev, "battery is not calibrated! ignoring 
> > capacity
> >> values\n"); +			dev_dbg(di->dev, "battery is not calibrated!
> >> ignoring capacity values\n"); cache.capacity = -ENODATA;
> >>  			cache.energy = -ENODATA;
> >>  			cache.time_to_empty = -ENODATA;
> >
> > Hi! I think that better approach would be to use WARN_ONCE or similar 
> > macro. Still use INFO level, just warn about this problem only once...
> 
> Why do you need any warning?
> The status of whether the battery is calibrated is trivially determined
> From the sysfs attributes (several of which will return ENODATA).
> So if some app is being used to report battery status, then it can
> easily report "not calibrated", and if no such app is being used, who
> will care to know?
> 
> I'm not exactly against a once-only warning (though not with WARN_ONCE,
> maybe dev_info_once()) but I don't think that it brings any value at
> all.

There is no use in having it printed every time, even for debugging.
It just adds lots of spam to the log/console. Anyone debugging the
driver will know, that the battery requires calibration and returns
-ENODATA in uncalibrated mode.

Users not knowing the bq27xxx chips might wonder why the driver
returns -ENODATA, though. For these users a single print
in the log is probably useful. Thus I will take this patch
with dev_info_once() and the following tags:

From: NeilBrown <neilb@suse.de>
Suggested-By: H. Nikolaus Schaller <hns@goldelico.com>
Suggested-By: Pali Rohár <pali.rohar@gmail.com>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]