[PATCH v3 0/3] Update cros-ec-spi for fingerprint devices

[PATCH v3 0/3] Update cros-ec-spi for fingerprint devices

[Stephen Boyd]

This patch series introduces a DT binding for chromeos fingerprint
devices and then implements support to boot those processors during
driver probe if the BIOS hasn't done it already.

Changes from v2 (https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org):
 * Dropped cros-ec spi dt properties that aren't of use right now
 * Picked up tags

Changes from v1 (https://lore.kernel.org/r/20220314232214.4183078-1-swboyd@chromium.org):
 * Properly do the boot sequence
 * Add a message that we're booting and delaying a while
 * Fix typo in commit text
 * Change binding to not spell out reset-gpios and indicate that boot0
   is about asserting boot mode
 * Split device id to different patch as it's a different topic from
   booting

Stephen Boyd (3):
  dt-bindings: chrome: Add ChromeOS fingerprint binding
  platform/chrome: cros_ec_spi: Match cros-ec-fp compatible
  platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

 .../bindings/chrome/google,cros-ec-fp.yaml    | 66 +++++++++++++++++++
 drivers/platform/chrome/cros_ec_spi.c         | 44 ++++++++++++-
 2 files changed, 107 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-fp.yaml

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>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Matthias Kaehlcke <mka@chromium.org>

base-commit: ffb217a13a2eaf6d5bd974fc83036a53ca69f1e2
-- 
https://chromeos.dev

[PATCH v3 1/3] dt-bindings: chrome: Add ChromeOS fingerprint binding

[Stephen Boyd]

Add a binding to describe the fingerprint processor found on Chromebooks
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>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Cc: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 .../bindings/chrome/google,cros-ec-fp.yaml    | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/chrome/google,cros-ec-fp.yaml

diff --git a/Documentation/devicetree/bindings/chrome/google,cros-ec-fp.yaml b/Documentation/devicetree/bindings/chrome/google,cros-ec-fp.yaml
new file mode 100644
index 000000000000..b7fbaaa94d65
--- /dev/null
+++ b/Documentation/devicetree/bindings/chrome/google,cros-ec-fp.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/chrome/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: true
+  boot0-gpios:
+    maxItems: 1
+    description: Assert for bootloader mode.
+
+  vdd-supply: true
+
+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>;
+        reset-gpios = <&gpio_controller 5 GPIO_ACTIVE_LOW>;
+        boot0-gpios = <&gpio_controller 10 GPIO_ACTIVE_HIGH>;
+        vdd-supply = <&pp3300_fp_mcu>;
+      };
+    };
+...
-- 
https://chromeos.dev

[PATCH v3 2/3] platform/chrome: cros_ec_spi: Match cros-ec-fp compatible

[Stephen Boyd]

Add the fingerprint cros-ec compatible and spi_device_id so that we can probe
fingerprint devices.

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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/platform/chrome/cros_ec_spi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 14c4046fa04d..d0f9496076d6 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -813,12 +813,14 @@ static SIMPLE_DEV_PM_OPS(cros_ec_spi_pm_ops, cros_ec_spi_suspend,
 			 cros_ec_spi_resume);
 
 static const struct of_device_id cros_ec_spi_of_match[] = {
+	{ .compatible = "google,cros-ec-fp", },
 	{ .compatible = "google,cros-ec-spi", },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, cros_ec_spi_of_match);
 
 static const struct spi_device_id cros_ec_spi_id[] = {
+	{ "cros-ec-fp", 0 },
 	{ "cros-ec-spi", 0 },
 	{ }
 };
-- 
https://chromeos.dev

[PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Stephen Boyd]

Add gpio control to this driver so that the fingerprint device can be
booted if the BIOS isn't doing it already. This eases bringup of new
hardware as we don't have to wait for the BIOS to be ready, supports
kexec where the GPIOs may not be configured by the previous boot stage,
and is all around good hygiene because we control GPIOs for this device
from the device driver.

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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index d0f9496076d6..13d413a2fe46 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -4,6 +4,7 @@
 // Copyright (C) 2012 Google, Inc
 
 #include <linux/delay.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -77,6 +78,8 @@ struct cros_ec_spi {
 	unsigned int start_of_msg_delay;
 	unsigned int end_of_msg_delay;
 	struct kthread_worker *high_pri_worker;
+	struct gpio_desc *boot0;
+	struct gpio_desc *reset;
 };
 
 typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
