[PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

[PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

[Hermes Zhang]

From: Hermes Zhang <chenhuiz@axis.com>

It might be better to return value (e.g. 65535) instead of an error
(e.g. No data available) for the time property.

Normally a common function will handle the read string and parse to
integer for all the properties, but will have problem when read the
time property because need to handle the NODATA error as non-error.
So it will make simple for application which indicate success when
read a number, otherwise as an error to handle.

Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
---
 drivers/power/supply/bq27xxx_battery.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 4c4a7b1c64c5..b75e54aa8ada 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1655,9 +1655,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
 		return tval;
 	}
 
-	if (tval == 65535)
-		return -ENODATA;
-
 	return tval * 60;
 }
 
-- 
2.20.1

Re: [PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

[Pali Rohár]

On Wednesday 31 March 2021 21:51:41 Hermes Zhang wrote:
> From: Hermes Zhang <chenhuiz@axis.com>
> 
> It might be better to return value (e.g. 65535) instead of an error
> (e.g. No data available) for the time property.
> 
> Normally a common function will handle the read string and parse to
> integer for all the properties, but will have problem when read the
> time property because need to handle the NODATA error as non-error.
> So it will make simple for application which indicate success when
> read a number, otherwise as an error to handle.
> 
> Signed-off-by: Hermes Zhang <chenhuiz@axis.com>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 4c4a7b1c64c5..b75e54aa8ada 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1655,9 +1655,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>  		return tval;
>  	}
>  
> -	if (tval == 65535)
> -		return -ENODATA;
> -
>  	return tval * 60;

I'm not sure if this is correct change. If value 65535 is special which
indicates that data are not available then driver should not return
(converted) value 65535*60. If -ENODATA is there to indicate that data
are not available then -ENODATA should not be used.

And if there is application which does not handle -ENODATA for state
when data are not available then it is a bug in application.

>  }
>  
> -- 
> 2.20.1
> 

Re: [PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

[Hermes Zhang]

On 3/31/21 10:02 PM, Pali Rohár wrote:
> @@ -1655,9 +1655,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>  		return tval;
>  	}
>  
> -	if (tval == 65535)
> -		return -ENODATA;
> -
>  	return tval * 60;
> I'm not sure if this is correct change. If value 65535 is special which
> indicates that data are not available then driver should not return
> (converted) value 65535*60. If -ENODATA is there to indicate that data
> are not available then -ENODATA should not be used.
>
> And if there is application which does not handle -ENODATA for state
> when data are not available then it is a bug in application.

Yeah, I just have a feeling return -ENODATA for time_to_full/empty is
not good here. Because:

1. From chip datasheet, it mentioned return 65535 when it's not
available (e.g. read time_to_full when discharging), but the driver
changes behavior here.

2. There is other case will return -ENODATA (e.g. the gauge not
calibrated), so it will confuse application which is real failure.

Could we change the value in minute instead of seconds in
bq27xxx_battery_read_time(), so that means driver do nothing but only
pass the value from the chip?


Best Regards,

Hermes

Re: [PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

[Pali Rohár]

On Thursday 01 April 2021 01:51:28 Hermes Zhang wrote:
> On 3/31/21 10:02 PM, Pali Rohár wrote:
> > @@ -1655,9 +1655,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
> >  		return tval;
> >  	}
> >  
> > -	if (tval == 65535)
> > -		return -ENODATA;
> > -
> >  	return tval * 60;
> > I'm not sure if this is correct change. If value 65535 is special which
> > indicates that data are not available then driver should not return
> > (converted) value 65535*60. If -ENODATA is there to indicate that data
> > are not available then -ENODATA should not be used.
> >
> > And if there is application which does not handle -ENODATA for state
> > when data are not available then it is a bug in application.
> 
> Yeah, I just have a feeling return -ENODATA for time_to_full/empty is
> not good here. Because:
> 
> 1. From chip datasheet, it mentioned return 65535 when it's not
> available (e.g. read time_to_full when discharging), but the driver
> changes behavior here.

So if chip reports that value is not available (by special value 65535)
then driver should not report that data are available with value 3932100s.

> 2. There is other case will return -ENODATA (e.g. the gauge not
> calibrated), so it will confuse application which is real failure.

In both cases, driver does not have (meaningful) value, so it reports
-ENODATA. If you think that some other error code is better for 2. then
we can discuss about it.

But -ENODATA seems a correct error code for reporting when data are not
available.

> Could we change the value in minute instead of seconds in
> bq27xxx_battery_read_time(), so that means driver do nothing but only
> pass the value from the chip?

No, according to API, driver must report value in seconds, not in
minutes, see documentation: Documentation/power/power_supply_class.rst

  All voltages, currents, charges, energies, time and temperatures in µV,
  µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise
  stated. It's driver's job to convert its raw values to units in which
  this class operates.

There is no API for reporting value in minutes.