Re: [RFC] chromeos_laptop: Make touchscreen work on C720

Re: [RFC] chromeos_laptop: Make touchscreen work on C720

[Stephen Oberholtzer]

On Tue, Dec 3, 2019 at 2:36 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Stephen,
>
> Does the new kernel work with the original firmware (it should as that's
> what it's been tested with)?

It took some finagling -- MrChromebox is UEFI, but the stock firmware
boots with SeaBIOS -- but I was able to confirm that yes, the
touchscreen works in 4.19 when booting from stock firmware.

> Acer C720 did not use ACPI to describe its devices, it relied on static
> board support to instantiate touchscreen and trackpad and other devices.

> I see that MrChromebox BIOS declares the peripherals on Peppy via ACPI.
> I'd recommend reaching out and ask to update the binding (maybe switch
> from ATML0001 to PRP0001 and full OF-style binding to avoid confusion).

(This is my first-ever foray into anything involving Linux and I2C,
ACPI, _or_ multi-module cooperation, and lucky me, I get all three!
Good thing it's easy to verify that a touchscreen is working.  I also
have no idea what "OF-style binding" is.)

Approaching from the perspective of "I'm designing a brand new kind of
machine that runs Linux, uses ACPI tables, and has one or more Atmel
MXT chips", several things become apparent:

* The driver expects a strange "compatible" property that communicates
no useful information.  The property's value is not read, only its
existence, which is one bit of information. '1' is interpreted as
'this is a supported device', while '0' is interpreted as 'this is not
a supported device'.  But if it's not a supported device, why was the
driver loaded in the first place?

* If it's a touchpad, the driver needs to know what buttons the
various GPIOs are mapped to.
  (Conceivably, it doesn't need to be a touchpad; in my hypothetical
machine, it could also be a touchscreen that also has some input
buttons wired up through the MXT chip, because that saved a nickel per
unit. To support that, however, we would also need an indicator
specifying whether it's a touchscreen or a touchpad, as the driver
interprets 'has buttons' as 'is touchpad'.)

This leads to my first question: With this in mind, couldn't simply
removing the "compatible" check work?
On any existing machine where this driver is loaded and working, the
chromeos_laptop driver has already run and set up the appropriate
device properties first.

--
Stephen

Re: [RFC] chromeos_laptop: Make touchscreen work on C720

[Dmitry Torokhov]

Hi Stephen,

On Tue, Dec 03, 2019 at 11:03:18AM -0500, Stephen Oberholtzer wrote:
> From 627be71ee77f4fc20cdb55c6e0a06826fb43ca9d Mon Sep 17 00:00:00 2001
> From: Stephen Oberholtzer <stevie@qrpff.net>
> Date: Tue, 3 Dec 2019 02:20:28 -0500
> Subject: [RFC] chromeos_laptop: Make touchscreen work on C720
> 
> I have an old Acer C720 Chromebook reflashed with MrChromebox, running
> Debian.  When I did an upgrade, the touchscreen stopped working; I traced
> it to the kernel.
> 
> After six hours of bisecting -- if anyone can tell me how to make a kernel
> build take less than 2 hours of CPU time while bisecting, I'd greatly
> appreciate it -- I tracked the problem to commit 7cf432bf0586
> ("Input: atmel_mxt_ts - require device properties present when probing").
> 
> Looking at https://lkml.org/lkml/2018/5/3/955, it appears that the idea
> was to reassign the responsibility for setting up ACPI data for
> atmel_mxt_ts into chromeos_laptop, which makes a lot of sense, as that
> would tidy up some potential maintenance issues.
> 
> However, that doesn't actually work.  The chromeos_laptop code appears to
> categorize every Chromebook as exactly one of the following:
> 
> (A) Having I2C devices (declared using DECLARE_CROS_LAPTOP)
> (B) Requiring ACPI munging (declared using DECLARE_ACPI_CROS_LAPTOP)
> 
> There's some stuff about a board_info.properties that looks like it's meant
> to do the job for I2C devices, but it doesn't seem to do anything;
> it *definitely* doesn't do what the atmel_mxt_ts driver is expecting.
> 
> On the other hand, when I apply the following patch to a recent kernel
> (5.2 is the one I tested), my touchscreen works again.
> 
> I'm still not sure if the appropriate solution is to ensure that the
> ACPI properties are set, or if atmel_mxt_ts should be checking both
> ACPI properties and the I2C board_info.properties, however.

Acer C720 did not use ACPI to describe its devices, it relied on static
board support to instantiate touchscreen and trackpad and other devices.
Does the new kernel work with the original firmware (it should as that's
what it's been tested with)?

I see that MrChromebox BIOS declares the peripherals on Peppy via ACPI.
I'd recommend reaching out and ask to update the binding (maybe switch
from ATML0001 to PRP0001 and full OF-style binding to avoid confusion).

