2019-11-27 08:45:23

by Geert Uytterhoeven

[permalink] [raw]
Subject: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup

Currently GPIOs can only be referred to by GPIO controller and offset in
GPIO lookup tables.

Add support for looking them up by line name.

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
If this is rejected, the GPIO Aggregator documentation and code must be
updated.

v3:
- New.
---
drivers/gpio/gpiolib.c | 12 ++++++++++++
include/linux/gpio/machine.h | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d24a3d79dcfe69ad..cb608512ad6bbded 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4475,6 +4475,18 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
continue;

+ if (p->chip_hwnum == (u16)-1) {
+ desc = gpio_name_to_desc(p->chip_label);
+ if (desc) {
+ *flags = p->flags;
+ return desc;
+ }
+
+ dev_warn(dev, "cannot find GPIO line %s, deferring\n",
+ p->chip_label);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
chip = find_chip_by_name(p->chip_label);

if (!chip) {
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -31,7 +31,7 @@ enum gpio_lookup_flags {
*/
struct gpiod_lookup {
const char *chip_label;
- u16 chip_hwnum;
+ u16 chip_hwnum; /* if -1, chip_label is named line */
const char *con_id;
unsigned int idx;
unsigned long flags;
--
2.17.1


2019-11-28 03:47:27

by Ulrich Hecht

[permalink] [raw]
Subject: Re: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup


> On November 27, 2019 at 9:42 AM Geert Uytterhoeven <[email protected]> wrote:
>
>
> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
>
> Add support for looking them up by line name.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> If this is rejected, the GPIO Aggregator documentation and code must be
> updated.
>
> v3:
> - New.
> ---
> drivers/gpio/gpiolib.c | 12 ++++++++++++
> include/linux/gpio/machine.h | 2 +-
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d24a3d79dcfe69ad..cb608512ad6bbded 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4475,6 +4475,18 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
> if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
> continue;
>
> + if (p->chip_hwnum == (u16)-1) {
> + desc = gpio_name_to_desc(p->chip_label);
> + if (desc) {
> + *flags = p->flags;
> + return desc;
> + }
> +
> + dev_warn(dev, "cannot find GPIO line %s, deferring\n",
> + p->chip_label);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> chip = find_chip_by_name(p->chip_label);
>
> if (!chip) {
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -31,7 +31,7 @@ enum gpio_lookup_flags {
> */
> struct gpiod_lookup {
> const char *chip_label;
> - u16 chip_hwnum;
> + u16 chip_hwnum; /* if -1, chip_label is named line */

The magic number "-1" might deserve a #define.

> const char *con_id;
> unsigned int idx;
> unsigned long flags;
> --
> 2.17.1
>

With or without #define,
Reviewed-by: Ulrich Hecht <[email protected]>

CU
Uli

2019-12-12 13:41:50

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup

On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
<[email protected]> wrote:

> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
>
> Add support for looking them up by line name.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>

OK I see what you want to do...

> + if (p->chip_hwnum == (u16)-1) {

If you want to use this then use:

if (p->chip_hwnum == U16_MAX)

from <linux/limits.h>

> + desc = gpio_name_to_desc(p->chip_label);
> + if (desc) {
> + *flags = p->flags;
> + return desc;
> + }
> +
> + dev_warn(dev, "cannot find GPIO line %s, deferring\n",
> + p->chip_label);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> chip = find_chip_by_name(p->chip_label);
>
> if (!chip) {
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -31,7 +31,7 @@ enum gpio_lookup_flags {
> */
> struct gpiod_lookup {
> const char *chip_label;
> - u16 chip_hwnum;
> + u16 chip_hwnum; /* if -1, chip_label is named line */

/* if U16_MAX then chip_label is the named line we are looking for */

But the member name "chip_label" is completely abused with this
setup, it should then be renamed as part of the patch set to something
like chip_label_or_line_name so it is clear what it is or
just name it "const char *key".

But I'm not entirely convinced about reusing the existing
struct gpio_lookup for this.

What about constructing a new lookup struct specifically for this?
I understand it is more work, but will that not be more
maintainable and readable?

Yours,
Linus Walleij