[v1,2/2] power: supply: sbs-battery: Fall back to Li-ion battery type for bq20z75

Message ID 20210510220827.11595-2-digetx@gmail.com
State New
Headers show
Series
  • [v1,1/2] power: supply: sbs-battery: Silence warning about unknown chemistry
Related show

Commit Message

Dmitry Osipenko May 10, 2021, 10:08 p.m.
The older bq20z75 controller doesn't support reporting the battery type
and the type is Li-ion in this case.

Tested-by: Antoni Aloy Torrens <aaloytorrens@gmail.com> # TF101
Tested-by: Nikola Milosavljević <mnidza@outlook.com> # TF101
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/power/supply/sbs-battery.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Sebastian Reichel May 13, 2021, 3:31 p.m. | #1
Hi,

On Tue, May 11, 2021 at 01:08:27AM +0300, Dmitry Osipenko wrote:
> The older bq20z75 controller doesn't support reporting the battery type

> and the type is Li-ion in this case.

> 

> Tested-by: Antoni Aloy Torrens <aaloytorrens@gmail.com> # TF101

> Tested-by: Nikola Milosavljević <mnidza@outlook.com> # TF101

> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

> ---


If it does not support reporting the battery type you should get an
error from sbs_get_battery_string_property. Obviously a string has
been returned, or you would not end up that far in the code. What
string do you see?

Considering BQ20Z65 and BQ20Z75 also support Li-Po I don't think
it's a good idea to fall back to Li-Ion. Kernel should never lie
about this, since I know some people use userspace based charging
setup and the charge limits are different for Li-Ion and Li-Po. When
reaching this place we do not know 100%, that it is a Li-ion, so
returning UNKNOWN is the safe option.

If you know, that your device (TF101) only supports Li-Ion
batteries, we can add a device specific override. But is this worth
the added maintenance burden? What is your plan for using this
information?

-- Sebastian

>  drivers/power/supply/sbs-battery.c | 10 +++++++++-

>  1 file changed, 9 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c

> index b71fbf543428..fec6c139d4ff 100644

> --- a/drivers/power/supply/sbs-battery.c

> +++ b/drivers/power/supply/sbs-battery.c

> @@ -813,9 +813,17 @@ static int sbs_get_chemistry(struct i2c_client *client,

>  	else

>  		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;

>  

> -	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN)

> +	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN) {

> +		struct sbs_info *chip = i2c_get_clientdata(client);

> +

>  		dev_warn_once(&client->dev, "Unknown chemistry: %s\n", chemistry);

>  

> +		if (chip->flags & SBS_FLAGS_TI_BQ20ZX5) {

> +			dev_warn_once(&client->dev, "Falling back to Li-ion\n");

> +			val->intval = POWER_SUPPLY_TECHNOLOGY_LION;

> +		}

> +	}

> +

>  	return 0;

>  }

>  

> -- 

> 2.30.2

>
Dmitry Osipenko May 14, 2021, 1:16 p.m. | #2
13.05.2021 18:31, Sebastian Reichel пишет:
> Hi,

> 

> On Tue, May 11, 2021 at 01:08:27AM +0300, Dmitry Osipenko wrote:

>> The older bq20z75 controller doesn't support reporting the battery type

>> and the type is Li-ion in this case.

>>

>> Tested-by: Antoni Aloy Torrens <aaloytorrens@gmail.com> # TF101

>> Tested-by: Nikola Milosavljević <mnidza@outlook.com> # TF101

>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

>> ---

> 

> If it does not support reporting the battery type you should get an

> error from sbs_get_battery_string_property. Obviously a string has

> been returned, or you would not end up that far in the code. What

> string do you see?


There is no visible error. Where the error condition should be set?

The returned string is:

sbs-battery 5-000b: Unknown chemistry: OAI0

> Considering BQ20Z65 and BQ20Z75 also support Li-Po I don't think

> it's a good idea to fall back to Li-Ion. Kernel should never lie

> about this, since I know some people use userspace based charging

> setup and the charge limits are different for Li-Ion and Li-Po. When

> reaching this place we do not know 100%, that it is a Li-ion, so

> returning UNKNOWN is the safe option.

> 

> If you know, that your device (TF101) only supports Li-Ion

> batteries, we can add a device specific override. But is this worth

> the added maintenance burden? What is your plan for using this

> information?


There is no plan of using that information. Previously battery type was
reported properly by userspace, then it regressed. There are other older
device-trees in upstream which should have seen the same regression,
apparently nobody noticed or cared about it. Yours variant of solution
will take more effort, in this case it should be better to leave the
regression as-is for now.

Patch

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index b71fbf543428..fec6c139d4ff 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -813,9 +813,17 @@  static int sbs_get_chemistry(struct i2c_client *client,
 	else
 		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 
-	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN)
+	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN) {
+		struct sbs_info *chip = i2c_get_clientdata(client);
+
 		dev_warn_once(&client->dev, "Unknown chemistry: %s\n", chemistry);
 
+		if (chip->flags & SBS_FLAGS_TI_BQ20ZX5) {
+			dev_warn_once(&client->dev, "Falling back to Li-ion\n");
+			val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		}
+	}
+
 	return 0;
 }