* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:18 [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no Hans de Goede
@ 2022-02-24 10:55 ` Benjamin Tissoires
2022-02-24 10:57 ` Hans de Goede
2022-02-24 10:55 ` Andy Shevchenko
2022-02-24 12:35 ` Maximilian Luz
2 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2022-02-24 10:55 UTC (permalink / raw)
To: Hans de Goede
Cc: Maximilian Luz, Mark Gross, Andy Shevchenko, Platform Driver
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:55 ` Benjamin Tissoires
@ 2022-02-24 10:57 ` Hans de Goede
2022-02-24 11:00 ` Benjamin Tissoires
0 siblings, 1 reply; 8+ messages in thread
From: Hans de Goede @ 2022-02-24 10:57 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Maximilian Luz, Mark Gross, Andy Shevchenko, Platform Driver
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
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:57 ` Hans de Goede
@ 2022-02-24 11:00 ` Benjamin Tissoires
0 siblings, 0 replies; 8+ messages in thread
From: Benjamin Tissoires @ 2022-02-24 11:00 UTC (permalink / raw)
To: Hans de Goede
Cc: Maximilian Luz, Mark Gross, Andy Shevchenko, Platform Driver
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
> >>
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:18 [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no Hans de Goede
2022-02-24 10:55 ` Benjamin Tissoires
@ 2022-02-24 10:55 ` Andy Shevchenko
2022-02-24 10:56 ` Hans de Goede
2022-02-24 12:35 ` Maximilian Luz
2 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2022-02-24 10:55 UTC (permalink / raw)
To: Hans de Goede
Cc: Benjamin Tissoires, Maximilian Luz, Mark Gross, Andy Shevchenko,
Platform Driver
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:55 ` Andy Shevchenko
@ 2022-02-24 10:56 ` Hans de Goede
0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2022-02-24 10:56 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Benjamin Tissoires, Maximilian Luz, Mark Gross, Andy Shevchenko,
Platform Driver
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 10:18 [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no Hans de Goede
2022-02-24 10:55 ` Benjamin Tissoires
2022-02-24 10:55 ` Andy Shevchenko
@ 2022-02-24 12:35 ` Maximilian Luz
2022-02-24 12:50 ` Hans de Goede
2 siblings, 1 reply; 8+ messages in thread
From: Maximilian Luz @ 2022-02-24 12:35 UTC (permalink / raw)
To: Hans de Goede, Benjamin Tissoires, Mark Gross, Andy Shevchenko
Cc: platform-driver-x86
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);
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] surface: surface3_power: Fix battery readings on batteries with a serial no
2022-02-24 12:35 ` Maximilian Luz
@ 2022-02-24 12:50 ` Hans de Goede
0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2022-02-24 12:50 UTC (permalink / raw)
To: Maximilian Luz, Benjamin Tissoires, Mark Gross, Andy Shevchenko
Cc: platform-driver-x86
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread