detect kbd backlight on Google Link (original Chromebook Pixel)

detect kbd backlight on Google Link (original Chromebook Pixel)

[Brandon Casey]

Hi,

Hopefully this is a trivial and obviously correct patch that will be
easy to accept.  Tested on my very own Chromebook Pixel.

Please advise if this is not the correct mailing list or procedure for
submitting this patch, and/or if there is anything else that I need to
do.

Thanks,
-Brandon

[PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Brandon Casey]

On the Google Pixel chromebook (aka Google Link), the keyboard led
backlight has the ACPI device id "GGL0002".  Let's add it to the
configuration array.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 drivers/platform/chrome/cros_kbd_led_backlight.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index 78097c8a4966..5b7766feb10d 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -268,6 +268,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id keyboard_led_acpi_match[] = {
 	{ "GOOG0002", (kernel_ulong_t)&keyboard_led_drvdata_acpi },
+	{ "GGL0002", (kernel_ulong_t)&keyboard_led_drvdata_acpi },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, keyboard_led_acpi_match);
-- 
2.43.0

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Tzung-Bi Shih]

On Tue, Nov 12, 2024 at 08:29:05PM -0600, Brandon Casey wrote:
> On the Google Pixel chromebook (aka Google Link), the keyboard led
> backlight has the ACPI device id "GGL0002".  Let's add it to the
> configuration array.

Why it can't be matched by "GOOG0002"? [1]  Have you tried to update your AP
firmware?

[1]: https://review.coreboot.org/c/coreboot/+/11478

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Brandon Casey]

On November 13, 2024 12:46:18 AM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>On Tue, Nov 12, 2024 at 08:29:05PM -0600, Brandon Casey wrote:
>> On the Google Pixel chromebook (aka Google Link), the keyboard led
>> backlight has the ACPI device id "GGL0002".  Let's add it to the
>> configuration array.
>
>Why it can't be matched by "GOOG0002"? [1] 

I'm not sure I understand this question. On my device the backlight has ACPI id "GGL0002", not "GOOG0002"?

>Have you tried to update your AP firmware?

I have not explicitly updated any firmware. It has whatever firmware Chromeos has installed.

-Brandon

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Tzung-Bi Shih]

On Wed, Nov 13, 2024 at 02:44:55AM -0600, Brandon Casey wrote:
> 
> 
> On November 13, 2024 12:46:18 AM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >On Tue, Nov 12, 2024 at 08:29:05PM -0600, Brandon Casey wrote:
> >> On the Google Pixel chromebook (aka Google Link), the keyboard led
> >> backlight has the ACPI device id "GGL0002".  Let's add it to the
> >> configuration array.
> >
> >Why it can't be matched by "GOOG0002"? [1] 
> 
> I'm not sure I understand this question. On my device the backlight has ACPI id "GGL0002", not "GOOG0002"?
> 
> >Have you tried to update your AP firmware?
> 
> I have not explicitly updated any firmware. It has whatever firmware Chromeos has installed.

IIUC, if your AP firmware includes patch [1] (submitted in 2015), it should
report "GOOG0002" instead of "GGL0002".

[1]: https://review.coreboot.org/c/coreboot/+/11478

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Brandon Casey]

On November 13, 2024 8:07:42 PM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>On Wed, Nov 13, 2024 at 02:44:55AM -0600, Brandon Casey wrote:
>> 
>> 
>> On November 13, 2024 12:46:18 AM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>> >On Tue, Nov 12, 2024 at 08:29:05PM -0600, Brandon Casey wrote:
>> >> On the Google Pixel chromebook (aka Google Link), the keyboard led
>> >> backlight has the ACPI device id "GGL0002".  Let's add it to the
>> >> configuration array.
>> >
>> >Why it can't be matched by "GOOG0002"? [1] 
>> 
>> I'm not sure I understand this question. On my device the backlight has ACPI id "GGL0002", not "GOOG0002"?
>> 
>> >Have you tried to update your AP firmware?
>> 
>> I have not explicitly updated any firmware. It has whatever firmware Chromeos has installed.
>
>IIUC, if your AP firmware includes patch [1] (submitted in 2015), it should
>report "GOOG0002" instead of "GGL0002".
>
>[1]: https://review.coreboot.org/c/coreboot/+/11478

I can only assume that Google did not ever ship a firmware update for Link that included that patch. I believe this device has all ChromeOS updates applied, so it represents a stock system, though no longer supported, and aside from being in developer mode and having a Linux partition.

My firmware also reports the acpi device as GGL0001.

    $ cat /sys/bus/acpi/devices/GGL000*/hid
    GGL0001
    GGL0002

    $ cat /sys/bus/acpi/devices/GGL000*/path
    \CRHW
    \_SB_.KBLT