@@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
 }
 
-static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
+static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	u32 val;
@@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
 	ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
 	if (!ret)
 		ec_spi->end_of_msg_delay = val;
+
+	if (!of_device_is_compatible(np, "google,cros-ec-fp"))
+		return 0;
+
+	ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0);
+	if (IS_ERR(ec_spi->boot0))
+		return PTR_ERR(ec_spi->boot0);
+
+	ec_spi->reset = devm_gpiod_get(dev, "reset", 0);
+	if (IS_ERR(ec_spi->reset))
+		return PTR_ERR(ec_spi->reset);
+
+	/*
+	 * Take the FPMCU out of reset and wait for it to boot if it's in
+	 * bootloader mode or held in reset. This isn't the normal flow because
+	 * typically the BIOS has already powered on the device to avoid the
+	 * multi-second delay waiting for the FPMCU to boot and be responsive.
+	 */
+	if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) {
+		/* Boot0 is sampled on reset deassertion */
+		gpiod_set_value(ec_spi->boot0, 0);
+		gpiod_set_value(ec_spi->reset, 1);
+		usleep_range(1000, 2000);
+		gpiod_set_value(ec_spi->reset, 0);
+
+		/* Wait for boot; there isn't a "boot done" signal */
+		dev_info(dev, "Waiting for FPMCU to boot\n");
+		msleep(2000);
+	}
+
+	return 0;
 }
 
 static void cros_ec_spi_high_pri_release(void *worker)
@@ -754,8 +788,10 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	if (!ec_dev)
 		return -ENOMEM;
 
-	/* Check for any DT properties */
-	cros_ec_spi_dt_probe(ec_spi, dev);
+	/* Check for any DT properties and boot FPMCU if applicable */
+	err = cros_ec_spi_dt_probe(ec_spi, dev);
+	if (err)
+		return err;
 
 	spi_set_drvdata(spi, ec_dev);
 	ec_dev->dev = dev;
-- 
https://chromeos.dev

Re: [PATCH v3 1/3] dt-bindings: chrome: Add ChromeOS fingerprint binding

[Doug Anderson]

Hi,

On Thu, Mar 17, 2022 at 6:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Add a binding to describe the fingerprint processor found on Chromebooks
> with a fingerprint sensor.

It might be worth mentioning that previously these fingerprint devices
were described using "google,cros-ec-spi" just to provide context?


> 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>
> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  .../bindings/chrome/google,cros-ec-fp.yaml    | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 2/3] platform/chrome: cros_ec_spi: Match cros-ec-fp compatible

[Doug Anderson]

Hi,

On Thu, Mar 17, 2022 at 6:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Add the fingerprint cros-ec compatible and spi_device_id so that we can probe
> fingerprint devices.
>
> 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>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_spi.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Doug Anderson]

Hi,

On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Add gpio control to this driver so that the fingerprint device can be
> booted if the BIOS isn't doing it already. This eases bringup of new
> hardware as we don't have to wait for the BIOS to be ready, supports
> kexec where the GPIOs may not be configured by the previous boot stage,
> and is all around good hygiene because we control GPIOs for this device
> from the device driver.
>
> 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>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> index d0f9496076d6..13d413a2fe46 100644
> --- a/drivers/platform/chrome/cros_ec_spi.c
> +++ b/drivers/platform/chrome/cros_ec_spi.c
> @@ -4,6 +4,7 @@
>  // Copyright (C) 2012 Google, Inc
>
>  #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -77,6 +78,8 @@ struct cros_ec_spi {
>         unsigned int start_of_msg_delay;
>         unsigned int end_of_msg_delay;
>         struct kthread_worker *high_pri_worker;
> +       struct gpio_desc *boot0;
> +       struct gpio_desc *reset;

This structure has members described with kernel-doc. You should
document your members.


>  };
>
>  typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
> @@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>         return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
>  }
>
> -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> +static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
>  {
>         struct device_node *np = dev->of_node;
>         u32 val;
> @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
>         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
>         if (!ret)
>                 ec_spi->end_of_msg_delay = val;
> +
> +       if (!of_device_is_compatible(np, "google,cros-ec-fp"))
> +               return 0;

I noticed in your previous patch that you not only added a device-tree
match for this device but also a "spi_device_id". ...but won't you
fail to do all this important GPIO work in that case?


> +       ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0);
> +       if (IS_ERR(ec_spi->boot0))
> +               return PTR_ERR(ec_spi->boot0);

