| Message ID | 20230214132052.1556699-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | power: supply: qcom_battmgr: remove bogus do_div() | expand |
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);
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);
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
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
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
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
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);