[PATCH 0/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with a CHPN0001 touchscreen

[PATCH 0/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with a CHPN0001 touchscreen

[Hans de Goede]

Hi Jiri, Benjamin,

As discussed with myself in the linux-input thread about adding
icn8505 (ACPI HID CHPN0001) devices to the icn8313 driver:
https://www.spinics.net/lists/linux-input/msg51916.html

Unfortunately the ACPI node for this non HID compatible device has an
ACPI CID (compatility ID) of PNP0C50 causing the i2c-hid driver to bind,
which leads to the touchscreen not working (see the second patch
commit message for details).

Although these 2 patches are not really pretty I believe they are the
best way to fix this.

Note that even if I do add firmware upload support to the icn8318 driver,
we still have the issue left of i2c-hid doing a
dev_err(dev, "hid_descr_cmd failed") which is also undesirable.

A normal system bootup should have no output for "dmesg --level=err",
this is esp. important for the flickerfree boot experience both Fedora
and Ubuntu have been working towards. Any _err kernel messages will
cause the splash screen to drop back to text-mode breaking the
flickerfree experience. So to silence that error we would need to
blacklist the CHPN0001 ACPI HID somewhere in the i2c-hid driver
anyways.

Regards,

Hans

[PATCH 1/2] HID: i2c-hid: Expand module_i2c_driver macro

[Hans de Goede]

This is a preparation patch for not registering the driver on some
machines where just trying to probe a non HID compliant device causes
issues.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index fb55fb4c39fc..54b53d8f96ce 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -1276,7 +1276,17 @@ static struct i2c_driver i2c_hid_driver = {
 	.id_table	= i2c_hid_id_table,
 };
 
-module_i2c_driver(i2c_hid_driver);
+static int __init i2c_hid_init(void)
+{
+	return i2c_add_driver(&i2c_hid_driver);
+}
+module_init(i2c_hid_init);
+
+static void __exit i2c_hid_exit(void)
+{
+	i2c_del_driver(&i2c_hid_driver);
+}
+module_exit(i2c_hid_exit);
 
 MODULE_DESCRIPTION("HID over I2C core driver");
 MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
-- 
2.13.0

[PATCH 2/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with CHPN0001 touchscreen

[Hans de Goede]

The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
it uses its own protocol which is handled by the chipone_icn8318 driver.

Registering the i2c_hid_driver first will cause probing to fail with a
"hid_descr_cmd failed" error.

Worse, after the probe failure the i2c / ACPI core code will put the ACPI
device in D3 state and when the chipone_icn8318 driver then loads the
device is put back in D0 state, executing its PS0 ACPI method, which
resets the controller, causing the controller to loose its firmware
which was loaded by the BIOS. The chipone_icn8318 driver has a workaround
for this, but that requires it to be the only (or the first) driver to
probe the device.

This commit adds a check for the presence of a CHPN0001 ACPI device to
i2c-hid and makes it not register the i2c_hid_driver on systems with this
touchscreen fixing the controller losing its firmware.

Note that acpi_dev_present is a stub always returning false on platforms
without ACPI so for non ACPI platforms this patch is a no-op.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 54b53d8f96ce..71a040d421c6 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -1278,6 +1278,14 @@ static struct i2c_driver i2c_hid_driver = {
 
 static int __init i2c_hid_init(void)
 {
+	/*
+	 * The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID
+	 * compatible, just probing it puts the device in an unusable state due
+	 * to it also have ACPI power management issues.
+	 */
+	if (acpi_dev_present("CHPN0001", NULL, -1))
+		return -ENODEV;
+
 	return i2c_add_driver(&i2c_hid_driver);
 }
 module_init(i2c_hid_init);
-- 
2.13.0

Re: [PATCH 0/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with a CHPN0001 touchscreen

[Jiri Kosina]

On Sun, 18 Jun 2017, Hans de Goede wrote:

> Hi Jiri, Benjamin,
> 
> As discussed with myself in the linux-input thread about adding
> icn8505 (ACPI HID CHPN0001) devices to the icn8313 driver:
> https://www.spinics.net/lists/linux-input/msg51916.html
> 
> Unfortunately the ACPI node for this non HID compatible device has an
> ACPI CID (compatility ID) of PNP0C50 causing the i2c-hid driver to bind,
> which leads to the touchscreen not working (see the second patch
> commit message for details).
> 
> Although these 2 patches are not really pretty I believe they are the
> best way to fix this.
> 
> Note that even if I do add firmware upload support to the icn8318 driver,
> we still have the issue left of i2c-hid doing a
> dev_err(dev, "hid_descr_cmd failed") which is also undesirable.
> 
> A normal system bootup should have no output for "dmesg --level=err",
> this is esp. important for the flickerfree boot experience both Fedora
> and Ubuntu have been working towards. Any _err kernel messages will
> cause the splash screen to drop back to text-mode breaking the
> flickerfree experience. So to silence that error we would need to
> blacklist the CHPN0001 ACPI HID somewhere in the i2c-hid driver
> anyways.

Benjamin, as i2c-hid is mainly your playground, would you mind (n)Acking 
this? :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with a CHPN0001 touchscreen

[Benjamin Tissoires]

On Jun 27 2017 or thereabouts, Jiri Kosina wrote:
> On Sun, 18 Jun 2017, Hans de Goede wrote:
> 
> > Hi Jiri, Benjamin,
> > 
> > As discussed with myself in the linux-input thread about adding
> > icn8505 (ACPI HID CHPN0001) devices to the icn8313 driver:
> > https://www.spinics.net/lists/linux-input/msg51916.html
> > 
> > Unfortunately the ACPI node for this non HID compatible device has an
> > ACPI CID (compatility ID) of PNP0C50 causing the i2c-hid driver to bind,
> > which leads to the touchscreen not working (see the second patch
> > commit message for details).
> > 
> > Although these 2 patches are not really pretty I believe they are the
> > best way to fix this.
> > 
> > Note that even if I do add firmware upload support to the icn8318 driver,
> > we still have the issue left of i2c-hid doing a
> > dev_err(dev, "hid_descr_cmd failed") which is also undesirable.
> > 
> > A normal system bootup should have no output for "dmesg --level=err",
> > this is esp. important for the flickerfree boot experience both Fedora
> > and Ubuntu have been working towards. Any _err kernel messages will
> > cause the splash screen to drop back to text-mode breaking the
> > flickerfree experience. So to silence that error we would need to
> > blacklist the CHPN0001 ACPI HID somewhere in the i2c-hid driver
> > anyways.
> 
> Benjamin, as i2c-hid is mainly your playground, would you mind (n)Acking 
> this? :)
> 

Sorry, I should have replied here too.

I still maintain: http://www.spinics.net/lists/linux-input/msg51961.html
So unless we have some new elements, I am NAcking this patch.

Cheers,
Benjamin

> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

Re: [PATCH 0/2] HID: i2c-hid: Do not register i2c_hid_driver on devices with a CHPN0001 touchscreen

[Hans de Goede]

Hi,

On 06/27/2017 02:48 PM, Benjamin Tissoires wrote:
> On Jun 27 2017 or thereabouts, Jiri Kosina wrote:
>> On Sun, 18 Jun 2017, Hans de Goede wrote:
>>
>>> Hi Jiri, Benjamin,
>>>
>>> As discussed with myself in the linux-input thread about adding
>>> icn8505 (ACPI HID CHPN0001) devices to the icn8313 driver:
>>> https://www.spinics.net/lists/linux-input/msg51916.html
>>>
>>> Unfortunately the ACPI node for this non HID compatible device has an
>>> ACPI CID (compatility ID) of PNP0C50 causing the i2c-hid driver to bind,
>>> which leads to the touchscreen not working (see the second patch
>>> commit message for details).
>>>
>>> Although these 2 patches are not really pretty I believe they are the
>>> best way to fix this.
>>>
>>> Note that even if I do add firmware upload support to the icn8318 driver,
>>> we still have the issue left of i2c-hid doing a
>>> dev_err(dev, "hid_descr_cmd failed") which is also undesirable.
>>>
>>> A normal system bootup should have no output for "dmesg --level=err",
>>> this is esp. important for the flickerfree boot experience both Fedora
>>> and Ubuntu have been working towards. Any _err kernel messages will
>>> cause the splash screen to drop back to text-mode breaking the
>>> flickerfree experience. So to silence that error we would need to
>>> blacklist the CHPN0001 ACPI HID somewhere in the i2c-hid driver
>>> anyways.
>>
>> Benjamin, as i2c-hid is mainly your playground, would you mind (n)Acking
>> this? :)
>>
> 
> Sorry, I should have replied here too.
> 
> I still maintain: http://www.spinics.net/lists/linux-input/msg51961.html
> So unless we have some new elements, I am NAcking this patch.

I was planning on implementing the suggested match stuff right away so I
did not answer you, but I did not find the time. So let me at least reply
now :)

I agree that the match callback solution you propose in the mail you
linked is better then my initial fix for this, so I will implement that
and then we will so how that is received.

Regards,

Hans