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@xxxxxxxxx> wrote: > > On Wed, 25 Sept 2024 at 13:54, Benjamin Tissoires <bentiss@xxxxxxxxxx> 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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxx/#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 > > >
