| Message ID | 20241113023800.6263-2-drafnel@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/chrome: cros_kbd_led_backlight: support Google Link | expand |
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
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
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
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
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.
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
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.
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);
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(+)