Thanks.

-- 
Dmitry

[RFC] chromeos_laptop: Make touchscreen work on C720

[Stephen Oberholtzer]

From 627be71ee77f4fc20cdb55c6e0a06826fb43ca9d Mon Sep 17 00:00:00 2001
From: Stephen Oberholtzer <stevie@qrpff.net>
Date: Tue, 3 Dec 2019 02:20:28 -0500
Subject: [RFC] chromeos_laptop: Make touchscreen work on C720

I have an old Acer C720 Chromebook reflashed with MrChromebox, running
Debian.  When I did an upgrade, the touchscreen stopped working; I traced
it to the kernel.

After six hours of bisecting -- if anyone can tell me how to make a kernel
build take less than 2 hours of CPU time while bisecting, I'd greatly
appreciate it -- I tracked the problem to commit 7cf432bf0586
("Input: atmel_mxt_ts - require device properties present when probing").

Looking at https://lkml.org/lkml/2018/5/3/955, it appears that the idea
was to reassign the responsibility for setting up ACPI data for
atmel_mxt_ts into chromeos_laptop, which makes a lot of sense, as that
would tidy up some potential maintenance issues.

However, that doesn't actually work.  The chromeos_laptop code appears to
categorize every Chromebook as exactly one of the following:

(A) Having I2C devices (declared using DECLARE_CROS_LAPTOP)
(B) Requiring ACPI munging (declared using DECLARE_ACPI_CROS_LAPTOP)

There's some stuff about a board_info.properties that looks like it's meant
to do the job for I2C devices, but it doesn't seem to do anything;
it *definitely* doesn't do what the atmel_mxt_ts driver is expecting.

On the other hand, when I apply the following patch to a recent kernel
(5.2 is the one I tested), my touchscreen works again.

I'm still not sure if the appropriate solution is to ensure that the
ACPI properties are set, or if atmel_mxt_ts should be checking both
ACPI properties and the I2C board_info.properties, however.

>8------------------------------------------------------8<

