* Re: [RESEND PATCH v6 05/11] mfd: core: document mfd_add_devices()
From: Pavel Machek @ 2019-03-22 9:10 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190318174228.18194-6-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 412 bytes --]
On Mon 2019-03-18 18:42:22, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add a kernel doc for mfd_add_devices().
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v6 04/11] dt-bindings: input: add DT bindings for max77650
From: Pavel Machek @ 2019-03-22 9:09 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski,
Rob Herring
In-Reply-To: <20190318174228.18194-5-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 762 bytes --]
On Mon 2019-03-18 18:42:21, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the onkey module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> + onkey {
> + compatible = "maxim,max77650-onkey";
> + linux,code = <KEY_END>;
> + maxim,onkey-slide;
> + };
This is certainly wrong.
"End" key is normal key on a keyboard, certainly not some kind of
slider.
And this controller is likely to be used for power key, not end key,
right?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v6 03/11] dt-bindings: leds: add DT bindings for max77650
From: Pavel Machek @ 2019-03-22 9:02 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski,
Rob Herring
In-Reply-To: <20190318174040.17899-4-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 651 bytes --]
On Mon 2019-03-18 18:40:32, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the LEDs module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
> + led@0 {
> + reg = <0>;
> + label = "blue:usr0";
> + };
I'd rather not see these "usrX" things... Does it have some kind of
real function?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v6 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Pavel Machek @ 2019-03-22 9:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190318174040.17899-3-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 1893 bytes --]
On Mon 2019-03-18 18:40:31, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the battery charger module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> .../power/supply/max77650-charger.txt | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
>
> diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> new file mode 100644
> index 000000000000..d25c95369616
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> @@ -0,0 +1,27 @@
> +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> +
> +This module is part of the MAX77650 MFD device. For more details
> +see Documentation/devicetree/bindings/mfd/max77650.txt.
> +
> +The charger is represented as a sub-node of the PMIC node on the device tree.
> +
> +Required properties:
> +--------------------
> +- compatible: Must be "maxim,max77650-charger"
> +
> +Optional properties:
> +--------------------
> +- min-microvolt: Minimum CHGIN regulation voltage (in microvolts). Must be
> + one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> + 4500000, 4600000, 4700000.
Probably needs "max," prefix. And .. what does this mean? Will charger
shutdown if input is less than this?
> +- curr-lim-microamp: CHGIN input current limit (in microamps). Must be one of:
> + 95000, 190000, 285000, 380000, 475000.
"current-limit-microamp", I guess. And probably "max,current-limit-microamp".
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v6 01/11] dt-bindings: mfd: add DT bindings for max77650
From: Pavel Machek @ 2019-03-22 8:57 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski,
Rob Herring
In-Reply-To: <20190318174040.17899-2-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
On Mon 2019-03-18 18:40:30, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add a DT binding document for max77650 ultra-low power PMIC. This
> describes the core mfd device and the GPIO module.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: Re: [PATCH V2 2/2] Input: rotaty-encoder - Add DT binding document
From: Alexey Slepov @ 2019-03-22 1:04 UTC (permalink / raw)
To: Rob Herring, Dmitry Torokhov
Cc: Donghoon Han, linux-input, Daniel Mack, linux-kernel, devicetree
In-Reply-To: <20190115202948.GA26482@bogus>
Hello,
i used this rotary-encoder patch in my embedded project and found two
errors:
First, in drivers/input/misc/rotary_encoder.c,
at @@ -237,6 +244,16 @@:
instead of
+ if (err)
+ dev_err(dev, "unable to get keycodes: %d\n", err);
+ return err;
it must be
+ if (err) {
+ dev_err(dev, "unable to get keycodes: %d\n", err);
+ return err;
+ }
otherwise successful creation of device is not possible.
Second, a typo in
Documentation/devicetree/bindings/input/rotary-encoder.txt,
at @@ -48,3 +52,11 @@:
instead of
+ rotary-encoder,relative-keycode = <103>, <108>;
it should be
+ rotary-encoder,relative-keycodes = <103>, <108>;
otherwise keycodes are not found.
I am sorry, I know that E-Mail style is not good.
I have no time right now, but I'll be back in two weeks.
Someone, maybe Mr. Han, could submit a new version of the patch.
If not, I'll try to do it on my return. (it could take some time, since
I am new to patchwork)
Best Regards and thanks
Alexey Slepov
^ permalink raw reply
* Re: [PATCH 2/7] [media] doc-rst: switch to new names for Full Screen/Aspect keys
From: Mauro Carvalho Chehab @ 2019-03-21 12:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
linux-media
In-Reply-To: <20190218072606.GG242714@dtor-ws>
Em Sun, 17 Feb 2019 23:26:06 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
> On Fri, Jan 18, 2019 at 03:30:32PM -0800, Dmitry Torokhov wrote:
> > We defined better names for keys to activate full screen mode or
> > change aspect ratio (while keeping the existing keycodes to avoid
> > breaking userspace), so let's use them in the document.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > Documentation/media/uapi/rc/rc-tables.rst | 4 ++--
>
> Mauro, do you want to take this through your tree or I should pick it up
> with the patch that does renames in uapi header?
Feel free to apply it via your tree. It probably makes sense to keep it
with the series that add the new codes.
Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
>
> Thanks!
>
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/media/uapi/rc/rc-tables.rst b/Documentation/media/uapi/rc/rc-tables.rst
> > index c8ae9479f842..57797e56f45e 100644
> > --- a/Documentation/media/uapi/rc/rc-tables.rst
> > +++ b/Documentation/media/uapi/rc/rc-tables.rst
> > @@ -616,7 +616,7 @@ the remote via /dev/input/event devices.
> >
> > - .. row 78
> >
> > - - ``KEY_SCREEN``
> > + - ``KEY_ASPECT_RATIO``
> >
> > - Select screen aspect ratio
> >
> > @@ -624,7 +624,7 @@ the remote via /dev/input/event devices.
> >
> > - .. row 79
> >
> > - - ``KEY_ZOOM``
> > + - ``KEY_FULL_SCREEN``
> >
> > - Put device into zoom/full screen mode
> >
> > --
> > 2.20.1.321.g9e740568ce-goog
> >
>
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH 1/7] Input: document meanings of KEY_SCREEN and KEY_ZOOM
From: Mauro Carvalho Chehab @ 2019-03-21 12:17 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
linux-media
In-Reply-To: <20190118233037.87318-1-dmitry.torokhov@gmail.com>
Em Fri, 18 Jan 2019 15:30:31 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
> It is hard to say what KEY_SCREEN and KEY_ZOOM mean, but historically DVB
> folks have used them to indicate switch to full screen mode. Later, they
> converged on using KEY_ZOOM to switch into full screen mode and KEY)SCREEN
> to control aspect ratio (see Documentation/media/uapi/rc/rc-tables.rst).
>
> Let's commit to these uses, and define:
>
> - KEY_FULL_SCREEN (and make KEY_ZOOM its alias)
> - KEY_ASPECT_RATIO (and make KEY_SCREEN its alias)
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Feel free to apply via your tree.
Acked-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>
> Please let me know how we want merge this. Some of patches can be applied
> independently and I tried marking them as such, but some require new key
> names from input.h
>
> include/uapi/linux/input-event-codes.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index ae366b87426a..bc5054e51bef 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -439,10 +439,12 @@
> #define KEY_TITLE 0x171
> #define KEY_SUBTITLE 0x172
> #define KEY_ANGLE 0x173
> -#define KEY_ZOOM 0x174
> +#define KEY_FULL_SCREEN 0x174 /* AC View Toggle */
> +#define KEY_ZOOM KEY_FULL_SCREEN
> #define KEY_MODE 0x175
> #define KEY_KEYBOARD 0x176
> -#define KEY_SCREEN 0x177
> +#define KEY_ASPECT_RATIO 0x177 /* HUTRR37: Aspect */
> +#define KEY_SCREEN KEY_ASPECT_RATIO
> #define KEY_PC 0x178 /* Media Select Computer */
> #define KEY_TV 0x179 /* Media Select TV */
> #define KEY_TV2 0x17a /* Media Select Cable */
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-21 9:48 UTC (permalink / raw)
To: Kai-Heng Feng, Mario Limonciello
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <220FEDBD-9A71-4C14-A7D6-3850D515865F@canonical.com>
+Cc: Mario
Mario, do you have any insights about the issue with touchpad on Dell
system described below?
On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >
> >>> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
> >>> is also used on Elan devices, I suspect that these Elan devices
> >>> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
> >>> and then they probably will no longer need the bogus IRQ flag,
> >>> if you know about bugreports with an acpidump for any of the devices
> >>> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
> >>> declared there, I suspect it will be declared as level-low, just like
> >>> with the laptop this patch was written for. And it probably need to
> >>> be edge-falling instead of level-low just like this case.
> >>
> >> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> >> doesn’t solve the issue for me.
> >>
> >> I talked to Elan once, and they confirm the correct IRQ trigger is level
> >> low. So forcing falling trigger may break other platforms.
> >
> > As far as I understood Vladislav the quirk he got from Elan as well.
>
> Ok, then this is really weird.
>
> >
> >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> >> Once the Interrupt() is used instead, the issue goes away.
> >
> > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > then falls back to GpioInt().
> > See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
>
> Scope (\_SB.PCI0.I2C4)
> {
> Device (TPD0)
> {
> Name (_ADR, One) // _ADR: Address
> Name (_HID, "DELL08AE") // _HID: Hardware ID
> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
> Name (_UID, One) // _UID: Unique ID
> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
> Method (_INI, 0, NotSerialized) // _INI: Initialize
> {
> }
> Method (_STA, 0, NotSerialized) // _STA: Status
> {
> If ((TCPD == One))
> {
> Return (0x0F)
> }
>
> Return (Zero)
> }
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> If ((OSYS < 0x07DC))
> {
> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> }
>
> Return (ConcatenateResTemplate (SBFB, SBFG))
> }
> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> {
> If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID I2C Device */))
> {
> If ((Arg2 == Zero))
> {
> If ((Arg1 == One))
> {
> Return (Buffer (One)
> {
> 0x03 // .
> })
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> ElseIf ((Arg2 == One))
> {
> Return (0x20)
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
> {
> If ((Arg2 == Zero))
> {
> If ((Arg1 == One))
> {
> Return (Buffer (One)
> {
> 0x03 // .
> })
> }
> }
>
> If ((Arg2 == One))
> {
> Return (ConcatenateResTemplate (SBFB, SBFG))
> }
>
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> Else
> {
> Return (Buffer (One)
> {
> 0x00 // .
> })
> }
> }
> }
> }
>
> Change SBFG to SBFI in its _CRS can workaround the issue.
> Is ASL in this form possible to do the flow you described?
>
> Kai-Heng
>
> >
> >> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Kai Heng Feng @ 2019-03-21 9:28 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <CAHp75VeavkzZDYK4CWAC62GciynQ3KYgmPpPRwsd3RcLFw4jxA@mail.gmail.com>
> On Mar 21, 2019, at 4:55 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>>> <kai.heng.feng@canonical.com> wrote:
>>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
>>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>>>> Once the Interrupt() is used instead, the issue goes away.
>>>
>>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>>> then falls back to GpioInt().
>>> See i2c_acpi_get_info() and i2c_device_probe().
>>
>> Here’s its ASL:
>
>> Name (SBFB, ResourceTemplate ()
>> {
>> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
>> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
>> 0x00, ResourceConsumer, , Exclusive,
>> )
>> })
>> Name (SBFG, ResourceTemplate ()
>> {
>> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
>> )
>> { // Pin list
>> 0x0012
>> }
>> })
>> Name (SBFI, ResourceTemplate ()
>> {
>> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
>> {
>> 0x0000003C,
>> }
>> })
>
>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
>> {
>
>> If ((OSYS < 0x07DC))
>> {
>> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
>> }
>
> This will return only Interrupt() resource
>
>>
>> Return (ConcatenateResTemplate (SBFB, SBFG))
>
> This one I2cSerialBus() and GpioInt().
>
>> }
>
>
>> }
>> }
>>
>> Change SBFG to SBFI in its _CRS can workaround the issue.
>> Is ASL in this form possible to do the flow you described?
>
> Since it's enumerated in Linux as I2C device, it means it gets I2C and
> GPIO resources.
> So, no, it's not possible.
>
> What are you describing might tell us about one of the following:
> - touchpad should be switched to PS/2 mode in order to get working
> - GPIO resource is not correct / bug in GPIO driver
>
> I don't believe the first one is a case here.
> If GPIO resource is not correct and main OS has some quirks, we need
> to do similar in Linux.
How do I check quirks on Windows?
> Otherwise, debugging GPIO driver, starting from what exactly (pin
> number, pin settings, etc) gets i2c-hid module.
Ok, please let me know where should I start looking into.
>
> Note, ACPICA and related stuff is done in order to be Windows compatible.
> If you have settings in BIOS that defines OS to boot, it should be
> chosen Windows.
Yes, it’s Windows.
Kai-Heng
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Hans de Goede @ 2019-03-21 8:57 UTC (permalink / raw)
To: Kai-Heng Feng, Andy Shevchenko
Cc: Benjamin Tissoires, hotwater438, Jiri Kosina, Stephen Boyd,
Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <220FEDBD-9A71-4C14-A7D6-3850D515865F@canonical.com>
Hi,
On 21-03-19 05:08, Kai-Heng Feng wrote:
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
>> <kai.heng.feng@canonical.com> wrote:
>>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>>
>>>> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
>>>> is also used on Elan devices, I suspect that these Elan devices
>>>> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
>>>> and then they probably will no longer need the bogus IRQ flag,
>>>> if you know about bugreports with an acpidump for any of the devices
>>>> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
>>>> declared there, I suspect it will be declared as level-low, just like
>>>> with the laptop this patch was written for. And it probably need to
>>>> be edge-falling instead of level-low just like this case.
>>>
>>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>>> doesn’t solve the issue for me.
>>>
>>> I talked to Elan once, and they confirm the correct IRQ trigger is level
>>> low. So forcing falling trigger may break other platforms.
>>
>> As far as I understood Vladislav the quirk he got from Elan as well.
>
> Ok, then this is really weird.
>
>>
>>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>>> Once the Interrupt() is used instead, the issue goes away.
>>
>> IIRC i2c core tries to get interrupt from Interrupt() resource and
>> then falls back to GpioInt().
>> See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
>
> Scope (\_SB.PCI0.I2C4)
> {
> Device (TPD0)
> {
> Name (_ADR, One) // _ADR: Address
> Name (_HID, "DELL08AE") // _HID: Hardware ID
> Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
> Name (_UID, One) // _UID: Unique ID
> Name (_S0W, 0x04) // _S0W: S0 Device Wake State
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
OK, so both interrupt definitions declare the interrupt as Level, ActiveLow, so forcing
falling-edge here *might* help too.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-21 8:55 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <220FEDBD-9A71-4C14-A7D6-3850D515865F@canonical.com>
On Thu, Mar 21, 2019 at 6:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> > <kai.heng.feng@canonical.com> wrote:
> >> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> >>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> >> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> >> Once the Interrupt() is used instead, the issue goes away.
> >
> > IIRC i2c core tries to get interrupt from Interrupt() resource and
> > then falls back to GpioInt().
> > See i2c_acpi_get_info() and i2c_device_probe().
>
> Here’s its ASL:
> Name (SBFB, ResourceTemplate ()
> {
> I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
> AddressingMode7Bit, "\\_SB.PCI0.I2C4",
> 0x00, ResourceConsumer, , Exclusive,
> )
> })
> Name (SBFG, ResourceTemplate ()
> {
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
> "\\_SB.GPO1", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0012
> }
> })
> Name (SBFI, ResourceTemplate ()
> {
> Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
> {
> 0x0000003C,
> }
> })
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> If ((OSYS < 0x07DC))
> {
> Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
> }
This will return only Interrupt() resource
>
> Return (ConcatenateResTemplate (SBFB, SBFG))
This one I2cSerialBus() and GpioInt().
> }
> }
> }
>
> Change SBFG to SBFI in its _CRS can workaround the issue.
> Is ASL in this form possible to do the flow you described?
Since it's enumerated in Linux as I2C device, it means it gets I2C and
GPIO resources.
So, no, it's not possible.
What are you describing might tell us about one of the following:
- touchpad should be switched to PS/2 mode in order to get working
- GPIO resource is not correct / bug in GPIO driver
I don't believe the first one is a case here.
If GPIO resource is not correct and main OS has some quirks, we need
to do similar in Linux.
Otherwise, debugging GPIO driver, starting from what exactly (pin
number, pin settings, etc) gets i2c-hid module.
Note, ACPICA and related stuff is done in order to be Windows compatible.
If you have settings in BIOS that defines OS to boot, it should be
chosen Windows.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v7 4/4] Input: goodix - Add GT5663 CTP support
From: Jagan Teki @ 2019-03-21 8:21 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190321082104.2874-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures.
Add support for it by adding compatible and supported chip data.
The chip data on GT5663 is similar to GT1151, like
- config data register has 0x8050 address
- config data register max len is 240
- config data checksum has 16-bit
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index de5b80a08f41..c558b091749c 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -219,6 +219,7 @@ static const struct goodix_chip_data *goodix_get_chip_data(u16 id)
{
switch (id) {
case 1151:
+ case 5663:
case 5688:
return >1x_chip_data;
@@ -1003,6 +1004,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
#ifdef CONFIG_OF
static const struct of_device_id goodix_of_match[] = {
{ .compatible = "goodix,gt1151" },
+ { .compatible = "goodix,gt5663" },
{ .compatible = "goodix,gt5688" },
{ .compatible = "goodix,gt911" },
{ .compatible = "goodix,gt9110" },
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v7 3/4] dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
From: Jagan Teki @ 2019-03-21 8:21 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190321082104.2874-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures, it support chipdata which is similar to
existing GT1151 and require AVDD28 supply for some boards.
Document the compatible for the same.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 255673250bbd..fc03ea4cf5ab 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series touchscreen controller
Required properties:
- compatible : Should be "goodix,gt1151"
+ or "goodix,gt5663"
or "goodix,gt5688"
or "goodix,gt911"
or "goodix,gt9110"
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v7 2/4] Input: goodix - Add regulators suppot
From: Jagan Teki @ 2019-03-21 8:21 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190321082104.2874-1-jagan@amarulasolutions.com>
Goodix CTP controllers require AVDD28, VDDIO regulators for power-on
sequence.
The delay between these regualtor operations as per Power-on Timing
from datasheet[1] is 0 (T1 >= 0 usec).
So, enable and disable these regulators in proper order using normal
regulator functions without any delay in between.
[1] GT5663 Datasheet_English_20151106_Rev.01
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 58 ++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f57d82220a88..de5b80a08f41 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/of.h>
@@ -47,6 +48,8 @@ struct goodix_ts_data {
struct touchscreen_properties prop;
unsigned int max_touch_num;
unsigned int int_trigger_type;
+ struct regulator *avdd28;
+ struct regulator *vddio;
struct gpio_desc *gpiod_int;
struct gpio_desc *gpiod_rst;
u16 id;
@@ -532,6 +535,24 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
return -EINVAL;
dev = &ts->client->dev;
+ ts->avdd28 = devm_regulator_get(dev, "AVDD28");
+ if (IS_ERR(ts->avdd28)) {
+ error = PTR_ERR(ts->avdd28);
+ if (error != -EPROBE_DEFER)
+ dev_err(dev,
+ "Failed to get AVDD28 regulator: %d\n", error);
+ return error;
+ }
+
+ ts->vddio = devm_regulator_get(dev, "VDDIO");
+ if (IS_ERR(ts->vddio)) {
+ error = PTR_ERR(ts->vddio);
+ if (error != -EPROBE_DEFER)
+ dev_err(dev,
+ "Failed to get VDDIO regulator: %d\n", error);
+ return error;
+ }
+
/* Get the interrupt GPIO pin number */
gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
if (IS_ERR(gpiod)) {
@@ -764,6 +785,17 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
complete_all(&ts->firmware_loading_complete);
}
+static void goodix_disable_regulator(void *arg)
+{
+ struct goodix_ts_data *ts = arg;
+
+ if (!IS_ERR(ts->vddio))
+ regulator_disable(ts->vddio);
+
+ if (!IS_ERR(ts->avdd28))
+ regulator_disable(ts->avdd28);
+}
+
static int goodix_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -789,6 +821,32 @@ static int goodix_ts_probe(struct i2c_client *client,
if (error)
return error;
+ error = devm_add_action_or_reset(&client->dev,
+ goodix_disable_regulator, ts);
+ if (error)
+ return error;
+
+ /* power the controller */
+ if (!IS_ERR(ts->avdd28)) {
+ error = regulator_enable(ts->avdd28);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to enable AVDD28 regulator: %d\n",
+ error);
+ return error;
+ }
+ }
+
+ if (!IS_ERR(ts->vddio)) {
+ error = regulator_enable(ts->vddio);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to enable VDDIO regulator: %d\n",
+ error);
+ return error;
+ }
+ }
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* reset the controller */
error = goodix_reset(ts);
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v7 1/4] dt-bindings: input: touchscreen: goodix: Document regulator properties
From: Jagan Teki @ 2019-03-21 8:21 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190321082104.2874-1-jagan@amarulasolutions.com>
Goodix CTP controllers support analog, digital and gpio regulator
supplies on relevant controller pin configurations.
Out of which AVDD28 and VDDIO regulators are required in few goodix CTP
chips during power-on sequence.
AVDD22, DVDD12 regulators have no relevant functionality described from
datasheet [1].
So, document both AVDD28, VDDIO regulators into optional properties since
few of the goodix chip do work without these regulator power-on sequence.
[1] GT5663 Datasheet_English_20151106_Rev.01
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8cf0b4d38a7e..255673250bbd 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -19,6 +19,8 @@ Optional properties:
- irq-gpios : GPIO pin used for IRQ. The driver uses the
interrupt gpio pin as output to reset the device.
- reset-gpios : GPIO pin used for reset
+ - AVDD28-supply : Analog power supply regulator on AVDD28 pin
+ - VDDIO-supply : GPIO power supply regulator on VDDIO pin
- touchscreen-inverted-x
- touchscreen-inverted-y
- touchscreen-size-x
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v7 0/4] input: touchscreen: Add goodix GT5553 CTP support
From: Jagan Teki @ 2019-03-21 8:21 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
This is v7 patchset for supporting goodix GT5553 CTP. Here is the
previous version[1]
Changes for v5:
- rebase on linux-next
Changes for v5:
- document bindings for required regulators, which are need during
power-on sequence
- enable, disable required regulators as described in power-on sequence
using normal regulator calls
- update the proper commi messages
Changes for v4:
- document AVDD22, DVDD12, VDDIO as optional properties
- use regulator bulk calls, for get, enable and disable functionalities
Changes for v4:
- devm_add_action_or_reset for disabling regulator
Changes for v3:
- add cover-letter
- s/ADVV28/AVDD28 on commit head
- fix few typo
Changes for v2:
- Rename vcc-supply with AVDD28-supply
- disable regulator in remove
- fix to setup regulator in probe code
- add chipdata
- drop example node in dt-bindings
[1] https://patchwork.kernel.org/cover/10819645/
Jagan Teki (4):
dt-bindings: input: touchscreen: goodix: Document regulator properties
Input: goodix - Add regulators suppot
dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
Input: goodix - Add GT5663 CTP support
.../bindings/input/touchscreen/goodix.txt | 3 +
drivers/input/touchscreen/goodix.c | 60 +++++++++++++++++++
2 files changed, 63 insertions(+)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Kai-Heng Feng @ 2019-03-21 4:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <CAHp75VcOG9H8u+tZyNN682r7+jsRZi8VoPPZ2uJPch1gNyFgmQ@mail.gmail.com>
at 01:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
>>> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
>>> is also used on Elan devices, I suspect that these Elan devices
>>> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
>>> and then they probably will no longer need the bogus IRQ flag,
>>> if you know about bugreports with an acpidump for any of the devices
>>> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
>>> declared there, I suspect it will be declared as level-low, just like
>>> with the laptop this patch was written for. And it probably need to
>>> be edge-falling instead of level-low just like this case.
>>
>> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
>> doesn’t solve the issue for me.
>>
>> I talked to Elan once, and they confirm the correct IRQ trigger is level
>> low. So forcing falling trigger may break other platforms.
>
> As far as I understood Vladislav the quirk he got from Elan as well.
Ok, then this is really weird.
>
>> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
>> Once the Interrupt() is used instead, the issue goes away.
>
> IIRC i2c core tries to get interrupt from Interrupt() resource and
> then falls back to GpioInt().
> See i2c_acpi_get_info() and i2c_device_probe().
Here’s its ASL:
Scope (\_SB.PCI0.I2C4)
{
Device (TPD0)
{
Name (_ADR, One) // _ADR: Address
Name (_HID, "DELL08AE") // _HID: Hardware ID
Name (_CID, "PNP0C50" /* HID Protocol Device (I2C bus) */) // _CID: Compatible ID
Name (_UID, One) // _UID: Unique ID
Name (_S0W, 0x04) // _S0W: S0 Device Wake State
Name (SBFB, ResourceTemplate ()
{
I2cSerialBusV2 (0x002C, ControllerInitiated, 0x00061A80,
AddressingMode7Bit, "\\_SB.PCI0.I2C4",
0x00, ResourceConsumer, , Exclusive,
)
})
Name (SBFG, ResourceTemplate ()
{
GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
"\\_SB.GPO1", 0x00, ResourceConsumer, ,
)
{ // Pin list
0x0012
}
})
Name (SBFI, ResourceTemplate ()
{
Interrupt (ResourceConsumer, Level, ActiveLow, ExclusiveAndWake, ,, )
{
0x0000003C,
}
})
Method (_INI, 0, NotSerialized) // _INI: Initialize
{
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
If ((TCPD == One))
{
Return (0x0F)
}
Return (Zero)
}
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
If ((OSYS < 0x07DC))
{
Return (SBFI) /* \_SB_.PCI0.I2C4.TPD0.SBFI */
}
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
{
If ((Arg0 == ToUUID ("3cdff6f7-4267-4555-ad05-b30a3d8938de") /* HID I2C Device */))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg2 == One))
{
Return (0x20)
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
ElseIf ((Arg0 == ToUUID ("ef87eb82-f951-46da-84ec-14871ac6f84b")))
{
If ((Arg2 == Zero))
{
If ((Arg1 == One))
{
Return (Buffer (One)
{
0x03 // .
})
}
}
If ((Arg2 == One))
{
Return (ConcatenateResTemplate (SBFB, SBFG))
}
Return (Buffer (One)
{
0x00 // .
})
}
}
Else
{
Return (Buffer (One)
{
0x00 // .
})
}
}
}
}
Change SBFG to SBFI in its _CRS can workaround the issue.
Is ASL in this form possible to do the flow you described?
Kai-Heng
>
>> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-20 17:18 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Hans de Goede, Benjamin Tissoires, hotwater438, Jiri Kosina,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <F03846A5-6C42-4290-AF72-F644C809A50A@canonical.com>
On Wed, Mar 20, 2019 at 6:55 PM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
> > Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
> > is also used on Elan devices, I suspect that these Elan devices
> > likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
> > and then they probably will no longer need the bogus IRQ flag,
> > if you know about bugreports with an acpidump for any of the devices
> > needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
> > declared there, I suspect it will be declared as level-low, just like
> > with the laptop this patch was written for. And it probably need to
> > be edge-falling instead of level-low just like this case.
>
> First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
> doesn’t solve the issue for me.
>
> I talked to Elan once, and they confirm the correct IRQ trigger is level
> low. So forcing falling trigger may break other platforms.
As far as I understood Vladislav the quirk he got from Elan as well.
> Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
> Once the Interrupt() is used instead, the issue goes away.
IIRC i2c core tries to get interrupt from Interrupt() resource and
then falls back to GpioInt().
See i2c_acpi_get_info() and i2c_device_probe().
> But I am not sure how to patch its DSDT/SSDT in i2c-hid.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Andy Shevchenko @ 2019-03-20 17:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: hotwater438, Jiri Kosina, Hans de Goede, Kai Heng Feng,
Stephen Boyd, Sebastian Andrzej Siewior, Dmitry Torokhov,
open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJLGABHATo-fOc_jgc8uNeLDv7cemsHuQMj7zRJWmfyp4g@mail.gmail.com>
On Wed, Mar 20, 2019 at 4:38 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
> > Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
>
> Don't take it wrong, but that's not how we give credit in a patch.
> In your case, if you do not have any Elantech engineer who agreed to
> sign off the patch, you can definitively mention them this way.
> But the other guys (Hans, Andy and myself) are well known kernel
> developers in the input and HID subsystems, so we usually take the
> fame by adding our Reviewed-by or Signed-off-by tag at the end of the
> commit message. (Also note that Andy is working for Intel ;-P )
>
> If you feel that these persons have an equal participation in the
> creation of the patch, you should ask for their Signed-off-by in the
> patch. My gut feelings tell me they would gladly give a Reviewed-by
> because they helped you on the patch, but they are not the "author",
> so the Signed-off-by should be you only.
Just in case, we have Co-developed-by: tag and for my opinion Hans'
name should go under this category.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Kai-Heng Feng @ 2019-03-20 16:53 UTC (permalink / raw)
To: Hans de Goede
Cc: Benjamin Tissoires, hotwater438, Jiri Kosina, Stephen Boyd,
bigeasy, Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <9151d116-958c-9298-9427-fe803a163e9f@redhat.com>
Hi Hans,
at 23:39, Hans de Goede <hdegoede@redhat.com> wrote:
> H,
>
> On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
>
> <snip>
>
>>> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
>>> <hdegoede@redhat.com>:
>>> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks
>>> before we init the irq, and we cannot setup the quirk earlier, so we
>>> must init the irq later.
>> I am pretty sure you can paraphrase Hans in your commit message
>> without having to formally quote him (Hans, please shout if you
>> disagree).
>
> Right, it is fine to explain why the irq init is moved without
> quoting or even referring to me.
>
>>> --- a/drivers/hid/hid-ids.h
>>> +++ b/drivers/hid/hid-ids.h
>>> @@ -389,6 +389,7 @@
>>> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
>>> #define USB_DEVICE_ID_HP_X2 0x074d
>>> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
>>> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>> I know Hans wanted you to use USB here, but I'd think we should have a
>> I2C_DEVICE_ID_*
>> There is no guarantees that this same PID will be used in a different
>> chip over USB.
>> We tend to not car about USB/I2C for the vendor IDs: the vendors
>> decided to reuse their USB VID, which makes our life easier.
>
> Ok, no objection from me to use an I2C_DEVICE_ID prefix, here it
> is after all an I2C device.
>
>>> #define USB_VENDOR_ID_ELECOM 0x056e
>>> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> index 90164fed08d3..16b55c45e2e8 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> @@ -50,7 +50,8 @@
>>> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
>>> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
>>> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
>>> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>>> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>>> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>>>
>>> /* flags */
>>> #define I2C_HID_STARTED 0
>>> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
>>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>>> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
>>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>>> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
>>> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
>>> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
>>> - I2C_HID_QUIRK_BOGUS_IRQ },
>>> + I2C_HID_QUIRK_BOGUS_IRQ },
>
> Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
> is also used on Elan devices, I suspect that these Elan devices
> likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
> and then they probably will no longer need the bogus IRQ flag,
> if you know about bugreports with an acpidump for any of the devices
> needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
> declared there, I suspect it will be declared as level-low, just like
> with the laptop this patch was written for. And it probably need to
> be edge-falling instead of level-low just like this case.
First, I’ve already tried using IRQF_TRIGGER_FALLING, unfortunately it
doesn’t solve the issue for me.
I talked to Elan once, and they confirm the correct IRQ trigger is level
low. So forcing falling trigger may break other platforms.
Recently we found that Elan touchpad doesn’t like GpioInt() from its _CRS.
Once the Interrupt() is used instead, the issue goes away.
But I am not sure how to patch its DSDT/SSDT in i2c-hid.
Kai-Heng
>
> Regards,
>
> Hans
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Hans de Goede @ 2019-03-20 15:39 UTC (permalink / raw)
To: Benjamin Tissoires, hotwater438
Cc: Jiri Kosina, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJLGABHATo-fOc_jgc8uNeLDv7cemsHuQMj7zRJWmfyp4g@mail.gmail.com>
H,
On 3/20/19 3:37 PM, Benjamin Tissoires wrote:
<snip>
>> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
>> <hdegoede@redhat.com>:
>> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks before we init the irq, and we cannot setup the quirk earlier, so we must init the irq later.
>
> I am pretty sure you can paraphrase Hans in your commit message
> without having to formally quote him (Hans, please shout if you
> disagree).
Right, it is fine to explain why the irq init is moved without
quoting or even referring to me.
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -389,6 +389,7 @@
>> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
>> #define USB_DEVICE_ID_HP_X2 0x074d
>> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
>> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
>
> I know Hans wanted you to use USB here, but I'd think we should have a
> I2C_DEVICE_ID_*
> There is no guarantees that this same PID will be used in a different
> chip over USB.
> We tend to not car about USB/I2C for the vendor IDs: the vendors
> decided to reuse their USB VID, which makes our life easier.
Ok, no objection from me to use an I2C_DEVICE_ID prefix, here it
is after all an I2C device.
>> #define USB_VENDOR_ID_ELECOM 0x056e
>> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>> index 90164fed08d3..16b55c45e2e8 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>> @@ -50,7 +50,8 @@
>> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
>> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
>> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
>> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
>> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>>
>> /* flags */
>> #define I2C_HID_STARTED 0
>> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
>> I2C_HID_QUIRK_NO_RUNTIME_PM },
>> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
>> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
>> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
>> - I2C_HID_QUIRK_BOGUS_IRQ },
>> + I2C_HID_QUIRK_BOGUS_IRQ },
Benjamin, what I find interesting here is that the BOGUS_IRQ quirk
is also used on Elan devices, I suspect that these Elan devices
likely also need the I2C_HID_QUIRK_FORCE_TRIGGER_FALLING quirk
and then they probably will no longer need the bogus IRQ flag,
if you know about bugreports with an acpidump for any of the devices
needing the bogus IRQ quirk, then I (or you) can check how the IRQ is
declared there, I suspect it will be declared as level-low, just like
with the laptop this patch was written for. And it probably need to
be edge-falling instead of level-low just like this case.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] ELAN touchpad i2c_hid bugs fix
From: Benjamin Tissoires @ 2019-03-20 14:37 UTC (permalink / raw)
To: hotwater438
Cc: Jiri Kosina, Hans de Goede, Kai Heng Feng, Stephen Boyd, bigeasy,
Dmitry Torokhov, open list:HID CORE LAYER, lkml
In-Reply-To: <LaQHUFs--3-1@tutanota.com>
Hi Vladislav,
I really appreciate the contribution, there are a few issues with the
formatting of the patch, thanks for taking care of those (I'll inline
the problems).
On Wed, Mar 20, 2019 at 2:28 PM <hotwater438@tutanota.com> wrote:
>
> Desciption: This patch add's additional quirk to force
> IRQ_TRIGGER_FALLING flag which resolves issues with ELAN1200:04F3:303E
> touchpad. Issues are the next:
> - Journalctl floods with next report:
> i2c_hid i2c-ELAN1200:00: i2c_hid_get_input: incomplete report (16/65535)
> - Five finger tap kills i2c_hid
> - When you scroll anything, and release one finger, driver thought you are still scrolling
You don't need to be that "formal" in the commit message.
The "description" part describes too much what the patch is (while we
can just read the code), the important part is the issues you are
fixing.
I'd like you to rewrite this in a way that doesn't feel like you are
writing a bug report.
For instance, you could write (but you can change this too):
---
The ELAN1200:04F3:303E touchpad exposes several issues, all caused by
an error setting the correct IRQ_TRIGGER flag:
- ...
- ...
- ...
Fix all of these with a new quirk that corrects the trigger flag
announced by the ACPI tables.
---
I know writing good commit message is a tricky part :)
>
> The reason why i2c_hid_init_irq was moved is told by Hans De Goede
> <hdegoede@redhat.com>:
> i2c_hid_init_irq now checks for a quirk, so we must setup the quirks before we init the irq, and we cannot setup the quirk earlier, so we must init the irq later.
I am pretty sure you can paraphrase Hans in your commit message
without having to formally quote him (Hans, please shout if you
disagree).
But this is a definitively good point to raise: your patch touches a
general path, so it might have unexpected issues if not carefully
reviewed by us.
>
>
> Patch is created by help of ELANTECH and RedHat guys (Hans De Goede <hdegoede@redhat.com>, Benjamin Tissoires <btissoir@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>) and me (Vladislav Dalechyn <hotwater438@tutanota.com>.
Don't take it wrong, but that's not how we give credit in a patch.
In your case, if you do not have any Elantech engineer who agreed to
sign off the patch, you can definitively mention them this way.
But the other guys (Hans, Andy and myself) are well known kernel
developers in the input and HID subsystems, so we usually take the
fame by adding our Reviewed-by or Signed-off-by tag at the end of the
commit message. (Also note that Andy is working for Intel ;-P )
If you feel that these persons have an equal participation in the
creation of the patch, you should ask for their Signed-off-by in the
patch. My gut feelings tell me they would gladly give a Reviewed-by
because they helped you on the patch, but they are not the "author",
so the Signed-off-by should be you only.
And yes, you should sign your work here saying that you wrote this
code in accordance to the Developer's Certificate of Origin 1.1
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst)
Then, either Jiri or myself will have a special role, and we will add
our own signed-of-by as the maintainers of the HID tree certifying
that this patch has been accepted by one of us.
>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/i2c-hid/i2c-hid-core.c | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index b6d93f4ad037..dc44b5661669 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -389,6 +389,7 @@
> #define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
> #define USB_DEVICE_ID_HP_X2 0x074d
> #define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
> +#define USB_DEVICE_ID_ELAN_TOUCHPAD 0x303e
I know Hans wanted you to use USB here, but I'd think we should have a
I2C_DEVICE_ID_*
There is no guarantees that this same PID will be used in a different
chip over USB.
We tend to not car about USB/I2C for the vendor IDs: the vendors
decided to reuse their USB VID, which makes our life easier.
>
> #define USB_VENDOR_ID_ELECOM 0x056e
> #define USB_DEVICE_ID_ELECOM_BM084 0x0061
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 90164fed08d3..16b55c45e2e8 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -50,7 +50,8 @@
> #define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
> #define I2C_HID_QUIRK_NO_RUNTIME_PM BIT(2)
> #define I2C_HID_QUIRK_DELAY_AFTER_SLEEP BIT(3)
> -#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
> +#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
your mail client is playing with tabs and spaces, but I don't expect
the line above to be changed at all.
> +#define I2C_HID_QUIRK_FORCE_TRIGGER_FALLING BIT(5)
>
> /* flags */
> #define I2C_HID_STARTED 0
> @@ -182,8 +183,10 @@ static const struct i2c_hid_quirks {
> I2C_HID_QUIRK_NO_RUNTIME_PM },
> { I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
> I2C_HID_QUIRK_NO_RUNTIME_PM },
> + { USB_VENDOR_ID_ELAN, USB_DEVICE_ID_ELAN_TOUCHPAD,
> + I2C_HID_QUIRK_BOGUS_IRQ | I2C_HID_QUIRK_FORCE_TRIGGER_FALLING },
> { USB_VENDOR_ID_ELAN, HID_ANY_ID,
> - I2C_HID_QUIRK_BOGUS_IRQ },
> + I2C_HID_QUIRK_BOGUS_IRQ },
same here, this line should not be changed.
> { 0, 0 }
> };
>
> @@ -854,6 +857,8 @@ static int i2c_hid_init_irq(struct i2c_client *client)
>
> if (!irq_get_trigger_type(client->irq))
> irqflags = IRQF_TRIGGER_LOW;
> + if (ihid->quirks & I2C_HID_QUIRK_FORCE_TRIGGER_FALLING)
> + irqflags = IRQF_TRIGGER_FALLING;
again, indentation problems, tabs are missing
>
> ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> irqflags | IRQF_ONESHOT, client->name, ihid);
> @@ -1123,10 +1128,6 @@ static int i2c_hid_probe(struct i2c_client *client,
> if (ret < 0)
> goto err_pm;
>
> - ret = i2c_hid_init_irq(client);
> - if (ret < 0)
> - goto err_pm;
> -
> hid = hid_allocate_device();
> if (IS_ERR(hid)) {
> ret = PTR_ERR(hid);
the next call is "goto err_irq", which should be changed to "goto err_pm"
> @@ -1149,6 +1150,10 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> ihid->quirks = i2c_hid_lookup_quirk(hid->vendor, hid->product);
>
> + ret = i2c_hid_init_irq(client);
> + if (ret < 0)
> + goto err_pm;
this should be "err_mem_free"
> +
> ret = hid_add_device(hid);
> if (ret) {
> if (ret != -ENODEV)
next one should be "err_irq"
and the err_irq and err_mem_free should be inverted at the end of probe.
Cheers,
Benjamin
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] HID: logitech: Handle 0 scroll events for the m560
From: Benjamin Tissoires @ 2019-03-20 14:29 UTC (permalink / raw)
To: Peter Hutterer
Cc: open list:HID CORE LAYER, Aimo Metsälä,
Nestor Lopez Casado, Jiri Kosina, lkml
In-Reply-To: <20190319224823.GA26366@jelly>
On Tue, Mar 19, 2019 at 11:48 PM Peter Hutterer
<peter.hutterer@who-t.net> wrote:
>
> hidpp_scroll_counter_handle_scroll() doesn't expect a 0-value scroll event, it
> gets interpreted as a negative scroll direction event. This can cause scroll
> direction resets and thus broken scrolling.
>
> Reported-and-tested-by: Aimo Metsälä <aimetsal@outlook.com>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
Added the 'Fixes' and "cc: stable" tags and patch applied to
for-5.1/upstream-fixes
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 15ed6177a7a3..f040c8a7f9a9 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -2608,8 +2608,9 @@ static int m560_raw_event(struct hid_device *hdev, u8 *data, int size)
> input_report_rel(mydata->input, REL_Y, v);
>
> v = hid_snto32(data[6], 8);
> - hidpp_scroll_counter_handle_scroll(
> - &hidpp->vertical_wheel_counter, v);
> + if (v != 0)
> + hidpp_scroll_counter_handle_scroll(
> + &hidpp->vertical_wheel_counter, v);
>
> input_sync(mydata->input);
> }
> --
> 2.20.1
>
^ permalink raw reply
* RE: must hold the driver_input_lock in hid_debug_rdesc_show
From: He, Bo @ 2019-03-20 1:50 UTC (permalink / raw)
To: Jiri Kosina
Cc: benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Zhang, Jun, Zhang, Yanmin
In-Reply-To: <nycvar.YFH.7.76.1903191541410.19912@cbobk.fhfr.pm>
thanks, without the patch we can reproduce with the way in 10 hours Suspend/Resume test, with the test, we can't reproduce for 30 hours.
-----Original Message-----
From: Jiri Kosina <jikos@kernel.org>
Sent: Tuesday, March 19, 2019 10:42 PM
To: He, Bo <bo.he@intel.com>
Cc: benjamin.tissoires@redhat.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Zhang, Jun <jun.zhang@intel.com>; Zhang, Yanmin <yanmin.zhang@intel.com>
Subject: Re: must hold the driver_input_lock in hid_debug_rdesc_show
On Thu, 14 Mar 2019, He, Bo wrote:
> we see the below kernel panic logs when run suspend resume test with
> usb mouse and usb keyboard connected.
>
> the scenario is the userspace call the hid_debug_rdesc_show to dump
> the input device while the device is removed. the patch hold the
> driver_input_lock to avoid the race.
>
> [ 5381.757295] selinux: SELinux: Could not stat
> /sys/devices/pci0000:00/0000:00:15.0/usb1/1-2/1-2:1.0/0003:03F0:0325.0320/input/input960/input960::scrolllock:
> No such file or directory.
> [ 5382.636498] BUG: unable to handle kernel paging request at 0000000783316040
> [ 5382.651950] CPU: 1 PID: 1512 Comm: getevent Tainted: G U O 4.19.20-quilt-2e5dc0ac-00029-gc455a447dd55 #1
> [ 5382.663797] RIP: 0010:hid_dump_device+0x9b/0x160 [ 5382.758853]
> Call Trace:
> [ 5382.761581] hid_debug_rdesc_show+0x72/0x1d0 [ 5382.766343]
> seq_read+0xe0/0x410 [ 5382.769941] full_proxy_read+0x5f/0x90 [
> 5382.774121] __vfs_read+0x3a/0x170 [ 5382.788392]
> vfs_read+0xa0/0x150 [ 5382.791984] ksys_read+0x58/0xc0 [ 5382.801404]
> __x64_sys_read+0x1a/0x20 [ 5382.805483] do_syscall_64+0x55/0x110 [
> 5382.809559] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Signed-off-by: he, bo <bo.he@intel.com>
> Signed-off-by: "Zhang, Jun" <jun.zhang@intel.com>
I rewrote the changelog to explain the situation a bit more clearly, and applied. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox