Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS

[Enric Balletbo i Serra]

Hi Rafael,

On 11/6/20 13:06, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 11/6/20 0:43, Dmitry Torokhov wrote:
>> On Wed, Jun 10, 2020 at 09:52:12PM +0000, Mario.Limonciello@xxxxxxxx wrote:
>>>> -----Original Message-----
>>>> From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>>>> Sent: Wednesday, June 10, 2020 4:41 PM
>>>> To: Limonciello, Mario
>>>> Cc: enric.balletbo@xxxxxxxxxxxxx; rjw@xxxxxxxxxxxxx; rafael@xxxxxxxxxx;
>>>> linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; lenb@xxxxxxxxxx;
>>>> kernel@xxxxxxxxxxxxx; groeck@xxxxxxxxxxxx; bleung@xxxxxxxxxxxx;
>>>> dtor@xxxxxxxxxxxx; gwendal@xxxxxxxxxxxx; vbendeb@xxxxxxxxxxxx;
>>>> andy@xxxxxxxxxxxxx; ayman.bagabas@xxxxxxxxx; benjamin.tissoires@xxxxxxxxxx;
>>>> blaz@xxxxxxx; dvhart@xxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
>>>> hdegoede@xxxxxxxxxx; jeremy@xxxxxxxxxxxx; 2pi@xxxxxx;
>>>> mchehab+samsung@xxxxxxxxxx; rajatja@xxxxxxxxxx;
>>>> srinivas.pandruvada@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v4] platform: x86: Add ACPI driver for ChromeOS
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> On Wed, Jun 10, 2020 at 09:28:36PM +0000, Mario.Limonciello@xxxxxxxx wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> If we use this prefix for hatch EOL is ~7 years from now.
>>>>
>>>
>>> Isn't the whole point of the ACPI registry and choosing an ID?  You know internally
>>> if you need to change the interface that a new ID is needed and a new driver will
>>> be needed that comprehends that ID change.  So if you can't guarantee that 0001 is
>>> the same driver interface in every firmware implementation google used then that is
>>> where this falls apart.
>>>
> 
> As far as I know GGL0001 has the same driver interface in every firmware
> implementation Google used. But I'll ask to make sure.
> 
>>> I know there is a long support lifecycle but you're talking about rebasing
>>> to new LTS kernels a handful of times between now and then.  If the interface really
>>> is stable the patch should be small and it shouldn't be a large amount of technical
>>> debt to carry downstream until EOL.
>>
>> I think we are talking about different things actually. Let's forget
>> about Chrome OS and downstream kernels. We have devices that have
>> already been shipped and in hands of users. Some of them are old, some
>> of them are new. We can't not enforce that firmware for these devices
>> will be either released or updated. Therefore, if we want expose this
>> device in mainline kernel, we need to have it handle "GGL0001" HID in
>> addition to whatever proper HID we may select for it.
>>
> 
> FWIW, after investigate a bit more, although GGL is not in the ACPI ID list it
> is in the PNP ID list [1]. So as far as I understand GGL0001 is valid ID. I know
> that PNP ID is the legacy identifier but since this was here for long time and
> will be here also for long time, I am wondering if we can take that as an
> argument to have GGL0001 as a valid device to be exposed in the kernel.
> 

So, as the GGL prefix is a valid ID in the PNP ID list, is this a valid argument
to take in consideration this patch and resolves your concern regarding the ID?

Thanks,
 Enric



> Thanks,
>  Enric
> 
> [1] https://uefi.org/pnp_id_list
> 
> 
>> We internally can fix it (HID) for next generation of devices.
>>
>> Thanks.
>>
>