[PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Hans de Goede]

The battery on the 2nd hand Surface 3 which I recently bought appears to
not have a serial no programmed in. This results in any I2C reads from
the registers containing the serial no failing with an I2C NACK.

This was causing mshw0011_bix() to fail causing the battery readings to
not work at all.

Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
continue with an empty serial no to fix this.

Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/surface/surface3_power.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
index abac3eec565e..b283bc9bb5fd 100644
--- a/drivers/platform/surface/surface3_power.c
+++ b/drivers/platform/surface/surface3_power.c
@@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
 	}
 	bix->last_full_charg_capacity = ret;
 
-	/* get serial number */
+	/*
+	 * get serial number, on some devices (with unofficial replacement
+	 * battery?) reading any of the serial no range addresses gets nacked
+	 * in this case just leave the serial no empty.
+	 */
 	ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
 					    sizeof(buf), buf);
-	if (ret != sizeof(buf)) {
+	if (ret == -EREMOTEIO) {
+		/* no serial number available */
+	} else if (ret != sizeof(buf)) {
 		dev_err(&client->dev, "Error reading serial no: %d\n", ret);
 		return ret;
+	} else {
+		snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
 	}
-	snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
 
 	/* get cycle count */
 	ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
-- 
2.35.1

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Benjamin Tissoires]

On Thu, Feb 24, 2022 at 11:19 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The battery on the 2nd hand Surface 3 which I recently bought appears to
> not have a serial no programmed in. This results in any I2C reads from
> the registers containing the serial no failing with an I2C NACK.
>
> This was causing mshw0011_bix() to fail causing the battery readings to
> not work at all.
>
> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
> continue with an empty serial no to fix this.
>
> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

LGTM, (and scratching this off my list):
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> ---
>  drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> index abac3eec565e..b283bc9bb5fd 100644
> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>         }
>         bix->last_full_charg_capacity = ret;
>
> -       /* get serial number */
> +       /*
> +        * get serial number, on some devices (with unofficial replacement
> +        * battery?) reading any of the serial no range addresses gets nacked
> +        * in this case just leave the serial no empty.
> +        */
>         ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>                                             sizeof(buf), buf);
> -       if (ret != sizeof(buf)) {
> +       if (ret == -EREMOTEIO) {
> +               /* no serial number available */
> +       } else if (ret != sizeof(buf)) {
>                 dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>                 return ret;
> +       } else {
> +               snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>         }
> -       snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>
>         /* get cycle count */
>         ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
> --
> 2.35.1
>

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Andy Shevchenko]

On Thu, Feb 24, 2022 at 12:19 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The battery on the 2nd hand Surface 3 which I recently bought appears to
> not have a serial no programmed in. This results in any I2C reads from
> the registers containing the serial no failing with an I2C NACK.
>
> This was causing mshw0011_bix() to fail causing the battery readings to
> not work at all.
>
> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
> continue with an empty serial no to fix this.

Maybe in all cases

serial no --> Serial Number

? This "no" requires to re-read again.

>
> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> index abac3eec565e..b283bc9bb5fd 100644
> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>         }
>         bix->last_full_charg_capacity = ret;
>
> -       /* get serial number */
> +       /*
> +        * get serial number, on some devices (with unofficial replacement

Get

> +        * battery?) reading any of the serial no range addresses gets nacked
> +        * in this case just leave the serial no empty.
> +        */
>         ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>                                             sizeof(buf), buf);
> -       if (ret != sizeof(buf)) {
> +       if (ret == -EREMOTEIO) {
> +               /* no serial number available */
> +       } else if (ret != sizeof(buf)) {
>                 dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>                 return ret;
> +       } else {
> +               snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>         }
> -       snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>
>         /* get cycle count */
>         ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
> --
> 2.35.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Hans de Goede]

Hi,

On 2/24/22 11:55, Andy Shevchenko wrote:
> On Thu, Feb 24, 2022 at 12:19 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The battery on the 2nd hand Surface 3 which I recently bought appears to
>> not have a serial no programmed in. This results in any I2C reads from
>> the registers containing the serial no failing with an I2C NACK.
>>
>> This was causing mshw0011_bix() to fail causing the battery readings to
>> not work at all.
>>
>> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
>> continue with an empty serial no to fix this.
> 
> Maybe in all cases
> 
> serial no --> Serial Number
> 
> ? This "no" requires to re-read again.

Ack, will fix before merging.

>> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
>> index abac3eec565e..b283bc9bb5fd 100644
>> --- a/drivers/platform/surface/surface3_power.c
>> +++ b/drivers/platform/surface/surface3_power.c
>> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>>         }
>>         bix->last_full_charg_capacity = ret;
>>
>> -       /* get serial number */
>> +       /*
>> +        * get serial number, on some devices (with unofficial replacement
> 
> Get

Ack.

> 
>> +        * battery?) reading any of the serial no range addresses gets nacked
>> +        * in this case just leave the serial no empty.
>> +        */
>>         ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>>                                             sizeof(buf), buf);
>> -       if (ret != sizeof(buf)) {
>> +       if (ret == -EREMOTEIO) {
>> +               /* no serial number available */
>> +       } else if (ret != sizeof(buf)) {
>>                 dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>>                 return ret;
>> +       } else {
>> +               snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>         }
>> -       snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>
>>         /* get cycle count */
>>         ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
>> --
>> 2.35.1
>>
> 
> 

Regards,

Hans

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Hans de Goede]

Hi,

On 2/24/22 11:55, Benjamin Tissoires wrote:
> On Thu, Feb 24, 2022 at 11:19 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The battery on the 2nd hand Surface 3 which I recently bought appears to
>> not have a serial no programmed in. This results in any I2C reads from
>> the registers containing the serial no failing with an I2C NACK.
>>
>> This was causing mshw0011_bix() to fail causing the battery readings to
>> not work at all.
>>
>> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
>> continue with an empty serial no to fix this.
>>
>> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> LGTM, (and scratching this off my list):

Ah, so I guess this was already a known issue also hit by others ?

Do you have a buglink which I can add to the commit message ?


> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks.

Regards,

Hans


>> ---
>>  drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
>> index abac3eec565e..b283bc9bb5fd 100644
>> --- a/drivers/platform/surface/surface3_power.c
>> +++ b/drivers/platform/surface/surface3_power.c
>> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>>         }
>>         bix->last_full_charg_capacity = ret;
>>
>> -       /* get serial number */
>> +       /*
>> +        * get serial number, on some devices (with unofficial replacement
>> +        * battery?) reading any of the serial no range addresses gets nacked
>> +        * in this case just leave the serial no empty.
>> +        */
>>         ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>>                                             sizeof(buf), buf);
>> -       if (ret != sizeof(buf)) {
>> +       if (ret == -EREMOTEIO) {
>> +               /* no serial number available */
>> +       } else if (ret != sizeof(buf)) {
>>                 dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>>                 return ret;
>> +       } else {
>> +               snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>         }
>> -       snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>
>>         /* get cycle count */
>>         ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
>> --
>> 2.35.1
>>
> 

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Benjamin Tissoires]

On Thu, Feb 24, 2022 at 11:57 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/24/22 11:55, Benjamin Tissoires wrote:
> > On Thu, Feb 24, 2022 at 11:19 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> The battery on the 2nd hand Surface 3 which I recently bought appears to
> >> not have a serial no programmed in. This results in any I2C reads from
> >> the registers containing the serial no failing with an I2C NACK.
> >>
> >> This was causing mshw0011_bix() to fail causing the battery readings to
> >> not work at all.
> >>
> >> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
> >> continue with an empty serial no to fix this.
> >>
> >> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > LGTM, (and scratching this off my list):
>
> Ah, so I guess this was already a known issue also hit by others ?
>
> Do you have a buglink which I can add to the commit message ?

Oh, no, sorry. The email popped up, and instead of adding it at the
end of the TODO list, I spent a few minutes checking it and answering.
Sorry for the confusion.

The problem in itself is entirely new to me.

Cheers,
Benjamin

>
>
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Thanks.
>
> Regards,
>
> Hans
>
>
> >> ---
> >>  drivers/platform/surface/surface3_power.c | 13 ++++++++++---
> >>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> >> index abac3eec565e..b283bc9bb5fd 100644
> >> --- a/drivers/platform/surface/surface3_power.c
> >> +++ b/drivers/platform/surface/surface3_power.c
> >> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
> >>         }
> >>         bix->last_full_charg_capacity = ret;
> >>
> >> -       /* get serial number */
> >> +       /*
> >> +        * get serial number, on some devices (with unofficial replacement
> >> +        * battery?) reading any of the serial no range addresses gets nacked
> >> +        * in this case just leave the serial no empty.
> >> +        */
> >>         ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
> >>                                             sizeof(buf), buf);
> >> -       if (ret != sizeof(buf)) {
> >> +       if (ret == -EREMOTEIO) {
> >> +               /* no serial number available */
> >> +       } else if (ret != sizeof(buf)) {
> >>                 dev_err(&client->dev, "Error reading serial no: %d\n", ret);
> >>                 return ret;
> >> +       } else {
> >> +               snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
> >>         }
> >> -       snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
> >>
> >>         /* get cycle count */
> >>         ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
> >> --
> >> 2.35.1
> >>
> >
>

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Maximilian Luz]