Right now these GPIOs don't actually need to be stored in the "ec_spi"
structure. They could just be local variables. I guess you're trying
to future proof?


> +       ec_spi->reset = devm_gpiod_get(dev, "reset", 0);
> +       if (IS_ERR(ec_spi->reset))
> +               return PTR_ERR(ec_spi->reset);
> +
> +       /*
> +        * Take the FPMCU out of reset and wait for it to boot if it's in
> +        * bootloader mode or held in reset. This isn't the normal flow because
> +        * typically the BIOS has already powered on the device to avoid the
> +        * multi-second delay waiting for the FPMCU to boot and be responsive.
> +        */
> +       if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) {
> +               /* Boot0 is sampled on reset deassertion */
> +               gpiod_set_value(ec_spi->boot0, 0);
> +               gpiod_set_value(ec_spi->reset, 1);
> +               usleep_range(1000, 2000);
> +               gpiod_set_value(ec_spi->reset, 0);
> +
> +               /* Wait for boot; there isn't a "boot done" signal */
> +               dev_info(dev, "Waiting for FPMCU to boot\n");
> +               msleep(2000);
> +       }

You added the regulator to the bindings. On herobrine I know that the
regulator is a bit of a dummy (at least on herobrine), but I wonder if
you should still get/enable it here? In the device tree bindings you
listed it as not-optional so, in theory, you could use this to give an
error if someone didn't provide the regulator.

BTW: it seems like it wouldn't be a _crazy_ amount of extra work to:

1. Add a sysfs hook for turning the regulator on/off

2. Change the Chrome OS userspace to actually use the sysfs hook if it's there.

3. Actually have the kernel in charge of turning the regulator off/on

Doing this at the same time as the transition over to the more real
"cros-ec-fp" would be nice so we don't have to figure out how to
transition later. Said another way: If we don't transition now then I
guess later we'd have to find some way to detect that the regulator
specified in the kernel was actually a dummy and didn't really control
the power?

Re: [PATCH v3 1/3] dt-bindings: chrome: Add ChromeOS fingerprint binding

[Stephen Boyd]

Quoting Doug Anderson (2022-03-18 13:49:05)
> Hi,
>
> On Thu, Mar 17, 2022 at 6:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Add a binding to describe the fingerprint processor found on Chromebooks
> > with a fingerprint sensor.
>
> It might be worth mentioning that previously these fingerprint devices
> were described using "google,cros-ec-spi" just to provide context?

Sure I squashed it in.

>
>
> > 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>
> > Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> > Cc: Matthias Kaehlcke <mka@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  .../bindings/chrome/google,cros-ec-fp.yaml    | 66 +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644 Documentation/
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Stephen Boyd]

Quoting Doug Anderson (2022-03-18 13:50:05)
> Hi,
>
> On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Add gpio control to this driver so that the fingerprint device can be
> > booted if the BIOS isn't doing it already. This eases bringup of new
> > hardware as we don't have to wait for the BIOS to be ready, supports
> > kexec where the GPIOs may not be configured by the previous boot stage,
> > and is all around good hygiene because we control GPIOs for this device
> > from the device driver.
> >
> > 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>
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_spi.c | 42 +++++++++++++++++++++++++--
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> > index d0f9496076d6..13d413a2fe46 100644
> > --- a/drivers/platform/chrome/cros_ec_spi.c
> > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > @@ -4,6 +4,7 @@
> >  // Copyright (C) 2012 Google, Inc
> >
> >  #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > @@ -77,6 +78,8 @@ struct cros_ec_spi {
> >         unsigned int start_of_msg_delay;
> >         unsigned int end_of_msg_delay;
> >         struct kthread_worker *high_pri_worker;
> > +       struct gpio_desc *boot0;
> > +       struct gpio_desc *reset;
>
> This structure has members described with kernel-doc. You should
> document your members.
>
>
> >  };
> >
> >  typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
> > @@ -690,7 +693,7 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> >         return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
> >  }
> >
> > -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> > +static int cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >  {
> >         struct device_node *np = dev->of_node;
> >         u32 val;
> > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >         if (!ret)
> >                 ec_spi->end_of_msg_delay = val;
> > +
> > +       if (!of_device_is_compatible(np, "google,cros-ec-fp"))
> > +               return 0;
>
> I noticed in your previous patch that you not only added a device-tree
> match for this device but also a "spi_device_id". ...but won't you
> fail to do all this important GPIO work in that case?

I don't know when the spi_device_id path will be used. Never? I can
simply drop it from the spi_device_id list for now and we can take up
this problem later or never.

>
>
> > +       ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0);
> > +       if (IS_ERR(ec_spi->boot0))
> > +               return PTR_ERR(ec_spi->boot0);
>
> Right now these GPIOs don't actually need to be stored in the "ec_spi"
> structure. They could just be local variables. I guess you're trying
> to future proof?

