Re: [PATCH v2 1/3] dt-bindings: platform: Add bindings for Qcom's EC on IT8987

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/12/2024 19:24, Stephan Gerhold wrote:
> On Fri, Dec 20, 2024 at 07:16:34PM +0100, Maya Matuszczyk wrote:
>> Excuse the formatting, I've typed this reply from my phone
>>
>> pt., 20 gru 2024, 19:05 użytkownik Stephan Gerhold <
>> stephan.gerhold@linaro.org> napisał:
>>
>>> On Thu, Dec 19, 2024 at 09:08:18PM +0100, Maya Matuszczyk wrote:
>>>> This patch adds bindings for the EC firmware running on IT8987 present
>>>> on most of X1E80100 devices
>>>>
>>>> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
>>>> ---
>>>>  .../bindings/platform/qcom,x1e-it8987-ec.yaml | 52 +++++++++++++++++++
>>>>  1 file changed, 52 insertions(+)
>>>>  create mode 100644
>>> Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
>>>>
>>>> diff --git
>>> a/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
>>> b/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
>>>> new file mode 100644
>>>> index 000000000000..4a4f6eb63072
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml
>>>> @@ -0,0 +1,52 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/platform/qcom,x1e-it8987-ec.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Qualcomm Embedded Controller on IT8987 chip.
>>>> +
>>>> +maintainers:
>>>> +  - Maya Matuszczyk <maccraft123mc@gmail.com>
>>>> +
>>>> +description:
>>>> +  Most x1e80100 laptops have an EC running on IT8987 MCU chip. The EC
>>> controls
>>>> +  minor functions, like fans, power LED, and on some laptops it also
>>> handles
>>>> +  keyboard hotkeys.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - const: qcom,x1e-it8987-ec
>>>
>>> Given that ECs tend to be somewhat device-specific and many vendors
>>> might have slightly customized the EC firmware(?), I think it would be
>>> better to disallow using this generic compatible without a more specific
>>> one. In other words, I would drop this line and just keep the case
>>> below:
>>>
>> I've looked at DSDT of other devices and they look to be compatible with
>> what's on the devkit, with differences being extra features on magicbook
>> art 14 and yoga slim 7x. Though this isn't a hill I'm willing to die on.
>>
> 
> I think it's fine to keep qcom,x1e-it8987-ec as second compatible.


No, because:
1. There is no such thing as x1e
2. If there was a soc like this, this has nothing to do with SoC. It is
not a component inside SoC and that is the only allowed case when you
use SoC compatibles.

> However, without a more specific compatible, there is a risk we have
> nothing to match on in case device-specific handling becomes necessary
> in the driver at some point.
> 
> It's certainly subjective, but it might be better to play it safe here
> and have a specific compatible that one can match, even if the behavior
> is 99% the same. There will often be subtly different behavior across
> devices, you mentioned the "keyboard backlight turning off and the power
> LED slowly blinking", who knows what else exists.
> 
> I suppose worst case we could also use of_machine_is_compatible() to
> just match the device the EC is running on, but I'm not sure if that
> would be frowned upon.


Unless you have some sort of insights or secret knowledge from Qualcomm
(Bjorn or Konrad can chime in here), I think these are pure guesses that
this is a Qualcomm product (implied by vendor prefix) or some sort of
re-usable generic firmware from Qualcomm present on multiple devices.

If the FW across devices is the same, then fallbacks for these are fine
with me.

Best regards,
Krzysztof