| Message ID | 20220512013921.164637-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dt-bindings: cros-ec: Update for fingerprint devices | expand |
Hi, On Wed, May 11, 2022 at 6:39 PM Stephen Boyd <swboyd@chromium.org> wrote: > > @@ -176,6 +189,37 @@ allOf: > - reg > - interrupts > > + - if: > + properties: > + compatible: > + contains: > + const: google,cros-ec-fp > + then: > + properties: > + '#address-cells': false > + '#size-cells': false > + typec: false > + ec-pwm: false > + keyboard-controller: false > + proximity: false > + codecs: false > + cbas: false > + > + patternProperties: > + "^i2c-tunnel[0-9]*$": false > + "^regulator@[0-9]+$": false > + "^extcon[0-9]*$": false > + > + required: > + - reset-gpios > + - boot0-gpios > + - vdd-supply > + else: > + properties: > + reset-gpios: false > + boot0-gpios: false > + vdd-supply: false Wow, that's a huge pain to have to specify it this way, but I don't know of any other way to make things happy and still have the "google,cros-ec-spi" fallback that we decided on having in previous versions. > additionalProperties: false > > examples: > @@ -231,4 +275,22 @@ examples: > compatible = "google,cros-ec-rpmsg"; > }; > }; > + > + - | nit: all the other examples have a little "# For <blah>" comment before them. It's not really necessary, but do we want one here to match? In any case, it seems OK to me. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Wed, May 11, 2022 at 06:39:21PM -0700, Stephen Boyd wrote: > Add a binding to describe the fingerprint processor found on Chromebooks > with a fingerprint sensor. Previously we've been describing this with > the google,cros-ec-spi binding but it lacks gpio and regulator control > used during firmware flashing. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > Cc: <devicetree@vger.kernel.org> > Cc: <chrome-platform@lists.linux.dev> > Cc: Guenter Roeck <groeck@chromium.org> > Cc: Douglas Anderson <dianders@chromium.org> > Cc: Craig Hesling <hesling@chromium.org> > Cc: Tom Hughes <tomhughes@chromium.org> > Cc: Alexandru M Stan <amstan@chromium.org> > Cc: Tzung-Bi Shih <tzungbi@kernel.org> > Cc: Matthias Kaehlcke <mka@chromium.org> > Cc: Benson Leung <bleung@chromium.org> > Cc: Lee Jones <lee.jones@linaro.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > .../bindings/mfd/google,cros-ec.yaml | 80 ++++++++++++++++--- > 1 file changed, 71 insertions(+), 9 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > index 409ecef967ce..e5fe60beb9fe 100644 > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > @@ -19,15 +19,19 @@ description: > properties: > compatible: > oneOf: > - - description: > - For implementations of the EC is connected through I2C. > - const: google,cros-ec-i2c > - - description: > - For implementations of the EC is connected through SPI. > - const: google,cros-ec-spi > - - description: > - For implementations of the EC is connected through RPMSG. > - const: google,cros-ec-rpmsg > + # For implementations of the EC is connected through I2C. Using 'description' was preferred. > + - items: > + - const: google,cros-ec-i2c > + # For implementations of the FPMCU connected through SPI. > + - items: > + - const: google,cros-ec-fp > + - const: google,cros-ec-spi > + # For implementations of the EC is connected through SPI. > + - items: > + - const: google,cros-ec-spi > + # For implementations of the EC is connected through RPMSG. > + - items: > + - const: google,cros-ec-rpmsg > > controller-data: > description: > @@ -132,6 +136,15 @@ properties: > > additionalProperties: false > > + reset-gpios: > + maxItems: 1 > + > + boot0-gpios: > + maxItems: 1 > + description: Assert for bootloader mode. > + > + vdd-supply: true > + > patternProperties: > "^i2c-tunnel[0-9]*$": > type: object > @@ -176,6 +189,37 @@ allOf: > - reg > - interrupts > > + - if: > + properties: > + compatible: > + contains: > + const: google,cros-ec-fp > + then: > + properties: > + '#address-cells': false > + '#size-cells': false > + typec: false > + ec-pwm: false > + keyboard-controller: false > + proximity: false > + codecs: false > + cbas: false > + > + patternProperties: > + "^i2c-tunnel[0-9]*$": false > + "^regulator@[0-9]+$": false > + "^extcon[0-9]*$": false Is the list of what's allowed shorter? If so, you could list those properties and use 'additionalProperties: false'. Or maybe this is a sign that this should be a separate schema document. > + > + required: > + - reset-gpios > + - boot0-gpios > + - vdd-supply > + else: > + properties: > + reset-gpios: false > + boot0-gpios: false > + vdd-supply: false
Quoting Rob Herring (2022-05-16 08:20:03) > On Wed, May 11, 2022 at 06:39:21PM -0700, Stephen Boyd wrote: > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > index 409ecef967ce..e5fe60beb9fe 100644 > > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > @@ -19,15 +19,19 @@ description: > > properties: > > compatible: > > oneOf: > > - - description: > > - For implementations of the EC is connected through I2C. > > - const: google,cros-ec-i2c > > - - description: > > - For implementations of the EC is connected through SPI. > > - const: google,cros-ec-spi > > - - description: > > - For implementations of the EC is connected through RPMSG. > > - const: google,cros-ec-rpmsg > > + # For implementations of the EC is connected through I2C. > > Using 'description' was preferred. I just learned that I should use anyOf instead of oneOf. Confusing! > > > + - items: > > + - const: google,cros-ec-i2c > > + # For implementations of the FPMCU connected through SPI. > > + - items: > > + - const: google,cros-ec-fp > > + - const: google,cros-ec-spi > > + # For implementations of the EC is connected through SPI. > > + - items: > > + - const: google,cros-ec-spi > > + # For implementations of the EC is connected through RPMSG. > > + - items: > > + - const: google,cros-ec-rpmsg > > > > controller-data: > > description: > > @@ -176,6 +189,37 @@ allOf: > > - reg > > - interrupts > > > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: google,cros-ec-fp > > + then: > > + properties: > > + '#address-cells': false > > + '#size-cells': false > > + typec: false > > + ec-pwm: false > > + keyboard-controller: false > > + proximity: false > > + codecs: false > > + cbas: false > > + > > + patternProperties: > > + "^i2c-tunnel[0-9]*$": false > > + "^regulator@[0-9]+$": false > > + "^extcon[0-9]*$": false > > Is the list of what's allowed shorter? If so, you could list those > properties and use 'additionalProperties: false'. Yes. > > Or maybe this is a sign that this should be a separate schema document. I couldn't figure that out. I tried to add new properties here but it didn't work. And then when I tried to make a different schema document it complained that the example for google,cros-ec-spi in here had a problem. Can properties be defined in this section? Or only listed and then additionalProperties be marked false?
On Mon, May 16, 2022 at 12:23:03PM -0700, Stephen Boyd wrote: > Quoting Rob Herring (2022-05-16 08:20:03) > > On Wed, May 11, 2022 at 06:39:21PM -0700, Stephen Boyd wrote: > > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > index 409ecef967ce..e5fe60beb9fe 100644 > > > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > @@ -19,15 +19,19 @@ description: > > > properties: > > > compatible: > > > oneOf: > > > - - description: > > > - For implementations of the EC is connected through I2C. > > > - const: google,cros-ec-i2c > > > - - description: > > > - For implementations of the EC is connected through SPI. > > > - const: google,cros-ec-spi > > > - - description: > > > - For implementations of the EC is connected through RPMSG. > > > - const: google,cros-ec-rpmsg > > > + # For implementations of the EC is connected through I2C. > > > > Using 'description' was preferred. > > I just learned that I should use anyOf instead of oneOf. Confusing! Uhh, no. It can only be one of these entries at a time. > > > > > + - items: > > > + - const: google,cros-ec-i2c > > > + # For implementations of the FPMCU connected through SPI. > > > + - items: > > > + - const: google,cros-ec-fp > > > + - const: google,cros-ec-spi > > > + # For implementations of the EC is connected through SPI. > > > + - items: > > > + - const: google,cros-ec-spi > > > + # For implementations of the EC is connected through RPMSG. > > > + - items: > > > + - const: google,cros-ec-rpmsg > > > > > > controller-data: > > > description: > > > @@ -176,6 +189,37 @@ allOf: > > > - reg > > > - interrupts > > > > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: google,cros-ec-fp > > > + then: > > > + properties: > > > + '#address-cells': false > > > + '#size-cells': false > > > + typec: false > > > + ec-pwm: false > > > + keyboard-controller: false > > > + proximity: false > > > + codecs: false > > > + cbas: false > > > + > > > + patternProperties: > > > + "^i2c-tunnel[0-9]*$": false > > > + "^regulator@[0-9]+$": false > > > + "^extcon[0-9]*$": false > > > > Is the list of what's allowed shorter? If so, you could list those > > properties and use 'additionalProperties: false'. > > Yes. > > > > > Or maybe this is a sign that this should be a separate schema document. > > I couldn't figure that out. I tried to add new properties here but it > didn't work. Like this?: then: properties: allowed-prop-1: true allowed-prop-2: true allowed-prop-3: true additionalProperties: false > And then when I tried to make a different schema document > it complained that the example for google,cros-ec-spi in here had a > problem. You probably need a custom 'select' so that both schemas aren't matching. Otherwise a schema is applied if any of the compatible strings match. > Can properties be defined in this section? Yes, but discouraged for vendor specific properties. The if/then should really be just additional constraints. Rob
Quoting Rob Herring (2022-05-18 09:13:48) > On Mon, May 16, 2022 at 12:23:03PM -0700, Stephen Boyd wrote: > > Quoting Rob Herring (2022-05-16 08:20:03) > > > On Wed, May 11, 2022 at 06:39:21PM -0700, Stephen Boyd wrote: > > > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > index 409ecef967ce..e5fe60beb9fe 100644 > > > > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > + - items: > > > > + - const: google,cros-ec-i2c > > > > + # For implementations of the FPMCU connected through SPI. > > > > + - items: > > > > + - const: google,cros-ec-fp > > > > + - const: google,cros-ec-spi > > > > + # For implementations of the EC is connected through SPI. > > > > + - items: > > > > + - const: google,cros-ec-spi > > > > + # For implementations of the EC is connected through RPMSG. > > > > + - items: > > > > + - const: google,cros-ec-rpmsg > > > > > > > > controller-data: > > > > description: > > > > @@ -176,6 +189,37 @@ allOf: > > > > - reg > > > > - interrupts > > > > > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: google,cros-ec-fp > > > > + then: > > > > + properties: > > > > + '#address-cells': false > > > > + '#size-cells': false > > > > + typec: false > > > > + ec-pwm: false > > > > + keyboard-controller: false > > > > + proximity: false > > > > + codecs: false > > > > + cbas: false > > > > + > > > > + patternProperties: > > > > + "^i2c-tunnel[0-9]*$": false > > > > + "^regulator@[0-9]+$": false > > > > + "^extcon[0-9]*$": false > > > > > > Is the list of what's allowed shorter? If so, you could list those > > > properties and use 'additionalProperties: false'. > > > > Yes. > > > > > > > > Or maybe this is a sign that this should be a separate schema document. > > > > I couldn't figure that out. I tried to add new properties here but it > > didn't work. > > Like this?: > > then: > properties: > allowed-prop-1: true > allowed-prop-2: true > allowed-prop-3: true > additionalProperties: false It doesn't seem to combine constraints? I get this error: Documentation/devicetree/bindings/mfd/google,cros-ec.example.dt.yaml: ec@0: Additional properties are not allowed ('$nodename' was unexpected) with this interdiff. I added the spi properties to reduce the additional properties that aren't allowed. ---8<---- diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml index 04b847fd5070..d47c7a273026 100644 --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml @@ -18,24 +18,21 @@ description: properties: compatible: - anyOf: + oneOf: - description: For implementations of the EC connected through I2C. - - items: - - const: google,cros-ec-i2c + const: google,cros-ec-i2c - description: For implementations of the FPMCU connected through SPI. - - items: + items: - const: google,cros-ec-fp - const: google,cros-ec-spi - description: For implementations of the EC connected through SPI. - - items: - - const: google,cros-ec-spi + const: google,cros-ec-spi - description: For implementations of the EC connected through RPMSG. - - items: - - const: google,cros-ec-rpmsg + const: google,cros-ec-rpmsg controller-data: description: @@ -200,19 +197,14 @@ allOf: const: google,cros-ec-fp then: properties: - '#address-cells': false - '#size-cells': false - typec: false - ec-pwm: false - keyboard-controller: false - proximity: false - codecs: false - cbas: false - - patternProperties: - "^i2c-tunnel[0-9]*$": false - "^regulator@[0-9]+$": false - "^extcon[0-9]*$": false + reset-gpios: true + boot0-gpios: true + vdd-supply: true + interrupts: true + compatible: true + reg: true + spi-max-frequency: true + additionalProperties: false required: - reset-gpios > > > And then when I tried to make a different schema document > > it complained that the example for google,cros-ec-spi in here had a > > problem. > > You probably need a custom 'select' so that both schemas aren't > matching. Otherwise a schema is applied if any of the compatible strings > match. I see. Maybe that's better vs. jamming it all into one file. > > > Can properties be defined in this section? > > Yes, but discouraged for vendor specific properties. The if/then should > really be just additional constraints. Ok.
On Wed, May 18, 2022 at 12:56:17PM -0700, Stephen Boyd wrote: > Quoting Rob Herring (2022-05-18 09:13:48) > > On Mon, May 16, 2022 at 12:23:03PM -0700, Stephen Boyd wrote: > > > Quoting Rob Herring (2022-05-16 08:20:03) > > > > On Wed, May 11, 2022 at 06:39:21PM -0700, Stephen Boyd wrote: > > > > > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > > index 409ecef967ce..e5fe60beb9fe 100644 > > > > > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml > > > > > + - items: > > > > > + - const: google,cros-ec-i2c > > > > > + # For implementations of the FPMCU connected through SPI. > > > > > + - items: > > > > > + - const: google,cros-ec-fp > > > > > + - const: google,cros-ec-spi > > > > > + # For implementations of the EC is connected through SPI. > > > > > + - items: > > > > > + - const: google,cros-ec-spi > > > > > + # For implementations of the EC is connected through RPMSG. > > > > > + - items: > > > > > + - const: google,cros-ec-rpmsg > > > > > > > > > > controller-data: > > > > > description: > > > > > @@ -176,6 +189,37 @@ allOf: > > > > > - reg > > > > > - interrupts > > > > > > > > > > + - if: > > > > > + properties: > > > > > + compatible: > > > > > + contains: > > > > > + const: google,cros-ec-fp > > > > > + then: > > > > > + properties: > > > > > + '#address-cells': false > > > > > + '#size-cells': false > > > > > + typec: false > > > > > + ec-pwm: false > > > > > + keyboard-controller: false > > > > > + proximity: false > > > > > + codecs: false > > > > > + cbas: false > > > > > + > > > > > + patternProperties: > > > > > + "^i2c-tunnel[0-9]*$": false > > > > > + "^regulator@[0-9]+$": false > > > > > + "^extcon[0-9]*$": false > > > > > > > > Is the list of what's allowed shorter? If so, you could list those > > > > properties and use 'additionalProperties: false'. > > > > > > Yes. > > > > > > > > > > > Or maybe this is a sign that this should be a separate schema document. > > > > > > I couldn't figure that out. I tried to add new properties here but it > > > didn't work. > > > > Like this?: > > > > then: > > properties: > > allowed-prop-1: true > > allowed-prop-2: true > > allowed-prop-3: true > > additionalProperties: false > > It doesn't seem to combine constraints? > > I get this error: > > Documentation/devicetree/bindings/mfd/google,cros-ec.example.dt.yaml: > ec@0: Additional properties are not allowed ('$nodename' was unexpected) That would be a bug or limitation in the tools. We would need to add all the other properties we automatically add as well... I think either leaving it as-is or a separate file is going to be the better route here. Rob
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml index 409ecef967ce..e5fe60beb9fe 100644 --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml @@ -19,15 +19,19 @@ description: properties: compatible: oneOf: - - description: - For implementations of the EC is connected through I2C. - const: google,cros-ec-i2c - - description: - For implementations of the EC is connected through SPI. - const: google,cros-ec-spi - - description: - For implementations of the EC is connected through RPMSG. - const: google,cros-ec-rpmsg + # For implementations of the EC is connected through I2C. + - items: + - const: google,cros-ec-i2c + # For implementations of the FPMCU connected through SPI. + - items: + - const: google,cros-ec-fp + - const: google,cros-ec-spi + # For implementations of the EC is connected through SPI. + - items: + - const: google,cros-ec-spi + # For implementations of the EC is connected through RPMSG. + - items: + - const: google,cros-ec-rpmsg controller-data: description: @@ -132,6 +136,15 @@ properties: additionalProperties: false + reset-gpios: + maxItems: 1 + + boot0-gpios: + maxItems: 1 + description: Assert for bootloader mode. + + vdd-supply: true + patternProperties: "^i2c-tunnel[0-9]*$": type: object @@ -176,6 +189,37 @@ allOf: - reg - interrupts + - if: + properties: + compatible: + contains: + const: google,cros-ec-fp + then: + properties: + '#address-cells': false + '#size-cells': false + typec: false + ec-pwm: false + keyboard-controller: false + proximity: false + codecs: false + cbas: false + + patternProperties: + "^i2c-tunnel[0-9]*$": false + "^regulator@[0-9]+$": false + "^extcon[0-9]*$": false + + required: + - reset-gpios + - boot0-gpios + - vdd-supply + else: + properties: + reset-gpios: false + boot0-gpios: false + vdd-supply: false + additionalProperties: false examples: @@ -231,4 +275,22 @@ examples: compatible = "google,cros-ec-rpmsg"; }; }; + + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/gpio/gpio.h> + spi { + #address-cells = <0x1>; + #size-cells = <0x0>; + ec@0 { + compatible = "google,cros-ec-fp", "google,cros-ec-spi"; + reg = <0>; + interrupt-parent = <&gpio_controller>; + interrupts = <4 IRQ_TYPE_LEVEL_LOW>; + spi-max-frequency = <3000000>; + reset-gpios = <&gpio_controller 5 GPIO_ACTIVE_LOW>; + boot0-gpios = <&gpio_controller 10 GPIO_ACTIVE_HIGH>; + vdd-supply = <&pp3300_fp_mcu>; + }; + }; ...
Add a binding to describe the fingerprint processor found on Chromebooks with a fingerprint sensor. Previously we've been describing this with the google,cros-ec-spi binding but it lacks gpio and regulator control used during firmware flashing. Cc: Rob Herring <robh+dt@kernel.org> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> Cc: <devicetree@vger.kernel.org> Cc: <chrome-platform@lists.linux.dev> Cc: Guenter Roeck <groeck@chromium.org> Cc: Douglas Anderson <dianders@chromium.org> Cc: Craig Hesling <hesling@chromium.org> Cc: Tom Hughes <tomhughes@chromium.org> Cc: Alexandru M Stan <amstan@chromium.org> Cc: Tzung-Bi Shih <tzungbi@kernel.org> Cc: Matthias Kaehlcke <mka@chromium.org> Cc: Benson Leung <bleung@chromium.org> Cc: Lee Jones <lee.jones@linaro.org> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- .../bindings/mfd/google,cros-ec.yaml | 80 ++++++++++++++++--- 1 file changed, 71 insertions(+), 9 deletions(-)