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@xxxxxxxxxxxx> 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