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

[Stephan Gerhold <stephan.gerhold@linaro.org>]

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.
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.

Thanks,
Stephan