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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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.