Sure I will drop them because they're not useful later and I can save on
the kernel-doc.

>
>
> > +       ec_spi->reset = devm_gpiod_get(dev, "reset", 0);
> > +       if (IS_ERR(ec_spi->reset))
> > +               return PTR_ERR(ec_spi->reset);
> > +
> > +       /*
> > +        * Take the FPMCU out of reset and wait for it to boot if it's in
> > +        * bootloader mode or held in reset. This isn't the normal flow because
> > +        * typically the BIOS has already powered on the device to avoid the
> > +        * multi-second delay waiting for the FPMCU to boot and be responsive.
> > +        */
> > +       if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) {
> > +               /* Boot0 is sampled on reset deassertion */
> > +               gpiod_set_value(ec_spi->boot0, 0);
> > +               gpiod_set_value(ec_spi->reset, 1);
> > +               usleep_range(1000, 2000);
> > +               gpiod_set_value(ec_spi->reset, 0);
> > +
> > +               /* Wait for boot; there isn't a "boot done" signal */
> > +               dev_info(dev, "Waiting for FPMCU to boot\n");
> > +               msleep(2000);
> > +       }
>
> You added the regulator to the bindings. On herobrine I know that the
> regulator is a bit of a dummy (at least on herobrine), but I wonder if
> you should still get/enable it here? In the device tree bindings you
> listed it as not-optional so, in theory, you could use this to give an
> error if someone didn't provide the regulator.

Won't the regulator framework introduce a dummy supply if there isn't a
supply in DT but this driver calls regulator_get()? Getting and enabling
it here will make this even more independent though so it sounds like a
good idea. That way we can make it a real regulator in the DTS as long
as the firmware isn't controlling it.

>
> BTW: it seems like it wouldn't be a _crazy_ amount of extra work to:
>
> 1. Add a sysfs hook for turning the regulator on/off
>
> 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there.
>
> 3. Actually have the kernel in charge of turning the regulator off/on
>
> Doing this at the same time as the transition over to the more real
> "cros-ec-fp" would be nice so we don't have to figure out how to
> transition later. Said another way: If we don't transition now then I
> guess later we'd have to find some way to detect that the regulator
> specified in the kernel was actually a dummy and didn't really control
> the power?

I'd rather not expose regulator control to userspace through some other
sysfs attribute. Instead I'd prefer the flashing logic that twiddles
gpios and power live all in the kernel and have userspace interact with
a character device to program the firmware.

Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Doug Anderson]

Hi,

On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> > >         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> > >         if (!ret)
> > >                 ec_spi->end_of_msg_delay = val;
> > > +
> > > +       if (!of_device_is_compatible(np, "google,cros-ec-fp"))
> > > +               return 0;
> >
> > I noticed in your previous patch that you not only added a device-tree
> > match for this device but also a "spi_device_id". ...but won't you
> > fail to do all this important GPIO work in that case?
>
> I don't know when the spi_device_id path will be used. Never? I can
> simply drop it from the spi_device_id list for now and we can take up
> this problem later or never.

That's fine with me. I was guessing it was relevant for x86 but my
experience with the way x86 does things is pretty minimal.


