diff mbox series

[1/2] dt-bindings: mfd: Add ChromeOS fingerprint binding

Message ID 20220314232214.4183078-2-swboyd@chromium.org (mailing list archive)
State Superseded
Headers show
Series Update cros-ec-spi for fingerprint devices | expand

Commit Message

Stephen Boyd March 14, 2022, 11:22 p.m. UTC
Add a binding to describe the fingerprint processor found on Chromeboks
with a fingerprint sensor.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: <devicetree@vger.kernel.org>
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>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml

Comments

Alexandru M Stan March 15, 2022, 12:23 a.m. UTC | #1
On Mon, Mar 14, 2022 at 4:22 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Add a binding to describe the fingerprint processor found on Chromeboks
> with a fingerprint sensor.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: <devicetree@vger.kernel.org>
> 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>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> new file mode 100644
> index 000000000000..05d2b2b9b713
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS Embedded Fingerprint Controller
> +
> +description:
> +  Google's ChromeOS embedded fingerprint controller is a device which
> +  implements fingerprint functionality such as unlocking a Chromebook
> +  without typing a password.
> +
> +maintainers:
> +  - Tom Hughes <tomhughes@chromium.org>
> +
> +properties:
> +  compatible:
> +    const: google,cros-ec-fp
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 3000000
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: reset signal (active low).
> +
> +  boot0-gpios:
> +    maxItems: 1
> +    description: boot signal (low for normal boot; high for bootloader).
Maybe add "active high, same polarity as the fpmcu sees physically".

> +  vdd-supply:
> +    description: Power supply for the fingerprint controller.
> +
> +  google,cros-ec-spi-pre-delay:
> +    description:
> +      This property specifies the delay in usecs between the
> +      assertion of the CS and the first clock pulse.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - default: 0
> +      - minimum: 0
> +
> +  google,cros-ec-spi-msg-delay:
> +    description:
> +      This property specifies the delay in usecs between messages.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - default: 0
> +      - minimum: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - reset-gpios
> +  - boot0-gpios
> +  - vdd-supply
> +  - spi-max-frequency
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #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";
> +        reg = <0>;
> +        interrupt-parent = <&gpio_controller>;
> +        interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> +        spi-max-frequency = <3000000>;
> +        google,cros-ec-spi-msg-delay = <37>;
> +        google,cros-ec-spi-pre-delay = <5>;
> +        reset-gpios = <&gpio_controller 5 GPIO_ACTIVE_LOW>;
> +        boot0-gpios = <&gpio_controller 10 GPIO_ACTIVE_LOW>;
This should say GPIO_ACTIVE_HIGH, since there's no inverting going on
either with a real inverter, or the convention (of 'N' being in the
pin name).

It might be easier to reason about if there's no invesion going for this signal.

Consider it like an enum instead of a verb (unlike active_low
reset-gpios which can be considered: in reset if it's set):

enum boot0 {
        normal = 0,
        bootloader = 1,
};

> +        vdd-supply = <&pp3300_fp_mcu>;
> +      };
> +    };
> +...
> --
> https://chromeos.dev
>
--
Alexandru Stan (amstan)
Tzung-Bi Shih March 15, 2022, 3:08 a.m. UTC | #2
On Mon, Mar 14, 2022 at 04:22:13PM -0700, Stephen Boyd wrote:
> Add a binding to describe the fingerprint processor found on Chromeboks
> with a fingerprint sensor.

Nit: s/Chromeboks/Chromebooks/.

> +properties:
> +  compatible:
> +    const: google,cros-ec-fp

Not sure if it could make sense for FPS with other interfaces: would
cros-ec-fp-spi or cros-ec-spi-fp be a better name?  I am wondering because
there are cros-ec-spi specific properties "google,cros-ec-spi-pre-delay" and
"google,cros-ec-spi-msg-delay" in the binding.

