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

[Doug Anderson]

Hi,

On Mon, Mar 21, 2022 at 12:11 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 51b64b392c51..92518f90f86e 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>
> @@ -690,11 +691,13 @@ 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;
>         int ret;
> +       struct gpio_desc *boot0;
> +       struct gpio_desc *reset;
>
>         ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
>         if (!ret)
> @@ -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;
> +
> +       boot0 = devm_gpiod_get(dev, "boot0", 0);

I think that the last parameter to devm_gpiod_get() is better
described by "GPIOD_ASIS", right? Same for the other one below.


> +       if (IS_ERR(boot0))
> +               return PTR_ERR(boot0);
> +
> +       reset = devm_gpiod_get(dev, "reset", 0);
> +       if (IS_ERR(reset))
> +               return PTR_ERR(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(boot0) || gpiod_get_value(reset)) {

I believe that the above two calls are illegal as documented. The file
`Documentation/driver-api/gpio/consumer.rst` says that if you use
`GPIOD_ASIS` to get the GPIO that "The direction must be set later
with one of the dedicated functions.". The "must" there is important.

Oh, and further down it appears to be even more explicit and says "Be
aware that there is no default direction for GPIOs. Therefore, **using
a GPIO without setting its direction first is illegal and will result
in undefined behavior!**".

I assume that "get" counts as using?

I think this sorta gets into some of the limitations of the GPIO APIs
in Linux that try to make sure that they work on a "lowest common
denominator" GPIO controller. I don't think they promise that
"get_value" while in output mode is legal across all GPIO controllers.

Maybe a solution is to at least add a comment saying that the code
will only work on GPIO controllers that will let you get the value
back if it's an output?


> +               /* Boot0 is sampled on reset deassertion */
> +               gpiod_set_value(boot0, 0);
> +               gpiod_set_value(reset, 1);

Those two calls are almost certainly illegal / not guaranteed to work
without setting a direction, at least in the general case. Luckily I
think it's easy to just change both of them to
"gpiod_direction_output", which takes a value.

Actually, even on Qualcomm hardware I don't think those will work if
the boot direction was input, will they? They'll set the value that
_will_ be driven but won't cause it to actually be driven, right?


-Doug