> -----Original Message----- > From: platform-driver-x86-owner@xxxxxxxxxxxxxxx <platform-driver-x86- > owner@xxxxxxxxxxxxxxx> On Behalf Of Enric Balletbo i Serra > Sent: Wednesday, June 10, 2020 4:22 PM > To: Rafael J. Wysocki > Cc: Rafael J. Wysocki; Linux Kernel Mailing List; ACPI Devel Maling List; > Len Brown; Collabora Kernel ML; Guenter Roeck; Benson Leung; Dmitry > Torokhov; Gwendal Grignou; vbendeb@xxxxxxxxxxxx; Andy Shevchenko; Ayman > Bagabas; Benjamin Tissoires; Blaž Hrastnik; Darren Hart; Dmitry Torokhov; > Greg Kroah-Hartman; Hans de Goede; Jeremy Soller; Mattias Jacobsson; Mauro > Carvalho Chehab; Rajat Jain; Srinivas Pandruvada; platform-driver- > x86@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS > > > [EXTERNAL EMAIL] > > Hi Rafael, > > Many thanks for your feedback. See my answers inline. > > On 5/6/20 13:17, Rafael J. Wysocki wrote: > > On Tuesday, April 14, 2020 4:35:38 PM CEST Enric Balletbo i Serra wrote: > >> Hi Rafael, > >> > >> On 13/4/20 22:41, Rafael J. Wysocki wrote: > >>> On Mon, Apr 13, 2020 at 3:46 PM Enric Balletbo i Serra > >>> <enric.balletbo@xxxxxxxxxxxxx> wrote: > >>>> > >>>> This driver attaches to the ChromeOS ACPI device and then exports the > values > >>>> reported by the ACPI in a sysfs directory. These values are not > exported > >>>> via the standard ACPI tables, hence a specific driver is needed to do > >>>> it. > >>> > >>> So how exactly are they exported? > >>> > >> > >> They are exported through sysfs. > >> > >>>> The ACPI values are presented in the string form (numbers as decimal > >>>> values) or binary blobs, and can be accessed as the contents of the > >>>> appropriate read only files in the standard ACPI devices sysfs > directory tree. > >>> > >>> My understanding based on a cursory look at the patch is that there is > >>> an ACPI device with _HID equal to "GGL0001" and one or more special > >>> methods under it that return values which you want to export over > >>> sysfs as binary attributes. They appear to be read-only. > >>> > >> > >> Exactly, there is an ACPI device equal to "GGL0001" and one special > method > >> called MLST that returns a list of the other control methods supported > by the > >> Chrome OS hardware device. The driver calls the special MLST method and > goes > >> through the list. > >> > >>> I guess that these data are to be consubed by user space? > >>> > >> > >> Yes, this is used by user space, to be more specific ChromeOS userspace > uses it. > > > > Well, let me start over. > > > > The subject and changelog of this patch are not precise enough IMO and > there is > > not enough information in the latter. > > > > Ok, I can improve that. > > > It is not clear what "ACPI driver for ChromeOS" means. There may be many > ACPI > > drivers in a Linux-based system as a rule. > > > > It is unclear what the ChromeOS ACPI device is and why it is there. Is > there > > any documentation of it you can point me to? > > > > I'm afraid, I don't think there is any documentation, I'll ask around. > > > It is unclear what you mean by "These values are not exported via the > standard > > ACPI tables". > > > > It looks like (but it is not actually documented in any way) the idea is > to > > get to the ACPI device object with _HID returning "GGL0001", evaluate the > > MLST method under it and then evaluate the methods listed by it and > export the > > data returned by them via sysfs, under the "GGL0001" device on the "acpi" > bus. > > Is this correct? > > > > Yes, this is correct. > > > If so, there is a couple of issues here. > > > > First off, GGL0001 is not a valid ACPI device ID, because the GGL prefix > is not > > present in the list at https://uefi.org/acpi_id_list > > > > There are two ways to address that. One would be to take the GOOG prefix > > (present in the list above), append a proper unique number (if I were to > > guess, I would say that 0001 had been reserved already) to it and then > > put the resulting device ID into the firmware, to be returned _HID for > the > > device in question (you can add a _CID returning "GGL0001" so it can be > > found by the old invalid ID at least from the kernel). > > As Dmitry said, this is not going to happen. I think it's probably worth grouping "existing" platforms and new platforms separately. More below. > > > > The other one would > > be to properly register the GGL prefix for Google and establish a process > for > > allocating IDs with that prefix internally. > > > > IIUC I think this is the option we should go, although I am not really sure > how > to do it (I will investigate or ask). > > To give you some references, if I'm not wrong, this prefix is used in all > or > almost all Intel Chromebook devices (auron, cyan, eve, fizz, hatch, > octopus, > poppy, strago ...) The ACPI source for this device can be found here [1], > and, > if not all, almost all Intel based Chromebooks are shipped with the > firmware > that supports this. You can potentially carry a small patch in your downstream kernel for the legacy stuff until it reaches EOL. At least for the new stuff you could enact a process that properly reserves unique numbers and changes the driver when the interface provided by the ACPI device has changed. > > > Next, device attributes in sysfs are part of the kernel ABI and once > defined, > > they cannot change (exceptions happen, but rarely), so you must guarantee > > that whatever appears in there, will always be present for devices with > the > > given device ID in the future in the same format. > > > > Can you actually guarantee that? If so, what is that guarantee based on? > > > > Although is not really documented, can we say that this is a standard "de > facto" > assuming that there are lots of devices in the field and for a long time > with > that? Can this be a guarantee? > > > Thanks! > > > > > > > > Thanks! > > [1] > https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/he > ads/chromeos-2016.05/src/vendorcode/google/chromeos/acpi/chromeos.asl
