2022-03-15 12:04:57

by Alexandru M Stan

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: mfd: Add ChromeOS fingerprint binding

On Mon, Mar 14, 2022 at 4:22 PM Stephen Boyd <[email protected]> wrote:
>
> Add a binding to describe the fingerprint processor found on Chromeboks
> with a fingerprint sensor.
>
> Cc: Rob Herring <[email protected]>
> Cc: <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Craig Hesling <[email protected]>
> Cc: Tom Hughes <[email protected]>
> Cc: Alexandru M Stan <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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)


2022-03-16 04:35:28

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: mfd: Add ChromeOS fingerprint binding

Quoting Alexandru M Stan (2022-03-14 17:23:38)
> On Mon, Mar 14, 2022 at 4:22 PM Stephen Boyd <[email protected]> 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.