Message-ID: <CAMcHhXojS-6J6HsJTBw5feKaM_wzi-tmVhNA+R4sdujYk32cTw@mail.gmail.com>
Date: Mon, 21 Oct 2024 14:07:54 +0200
From: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] HID: i2c-hid: introduce re-power-on quirk

Again, in plain text.
P.S. Ive noticed patches do not apply cleanly on the latest tree
anymore. Will resubmit v2 once I have the feedback.

On Mon, 14 Oct 2024 at 02:16, Aleksandrs Vinarskis
<alex.vinarskis@...il.com> wrote:
>
> On Wed, 25 Sept 2024 at 13:54, Benjamin Tissoires <bentiss@...nel.org> wrote:
> >
> > On Sep 25 2024, Aleksandrs Vinarskis wrote:
> > > It appears some keyboards from vendor 'QTEC' will not work properly until
> > > suspend & resume.
> > >
> > > Empirically narrowed down to solution of re-sending power on command
> > > _after_ initialization was completed before the end of initial probing.
> > >
> > > Signed-off-by: Aleksandrs Vinarskis <alex.vinarskis@...il.com>
> > > ---
> > >  drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > index 632eaf9e11a6..087ca2474176 100644
> > > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > > @@ -50,6 +50,7 @@
> > >  #define I2C_HID_QUIRK_BAD_INPUT_SIZE         BIT(3)
> > >  #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET  BIT(4)
> > >  #define I2C_HID_QUIRK_NO_SLEEP_ON_SUSPEND    BIT(5)
> > > +#define I2C_HID_QUIRK_RE_POWER_ON            BIT(6)
> > >
> > >  /* Command opcodes */
> > >  #define I2C_HID_OPCODE_RESET                 0x01
> > > @@ -1048,7 +1049,11 @@ static int i2c_hid_core_register_hid(struct i2c_hid *ihid)
> > >               return ret;
> > >       }
> > >
> > > -     return 0;
> > > +     /* At least some QTEC devices need this after initialization */
> > > +     if (ihid->quirks & I2C_HID_QUIRK_RE_POWER_ON)
> > > +             ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> >
> > I'd rather not have this in i2c-hid-core.c, TBH.
> >
> > We do have a nice split separation of i2c-hid which allows to add vendor
> > specific i2c-hid-of drivers. We currently have 2 (goodix and elan) and a
> > third wouldn't be much of an issue.
>
> Hi,
>
> Thanks for the input.
> I did some further digging, as I did not understand how to implement
> your suggestion right away, and in addition I think I am a bit short
> on data about this keyboard to create a dedicated driver. I am still
> not 100% sure how to proceed, so would like to share my findings
> first, perhaps you would have something else to add.
>
> Firstly, I am not quite sure what/who is the 'QTEC' manufacturer. I
> could not find it online by VID. In DSDT tables it's listed as
> 'QTEC0001', which sounds very generic. Only existing reference to QTEC
> that I could find in the kernel was in this [1] patch, where it
> appears to be a combo Elan touchpad+keyboard device, at least based on
> VID, though it was listed in ACPI as 'QTEC0001' as well. This is not
> the case with this device, as VID is a new, never seen before value.
> Which in turn means we could not use ACPI ID matching in case of a
> dedicated driver.
>
> For reference, XPS 9345 has also a somewhat combo solution - the
> keyboard has a separate touchbar-like Functions keys row. I opened up
> the device to inspect it - keyboard's IC is marked as ECE117, which
> appears to be a Microchip keyboard IC [2]. Touchbar is routed to the
> motherboard via a different connector, which may be routed back to the
> same IC via the keyboard's connector (based on the amount of wires in
> the keyboard-motherboard connector being way more than just
> sda/scl/gnd/3v3/5v), but I cannot be sure without in-detail electrical
> tests. This puzzles me a bit, as in addition, IC's datasheet refers to
> being connected to 'host EC' rather than just host - perhaps then
> otherwise, the onboard EC (present on this laptop, but no drivers
> available for linux at present) is acting like a bridge that is
> presented as this 'combo' device. Either way, neither of this explains
> what is actually from QTEC, and rather points it to be an embedded
> firmware from Dell, if I interpret my findings correctly, but please
> correct me if you think otherwise.
>
> Finally, during the BIOS update, one of the stages mentioned 'updating
> ELAN touchbar firmware' (not keyboard). Which confirms suspicion that
> the 'combo' device may be created by onboard EC, since any press of
> keyboard's usual or Function keys sends data from the same 'QTEC'
> keyboard as if it was one device, and it certainly does not identify
> as ELAN.
>
> >
> > I'm not really happy of this admittely simple solution in this patch
> > because:
> > - what if QTEC "fixes" that behavior in the future?
>
> That is a very valid point indeed. Especially with PID being rather
> useless, and ACPI ID apparently being shared with other devices, this
> may become an issue, as only VID stays unique - at least for now.
> However, I did not fully understand how making a dedicated driver is
> advantageous over a quirk, if we are limited by VID matching either
> way? Or did you mean to only have that keyboard selectable by dt via
> compatible?
>
> > - what if you actually need to enable/disable regulators like goodix and
> >   elan do
>
> At least at the moment it seems there is no need for that.
>
> >
> > So to me, a better solution would be to create a i2c-hid-of-qtec.c,
> > assign a new compatible for this keyboard, and try to fix up the initial
> > powerup in .power_up in that particular driver. This way, we can extend
>
> If I managed to narrow down the issue correctly, fixing the
> '.power_up' stage won't resolve the particular issue unfortunately. As
> per my original patch, re-running power on command has to be done
> _after_ device registration (which in turn is after power up phase).
> If I would be to move re-power up any earlier, eg, between power up
> and `i2c_hid_init_irq`, it would have no effect again, the keyboard
> won't work until suspend & resume. In other words it appears that the
> process of registering hid what 'breaks' the controller, and power-up
> command has to be resent only after it. This is also how I discovered
> the solution in the first place - suspending the laptop and resuming
> it magically 'fixed' the keyboard. Given that due to lack of
> schematics no regulators are defined in device tree at the moment, I
> deduced that it was software init that broke the keyboard, and
> pm_resume 'fixed' it, which then allowed me to narrow it down to the
> proposed patch. But again, please correct me if you think I
> interpreted it wrong.
>
> I thus tried to implement this quirk similarly to existing
> `I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET`, which is used for ELAN
> touchscreens and is present in this core file, and not in
> i2c-hid-of-elan.c. I do agree that making a dedicated i2c-hid-of-
> driver is cleaner, though I am not sure I understood full advantage of
> it in this context, and not sure it will actually solve a particular
> issue as its not the problem with power up itself. On the other hand,
> perhaps as you mentioned enabling/disabling regulators first would in
> turn fix this weird behaviour? Though sadly I have no way to test it,
> since only got a  using a dummy regulator for this keyboard...
>
> Would like to hear your thoughts,

Hi,

A kind reminder.
Would really like to see this land, as is or after appropriate changes
if still required.

Thanks,
Alex

> Thanks in advance,
> Alex
>
> [1] https://patchwork.kernel.org/project/linux-input/patch/20190415161108.16419-1-jeffrey.l.hugo@gmail.com/#22595417
> [2] https://ww1.microchip.com/downloads/en/DeviceDoc/00001860D.pdf
>
> > the driver for the regulators, and we can also fix this issue while being
> > sure we do not touch at anything else.
> >
> > Anyway, glad to see the bringup of the new arm based XPS-13 taking
> > shape!
> >
> > Cheers,
> > Benjamin
> >
> >
> > > +
> > > +     return ret;
> > >  }
> > >
> > >  static int i2c_hid_core_probe_panel_follower(struct i2c_hid *ihid)
> > > --
> > > 2.43.0
> > >

Powered by blists - more mailing lists