> +  reset-gpios:
> +    maxItems: 1
> +    description: reset signal (active low).
> +
> +  boot0-gpios:
> +    maxItems: 1
> +    description: boot signal (low for normal boot; high for bootloader).
> +
> +  vdd-supply:
> +    description: Power supply for the fingerprint controller.

To be consistent: s/Power/power/.
Krzysztof Kozlowski March 15, 2022, 10:41 a.m. UTC | #3
On 15/03/2022 00:22, Stephen Boyd wrote:
> Add a binding to describe the fingerprint processor found on Chromeboks
> with a fingerprint sensor.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: <devicetree@vger.kernel.org>
> 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>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> new file mode 100644
> index 000000000000..05d2b2b9b713
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#

Why is this in the MFD directory? Is it really a Multi Function Device?
Description is rather opposite. You also did not CC MFD maintainer.

Best regards,
Krzysztof
Lee Jones March 15, 2022, 11:10 a.m. UTC | #4
On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:

> On 15/03/2022 00:22, Stephen Boyd wrote:
> > Add a binding to describe the fingerprint processor found on Chromeboks
> > with a fingerprint sensor.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: <devicetree@vger.kernel.org>
> > 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>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > new file mode 100644
> > index 000000000000..05d2b2b9b713
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > @@ -0,0 +1,89 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
> 
> Why is this in the MFD directory? Is it really a Multi Function Device?
> Description is rather opposite. You also did not CC MFD maintainer.

A lot of the ChromeOS Embedded Controller support used to be located
in MFD.  There are still remnants, but most was moved to
drivers/platform IIRC.

Please see: drivers/mfd/cros_ec_dev.c

However, yes, I should have been on the recipients list.
Krzysztof Kozlowski March 15, 2022, 11:22 a.m. UTC | #5
On 15/03/2022 12:10, Lee Jones wrote:
> On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
> 
>> On 15/03/2022 00:22, Stephen Boyd wrote:
>>> Add a binding to describe the fingerprint processor found on Chromeboks
>>> with a fingerprint sensor.
>>>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: <devicetree@vger.kernel.org>
>>> 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>
>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>> ---
>>>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
>>>  1 file changed, 89 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
>>> new file mode 100644
>>> index 000000000000..05d2b2b9b713
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
>>> @@ -0,0 +1,89 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
>>
>> Why is this in the MFD directory? Is it really a Multi Function Device?
>> Description is rather opposite. You also did not CC MFD maintainer.
> 
> A lot of the ChromeOS Embedded Controller support used to be located
> in MFD.  There are still remnants, but most was moved to
> drivers/platform IIRC.
> 
> Please see: drivers/mfd/cros_ec_dev.c

Yes, I know, that part is a MFD. But why the fingerprint controller part
is MFD? To me it is closer to input device.

Best regards,
Krzysztof
Lee Jones March 15, 2022, 11:30 a.m. UTC | #6
On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:

> On 15/03/2022 12:10, Lee Jones wrote:
> > On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
> > 
> >> On 15/03/2022 00:22, Stephen Boyd wrote:
> >>> Add a binding to describe the fingerprint processor found on Chromeboks
> >>> with a fingerprint sensor.
> >>>
> >>> Cc: Rob Herring <robh+dt@kernel.org>
> >>> Cc: <devicetree@vger.kernel.org>
> >>> 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>
> >>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> >>> ---
> >>>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
> >>>  1 file changed, 89 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> >>> new file mode 100644
> >>> index 000000000000..05d2b2b9b713
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> >>> @@ -0,0 +1,89 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
> >>
> >> Why is this in the MFD directory? Is it really a Multi Function Device?
> >> Description is rather opposite. You also did not CC MFD maintainer.
> > 
> > A lot of the ChromeOS Embedded Controller support used to be located
> > in MFD.  There are still remnants, but most was moved to
> > drivers/platform IIRC.
> > 
> > Please see: drivers/mfd/cros_ec_dev.c
> 
> Yes, I know, that part is a MFD. But why the fingerprint controller part
> is MFD? To me it is closer to input device.

