[PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The argument to do_div() is a 32-bit integer, and it was read from a
32-bit register so there is no point in doing a 64-bit division on it.

On 32-bit arm, do_div() causes a compile-time warning here:

include/asm-generic/div64.h:238:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
  238 |   __rem = __div64_32(&(n), __base); \
      |                      ^~~~
      |                      |
      |                      unsigned int *
drivers/power/supply/qcom_battmgr.c:1130:4: note: in expansion of macro 'do_div'
 1130 |    do_div(battmgr->status.percent, 100);

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/power/supply/qcom_battmgr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
index ec31f887184f..de77df97b3a4 100644
--- a/drivers/power/supply/qcom_battmgr.c
+++ b/drivers/power/supply/qcom_battmgr.c
@@ -1126,8 +1126,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr,
 			battmgr->info.charge_type = le32_to_cpu(resp->intval.value);
 			break;
 		case BATT_CAPACITY:
-			battmgr->status.percent = le32_to_cpu(resp->intval.value);
-			do_div(battmgr->status.percent, 100);
+			battmgr->status.percent = le32_to_cpu(resp->intval.value) / 100;
 			break;
 		case BATT_VOLT_OCV:
 			battmgr->status.voltage_ocv = le32_to_cpu(resp->intval.value);
-- 
2.39.1

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Konrad Dybcio]

On 14.02.2023 14:20, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The argument to do_div() is a 32-bit integer, and it was read from a
> 32-bit register so there is no point in doing a 64-bit division on it.
> 
> On 32-bit arm, do_div() causes a compile-time warning here:
> 
> include/asm-generic/div64.h:238:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
>   238 |   __rem = __div64_32(&(n), __base); \
>       |                      ^~~~
>       |                      |
>       |                      unsigned int *
> drivers/power/supply/qcom_battmgr.c:1130:4: note: in expansion of macro 'do_div'
>  1130 |    do_div(battmgr->status.percent, 100);
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/power/supply/qcom_battmgr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index ec31f887184f..de77df97b3a4 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -1126,8 +1126,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr,
>  			battmgr->info.charge_type = le32_to_cpu(resp->intval.value);
>  			break;
>  		case BATT_CAPACITY:
> -			battmgr->status.percent = le32_to_cpu(resp->intval.value);
> -			do_div(battmgr->status.percent, 100);
> +			battmgr->status.percent = le32_to_cpu(resp->intval.value) / 100;
>  			break;
>  		case BATT_VOLT_OCV:
>  			battmgr->status.voltage_ocv = le32_to_cpu(resp->intval.value);

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Sebastian Reichel]

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

Hi,

On Tue, Feb 14, 2023 at 02:36:03PM +0100, Konrad Dybcio wrote:
> On 14.02.2023 14:20, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The argument to do_div() is a 32-bit integer, and it was read from a
> > 32-bit register so there is no point in doing a 64-bit division on it.
> > 
> > On 32-bit arm, do_div() causes a compile-time warning here:
> > 
> > include/asm-generic/div64.h:238:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
> >   238 |   __rem = __div64_32(&(n), __base); \
> >       |                      ^~~~
> >       |                      |
> >       |                      unsigned int *
> > drivers/power/supply/qcom_battmgr.c:1130:4: note: in expansion of macro 'do_div'
> >  1130 |    do_div(battmgr->status.percent, 100);
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Needs to go through the Qualcomm tree:

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> >  drivers/power/supply/qcom_battmgr.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> > index ec31f887184f..de77df97b3a4 100644
> > --- a/drivers/power/supply/qcom_battmgr.c
> > +++ b/drivers/power/supply/qcom_battmgr.c
> > @@ -1126,8 +1126,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr,
> >  			battmgr->info.charge_type = le32_to_cpu(resp->intval.value);
> >  			break;
> >  		case BATT_CAPACITY:
> > -			battmgr->status.percent = le32_to_cpu(resp->intval.value);
> > -			do_div(battmgr->status.percent, 100);
> > +			battmgr->status.percent = le32_to_cpu(resp->intval.value) / 100;
> >  			break;
> >  		case BATT_VOLT_OCV:
> >  			battmgr->status.voltage_ocv = le32_to_cpu(resp->intval.value);

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

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Geert Uytterhoeven]

