Message-ID: <CAD=FV=VJFoNgfMUHE5mUY6Uq2Jj5cyQVyW0osUm6jyHRE1no=w@mail.gmail.com>
Date:   Fri, 18 Mar 2022 13:50:05 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        chrome-platform@...ts.linux.dev,
        Guenter Roeck <groeck@...omium.org>,
        Craig Hesling <hesling@...omium.org>,
        Tom Hughes <tomhughes@...omium.org>,
        Alexandru M Stan <amstan@...omium.org>,
        Tzung-Bi Shih <tzungbi@...nel.org>,
        Matthias Kaehlcke <mka@...omium.org>
Subject: Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint
 processor during probe

Hi,

On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@...omium.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@...omium.org>
> Cc: Douglas Anderson <dianders@...omium.org>
> Cc: Craig Hesling <hesling@...omium.org>
> Cc: Tom Hughes <tomhughes@...omium.org>
> Cc: Alexandru M Stan <amstan@...omium.org>
> Cc: Tzung-Bi Shih <tzungbi@...nel.org>
> Reviewed-by: Matthias Kaehlcke <mka@...omium.org>
> Signed-off-by: Stephen Boyd <swboyd@...omium.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?

Powered by blists - more mailing lists