linux-kernel - Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAVeFuJZ3TwozZzpuHcYPDLjEsb2D_qvQADf85s3-eywYYHs8A@mail.gmail.com>
Date:	Mon, 7 Oct 2013 21:54:13 -0700
From:	Alexandre Courbot <gnurou@...il.com>
To:	Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	rjw@...ysocki.net, Linus Walleij <linus.walleij@...aro.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	Alexandre Courbot <acourbot@...dia.com>,
	Rob Landley <rob@...dley.net>, linux-acpi@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

On Tue, Oct 1, 2013 at 7:58 AM, Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> The new GPIO descriptor based interface is now preferred over the old
> integer based one. This patch converts the ACPI GPIO helpers to use this
> new interface internally. In addition to that provide compatibility
> functions acpi_get_gpio() and acpi_get_gpio_by_index() that convert the
> returned GPIO descriptors to integers.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 51 +++++++++++++++++++++------------------------
>  include/linux/acpi_gpio.h   | 38 ++++++++++++++++++++++++++-------
>  2 files changed, 54 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 1745ce5..333297c 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -11,7 +11,7 @@
>   */
>
>  #include <linux/errno.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/export.h>
>  #include <linux/acpi_gpio.h>
>  #include <linux/acpi.h>
> @@ -33,14 +33,15 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  }
>
>  /**
> - * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API
> + * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
>   * @path:      ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
>   * @pin:       ACPI GPIO pin number (0-based, controller-relative)
>   *
> - * Returns GPIO number to use with Linux generic GPIO API, or errno error value
> + * Returns GPIO descriptor to use with Linux generic GPIO API, or ERR_PTR
> + * error value
>   */
>
> -int acpi_get_gpio(char *path, int pin)
> +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  {
>         struct gpio_chip *chip;
>         acpi_handle handle;
> @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
>
>         status = acpi_get_handle(NULL, path, &handle);
>         if (ACPI_FAILURE(status))
> -               return -ENODEV;
> +               return ERR_PTR(-ENODEV);
>
>         chip = gpiochip_find(handle, acpi_gpiochip_find);
>         if (!chip)
> -               return -ENODEV;
> +               return ERR_PTR(-ENODEV);
>
> -       if (!gpio_is_valid(chip->base + pin))
> -               return -EINVAL;
> -
> -       return chip->base + pin;
> +       return gpio_to_desc(chip->base + pin);

I think you want to avoid using gpio_to_desc(). It is just a
convenience function provided to ease the transition for consumers
that still need to rely on GPIO numbers for some reason. The same
applies to desc_to_gpio(), although the usage you are making of it
(implemented fallbacks for the integer space) is the one that is
intended.

The last line could be changed to "return &chip->desc[pin];" after
checking that 0 <= pin <= chip->ngpio.

This minor issue apart, I really like the fact you are moving this
under the gpiolib APIs (also appreciate the testing of the new API
this patchset provides!).

I also wonder whether you could also avoid exporting acpi_get_gpiod*
(which allow GPIO consumers to shortcut gpiolib) by implementing
acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
"char *path" parameter though, so maybe that would not be possible at
the moment.

Once the gpio_to_desc() issue mentioned above is fixed I think I can give my

Reviewed-by: Alexandre Courbot <acourbot@...dia.com>

on patches 2-5.

Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/