[PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Vittorio Gambaletta (VittGam)]

This patch adds the correct platform data information for the Caroline
Chromebook, so that the mouse button does not get stuck in pressed state
after the first click.

The Samus button keymap and platform data definition are the correct
ones for Caroline, so they have been reused here.

v2: updated patch offset after 20180409 changes.

Cc: stable@vger.kernel.org
Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>

---

--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -3035,6 +3035,15 @@
 		.driver_data = samus_platform_data,
 	},
 	{
+		/* Samsung Chromebook Pro */
+		.ident = "Samsung Chromebook Pro",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Caroline"),
+		},
+		.driver_data = samus_platform_data,
+	},
+	{
 		/* Other Google Chromebooks */
 		.ident = "Chromebook",
 		.matches = {

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Dmitry Torokhov]

On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam) wrote:
> This patch adds the correct platform data information for the Caroline
> Chromebook, so that the mouse button does not get stuck in pressed state
> after the first click.
> 
> The Samus button keymap and platform data definition are the correct
> ones for Caroline, so they have been reused here.
> 
> v2: updated patch offset after 20180409 changes.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
> Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
> 

Applied, thank you.

> ---
> 
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3035,6 +3035,15 @@
>  		.driver_data = samus_platform_data,
>  	},
>  	{
> +		/* Samsung Chromebook Pro */
> +		.ident = "Samsung Chromebook Pro",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Caroline"),
> +		},
> +		.driver_data = samus_platform_data,
> +	},
> +	{
>  		/* Other Google Chromebooks */
>  		.ident = "Chromebook",
>  		.matches = {

-- 
Dmitry

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Dmitry Torokhov]

On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam) wrote:
> > This patch adds the correct platform data information for the Caroline
> > Chromebook, so that the mouse button does not get stuck in pressed state
> > after the first click.
> > 
> > The Samus button keymap and platform data definition are the correct
> > ones for Caroline, so they have been reused here.
> > 
> > v2: updated patch offset after 20180409 changes.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
> > 
> 
> Applied, thank you.
> 
> > ---
> > 
> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > @@ -3035,6 +3035,15 @@
> >  		.driver_data = samus_platform_data,
> >  	},
> >  	{
> > +		/* Samsung Chromebook Pro */
> > +		.ident = "Samsung Chromebook Pro",
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),

I've been alerted that shipping BIOS has vendor not capitalized; where
did you get the firmware that has vendor all capitals?

Thanks.