Is there any reason why we shouldn't do the same thing here for the keyboard backlight that was done in drivers/platform/chrome/chromeos_acpi.c and support both the legacy ID and the official ACPI ID? I see your commit 703e77134 which added GOOG0016 in addition to GGL0001.

-Brandon

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Tzung-Bi Shih]

On Wed, Nov 13, 2024 at 10:38:36PM -0600, Brandon Casey wrote:
> 
> 
> On November 13, 2024 8:07:42 PM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >On Wed, Nov 13, 2024 at 02:44:55AM -0600, Brandon Casey wrote:
> >> 
> >> 
> >> On November 13, 2024 12:46:18 AM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
> >> >On Tue, Nov 12, 2024 at 08:29:05PM -0600, Brandon Casey wrote:
> >> >> On the Google Pixel chromebook (aka Google Link), the keyboard led
> >> >> backlight has the ACPI device id "GGL0002".  Let's add it to the
> >> >> configuration array.
> >> >
> >> >Why it can't be matched by "GOOG0002"? [1] 
> >> 
> >> I'm not sure I understand this question. On my device the backlight has ACPI id "GGL0002", not "GOOG0002"?
> >> 
> >> >Have you tried to update your AP firmware?
> >> 
> >> I have not explicitly updated any firmware. It has whatever firmware Chromeos has installed.
> >
> >IIUC, if your AP firmware includes patch [1] (submitted in 2015), it should
> >report "GOOG0002" instead of "GGL0002".
> >
> >[1]: https://review.coreboot.org/c/coreboot/+/11478
> 
> I can only assume that Google did not ever ship a firmware update for Link that included that patch. I believe this device has all ChromeOS updates applied, so it represents a stock system, though no longer supported, and aside from being in developer mode and having a Linux partition.

Please try to wrap your message to fit to 80-column.  See [2].

[2]: https://people.kernel.org/tglx/notes-about-netiquette

> My firmware also reports the acpi device as GGL0001.
> 
>     $ cat /sys/bus/acpi/devices/GGL000*/hid
>     GGL0001
>     GGL0002
> 
>     $ cat /sys/bus/acpi/devices/GGL000*/path
>     \CRHW
>     \_SB_.KBLT
> 
> Is there any reason why we shouldn't do the same thing here for the keyboard backlight that was done in drivers/platform/chrome/chromeos_acpi.c and support both the legacy ID and the official ACPI ID? I see your commit 703e77134 which added GOOG0016 in addition to GGL0001.

- The most important reason: "GGL0002" is assigned to other purpose.
- For some reason, your machine is using a quite old firmware.  It makes
  less sense to me for supporting a should-be-deprecated PNP ID by [1].
- The case is different from 703e77134.  We prefer HID to PNP ID.

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Brandon Casey]

On November 14, 2024 2:53:47 AM CST, Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>On Wed, Nov 13, 2024 at 10:38:36PM -0600, Brandon Casey wrote:

>> I can only assume that Google did not ever ship a firmware update
>> for Link that included that patch. I believe this device has all
>> ChromeOS updates applied, so it represents a stock
>> system, though no longer supported, and aside from being in
>> developer mode and having a Linux partition.

>Please try to wrap your message to fit to 80-column.  See [2].

Sorry about that. I didn't realize my email client was doing that.

>> Is there any reason why we shouldn't do the same thing here for the 
>> keyboard backlight that was done in drivers/platform/chrome/chromeos_acpi.c
>> and support both the legacy ID and the official ACPI ID? I see your
>> commit 703e77134 which added GOOG0016 in addition to GGL0001.
>
>- The most important reason: "GGL0002" is assigned to other purpose.

Ah, so this seems to be the crux of it :-(

>- For some reason, your machine is using a quite old firmware.  It makes
>  less sense to me for supporting a should-be-deprecated PNP ID by [1].

You seem to be suggesting that my machine is in some odd/abnormal state.
Do you know whether Google has actually ever pushed an updated firmware
for this device (LINK) that included the patch referenced in [1]? If not, then
shouldn't my machine be considered to be in the "normal" state, not the
oddity?

-Brandon

Re: [PATCH] platform/chrome: cros_kbd_led_backlight: support Google Link

[Tzung-Bi Shih]

On Mon, Nov 18, 2024 at 12:58:52PM -0600, Brandon Casey wrote:
> You seem to be suggesting that my machine is in some odd/abnormal state.
> Do you know whether Google has actually ever pushed an updated firmware
> for this device (LINK) that included the patch referenced in [1]? If not, then
> shouldn't my machine be considered to be in the "normal" state, not the
> oddity?

No, I don't know.