| Message ID | 73cb8d96728805597ecb50e7e7b51bf6272b6d61.1444243358.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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...
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. Thanks, NeilBrown
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
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;