Fixes: 7cf432bf058 ("Input: atmel_mxt_ts - require device properties
present when probing")

The ACPI data for the Atmel touchscreen found in many Chromebooks is
incomplete.  Originally, the atmel_mxt_ts driver had a whitelist of known
Chromebooks, but that changed in commit 7cf432bf0586 ("Input: atmel_mxt_ts
 - require device properties present when probing").
As of that commit, the atmel_mxt_ts driver expects the chromeos_laptop
driver to inject the required ACPI properties into the device node.

However, this doesn't actually work in all cases -- particularly, the Acer
C720.  This is because the C720's definition does not contain any rules to
set up the ACPI properties for the touchscreen.

Signed-off-by: Stephen Oberholtzer <stevie@qrpff.net>

---
 drivers/platform/chrome/chromeos_laptop.c | 47 +++++++++++++++--------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/chrome/chromeos_laptop.c
b/drivers/platform/chrome/chromeos_laptop.c
index 7abbb6167766..b6487a19f04a 100644
--- a/drivers/platform/chrome/chromeos_laptop.c
+++ b/drivers/platform/chrome/chromeos_laptop.c
@@ -228,16 +228,28 @@ static struct notifier_block
chromeos_laptop_i2c_notifier = {
     .notifier_call = chromeos_laptop_i2c_notifier_call,
 };

-#define DECLARE_CROS_LAPTOP(_name)                    \
-static const struct chromeos_laptop _name __initconst = {        \
+#define _DECLARE_I2C_CROS_LAPTOP(_name)                    \
     .i2c_peripherals    = _name##_peripherals,            \
-    .num_i2c_peripherals    = ARRAY_SIZE(_name##_peripherals),    \
+    .num_i2c_peripherals    = ARRAY_SIZE(_name##_peripherals),
+
+#define _DECLARE_ACPI_CROS_LAPTOP(_name)                \
+    .acpi_peripherals    = _name##_peripherals,            \
+    .num_acpi_peripherals    = ARRAY_SIZE(_name##_peripherals),
+
+#define DECLARE_I2C_CROS_LAPTOP(_name)                    \
+static const struct chromeos_laptop _name __initconst = {        \
+    _DECLARE_I2C_CROS_LAPTOP(_name)                    \
 }

 #define DECLARE_ACPI_CROS_LAPTOP(_name)                    \
 static const struct chromeos_laptop _name __initconst = {        \
-    .acpi_peripherals    = _name##_peripherals,            \
-    .num_acpi_peripherals    = ARRAY_SIZE(_name##_peripherals),    \
+    _DECLARE_ACPI_CROS_LAPTOP(_name)                \
+}
+
+#define DECLARE_I2C_ACPI_CROS_LAPTOP(_name, _acpiname)            \
+static const struct chromeos_laptop _name __initconst = {        \
+    _DECLARE_I2C_CROS_LAPTOP(_name)                    \
+    _DECLARE_ACPI_CROS_LAPTOP(_acpiname)                \
 }

 static struct i2c_peripheral samsung_series_5_550_peripherals[] __initdata = {
@@ -259,7 +271,7 @@ static struct i2c_peripheral
samsung_series_5_550_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(samsung_series_5_550);
+DECLARE_I2C_CROS_LAPTOP(samsung_series_5_550);

 static struct i2c_peripheral samsung_series_5_peripherals[] __initdata = {
     /* Light Sensor. */
@@ -270,7 +282,7 @@ static struct i2c_peripheral
samsung_series_5_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(samsung_series_5);
+DECLARE_I2C_CROS_LAPTOP(samsung_series_5);

 static const int chromebook_pixel_tp_keys[] __initconst = {
     KEY_RESERVED,
@@ -332,7 +344,7 @@ static struct i2c_peripheral
chromebook_pixel_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_PANEL,
     },
 };
-DECLARE_CROS_LAPTOP(chromebook_pixel);
+DECLARE_I2C_CROS_LAPTOP(chromebook_pixel);

 static struct i2c_peripheral hp_chromebook_14_peripherals[] __initdata = {
     /* Touchpad. */
@@ -345,7 +357,7 @@ static struct i2c_peripheral
hp_chromebook_14_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_DESIGNWARE,
     },
 };
-DECLARE_CROS_LAPTOP(hp_chromebook_14);
+DECLARE_I2C_CROS_LAPTOP(hp_chromebook_14);

 static struct i2c_peripheral dell_chromebook_11_peripherals[] __initdata = {
     /* Touchpad. */
@@ -367,7 +379,7 @@ static struct i2c_peripheral
dell_chromebook_11_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_DESIGNWARE,
     },
 };
-DECLARE_CROS_LAPTOP(dell_chromebook_11);
+DECLARE_I2C_CROS_LAPTOP(dell_chromebook_11);

 static struct i2c_peripheral toshiba_cb35_peripherals[] __initdata = {
     /* Touchpad. */
@@ -380,7 +392,7 @@ static struct i2c_peripheral
toshiba_cb35_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_DESIGNWARE,
     },
 };
-DECLARE_CROS_LAPTOP(toshiba_cb35);
+DECLARE_I2C_CROS_LAPTOP(toshiba_cb35);

 static struct i2c_peripheral acer_c7_chromebook_peripherals[] __initdata = {
     /* Touchpad. */
@@ -393,7 +405,7 @@ static struct i2c_peripheral
acer_c7_chromebook_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(acer_c7_chromebook);
+DECLARE_I2C_CROS_LAPTOP(acer_c7_chromebook);

 static struct i2c_peripheral acer_ac700_peripherals[] __initdata = {
     /* Light Sensor. */
@@ -404,7 +416,7 @@ static struct i2c_peripheral
acer_ac700_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(acer_ac700);
+DECLARE_I2C_CROS_LAPTOP(acer_ac700);

 static struct i2c_peripheral acer_c720_peripherals[] __initdata = {
     /* Touchscreen. */
@@ -452,7 +464,8 @@ static struct i2c_peripheral
acer_c720_peripherals[] __initdata = {
         .pci_devid    = PCI_DEVID(0, PCI_DEVFN(0x15, 0x2)),
     },
 };
-DECLARE_CROS_LAPTOP(acer_c720);
+
+/* C720 also needs generic_atmel ACPI stuff declared below. */

 static struct i2c_peripheral
 hp_pavilion_14_chromebook_peripherals[] __initdata = {
@@ -466,7 +479,7 @@ hp_pavilion_14_chromebook_peripherals[] __initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(hp_pavilion_14_chromebook);
+DECLARE_I2C_CROS_LAPTOP(hp_pavilion_14_chromebook);

 static struct i2c_peripheral cr48_peripherals[] __initdata = {
     /* Light Sensor. */
@@ -477,7 +490,7 @@ static struct i2c_peripheral cr48_peripherals[]
__initdata = {
         .type        = I2C_ADAPTER_SMBUS,
     },
 };
-DECLARE_CROS_LAPTOP(cr48);
+DECLARE_I2C_CROS_LAPTOP(cr48);

 static const u32 samus_touchpad_buttons[] __initconst = {
     KEY_RESERVED,
@@ -520,6 +533,8 @@ static struct acpi_peripheral
generic_atmel_peripherals[] __initdata = {
 };
 DECLARE_ACPI_CROS_LAPTOP(generic_atmel);

+DECLARE_I2C_ACPI_CROS_LAPTOP(acer_c720, generic_atmel);
+
 static const struct dmi_system_id chromeos_laptop_dmi_table[] __initconst = {
     {
         .ident = "Samsung Series 5 550",
-- 
2.20.1