It's tough to say from what I was sent above.

But yes, sounds like it.

We do not want any device 'functionality' in MFD ideally.
Stephen Boyd March 15, 2022, 3:38 p.m. UTC | #7
Quoting Tzung-Bi Shih (2022-03-14 20:08:47)
> On Mon, Mar 14, 2022 at 04:22:13PM -0700, Stephen Boyd wrote:
> > Add a binding to describe the fingerprint processor found on Chromeboks
> > with a fingerprint sensor.
>
> Nit: s/Chromeboks/Chromebooks/.
>
> > +properties:
> > +  compatible:
> > +    const: google,cros-ec-fp
>
> Not sure if it could make sense for FPS with other interfaces: would
> cros-ec-fp-spi or cros-ec-spi-fp be a better name?  I am wondering because
> there are cros-ec-spi specific properties "google,cros-ec-spi-pre-delay" and
> "google,cros-ec-spi-msg-delay" in the binding.

The delays are optional properties so I don't see much value in encoding
the bus type into the compatible string. It would only help us find
properties that are unused on a bus that isn't SPI. Let's not
overcomplicate things.

>
> > +  reset-gpios:
> > +    maxItems: 1
> > +    description: reset signal (active low).
> > +
> > +  boot0-gpios:
> > +    maxItems: 1
> > +    description: boot signal (low for normal boot; high for bootloader).
> > +
> > +  vdd-supply:
> > +    description: Power supply for the fingerprint controller.
>
> To be consistent: s/Power/power/.

I fixed the other two, thanks!
Stephen Boyd March 15, 2022, 3:41 p.m. UTC | #8
Quoting Lee Jones (2022-03-15 04:30:49)
> On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
>
> > On 15/03/2022 12:10, Lee Jones wrote:
> > > On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
> > >
> > >> On 15/03/2022 00:22, Stephen Boyd wrote:
> > >>> Add a binding to describe the fingerprint processor found on Chromeboks
> > >>> with a fingerprint sensor.
> > >>>
> > >>> Cc: Rob Herring <robh+dt@kernel.org>
> > >>> Cc: <devicetree@vger.kernel.org>
> > >>> 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>
> > >>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > >>> ---
> > >>>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
> > >>>  1 file changed, 89 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..05d2b2b9b713
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > >>> @@ -0,0 +1,89 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
> > >>
> > >> Why is this in the MFD directory? Is it really a Multi Function Device?
> > >> Description is rather opposite. You also did not CC MFD maintainer.
> > >
> > > A lot of the ChromeOS Embedded Controller support used to be located
> > > in MFD.  There are still remnants, but most was moved to
> > > drivers/platform IIRC.
> > >
> > > Please see: drivers/mfd/cros_ec_dev.c
> >
> > Yes, I know, that part is a MFD. But why the fingerprint controller part
> > is MFD? To me it is closer to input device.
>
> It's tough to say from what I was sent above.
>
> But yes, sounds like it.
>
> We do not want any device 'functionality' in MFD ideally.
>

I put it next to the existing cros-ec binding. The existing binding is
there because of historical reasons as far as I know. Otherwise it
didn't seem MFD related so I didn't Cc mfd maintainer/list. New file
additions don't usually conflict with anything and this is in the
bindings directory so the driver side maintainer would be picking up the
binding.
Lee Jones March 15, 2022, 3:48 p.m. UTC | #9
On Tue, 15 Mar 2022, Stephen Boyd wrote:

