Re: Enable buttons GPIO passthrough

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Kwin,

thanks for your patch!

On Thu, Jan 31, 2019 at 4:11 PM Wang, Kuiying <kuiying.wang@xxxxxxxxx> wrote:

> Below patch is based gpio character device and extend .set_config() property to
> support pass-through.

This is starting to look good!

> And I plan to add passthrough setting for gpioset of libgpiod, patch is attached.
> If you agree I will upstream it together w/ this pass-through patch.

Bartosz is the maintainer of libgpiod so he can answer about that.

I'm not sure why it needs to be a userspace thing, can you explain
this?

Why can you not just set up passthrough in the device tree file?

It seems like a dangerous thing for userspace to deal with.

> BTW: I have 2 questions hope could get your answer:
>         1. why we have to switch to gpio character device and what's
>             the advantage of it?

- We get a proper topology with local numberspace per gpiochip
  instead of an unmaintainable large global numberspace with
  numbers that are magic and different on every system.

- Discovery mechanism, you can inspect the available GPIOs
  by gpiochip and name.

- When a process using a character device crashes, it will
  release its resources and release the GPIO lines it is holding.
  When using sysfs, if a process that exported a few GPIOs
  crashes, sometimes things are left in an undefined state,
  never cleaned up and the best way to get control over the
  machine again is reboot.

All of the above issue of the sysfs ABI were pretty
serious.

>         2. As you mentioned " sysfs ABI is deprecated since
>             over three years ", why sysfs ABI is still in use and
>             most of cases are still using sysfs ABI?

What are you basing this statement on? It is deprecated,
and people have to actively choose to compile it into their
kernel. Big distributions like Fedora does not support the sysfs
ABI and disable it in their kernels. If others enable it, please
tell then they are wrong and the GPIO maintainer want them
to stop using it.

>  And when sysfs ABI is removed totally?

Around the year 2020.

Maybe not even then, if people are still using it. However
the GPIO character device is design to last ~100 years so
I think it will win in the end. I have patience.

> Please share all the related information (including web page/video/PPT/etc.)  ASAP. :)

I have some presentation here:
https://dflund.se/~triad/papers/GPIO-for-Engineers-and-Makers.pdf

> +static int aspeed_gpio_pass_through(struct gpio_chip *chip, unsigned int offset,
> +                                   unsigned long usecs)
> +{
> +       printk("kwin::aspeed_gpio_pass_through is called");
> +       return 0;
> +}

I guess you want to actually implement this too :D

>  static int aspeed_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
>                                   unsigned long config)
>  {
>         unsigned long param = pinconf_to_config_param(config);
>         u32 arg = pinconf_to_config_argument(config);
> -
> +       printk("kwin:: aspeed_gpio_set_config");

Please remove debug prints when submitting the final patch.
BTW pr_info() is better for this kind of stuff.

> +       else if (param == PIN_CONFIG_PASS_THROUGH)
> +               return aspeed_gpio_pass_through((chip, offset, arg);

Yes this is good.

> @@ -579,6 +579,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>                         set_bit(FLAG_OPEN_DRAIN, &desc->flags);
>                 if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
>                         set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +               if (lflags & GPIOHANDLE_REQUEST_PASS_THROUGH)
> +                       set_bit(FLAG_PASS_THROUGH, &desc->flags);

This is good if we support this on the character device.

> @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>                         ret = gpiod_direction_input(desc);
>                         if (ret)
>                                 goto out_free_descs;
> +               } else if (lflags & GPIOHANDLE_REQUEST_PASS_THROUGH) {
> +                       ret = gpiod_direction_pass_through(desc);
> +                       if (ret)
> +                               goto out_free_descs;

OK looks good too.

> +/**
> + * gpiod_direction_pass_through - set the GPIO direction to pass-through
> + * @desc:      GPIO to set to pass-through
> + *
> + * Set the direction of the passed GPIO to passthrough.
> + *
> + * Return 0 in case of success, else an error code.
> + */
> +int gpiod_direction_pass_through(struct gpio_desc *desc)
> +{
> +       struct gpio_chip *gc;
> +       printk("kwin:: gpiod_direction_pass_through");
> +       VALIDATE_DESC(desc);
> +       /* GPIOs used for IRQs shall not be set as pass-through */
> +       if (test_bit(FLAG_USED_AS_IRQ, &desc->flags)) {
> +               gpiod_err(desc,
> +                         "%s: tried to set a GPIO tied to an IRQ as pass-through\n",
> +                         __func__);
> +               return -EIO;
> +       }
> +       gc = desc->gdev->chip;
> +       if (test_bit(FLAG_PASS_THROUGH, &desc->flags)) {
> +               gpio_set_drive_single_ended(gc, gpio_chip_hwgpio(desc),
> +                                           PIN_CONFIG_PASS_THROUGH);
> +       } else {
> +               gpiod_err(desc,
> +                         "%s: desc->flags is not set to FLAG_PASS_THROUGH\n",
> +                         __func__);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_direction_pass_through);

I think you should skip this. I don't think users in the kernel need
to dynamically set this up. It should be done as a flag in either
device trees, machine tables or from explicit request from the
character device ioctl().

>  #define FLAG_SYSFS     3       /* exported via /sys/class/gpio/control */
> +#define FLAG_PASS_THROUGH 4 /*Gpio is passthrough type*/

OK good.

> @@ -124,6 +124,7 @@ enum pin_config_param {
>         PIN_CONFIG_SLEW_RATE,
>         PIN_CONFIG_SKEW_DELAY,
>         PIN_CONFIG_PERSIST_STATE,
> +       PIN_CONFIG_PASS_THROUGH,

Good.


> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 1bf6e6df084b..44d9876b4aa2 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -62,6 +62,7 @@ struct gpioline_info {
>  #define GPIOHANDLE_REQUEST_ACTIVE_LOW  (1UL << 2)
>  #define GPIOHANDLE_REQUEST_OPEN_DRAIN  (1UL << 3)
>  #define GPIOHANDLE_REQUEST_OPEN_SOURCE (1UL << 4)
> +#define GPIOHANDLE_REQUEST_PASS_THROUGH (1UL << 5)

Seems right, if we should support userspace.


Things are missing from this patch set:

- Adding the flag to include/linux/gpio/machine.h so we can set this
  up from machine descriptor tables.
- Adding the flag to include/dt-bindings/gpio/gpio.h
  so we can set this from the device tree.

Also code for handling both above ways of setting it up.

And I think what you want to do with Aspeed is to set this up using
device tree and not in userspace, unless you can explain some
reason why you wouldn't do that and instead do this complex
and dangerous system operation in userspace. Definately you
want the system default to be set up from device tree and then
that should support passthrough, right?

I can't comment much on libgpiod, we'll see if we want this for
userspace first.

Yours,
Linus Walleij