Re: [PATCH v3 3/3] platform/chrome: cros_ec_spi: Boot fingerprint processor during probe
From: Stephen Boyd
Date: Fri Mar 18 2022 - 18:01:15 EST
Quoting Doug Anderson (2022-03-18 13:50:05)
> Hi,
>
> On Thu, Mar 17, 2022 at 6:55 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Craig Hesling <hesling@xxxxxxxxxxxx>
> > Cc: Tom Hughes <tomhughes@xxxxxxxxxxxx>
> > Cc: Alexandru M Stan <amstan@xxxxxxxxxxxx>
> > Cc: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>
> > Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> > 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.