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