On Tue, Feb 14, 2023 at 2:23 PM Arnd Bergmann <arnd@kernel.org> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The argument to do_div() is a 32-bit integer, and it was read from a
> 32-bit register so there is no point in doing a 64-bit division on it.
>
> On 32-bit arm, do_div() causes a compile-time warning here:
>
> include/asm-generic/div64.h:238:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
>   238 |   __rem = __div64_32(&(n), __base); \
>       |                      ^~~~
>       |                      |
>       |                      unsigned int *
> drivers/power/supply/qcom_battmgr.c:1130:4: note: in expansion of macro 'do_div'
>  1130 |    do_div(battmgr->status.percent, 100);
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Nathan Chancellor]

Hi Linus,

On Tue, Feb 14, 2023 at 02:20:42PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The argument to do_div() is a 32-bit integer, and it was read from a
> 32-bit register so there is no point in doing a 64-bit division on it.
> 
> On 32-bit arm, do_div() causes a compile-time warning here:
> 
> include/asm-generic/div64.h:238:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
>   238 |   __rem = __div64_32(&(n), __base); \
>       |                      ^~~~
>       |                      |
>       |                      unsigned int *
> drivers/power/supply/qcom_battmgr.c:1130:4: note: in expansion of macro 'do_div'
>  1130 |    do_div(battmgr->status.percent, 100);
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/power/supply/qcom_battmgr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index ec31f887184f..de77df97b3a4 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -1126,8 +1126,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr,
>  			battmgr->info.charge_type = le32_to_cpu(resp->intval.value);
>  			break;
>  		case BATT_CAPACITY:
> -			battmgr->status.percent = le32_to_cpu(resp->intval.value);
> -			do_div(battmgr->status.percent, 100);
> +			battmgr->status.percent = le32_to_cpu(resp->intval.value) / 100;
>  			break;
>  		case BATT_VOLT_OCV:
>  			battmgr->status.voltage_ocv = le32_to_cpu(resp->intval.value);
> -- 
> 2.39.1
> 

Would you be able to take this patch directly? It seems obviously
correctTM, has an ack from Sebastian [1], and without it, 32-bit
allmodconfig builds are broken [2] (the other warning in that log has a
fix on the way to you soon).

[1]: https://lore.kernel.org/20230214220210.cpviycsmcppylkgj@mercury.elektranox.org/
[2]: https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2MPmxwvmQ7FdpiMhdQN2ZJhcoUP/build.log

Cheers,
Nathan

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Linus Torvalds]

On Wed, Mar 1, 2023 at 10:16 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Would you be able to take this patch directly? It seems obviously
> correctTM, has an ack from Sebastian [1], and without it, 32-bit
> allmodconfig builds are broken [2] (the other warning in that log has a
> fix on the way to you soon).

Ok, I've taken it directly.

However, the whole "seems obviously correct" is true in the sense that
it now doesn't complain (and doesn't overwrite an "int" with a 64-bit
value.

The actual code still looks odd. Is that return value for
BATT_CAPACITY truly in that odd "hundredths of percent" format, where
dividing it by 100 gives you whole percent?

Because "hundredths of percent" strikes me as a very odd interface.
Even for some firmware interface. I realize that anything is possible
with strange firmware, but still...

             Linus

Re: [PATCH] power: supply: qcom_battmgr: remove bogus do_div()

[Sebastian Reichel]

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

Hi,

On Wed, Mar 01, 2023 at 10:50:33AM -0800, Linus Torvalds wrote:
> On Wed, Mar 1, 2023 at 10:16 AM Nathan Chancellor <nathan@kernel.org> wrote:
> > Would you be able to take this patch directly? It seems obviously
> > correctTM, has an ack from Sebastian [1], and without it, 32-bit
> > allmodconfig builds are broken [2] (the other warning in that log has a
> > fix on the way to you soon).
> 
> Ok, I've taken it directly.
> 
> However, the whole "seems obviously correct" is true in the sense that
> it now doesn't complain (and doesn't overwrite an "int" with a 64-bit
> value.
> 
> The actual code still looks odd. Is that return value for
> BATT_CAPACITY truly in that odd "hundredths of percent" format,
> where dividing it by 100 gives you whole percent?
> 
> Because "hundredths of percent" strikes me as a very odd interface.
> Even for some firmware interface. I realize that anything is possible
> with strange firmware, but still...

I don't have the documentation for this Qualcomm interface, but I
remember somebody from Qualcomm asking for a power-supply property
exposing "hundredths of percent" to userspace some years ago (with
the rationale, that percent was not precise enough).

For reference: The upstream solution for that is exposing ENERGY_NOW
and ENERGY_FULL in µWh (or CHARGE_NOW + CHARGE_FULL in µAh depending
on the fuel-gauge's capabilities), which is even more precise.

-- Sebastian

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