> Quoting Lee Jones (2022-03-15 04:30:49)
> > On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
> >
> > > On 15/03/2022 12:10, Lee Jones wrote:
> > > > On Tue, 15 Mar 2022, Krzysztof Kozlowski wrote:
> > > >
> > > >> On 15/03/2022 00:22, Stephen Boyd wrote:
> > > >>> Add a binding to describe the fingerprint processor found on Chromeboks
> > > >>> with a fingerprint sensor.
> > > >>>
> > > >>> Cc: Rob Herring <robh+dt@kernel.org>
> > > >>> Cc: <devicetree@vger.kernel.org>
> > > >>> 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>
> > > >>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > >>> ---
> > > >>>  .../bindings/mfd/google,cros-ec-fp.yaml       | 89 +++++++++++++++++++
> > > >>>  1 file changed, 89 insertions(+)
> > > >>>  create mode 100644 Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > > >>> new file mode 100644
> > > >>> index 000000000000..05d2b2b9b713
> > > >>> --- /dev/null
> > > >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
> > > >>> @@ -0,0 +1,89 @@
> > > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > >>> +%YAML 1.2
> > > >>> +---
> > > >>> +$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
> > > >>
> > > >> Why is this in the MFD directory? Is it really a Multi Function Device?
> > > >> Description is rather opposite. You also did not CC MFD maintainer.
> > > >
> > > > A lot of the ChromeOS Embedded Controller support used to be located
> > > > in MFD.  There are still remnants, but most was moved to
> > > > drivers/platform IIRC.
> > > >
> > > > Please see: drivers/mfd/cros_ec_dev.c
> > >
> > > Yes, I know, that part is a MFD. But why the fingerprint controller part
> > > is MFD? To me it is closer to input device.
> >
> > It's tough to say from what I was sent above.
> >
> > But yes, sounds like it.
> >
> > We do not want any device 'functionality' in MFD ideally.
> >
> 
> I put it next to the existing cros-ec binding. The existing binding is
> there because of historical reasons as far as I know. Otherwise it
> didn't seem MFD related so I didn't Cc mfd maintainer/list. New file
> additions don't usually conflict with anything and this is in the
> bindings directory so the driver side maintainer would be picking up the
> binding.

That's not how it works unfortunately.

This file is located in the MFD bindings directory, so I would be
picking it up (if it ends up staying here).

Best to rely on `get_maintainer.pl` for this:

$ ./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/mfd/
Lee Jones <lee.jones@linaro.org> (supporter:MULTIFUNCTION DEVICES (MFD))
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-kernel@vger.kernel.org (open list)
Stephen Boyd March 15, 2022, 3:50 p.m. UTC | #10
Quoting Alexandru M Stan (2022-03-14 17:23:38)
> On Mon, Mar 14, 2022 at 4:22 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > +        compatible = "google,cros-ec-fp";
> > +        reg = <0>;
> > +        interrupt-parent = <&gpio_controller>;
> > +        interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> > +        spi-max-frequency = <3000000>;
> > +        google,cros-ec-spi-msg-delay = <37>;
> > +        google,cros-ec-spi-pre-delay = <5>;
> > +        reset-gpios = <&gpio_controller 5 GPIO_ACTIVE_LOW>;
> > +        boot0-gpios = <&gpio_controller 10 GPIO_ACTIVE_LOW>;
> This should say GPIO_ACTIVE_HIGH, since there's no inverting going on
> either with a real inverter, or the convention (of 'N' being in the
> pin name).
>
> It might be easier to reason about if there's no invesion going for this signal.
>
> Consider it like an enum instead of a verb (unlike active_low
> reset-gpios which can be considered: in reset if it's set):
>
> enum boot0 {
>         normal = 0,
>         bootloader = 1,
> };

Ok got it! I have in my notes that physically high line means normal
boot mode and physically low is bootloader mode. I confused myself. I'll
fix this.
Stephen Boyd March 15, 2022, 4:19 p.m. UTC | #11
Quoting Lee Jones (2022-03-15 08:48:08)
> On Tue, 15 Mar 2022, Stephen Boyd wrote:
>
> > Quoting Lee Jones (2022-03-15 04:30:49)
> > > It's tough to say from what I was sent above.
> > >
> > > But yes, sounds like it.
> > >
> > > We do not want any device 'functionality' in MFD ideally.
> > >
> >
> > I put it next to the existing cros-ec binding. The existing binding is
> > there because of historical reasons as far as I know. Otherwise it
> > didn't seem MFD related so I didn't Cc mfd maintainer/list. New file
> > additions don't usually conflict with anything and this is in the
> > bindings directory so the driver side maintainer would be picking up the
> > binding.
>
> That's not how it works unfortunately.
>
> This file is located in the MFD bindings directory, so I would be
> picking it up (if it ends up staying here).

The way it works is arbitrary and up to maintainer's choice. I'll move
it out of the mfd directory :)
Lee Jones March 16, 2022, 7:25 a.m. UTC | #12
On Tue, 15 Mar 2022, Stephen Boyd wrote:

> Quoting Lee Jones (2022-03-15 08:48:08)
> > On Tue, 15 Mar 2022, Stephen Boyd wrote:
> >
> > > Quoting Lee Jones (2022-03-15 04:30:49)
> > > > It's tough to say from what I was sent above.
> > > >
> > > > But yes, sounds like it.
> > > >
> > > > We do not want any device 'functionality' in MFD ideally.
> > > >
> > >
> > > I put it next to the existing cros-ec binding. The existing binding is
> > > there because of historical reasons as far as I know. Otherwise it
> > > didn't seem MFD related so I didn't Cc mfd maintainer/list. New file
> > > additions don't usually conflict with anything and this is in the
> > > bindings directory so the driver side maintainer would be picking up the
> > > binding.
> >
> > That's not how it works unfortunately.
> >
> > This file is located in the MFD bindings directory, so I would be
> > picking it up (if it ends up staying here).
> 
> The way it works is arbitrary 

Correct.

> and up to maintainer's choice.

Which *should* be reflected in MAINTAINERS and by extension
get_maintainer.pl.  As is the case here.  :)

> I'll move it out of the mfd directory :)

That does sound like a good solution, thanks Stephen.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
new file mode 100644
index 000000000000..05d2b2b9b713
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec-fp.yaml
@@ -0,0 +1,89 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/google,cros-ec-fp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS Embedded Fingerprint Controller
+
+description:
+  Google's ChromeOS embedded fingerprint controller is a device which
+  implements fingerprint functionality such as unlocking a Chromebook
+  without typing a password.
+
+maintainers:
+  - Tom Hughes <tomhughes@chromium.org>
+
+properties:
+  compatible:
+    const: google,cros-ec-fp
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 3000000
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+    description: reset signal (active low).
+
+  boot0-gpios:
+    maxItems: 1
+    description: boot signal (low for normal boot; high for bootloader).
+
+  vdd-supply:
+    description: Power supply for the fingerprint controller.
+
+  google,cros-ec-spi-pre-delay:
+    description:
+      This property specifies the delay in usecs between the
+      assertion of the CS and the first clock pulse.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - default: 0
+      - minimum: 0
+
+  google,cros-ec-spi-msg-delay:
+    description:
+      This property specifies the delay in usecs between messages.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - default: 0
+      - minimum: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+  - boot0-gpios
+  - vdd-supply
+  - spi-max-frequency
+
+additionalProperties: false
+
+examples:
+  - |
+    #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";
+        reg = <0>;
+        interrupt-parent = <&gpio_controller>;
+        interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
+        spi-max-frequency = <3000000>;
+        google,cros-ec-spi-msg-delay = <37>;
+        google,cros-ec-spi-pre-delay = <5>;
+        reset-gpios = <&gpio_controller 5 GPIO_ACTIVE_LOW>;
+        boot0-gpios = <&gpio_controller 10 GPIO_ACTIVE_LOW>;
+        vdd-supply = <&pp3300_fp_mcu>;
+      };
+    };
+...