diff mbox series

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

Message ID 20241219200821.8328-1-maccraft123mc@gmail.com (mailing list archive)
State New
Headers show
Series [v2,1/3] dt-bindings: platform: Add bindings for Qcom's EC on IT8987 | expand

Commit Message

Maya Matuszczyk Dec. 19, 2024, 8:08 p.m. UTC 
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

Comments

Rob Herring Dec. 19, 2024, 11:40 p.m. UTC | #1 
On Thu, 19 Dec 2024 21:08:18 +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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/platform/qcom,x1e-it8987-ec.yaml:22:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241219200821.8328-1-maccraft123mc@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Stephan Gerhold Dec. 20, 2024, 6:05 p.m. UTC | #2 
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:

> +      - items:
> +        - const: lenovo,yoga-slim7x-ec
> +        - const: qcom,x1e-it8987-ec

People can add compatible entries for other devices as needed.

Thanks,
Stephan
Stephan Gerhold Dec. 20, 2024, 6:24 p.m. UTC | #3 
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
diff mbox series

Patch

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
+      - items:
+        - const: lenovo,yoga-slim7x-ec
+        - const: qcom,x1e-it8987-ec
+
+  reg:
+    const: 0x76
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        embedded-controller@76 {
+            compatible = "lenovo,yoga-slim7x-ec", "qcom,x1e-it8987-ec";
+            reg = <0x76>;
+
+            interrupts-extended = <&tlmm 66 IRQ_TYPE_LEVEL_HIGH>;
+        };
+    };
+...