lkml.org 
[lkml]   [2022]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@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?

\
 
 \ /
  Last update: 2022-03-18 21:51    [W:0.099 / U:0.960 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site