Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Jul 2021 18:02:15 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Bastien Nocera <hadess@hadess.net>, Jiri Kosina <jikos@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Kenneth Albanowski <kenalba@google.com>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] HID: input: do not report stylus battery state as "full"
Message-ID: <YN5llwVfb0abPJZU@google.com>
References: <YNtlrrKZVQY4byVa@google.com>
 <CAO-hwJJ-VyKBohETJabxmgjZ8RtmZHWWOBr2kZNC=feOxHgTtQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAO-hwJJ-VyKBohETJabxmgjZ8RtmZHWWOBr2kZNC=feOxHgTtQ@mail.gmail.com>
Precedence: bulk

Hi Benjamin,

On Wed, Jun 30, 2021 at 09:09:27AM +0200, Benjamin Tissoires wrote:
> Hi Dmitry,
> 
> On Tue, Jun 29, 2021 at 8:26 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > The power supply states of discharging, charging, full, etc, represent
> > state of charging, not the capacity level of the battery (for which
> > we have a separate property). Current HID usage tables to not allow
> > for expressing charging state of the batteries found in generic
> > styli, so we should simply assume that the battery is discharging
> > even if current capacity is at 100% when battery strength reporting
> > is done via HID interface. In fact, we were doing just that before
> > commit 581c4484769e.
> 
> This commit is 4 year old already, so I'd like to have the opinion of
> Bastien on the matter for the upower side (or at least notify him).
> 
> >
> > This change helps UIs to not mis-represent fully charged batteries in
> > styli as being charging/topping-off.
> >
> > Fixes: 581c4484769e ("HID: input: map digitizer battery usage")
> > Reported-by: Kenneth Albanowski <kenalba@google.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/hid/hid-input.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index e982d8173c9c..e85a1a34ff39 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -417,8 +417,6 @@ static int hidinput_get_battery_property(struct power_supply *psy,
> >
> >                 if (dev->battery_status == HID_BATTERY_UNKNOWN)
> >                         val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> 
> What's the point of keeping the HID_BATTERY_UNKNOWN code path? AFAICT,
> before 581c4484769e, we were just returning
> POWER_SUPPLY_STATUS_DISCHARGING. If we don't need to check for the
> capacity, I think we could also drop the hunk just before where we do
> the query of the capacity.

I believe it is beneficial to keep this check: prior to 581c4484769e we
were only handling batteries reported via generic device control -
HID_DC_BATTERYSTRENGTH - essentially UPS batteries that normally can be
queried at will. Stylus batteries are typically only reported when
stylus is in contact with the digitzer, so until user actually engages
stylus we do not have idea about its level/capacity. For this reason I
think we should keep reporting POWER_SUPPLY_STATUS_UNKNOWN.

Thanks.

-- 
Dmitry