| Message ID | 20240925100303.9112-2-alex.vinarskis@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: i2c-hid: re-power-on quirk for QTEC kbrd | expand |
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@gmail.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. I'm not really happy of this admittely simple solution in this patch because: - what if QTEC "fixes" that behavior in the future? - what if you actually need to enable/disable regulators like goodix and elan do 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 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 >
On Wed, 25 Sept 2024 at 13:54, Benjamin Tissoires <bentiss@kernel.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@gmail.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 > >
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@gmail.com> wrote: > > On Wed, 25 Sept 2024 at 13:54, Benjamin Tissoires <bentiss@kernel.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@gmail.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 > > >
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); + + return ret; } static int i2c_hid_core_probe_panel_follower(struct i2c_hid *ihid)
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@gmail.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)