[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: "proximity" attribute for input devices

[Jason Gerecke]

Apologies for the delay in replying.

First off: as an immediate action, I'm thinking of changing the Wacom
driver to do lazy creation of the power_supply device. This would mean
that rather than creating it at the same time as the input device, we
would wait until we receive some kind of affirmative indication of a
battery being present. Most Wacom peripherals report battery status in
a "heartbeat" report that is sent every few seconds, so there wouldn't
be much change for the typical user (just a few-second delay between
connecting the hardware and a power_supply device being created). In
the case of the "component" devices that are built into laptops and
other computers, however, the battery status is only reported while
the pen is actually in proximity. For users like you who don't own (or
never use) a pen, this means that our driver would never create a
power_supply device -- preventing GNOME from showing a battery that
isn't relevant. I'm working on a patch set that does this but need a
bit more time to finish it.

That's only a partial solution, however. If a component user were to
bring a pen into prox even just briefly, then a power_supply device
would be created and not go away until the user reboots. The pen's
battery status will become stale at some point in time though --
self-discharge, use of the pen on another device, and even just simple
irrelevance if the user hasn't used the pen in a while (do I care that
the pen which I left at home is at 50% battery?). I agree that it
makes sense for userspace to hide the battery after a while due to
things like this. Are there new signals that the kernel should be
providing to userspace (e.g. an attribute that indicates if we're in
communication with power_supply? an attribute signaling that data
might be stale)? Or are the signals that are already provided
sufficient (e.g. should GNOME just hide power_supply devices that are
in an "Unknown" state? or maybe hide power_supplies which haven't been
updated in more than 1 hour?)

I've added the power_supply list and maintainer for some additional
viewpoints on the second paragraph... If there are questions about how
the Wacom hardware and its battery reporting works, I'm happy to
provide answers...

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....



On Thu, Feb 23, 2023 at 7:39 AM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> [Public]
>
> Hello,
>
> A problem was found that a Wacom device's stylus battery showed up in GNOME even though the stylus wasn't near by.
> As part of discussing how to handle it Bastien proposes a new sysfs attribute "proximity" to let userspace know this.
>
> He asked[1] to start the discussion on Linux-input ML.
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=217062#c7
>
> Thanks,

Re: "proximity" attribute for input devices

[Sebastian Reichel]

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

Hi,

On Fri, Mar 03, 2023 at 03:05:41PM -0800, Jason Gerecke wrote:
> Apologies for the delay in replying.
> 
> First off: as an immediate action, I'm thinking of changing the Wacom
> driver to do lazy creation of the power_supply device. This would mean
> that rather than creating it at the same time as the input device, we
> would wait until we receive some kind of affirmative indication of a
> battery being present. Most Wacom peripherals report battery status in
> a "heartbeat" report that is sent every few seconds, so there wouldn't
> be much change for the typical user (just a few-second delay between
> connecting the hardware and a power_supply device being created). In
> the case of the "component" devices that are built into laptops and
> other computers, however, the battery status is only reported while
> the pen is actually in proximity. For users like you who don't own (or
> never use) a pen, this means that our driver would never create a
> power_supply device -- preventing GNOME from showing a battery that
> isn't relevant. I'm working on a patch set that does this but need a
> bit more time to finish it.
>
> That's only a partial solution, however. If a component user were to
> bring a pen into prox even just briefly, then a power_supply device
> would be created and not go away until the user reboots. The pen's
> battery status will become stale at some point in time though --
> self-discharge, use of the pen on another device, and even just simple
> irrelevance if the user hasn't used the pen in a while (do I care that
> the pen which I left at home is at 50% battery?). I agree that it
> makes sense for userspace to hide the battery after a while due to
> things like this. Are there new signals that the kernel should be
> providing to userspace (e.g. an attribute that indicates if we're in
> communication with power_supply? an attribute signaling that data
> might be stale)? Or are the signals that are already provided
> sufficient (e.g. should GNOME just hide power_supply devices that are
> in an "Unknown" state? or maybe hide power_supplies which haven't been
> updated in more than 1 hour?)