On 2/24/22 11:18, Hans de Goede wrote:
> The battery on the 2nd hand Surface 3 which I recently bought appears to
> not have a serial no programmed in. This results in any I2C reads from
> the registers containing the serial no failing with an I2C NACK.
> 
> This was causing mshw0011_bix() to fail causing the battery readings to
> not work at all.
> 
> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
> continue with an empty serial no to fix this.
> 
> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks for fixing this! This has been bugging us over at
https://github.com/linux-surface/linux-surface/issues/608.

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

> ---
>   drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> index abac3eec565e..b283bc9bb5fd 100644
> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>   	}
>   	bix->last_full_charg_capacity = ret;
>   
> -	/* get serial number */
> +	/*
> +	 * get serial number, on some devices (with unofficial replacement
> +	 * battery?) reading any of the serial no range addresses gets nacked
> +	 * in this case just leave the serial no empty.
> +	 */
>   	ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>   					    sizeof(buf), buf);
> -	if (ret != sizeof(buf)) {
> +	if (ret == -EREMOTEIO) {
> +		/* no serial number available */
> +	} else if (ret != sizeof(buf)) {
>   		dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>   		return ret;
> +	} else {
> +		snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>   	}
> -	snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>   
>   	/* get cycle count */
>   	ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);

Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no

[Hans de Goede]

Hi,

On 2/24/22 13:35, Maximilian Luz wrote:
> On 2/24/22 11:18, Hans de Goede wrote:
>> The battery on the 2nd hand Surface 3 which I recently bought appears to
>> not have a serial no programmed in. This results in any I2C reads from
>> the registers containing the serial no failing with an I2C NACK.
>>
>> This was causing mshw0011_bix() to fail causing the battery readings to
>> not work at all.
>>
>> Ignore EREMOTEIO (I2C NACK) errors when retrieving the serial no and
>> continue with an empty serial no to fix this.
>>
>> Fixes: b1f81b496b0d ("platform/x86: surface3_power: MSHW0011 rev-eng implementation")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks for fixing this! This has been bugging us over at
> https://github.com/linux-surface/linux-surface/issues/608.
> 
> Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>

Thank you, I have merged this into the pdx86/review-hans and /fixes
branches now. I've also added a buglink to the commit message.

I'll also take a look at some of the other bugs from:

https://github.com/linux-surface/linux-surface/labels/D%3A%20Surface%203

when I can make some time for this.

Regards,

Hans


> 
>> ---
>>   drivers/platform/surface/surface3_power.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
>> index abac3eec565e..b283bc9bb5fd 100644
>> --- a/drivers/platform/surface/surface3_power.c
>> +++ b/drivers/platform/surface/surface3_power.c
>> @@ -232,14 +232,21 @@ static int mshw0011_bix(struct mshw0011_data *cdata, struct bix *bix)
>>       }
>>       bix->last_full_charg_capacity = ret;
>>   -    /* get serial number */
>> +    /*
>> +     * get serial number, on some devices (with unofficial replacement
>> +     * battery?) reading any of the serial no range addresses gets nacked
>> +     * in this case just leave the serial no empty.
>> +     */
>>       ret = i2c_smbus_read_i2c_block_data(client, MSHW0011_BAT0_REG_SERIAL_NO,
>>                           sizeof(buf), buf);
>> -    if (ret != sizeof(buf)) {
>> +    if (ret == -EREMOTEIO) {
>> +        /* no serial number available */
>> +    } else if (ret != sizeof(buf)) {
>>           dev_err(&client->dev, "Error reading serial no: %d\n", ret);
>>           return ret;
>> +    } else {
>> +        snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>       }
>> -    snprintf(bix->serial, ARRAY_SIZE(bix->serial), "%3pE%6pE", buf + 7, buf);
>>         /* get cycle count */
>>       ret = i2c_smbus_read_word_data(client, MSHW0011_BAT0_REG_CYCLE_CNT);
>