Message-ID: <CAMcHhXoKdXODc+4Bs-o2WXvxXiWpJHLBupnoqLyTa9m5KrNWbA@mail.gmail.com>
Date: Mon, 14 Oct 2024 02:16:43 +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

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