[PATCH v1 1/2] power: supply: sbs-battery: Silence warning about unknown chemistry

[PATCH v1 1/2] power: supply: sbs-battery: Silence warning about unknown chemistry

[Dmitry Osipenko]

Older variants of controller don't support reporting type of the battery.
Make warning message about unknown chemistry to be printed only once in
order to stop flooding kernel log with the message on each request of the
property. This patch fixes the noisy messages on Asus Transformer TF101.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 8d7a10730e43..b71fbf543428 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -814,7 +814,7 @@ static int sbs_get_chemistry(struct i2c_client *client,
 		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
 
 	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN)
-		dev_warn(&client->dev, "Unknown chemistry: %s\n", chemistry);
+		dev_warn_once(&client->dev, "Unknown chemistry: %s\n", chemistry);
 
 	return 0;
 }
-- 
2.30.2

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

[Dmitry Osipenko]

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(-)

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

Re: [PATCH v1 1/2] power: supply: sbs-battery: Silence warning about unknown chemistry

[Sebastian Reichel]

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

Hi,

On Tue, May 11, 2021 at 01:08:26AM +0300, Dmitry Osipenko wrote:
> Older variants of controller don't support reporting type of the battery.
> Make warning message about unknown chemistry to be printed only once in
> order to stop flooding kernel log with the message on each request of the
> property. This patch fixes the noisy messages on Asus Transformer TF101.
> 
> 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>
> ---

I believe the problem should be fixed as side-effect of the
following patch:

https://lore.kernel.org/linux-pm/20210513020308.4011440-1-ikjn@chromium.org/

With my suggested change the message is printed once for each
battery plug, so probably only once per boot for most users.

-- Sebastian

>  drivers/power/supply/sbs-battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 8d7a10730e43..b71fbf543428 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -814,7 +814,7 @@ static int sbs_get_chemistry(struct i2c_client *client,
>  		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
>  
>  	if (val->intval == POWER_SUPPLY_TECHNOLOGY_UNKNOWN)
> -		dev_warn(&client->dev, "Unknown chemistry: %s\n", chemistry);
> +		dev_warn_once(&client->dev, "Unknown chemistry: %s\n", chemistry);
>  
>  	return 0;
>  }
> -- 
> 2.30.2
> 

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

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

[Sebastian Reichel]

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

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
> 

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

Re: [PATCH v1 1/2] power: supply: sbs-battery: Silence warning about unknown chemistry

[Dmitry Osipenko]

13.05.2021 18:11, Sebastian Reichel пишет:
> Hi,
> 
> On Tue, May 11, 2021 at 01:08:26AM +0300, Dmitry Osipenko wrote:
>> Older variants of controller don't support reporting type of the battery.
>> Make warning message about unknown chemistry to be printed only once in
>> order to stop flooding kernel log with the message on each request of the
>> property. This patch fixes the noisy messages on Asus Transformer TF101.
>>
>> 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>
>> ---
> 
> I believe the problem should be fixed as side-effect of the
> following patch:
> 
> https://lore.kernel.org/linux-pm/20210513020308.4011440-1-ikjn@chromium.org/
> 
> With my suggested change the message is printed once for each
> battery plug, so probably only once per boot for most users.

Looks like that patch indeed should work too, thank you.

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

[Dmitry Osipenko]

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.