> > > +       ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0);
> > > +       if (IS_ERR(ec_spi->boot0))
> > > +               return PTR_ERR(ec_spi->boot0);
> >
> > Right now these GPIOs don't actually need to be stored in the "ec_spi"
> > structure. They could just be local variables. I guess you're trying
> > to future proof?
>
> Sure I will drop them because they're not useful later and I can save on
> the kernel-doc.
>
> >
> >
> > > +       ec_spi->reset = devm_gpiod_get(dev, "reset", 0);
> > > +       if (IS_ERR(ec_spi->reset))
> > > +               return PTR_ERR(ec_spi->reset);
> > > +
> > > +       /*
> > > +        * Take the FPMCU out of reset and wait for it to boot if it's in
> > > +        * bootloader mode or held in reset. This isn't the normal flow because
> > > +        * typically the BIOS has already powered on the device to avoid the
> > > +        * multi-second delay waiting for the FPMCU to boot and be responsive.
> > > +        */
> > > +       if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) {
> > > +               /* Boot0 is sampled on reset deassertion */
> > > +               gpiod_set_value(ec_spi->boot0, 0);
> > > +               gpiod_set_value(ec_spi->reset, 1);
> > > +               usleep_range(1000, 2000);
> > > +               gpiod_set_value(ec_spi->reset, 0);
> > > +
> > > +               /* Wait for boot; there isn't a "boot done" signal */
> > > +               dev_info(dev, "Waiting for FPMCU to boot\n");
> > > +               msleep(2000);
> > > +       }
> >
> > You added the regulator to the bindings. On herobrine I know that the
> > regulator is a bit of a dummy (at least on herobrine), but I wonder if
> > you should still get/enable it here? In the device tree bindings you
> > listed it as not-optional so, in theory, you could use this to give an
> > error if someone didn't provide the regulator.
>
> Won't the regulator framework introduce a dummy supply if there isn't a
> supply in DT but this driver calls regulator_get()? Getting and enabling
> it here will make this even more independent though so it sounds like a
> good idea. That way we can make it a real regulator in the DTS as long
> as the firmware isn't controlling it.

I was thinking of regulator_get_optional(). You know the call you use
when your regulator isn't "optional"? (Sorry, it always cracks me up
that "optional" is exactly opposite the meaning for regulator compared
to everyone else).


> > BTW: it seems like it wouldn't be a _crazy_ amount of extra work to:
> >
> > 1. Add a sysfs hook for turning the regulator on/off
> >
> > 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there.
> >
> > 3. Actually have the kernel in charge of turning the regulator off/on
> >
> > Doing this at the same time as the transition over to the more real
> > "cros-ec-fp" would be nice so we don't have to figure out how to
> > transition later. Said another way: If we don't transition now then I
> > guess later we'd have to find some way to detect that the regulator
> > specified in the kernel was actually a dummy and didn't really control
> > the power?
>
> I'd rather not expose regulator control to userspace through some other
> sysfs attribute. Instead I'd prefer the flashing logic that twiddles
> gpios and power live all in the kernel and have userspace interact with
> a character device to program the firmware.

Yeah, that would be even better, you're right.

Hmmm, so maybe the answer is to just delay adding the regulator until
we're actually ready to specify it correctly and have the flashing
happen in the kernel?

-Doug

Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Stephen Boyd]

Quoting Doug Anderson (2022-03-18 15:06:59)
> Hi,
>
> On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > > @@ -703,6 +706,37 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> > > >         ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> > > >         if (!ret)
> > > >                 ec_spi->end_of_msg_delay = val;
> > > > +
> > > > +       if (!of_device_is_compatible(np, "google,cros-ec-fp"))
> > > > +               return 0;
> > >
> > > I noticed in your previous patch that you not only added a device-tree
> > > match for this device but also a "spi_device_id". ...but won't you
> > > fail to do all this important GPIO work in that case?
> >
> > I don't know when the spi_device_id path will be used. Never? I can
> > simply drop it from the spi_device_id list for now and we can take up
> > this problem later or never.
>
> That's fine with me. I was guessing it was relevant for x86 but my
> experience with the way x86 does things is pretty minimal.
>

Ah, I think the x86 path also uses the of_device_id, but it is still
google,cros-ec-spi so it won't be affected until conforming to the new
binding.