> > +			DMI_MATCH(DMI_PRODUCT_NAME, "Caroline"),
> > +		},
> > +		.driver_data = samus_platform_data,
> > +	},
> > +	{
> >  		/* Other Google Chromebooks */
> >  		.ident = "Chromebook",
> >  		.matches = {
> 
> -- 
> Dmitry

-- 
Dmitry

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Vittorio Gambaletta (VittGam)]

Hello,

On 26/04/2018 00:57:34 CEST, Dmitry Torokhov wrote:
> On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
>> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam) wrote:
>> > This patch adds the correct platform data information for the Caroline
>> > Chromebook, so that the mouse button does not get stuck in pressed state
>> > after the first click.
>> >
>> > The Samus button keymap and platform data definition are the correct
>> > ones for Caroline, so they have been reused here.
>> >
>> > v2: updated patch offset after 20180409 changes.
>> >
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
>> >
>>
>> Applied, thank you.
>>
>> > ---
>> >
>> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> > @@ -3035,6 +3035,15 @@
>> >  		.driver_data = samus_platform_data,
>> >  	},
>> >  	{
>> > +		/* Samsung Chromebook Pro */
>> > +		.ident = "Samsung Chromebook Pro",
>> > +		.matches = {
>> > +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> 
> I've been alerted that shipping BIOS has vendor not capitalized; where
> did you get the firmware that has vendor all capitals?

Upstream coreboot and mrchromebox's coreboot, which do support loading Linux
natively or with UEFI, both use capitalized GOOGLE as DMI System Vendor
as far as I know.

By the way, the shipping BIOS (coreboot customized by Google) does not support
mainline Linux natively (eg. without seabios and nasty tricks), but only
Chromium OS, which for Caroline is an old 3.something fork of Linux, so this
patch does not really apply to Chromium OS or to Linux running on shipping BIOS.

Anyway if I recall correctly, the DMI System Vendor should be GOOGLE in
Google's coreboot for Caroline too, but I'm not 100% sure and cannot check
right now.

By the way please note that this is the DMI System Vendor and not the DMI
BIOS version string, which indeed starts with "Google_" in the shipping BIOS
(but not in coreboot, see f56db262e46d3368ee4e5c9e19797853cab382cd).

> Thanks.

Cheers,
Vittorio G

>> > +			DMI_MATCH(DMI_PRODUCT_NAME, "Caroline"),
>> > +		},
>> > +		.driver_data = samus_platform_data,
>> > +	},
>> > +	{
>> >  		/* Other Google Chromebooks */
>> >  		.ident = "Chromebook",
>> >  		.matches = {
>>
>> --
>> Dmitry

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Dmitry Torokhov]

On Thu, Apr 26, 2018 at 06:45:32AM -0700, Guenter Roeck wrote:
> On Wed, Apr 25, 2018 at 10:37 PM, Vittorio Gambaletta (VittGam) <
> linuxbugs@vittgam.net> wrote:
> 
> > Hello,
> >
> > On 26/04/2018 00:57:34 CEST, Dmitry Torokhov wrote:
> > > On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
> > >> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam)
> > wrote:
> > >> > This patch adds the correct platform data information for the Caroline
> > >> > Chromebook, so that the mouse button does not get stuck in pressed
> > state
> > >> > after the first click.
> > >> >
> > >> > The Samus button keymap and platform data definition are the correct
> > >> > ones for Caroline, so they have been reused here.
> > >> >
> > >> > v2: updated patch offset after 20180409 changes.
> > >> >
> > >> > Cc: stable@vger.kernel.org
> > >> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
> > >> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
> > >> >
> > >>
> > >> Applied, thank you.
> > >>
> > >> > ---
> > >> >
> > >> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> > >> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> > >> > @@ -3035,6 +3035,15 @@
> > >> >            .driver_data = samus_platform_data,
> > >> >    },
> > >> >    {
> > >> > +          /* Samsung Chromebook Pro */
> > >> > +          .ident = "Samsung Chromebook Pro",
> > >> > +          .matches = {
> > >> > +                  DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> > >
> > > I've been alerted that shipping BIOS has vendor not capitalized; where
> > > did you get the firmware that has vendor all capitals?
> >
> > Upstream coreboot and mrchromebox's coreboot, which do support loading
> > Linux
> > natively or with UEFI, both use capitalized GOOGLE as DMI System Vendor
> > as far as I know.
> >
> > By the way, the shipping BIOS (coreboot customized by Google) does not
> > support
> > mainline Linux natively (eg. without seabios and nasty tricks), but only
> > Chromium OS, which for Caroline is an old 3.something fork of Linux, so
> > this
> > patch does not really apply to Chromium OS or to Linux running on shipping
> > BIOS.
> >
> > Can you provide details or a pointer to it ? The mainline kernel boots
> fine for me (with a ChromeOS distribution), with the exception of the
> Google/GOOGLE problem.
> 
> 
> > Anyway if I recall correctly, the DMI System Vendor should be GOOGLE in
> > Google's coreboot for Caroline too, but I'm not 100% sure and cannot check
> > right now.
> >
> >
> FWIW, the DMI system vendor is "Google"  in my Caroline. I'll dig up
> configuration files later today - maybe it was changed to "GOOGLE" at some
> point and I have an old Bios in my system.

I believe all shipping BIOSes use "Google" as vendor, as it is what is
specified in src/mainboard/google/Kconfig::MAINBOARD_VENDOR and our
configs/build scripts do not override it.

I do not know why we have this difference.

> 
> Thanks,
> Guenter
> 
> By the way please note that this is the DMI System Vendor and not the DMI
> > BIOS version string, which indeed starts with "Google_" in the shipping
> > BIOS
> > (but not in coreboot, see f56db262e46d3368ee4e5c9e19797853cab382cd).

Hmm, I just tried checking out tree from
https://review.coreboot.org/coreboot.git and do not see this commit:

dtor@dtor-ws:~/coreboot $ git show f56db262e46d3368ee4e5c9e19797853cab382cd
fatal: bad object f56db262e46d3368ee4e5c9e19797853cab382cd
dtor@dtor-ws:~/coreboot $ git log --oneline -1 HEAD
82d0f91420 (HEAD -> master, origin/master, origin/HEAD) soc/intel/apollolake: enable exception handling in every stage for glk      

Anyway, if you are overriding the shipping BIOS, then please adjust
coreboot to use device-tree compatible bindings (via PRP0001 HID and
_DSD properties - you can check out bindings for PENH device in Caroline
to see how they can be used). This way we would not need to breed DMI
quirks.

Thanks.

-- 
Dmitry

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Vittorio Gambaletta (VittGam)]

Hello,

On 26/04/2018 18:42:18 CEST, Dmitry Torokhov wrote:
> On Thu, Apr 26, 2018 at 06:45:32AM -0700, Guenter Roeck wrote:
>> On Wed, Apr 25, 2018 at 10:37 PM, Vittorio Gambaletta (VittGam) <
>> linuxbugs@vittgam.net> wrote:
>>
>> > Hello,
>> >
>> > On 26/04/2018 00:57:34 CEST, Dmitry Torokhov wrote:
>> > > On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
>> > >> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam)
>> > wrote:
>> > >> > This patch adds the correct platform data information for the Caroline
>> > >> > Chromebook, so that the mouse button does not get stuck in pressed
>> > state
>> > >> > after the first click.
>> > >> >
>> > >> > The Samus button keymap and platform data definition are the correct
>> > >> > ones for Caroline, so they have been reused here.
>> > >> >
>> > >> > v2: updated patch offset after 20180409 changes.
>> > >> >
>> > >> > Cc: stable@vger.kernel.org
>> > >> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>> > >> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
>> > >> >
>> > >>
>> > >> Applied, thank you.
>> > >>
>> > >> > ---
>> > >> >
>> > >> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> > >> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> > >> > @@ -3035,6 +3035,15 @@
>> > >> >            .driver_data = samus_platform_data,
>> > >> >    },
>> > >> >    {
>> > >> > +          /* Samsung Chromebook Pro */
>> > >> > +          .ident = "Samsung Chromebook Pro",
>> > >> > +          .matches = {
>> > >> > +                  DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
>> > >
>> > > I've been alerted that shipping BIOS has vendor not capitalized; where
>> > > did you get the firmware that has vendor all capitals?
>> >
>> > Upstream coreboot and mrchromebox's coreboot, which do support loading
>> > Linux
>> > natively or with UEFI, both use capitalized GOOGLE as DMI System Vendor
>> > as far as I know.
>> >
>> > By the way, the shipping BIOS (coreboot customized by Google) does not
>> > support
>> > mainline Linux natively (eg. without seabios and nasty tricks), but only
>> > Chromium OS, which for Caroline is an old 3.something fork of Linux, so
>> > this
>> > patch does not really apply to Chromium OS or to Linux running on shipping
>> > BIOS.
>> >
>> Can you provide details or a pointer to it ? The mainline kernel boots
>> fine for me (with a ChromeOS distribution), with the exception of the
>> Google/GOOGLE problem.

Hmmm, did you change the kernel on your ChromeOS system, or did Google update it?

My Chromebook was on 3.18 when I was still having stock Google Chrome OS on it.

This should be the relevant tree:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-caroline-chromeos-3.18

>> > Anyway if I recall correctly, the DMI System Vendor should be GOOGLE in
>> > Google's coreboot for Caroline too, but I'm not 100% sure and cannot check
>> > right now.
>> >
>> >
>> FWIW, the DMI system vendor is "Google"  in my Caroline. I'll dig up
>> configuration files later today - maybe it was changed to "GOOGLE" at some
>> point and I have an old Bios in my system.
> 
> I believe all shipping BIOSes use "Google" as vendor, as it is what is
> specified in src/mainboard/google/Kconfig::MAINBOARD_VENDOR and our
> configs/build scripts do not override it.
> 
> I do not know why we have this difference.

Okay, so first of all sorry for the confusion.

The upstream coreboot still does not have Caroline support as far as I can tell.

Instead, I'm using coreboot by Matt aka Mr. Chromebox, who is working towards
upstreaming all of Google Skylake boards, such as Caroline, into coreboot.

His default configs were overriding the CONFIG_MAINBOARD_SMBIOS_MANUFACTURER
option as "GOOGLE" for every board, including recent ones which started to
use "Google". I've contacted him about this, and at the end he will change
the string back to "Google".

>> Thanks,
>> Guenter
>>
>> By the way please note that this is the DMI System Vendor and not the DMI
>> > BIOS version string, which indeed starts with "Google_" in the shipping
>> > BIOS
>> > (but not in coreboot, see f56db262e46d3368ee4e5c9e19797853cab382cd).
> 
> Hmm, I just tried checking out tree from
> https://review.coreboot.org/coreboot.git and do not see this commit:
> 
> dtor@dtor-ws:~/coreboot $ git show f56db262e46d3368ee4e5c9e19797853cab382cd
> fatal: bad object f56db262e46d3368ee4e5c9e19797853cab382cd
> dtor@dtor-ws:~/coreboot $ git log --oneline -1 HEAD
> 82d0f91420 (HEAD -> master, origin/master, origin/HEAD) soc/intel/apollolake: enable exception handling in every stage for glk

Sorry, this was a Linux commit hash:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f56db262e46d3368ee4e5c9e19797853cab382cd

> Anyway, if you are overriding the shipping BIOS, then please adjust
> coreboot to use device-tree compatible bindings (via PRP0001 HID and
> _DSD properties - you can check out bindings for PENH device in Caroline
> to see how they can be used). This way we would not need to breed DMI
> quirks.

This should be the correct fix, at least for custom firmware.

As I said, the string in custom firmware will be changed to "Google", so it
won't match the wrong quirk anymore.

Then the ACPI table in custom firmware (and upstream coreboot in the future)
can be fixed too.

But with stock firmware the situation is unchanged: the touchpad would
still not work correctly because it would be recognized as a touchscreen,
since it wouldn't match any quirk and it wouldn't have the correct ACPI
properties.

Because of this, should I still resend a touchpad patch v3 matching "Google"?

Or will maybe Google integrate the ACPI fix into their stock version of
coreboot too?

> Thanks.

Cheers,
Vittorio G

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Dmitry Torokhov]

On Mon, Apr 30, 2018 at 01:44:07AM +0200, Vittorio Gambaletta (VittGam) wrote:
> Hello,
> 
> On 26/04/2018 18:42:18 CEST, Dmitry Torokhov wrote:
> > On Thu, Apr 26, 2018 at 06:45:32AM -0700, Guenter Roeck wrote:
> >> On Wed, Apr 25, 2018 at 10:37 PM, Vittorio Gambaletta (VittGam) <
> >> linuxbugs@vittgam.net> wrote:
> >>
> >> > Hello,
> >> >
> >> > On 26/04/2018 00:57:34 CEST, Dmitry Torokhov wrote:
> >> > > On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
> >> > >> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam)
> >> > wrote:
> >> > >> > This patch adds the correct platform data information for the Caroline
> >> > >> > Chromebook, so that the mouse button does not get stuck in pressed
> >> > state
> >> > >> > after the first click.
> >> > >> >
> >> > >> > The Samus button keymap and platform data definition are the correct
> >> > >> > ones for Caroline, so they have been reused here.
> >> > >> >
> >> > >> > v2: updated patch offset after 20180409 changes.
> >> > >> >
> >> > >> > Cc: stable@vger.kernel.org
> >> > >> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
> >> > >> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
> >> > >> >
> >> > >>
> >> > >> Applied, thank you.
> >> > >>
> >> > >> > ---
> >> > >> >
> >> > >> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> >> > >> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> >> > >> > @@ -3035,6 +3035,15 @@
> >> > >> >            .driver_data = samus_platform_data,
> >> > >> >    },
> >> > >> >    {
> >> > >> > +          /* Samsung Chromebook Pro */
> >> > >> > +          .ident = "Samsung Chromebook Pro",
> >> > >> > +          .matches = {
> >> > >> > +                  DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> >> > >
> >> > > I've been alerted that shipping BIOS has vendor not capitalized; where
> >> > > did you get the firmware that has vendor all capitals?
> >> >
> >> > Upstream coreboot and mrchromebox's coreboot, which do support loading
> >> > Linux
> >> > natively or with UEFI, both use capitalized GOOGLE as DMI System Vendor
> >> > as far as I know.
> >> >
> >> > By the way, the shipping BIOS (coreboot customized by Google) does not
> >> > support
> >> > mainline Linux natively (eg. without seabios and nasty tricks), but only
> >> > Chromium OS, which for Caroline is an old 3.something fork of Linux, so
> >> > this
> >> > patch does not really apply to Chromium OS or to Linux running on shipping
> >> > BIOS.
> >> >
> >> Can you provide details or a pointer to it ? The mainline kernel boots
> >> fine for me (with a ChromeOS distribution), with the exception of the
> >> Google/GOOGLE problem.
> 
> Hmmm, did you change the kernel on your ChromeOS system, or did Google update it?
> 
> My Chromebook was on 3.18 when I was still having stock Google Chrome OS on it.
> 
> This should be the relevant tree:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-caroline-chromeos-3.18

We normally just pull in a newer kernel into ChromeOS build for testing.
This usually works OK with x86 devices.

> 
> >> > Anyway if I recall correctly, the DMI System Vendor should be GOOGLE in
> >> > Google's coreboot for Caroline too, but I'm not 100% sure and cannot check
> >> > right now.
> >> >
> >> >
> >> FWIW, the DMI system vendor is "Google"  in my Caroline. I'll dig up
> >> configuration files later today - maybe it was changed to "GOOGLE" at some
> >> point and I have an old Bios in my system.
> > 
> > I believe all shipping BIOSes use "Google" as vendor, as it is what is
> > specified in src/mainboard/google/Kconfig::MAINBOARD_VENDOR and our
> > configs/build scripts do not override it.
> > 
> > I do not know why we have this difference.
> 
> Okay, so first of all sorry for the confusion.
> 
> The upstream coreboot still does not have Caroline support as far as I can tell.
> 
> Instead, I'm using coreboot by Matt aka Mr. Chromebox, who is working towards
> upstreaming all of Google Skylake boards, such as Caroline, into coreboot.
> 
> His default configs were overriding the CONFIG_MAINBOARD_SMBIOS_MANUFACTURER
> option as "GOOGLE" for every board, including recent ones which started to
> use "Google". I've contacted him about this, and at the end he will change
> the string back to "Google".

It appears that we have not been consistent with Google/GOOGLE ourselves
(across different products, not between different versions fir the same
product), but it would be great if 3rd party firmware matched the
shipping firmware in this regard, otherwise we'll have issues with DMI
matches.

> 
> >> Thanks,
> >> Guenter
> >>
> >> By the way please note that this is the DMI System Vendor and not the DMI
> >> > BIOS version string, which indeed starts with "Google_" in the shipping
> >> > BIOS
> >> > (but not in coreboot, see f56db262e46d3368ee4e5c9e19797853cab382cd).
> > 
> > Hmm, I just tried checking out tree from
> > https://review.coreboot.org/coreboot.git and do not see this commit:
> > 
> > dtor@dtor-ws:~/coreboot $ git show f56db262e46d3368ee4e5c9e19797853cab382cd
> > fatal: bad object f56db262e46d3368ee4e5c9e19797853cab382cd
> > dtor@dtor-ws:~/coreboot $ git log --oneline -1 HEAD
> > 82d0f91420 (HEAD -> master, origin/master, origin/HEAD) soc/intel/apollolake: enable exception handling in every stage for glk
> 
> Sorry, this was a Linux commit hash:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f56db262e46d3368ee4e5c9e19797853cab382cd

OK, I see. I'd say we should not have taken this in.

> 
> > Anyway, if you are overriding the shipping BIOS, then please adjust
> > coreboot to use device-tree compatible bindings (via PRP0001 HID and
> > _DSD properties - you can check out bindings for PENH device in Caroline
> > to see how they can be used). This way we would not need to breed DMI
> > quirks.
> 
> This should be the correct fix, at least for custom firmware.
> 
> As I said, the string in custom firmware will be changed to "Google", so it
> won't match the wrong quirk anymore.
> 
> Then the ACPI table in custom firmware (and upstream coreboot in the future)
> can be fixed too.
> 
> But with stock firmware the situation is unchanged: the touchpad would
> still not work correctly because it would be recognized as a touchscreen,
> since it wouldn't match any quirk and it wouldn't have the correct ACPI
> properties.
> 
> Because of this, should I still resend a touchpad patch v3 matching "Google"?

I can fix it up here, no need to resubmit.

> 
> Or will maybe Google integrate the ACPI fix into their stock version of
> coreboot too?

The PRP0001? No, as we do not currently have plans to move Caroline to a
newer kernel and 3.18 does not have infrastructure for generic device
properties. The pen binding was done when we were considering pulling in
generic device properties into 3.18, but later decided against it.

Thanks.

-- 
Dmitry

Re: [PATCH v2] input/touchscreen: atmel_mxt_ts: Add correct touchpad button mapping for the Caroline Chromebook.

[Vittorio Gambaletta (VittGam)]

Hello,

On 30/04/2018 19:38:59 CEST, Dmitry Torokhov wrote:
> On Mon, Apr 30, 2018 at 01:44:07AM +0200, Vittorio Gambaletta (VittGam) wrote:
>> Hello,
>>
>> On 26/04/2018 18:42:18 CEST, Dmitry Torokhov wrote:
>> > On Thu, Apr 26, 2018 at 06:45:32AM -0700, Guenter Roeck wrote:
>> >> On Wed, Apr 25, 2018 at 10:37 PM, Vittorio Gambaletta (VittGam) <
>> >> linuxbugs@vittgam.net> wrote:
>> >>
>> >> > Hello,
>> >> >
>> >> > On 26/04/2018 00:57:34 CEST, Dmitry Torokhov wrote:
>> >> > > On Wed, Apr 25, 2018 at 03:26:50PM -0700, Dmitry Torokhov wrote:
>> >> > >> On Wed, Apr 25, 2018 at 02:32:58PM +0200, Vittorio Gambaletta (VittGam)
>> >> > wrote:
>> >> > >> > This patch adds the correct platform data information for the Caroline
>> >> > >> > Chromebook, so that the mouse button does not get stuck in pressed
>> >> > state
>> >> > >> > after the first click.
>> >> > >> >
>> >> > >> > The Samus button keymap and platform data definition are the correct
>> >> > >> > ones for Caroline, so they have been reused here.
>> >> > >> >
>> >> > >> > v2: updated patch offset after 20180409 changes.
>> >> > >> >
>> >> > >> > Cc: stable@vger.kernel.org
>> >> > >> > Signed-off-by: Vittorio Gambaletta <linuxbugs@vittgam.net>
>> >> > >> > Signed-off-by: Salvatore Bellizzi <lkml@seppia.net>
>> >> > >> >
>> >> > >>
>> >> > >> Applied, thank you.
>> >> > >>
>> >> > >> > ---
>> >> > >> >
>> >> > >> > --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> >> > >> > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> >> > >> > @@ -3035,6 +3035,15 @@
>> >> > >> >            .driver_data = samus_platform_data,
>> >> > >> >    },
>> >> > >> >    {
>> >> > >> > +          /* Samsung Chromebook Pro */
>> >> > >> > +          .ident = "Samsung Chromebook Pro",
>> >> > >> > +          .matches = {
>> >> > >> > +                  DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
>> >> > >
>> >> > > I've been alerted that shipping BIOS has vendor not capitalized; where
>> >> > > did you get the firmware that has vendor all capitals?
>> >> >
>> >> > Upstream coreboot and mrchromebox's coreboot, which do support loading
>> >> > Linux
>> >> > natively or with UEFI, both use capitalized GOOGLE as DMI System Vendor
>> >> > as far as I know.
>> >> >
>> >> > By the way, the shipping BIOS (coreboot customized by Google) does not
>> >> > support
>> >> > mainline Linux natively (eg. without seabios and nasty tricks), but only
>> >> > Chromium OS, which for Caroline is an old 3.something fork of Linux, so
>> >> > this
>> >> > patch does not really apply to Chromium OS or to Linux running on shipping
>> >> > BIOS.
>> >> >
>> >> Can you provide details or a pointer to it ? The mainline kernel boots
>> >> fine for me (with a ChromeOS distribution), with the exception of the
>> >> Google/GOOGLE problem.
>>
>> Hmmm, did you change the kernel on your ChromeOS system, or did Google update it?
>>
>> My Chromebook was on 3.18 when I was still having stock Google Chrome OS on it.
>>
>> This should be the relevant tree:
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/release-R58-9334.B-caroline-chromeos-3.18
> 
> We normally just pull in a newer kernel into ChromeOS build for testing.
> This usually works OK with x86 devices.

Ok, nice to know!

>> >> > Anyway if I recall correctly, the DMI System Vendor should be GOOGLE in
>> >> > Google's coreboot for Caroline too, but I'm not 100% sure and cannot check
>> >> > right now.
>> >> >
>> >> >
>> >> FWIW, the DMI system vendor is "Google"  in my Caroline. I'll dig up
>> >> configuration files later today - maybe it was changed to "GOOGLE" at some
>> >> point and I have an old Bios in my system.
>> >
>> > I believe all shipping BIOSes use "Google" as vendor, as it is what is
>> > specified in src/mainboard/google/Kconfig::MAINBOARD_VENDOR and our
>> > configs/build scripts do not override it.
>> >
>> > I do not know why we have this difference.
>>
>> Okay, so first of all sorry for the confusion.
>>
>> The upstream coreboot still does not have Caroline support as far as I can tell.
>>
>> Instead, I'm using coreboot by Matt aka Mr. Chromebox, who is working towards
>> upstreaming all of Google Skylake boards, such as Caroline, into coreboot.
>>
>> His default configs were overriding the CONFIG_MAINBOARD_SMBIOS_MANUFACTURER
>> option as "GOOGLE" for every board, including recent ones which started to
>> use "Google". I've contacted him about this, and at the end he will change
>> the string back to "Google".
> 
> It appears that we have not been consistent with Google/GOOGLE ourselves
> (across different products, not between different versions fir the same
> product), but it would be great if 3rd party firmware matched the
> shipping firmware in this regard, otherwise we'll have issues with DMI
> matches.
> 
>>
>> >> Thanks,
>> >> Guenter
>> >>
>> >> By the way please note that this is the DMI System Vendor and not the DMI
>> >> > BIOS version string, which indeed starts with "Google_" in the shipping
>> >> > BIOS
>> >> > (but not in coreboot, see f56db262e46d3368ee4e5c9e19797853cab382cd).
>> >
>> > Hmm, I just tried checking out tree from
>> > https://review.coreboot.org/coreboot.git and do not see this commit:
>> >
>> > dtor@dtor-ws:~/coreboot $ git show f56db262e46d3368ee4e5c9e19797853cab382cd
>> > fatal: bad object f56db262e46d3368ee4e5c9e19797853cab382cd
>> > dtor@dtor-ws:~/coreboot $ git log --oneline -1 HEAD
>> > 82d0f91420 (HEAD -> master, origin/master, origin/HEAD) soc/intel/apollolake: enable exception handling in every stage for glk
>>
>> Sorry, this was a Linux commit hash:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f56db262e46d3368ee4e5c9e19797853cab382cd
> 
> OK, I see. I'd say we should not have taken this in.

At least on Caroline, when booting with SeaBIOS on shipping BIOS then the
DMI BIOS version string will be empty.

So the lowercase is needed for Caroline even for shipping BIOS (and I'm
going to send a new patch adding the lowercase match there), but the
uppercase match might still be needed for other products that exhibit the
same behavior with SeaBIOS.

Do you have any data about this perhaps?

>> > Anyway, if you are overriding the shipping BIOS, then please adjust
>> > coreboot to use device-tree compatible bindings (via PRP0001 HID and
>> > _DSD properties - you can check out bindings for PENH device in Caroline
>> > to see how they can be used). This way we would not need to breed DMI
>> > quirks.
>>
>> This should be the correct fix, at least for custom firmware.
>>
>> As I said, the string in custom firmware will be changed to "Google", so it
>> won't match the wrong quirk anymore.
>>
>> Then the ACPI table in custom firmware (and upstream coreboot in the future)
>> can be fixed too.
>>
>> But with stock firmware the situation is unchanged: the touchpad would
>> still not work correctly because it would be recognized as a touchscreen,
>> since it wouldn't match any quirk and it wouldn't have the correct ACPI
>> properties.
>>
>> Because of this, should I still resend a touchpad patch v3 matching "Google"?
> 
> I can fix it up here, no need to resubmit.

Ok, perfect, thanks.

>> Or will maybe Google integrate the ACPI fix into their stock version of
>> coreboot too?
> 
> The PRP0001? No, as we do not currently have plans to move Caroline to a
> newer kernel and 3.18 does not have infrastructure for generic device
> properties. The pen binding was done when we were considering pulling in
> generic device properties into 3.18, but later decided against it.

Oh, ok. So with the atmel_mxt_ts quirk now in place I'm not sure if it's
still worth it adding the PRP0001 to custom (future upstream) coreboot...

> Thanks.

Cheers,
Vittorio