Can't you just unregister the power-supply device if there hasn't
been any heartbeat for some minutes and go back to the init state
(i.e. the one where you are waiting for a heartbeat to appear for
the first time)?

> I've added the power_supply list and maintainer for some additional
> viewpoints on the second paragraph... If there are questions about how
> the Wacom hardware and its battery reporting works, I'm happy to
> provide answers...

I think the problem you have is quite specific to the Wacom
hardware. Usually wireless devices are either connected or
disconnected and drivers unregister the battery device on
disconnect. So I think this logic belongs into Wacom specific
code and not in generic power-supply core code.

-- Sebastian

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

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 --]

RE: "proximity" attribute for input devices

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> Sent: Friday, March 3, 2023 19:09
> To: Jason Gerecke <killertofu@gmail.com>
> Cc: Limonciello, Mario <Mario.Limonciello@amd.com>; linux-
> input@vger.kernel.org; Bastien Nocera <hadess@hadess.net>;
> ping.cheng@wacom.com; jason.gerecke@wacom.com; linux-
> pm@vger.kernel.org
> Subject: Re: "proximity" attribute for input devices
> 
> Hi,
> 
> On Fri, Mar 03, 2023 at 03:05:41PM -0800, Jason Gerecke wrote:
> > Apologies for the delay in replying.
> >
> > First off: as an immediate action, I'm thinking of changing the Wacom
> > driver to do lazy creation of the power_supply device. This would mean
> > that rather than creating it at the same time as the input device, we
> > would wait until we receive some kind of affirmative indication of a
> > battery being present. Most Wacom peripherals report battery status in
> > a "heartbeat" report that is sent every few seconds, so there wouldn't
> > be much change for the typical user (just a few-second delay between
> > connecting the hardware and a power_supply device being created). In
> > the case of the "component" devices that are built into laptops and
> > other computers, however, the battery status is only reported while
> > the pen is actually in proximity. For users like you who don't own (or
> > never use) a pen, this means that our driver would never create a
> > power_supply device -- preventing GNOME from showing a battery that
> > isn't relevant. I'm working on a patch set that does this but need a
> > bit more time to finish it.
> >
> > That's only a partial solution, however. If a component user were to
> > bring a pen into prox even just briefly, then a power_supply device
> > would be created and not go away until the user reboots. The pen's
> > battery status will become stale at some point in time though --
> > self-discharge, use of the pen on another device, and even just simple
> > irrelevance if the user hasn't used the pen in a while (do I care that
> > the pen which I left at home is at 50% battery?). I agree that it
> > makes sense for userspace to hide the battery after a while due to
> > things like this. Are there new signals that the kernel should be
> > providing to userspace (e.g. an attribute that indicates if we're in
> > communication with power_supply? an attribute signaling that data
> > might be stale)? Or are the signals that are already provided
> > sufficient (e.g. should GNOME just hide power_supply devices that are
> > in an "Unknown" state? or maybe hide power_supplies which haven't
> been
> > updated in more than 1 hour?)
> 
> Can't you just unregister the power-supply device if there hasn't
> been any heartbeat for some minutes and go back to the init state
> (i.e. the one where you are waiting for a heartbeat to appear for
> the first time)?
> 
> > I've added the power_supply list and maintainer for some additional
> > viewpoints on the second paragraph... If there are questions about how
> > the Wacom hardware and its battery reporting works, I'm happy to
> > provide answers...
> 
> I think the problem you have is quite specific to the Wacom
> hardware. Usually wireless devices are either connected or
> disconnected and drivers unregister the battery device on
> disconnect. So I think this logic belongs into Wacom specific
> code and not in generic power-supply core code.

Conceivably can the input device come and go too when in and out of
proximity?  If so, you may be able to just tie the power supply creation
and destruction directly to the creation and destruction of that input
device.