>
> > > > +       ec_spi->boot0 = devm_gpiod_get(dev, "boot0", 0);
> > > > +       if (IS_ERR(ec_spi->boot0))
> > > > +               return PTR_ERR(ec_spi->boot0);
> > >
> > > Right now these GPIOs don't actually need to be stored in the "ec_spi"
> > > structure. They could just be local variables. I guess you're trying
> > > to future proof?
> >
> > Sure I will drop them because they're not useful later and I can save on
> > the kernel-doc.
> >
> > >
> > >
> > > > +       ec_spi->reset = devm_gpiod_get(dev, "reset", 0);
> > > > +       if (IS_ERR(ec_spi->reset))
> > > > +               return PTR_ERR(ec_spi->reset);
> > > > +
> > > > +       /*
> > > > +        * Take the FPMCU out of reset and wait for it to boot if it's in
> > > > +        * bootloader mode or held in reset. This isn't the normal flow because
> > > > +        * typically the BIOS has already powered on the device to avoid the
> > > > +        * multi-second delay waiting for the FPMCU to boot and be responsive.
> > > > +        */
> > > > +       if (gpiod_get_value(ec_spi->boot0) || gpiod_get_value(ec_spi->reset)) {
> > > > +               /* Boot0 is sampled on reset deassertion */
> > > > +               gpiod_set_value(ec_spi->boot0, 0);
> > > > +               gpiod_set_value(ec_spi->reset, 1);
> > > > +               usleep_range(1000, 2000);
> > > > +               gpiod_set_value(ec_spi->reset, 0);
> > > > +
> > > > +               /* Wait for boot; there isn't a "boot done" signal */
> > > > +               dev_info(dev, "Waiting for FPMCU to boot\n");
> > > > +               msleep(2000);
> > > > +       }
> > >
> > > You added the regulator to the bindings. On herobrine I know that the
> > > regulator is a bit of a dummy (at least on herobrine), but I wonder if
> > > you should still get/enable it here? In the device tree bindings you
> > > listed it as not-optional so, in theory, you could use this to give an
> > > error if someone didn't provide the regulator.
> >
> > Won't the regulator framework introduce a dummy supply if there isn't a
> > supply in DT but this driver calls regulator_get()? Getting and enabling
> > it here will make this even more independent though so it sounds like a
> > good idea. That way we can make it a real regulator in the DTS as long
> > as the firmware isn't controlling it.
>
> I was thinking of regulator_get_optional(). You know the call you use
> when your regulator isn't "optional"? (Sorry, it always cracks me up
> that "optional" is exactly opposite the meaning for regulator compared
> to everyone else).

Oh my.

>
>
> > > BTW: it seems like it wouldn't be a _crazy_ amount of extra work to:
> > >
> > > 1. Add a sysfs hook for turning the regulator on/off
> > >
> > > 2. Change the Chrome OS userspace to actually use the sysfs hook if it's there.
> > >
> > > 3. Actually have the kernel in charge of turning the regulator off/on
> > >
> > > Doing this at the same time as the transition over to the more real
> > > "cros-ec-fp" would be nice so we don't have to figure out how to
> > > transition later. Said another way: If we don't transition now then I
> > > guess later we'd have to find some way to detect that the regulator
> > > specified in the kernel was actually a dummy and didn't really control
> > > the power?
> >
> > I'd rather not expose regulator control to userspace through some other
> > sysfs attribute. Instead I'd prefer the flashing logic that twiddles
> > gpios and power live all in the kernel and have userspace interact with
> > a character device to program the firmware.
>
> Yeah, that would be even better, you're right.
>
> Hmmm, so maybe the answer is to just delay adding the regulator until
> we're actually ready to specify it correctly and have the flashing
> happen in the kernel?
>

I can enable it during probe just so that if the BIOS isn't doing it
we'll have something that works assuming the DT is actually controlling
the regulator. Or do nothing. It doesn't matter right now.

Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe

[Stephen Boyd]

Quoting Stephen Boyd (2022-03-18 16:36:32)
> Quoting Doug Anderson (2022-03-18 15:06:59)
> > On Fri, Mar 18, 2022 at 3:01 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > I'd rather not expose regulator control to userspace through some other
> > > sysfs attribute. Instead I'd prefer the flashing logic that twiddles
> > > gpios and power live all in the kernel and have userspace interact with
> > > a character device to program the firmware.
> >
> > Yeah, that would be even better, you're right.
> >
> > Hmmm, so maybe the answer is to just delay adding the regulator until
> > we're actually ready to specify it correctly and have the flashing
> > happen in the kernel?
> >
>
> I can enable it during probe just so that if the BIOS isn't doing it
> we'll have something that works assuming the DT is actually controlling
> the regulator. Or do nothing. It doesn't matter right now.

Thinking about it more there's no point in controlling the supply here
until we support flashing logic in the kernel. Without flashing support
in the kernel and without the BIOS turning on the power we can simply
make the regulator always-on and boot-on with real gpio control and then
it will be turned on during boot, emulating what the BIOS is doing. The
power cycling after flashing the firmware doesn't seem to be necessary
from my testing. And even then, the flashing script could unbind that
regulator driver if it really needed to control the power.