[PATCH v2 0/6] Add support for software nodes to gpiolib

[PATCH v2 0/6] Add support for software nodes to gpiolib

[Dmitry Torokhov]

This series attempts to add support for software nodes to gpiolib, using
software node references. This allows us to convert more drivers to the
generic device properties and drop support for custom platform data.

To describe a GPIO via software nodes we can create the following data
items:

/* Node representing the GPIO controller/GPIO bank */
static const struct software_node gpio_bank_b_node = {
        .name = "B",
};

/*
 * Properties that will be assigned to a software node assigned to
 * the device that used platform data.
 */
static const struct property_entry simone_key_enter_props[] = {
        PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
        PROPERTY_ENTRY_STRING("label", "enter"),
        PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
        { }
};

The code in gpiolib handling software nodes uses the name in the
software node representing GPIO controller to locate the actual instance
of GPIO controller.

To: Linus Walleij <linus.walleij@linaro.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-acpi@vger.kernel.org

---
Changes in v2:
- reworked the series to be independent of other in-flight patches.
  That meant keeping devm_gpiod_get_from_of_node() for now.
- removed handling of secondary nodes, it deserves a separate patch
  series
- fixed refcounting when handling swnodes (Andy)
- added include/linux/gpio/property.h with PROPERTY_ENTRY_GPIO (Andy)
- addressed most of the rest of Andy's comments
- collected reviewed-by and acked-by
- Link to v1: https://lore.kernel.org/r/20221031-gpiolib-swnode-v1-0-a0ab48d229c7@gmail.com

---
Dmitry Torokhov (6):
      gpiolib: of: change of_find_gpio() to accept device node
      gpiolib: acpi: change acpi_find_gpio() to accept firmware node
      gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes
      gpiolib: acpi: avoid leaking ACPI details into upper gpiolib layers
      gpiolib: consolidate GPIO lookups
      gpiolib: add support for software nodes

 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpiolib-acpi.c   | 132 ++++++++++++++-----------
 drivers/gpio/gpiolib-acpi.h   |  54 +----------
 drivers/gpio/gpiolib-of.c     |   7 +-
 drivers/gpio/gpiolib-of.h     |   4 +-
 drivers/gpio/gpiolib-swnode.c | 128 ++++++++++++++++++++++++
 drivers/gpio/gpiolib-swnode.h |  14 +++
 drivers/gpio/gpiolib.c        | 219 +++++++++++++++++-------------------------
 include/linux/gpio/property.h |  11 +++
 9 files changed, 329 insertions(+), 241 deletions(-)
---
base-commit: b6fc3fddade7a194bd141a49f2689e50f796ef46
change-id: 20221031-gpiolib-swnode-948203f49b23

-- 
Dmitry

[PATCH v2 1/6] gpiolib: of: change of_find_gpio() to accept device node

[Dmitry Torokhov]

In preparation of switching all OF-based GPIO lookups to go through
of_find_gpio() let's change it to accept device node as its argument as
we do not always have access to device structure.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-of.c | 7 +++----
 drivers/gpio/gpiolib-of.h | 4 ++--
 drivers/gpio/gpiolib.c    | 5 +++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index be9c34cca322..52e4ac0fcaa1 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -623,7 +623,7 @@ static const of_find_gpio_quirk of_find_gpio_quirks[] = {
 	NULL
 };
 
-struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
+struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id,
 			       unsigned int idx, unsigned long *flags)
 {
 	char prop_name[32]; /* 32 is max size of property name */
@@ -641,8 +641,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 			snprintf(prop_name, sizeof(prop_name), "%s",
 				 gpio_suffixes[i]);
 
-		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
-						&of_flags);
+		desc = of_get_named_gpiod_flags(np, prop_name, idx, &of_flags);
 
 		if (!gpiod_not_found(desc))
 			break;
@@ -650,7 +649,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 	/* Properly named GPIO was not found, try workarounds */
 	for (q = of_find_gpio_quirks; gpiod_not_found(desc) && *q; q++)
-		desc = (*q)(dev->of_node, con_id, idx, &of_flags);
+		desc = (*q)(np, con_id, idx, &of_flags);
 
 	if (IS_ERR(desc))
 		return desc;
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 1b5df39a952e..cb8b70a2723e 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -16,7 +16,7 @@ struct gpio_desc;
 struct gpio_device;
 
 #ifdef CONFIG_OF_GPIO
-struct gpio_desc *of_find_gpio(struct device *dev,
+struct gpio_desc *of_find_gpio(struct device_node *np,
 			       const char *con_id,
 			       unsigned int idx,
 			       unsigned long *lookupflags);
@@ -26,7 +26,7 @@ int of_gpio_get_count(struct device *dev, const char *con_id);
 bool of_gpio_need_valid_mask(const struct gpio_chip *gc);
 void of_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
 #else
-static inline struct gpio_desc *of_find_gpio(struct device *dev,
+static inline struct gpio_desc *of_find_gpio(struct device_node *np,
 					     const char *con_id,
 					     unsigned int idx,
 					     unsigned long *lookupflags)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e8faedca6b14..c5a80def8be4 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4070,14 +4070,15 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	int ret;
 	/* Maybe we have a device name, maybe not */
 	const char *devname = dev ? dev_name(dev) : "?";
-	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
+	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
 
 	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
 
 	/* Using device tree? */
 	if (is_of_node(fwnode)) {
 		dev_dbg(dev, "using device tree for GPIO lookup\n");
-		desc = of_find_gpio(dev, con_id, idx, &lookupflags);
+		desc = of_find_gpio(to_of_node(fwnode),
+				    con_id, idx, &lookupflags);
 	} else if (is_acpi_node(fwnode)) {
 		dev_dbg(dev, "using ACPI for GPIO lookup\n");
 		desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);

-- 
b4 0.11.0-dev-28747

[PATCH v2 2/6] gpiolib: acpi: change acpi_find_gpio() to accept firmware node

[Dmitry Torokhov]

In preparation of switching all ACPI-based GPIO lookups to go through
acpi_find_gpio() let's change it to accept device node as its argument
as we do not always have access to device structure.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-acpi.c | 8 ++++++--
 drivers/gpio/gpiolib-acpi.h | 4 ++--
 drivers/gpio/gpiolib.c      | 3 ++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 064ba5150fd4..ccb74e208989 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -906,18 +906,22 @@ static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
 	return con_id == NULL;
 }
 
-struct gpio_desc *acpi_find_gpio(struct device *dev,
+struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 const char *con_id,
 				 unsigned int idx,
 				 enum gpiod_flags *dflags,
 				 unsigned long *lookupflags)
 {
-	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct acpi_device *adev;
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
 	char propname[32];
 	int i;
 
+	adev = to_acpi_device_node(fwnode);
+	if (!adev)
+		return ERR_PTR(-ENODEV);
+
 	/* Try first from _DSD */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
 		if (con_id) {
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index 01e0cb480a00..bd1f9b92ea9e 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -60,7 +60,7 @@ int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
 int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
 					struct acpi_gpio_info *info);
 
-struct gpio_desc *acpi_find_gpio(struct device *dev,
+struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 const char *con_id,
 				 unsigned int idx,
 				 enum gpiod_flags *dflags,
@@ -95,7 +95,7 @@ acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
 }
 
 static inline struct gpio_desc *
-acpi_find_gpio(struct device *dev, const char *con_id,
+acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id,
 	       unsigned int idx, enum gpiod_flags *dflags,
 	       unsigned long *lookupflags)
 {
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c5a80def8be4..eebcdaca5e06 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4081,7 +4081,8 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 				    con_id, idx, &lookupflags);
 	} else if (is_acpi_node(fwnode)) {
 		dev_dbg(dev, "using ACPI for GPIO lookup\n");
-		desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
+		desc = acpi_find_gpio(fwnode,
+				      con_id, idx, &flags, &lookupflags);
 	}
 
 	/*

-- 
b4 0.11.0-dev-28747

[PATCH v2 3/6] gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes

[Dmitry Torokhov]

In preparation of switching all ACPI-based GPIO lookups to go through
acpi_find_gpio() we need to make sure it can handle data-only ACPI
nodes, same as existing acpi_node_get_gpiod().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-acpi.c | 76 +++++++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index ccb74e208989..d51bf2a3203d 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -864,8 +864,9 @@ static int acpi_gpio_property_lookup(struct fwnode_handle *fwnode,
  * function only returns the first.
  */
 static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
-					  const char *propname, int index,
-					  struct acpi_gpio_info *info)
+						 const char *propname,
+						 int index,
+						 struct acpi_gpio_info *info)
 {
 	struct acpi_gpio_lookup lookup;
 	int ret;
@@ -896,6 +897,44 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
 	return ret ? ERR_PTR(ret) : lookup.desc;
 }
 
+/**
+ * acpi_get_gpiod_from_data() - get a GPIO descriptor from ACPI data node
+ * @fwnode: pointer to an ACPI firmware node to get the GPIO information from
+ * @propname: Property name of the GPIO
+ * @index: index of GpioIo/GpioInt resource (starting from %0)
+ * @info: info pointer to fill in (optional)
+ *
+ * This function uses the property-based GPIO lookup to get to the GPIO
+ * resource with the relevant information from a data-only ACPI firmware node
+ * and uses that to obtain the GPIO descriptor to return.
+ *
+ * If the GPIO cannot be translated or there is an error an ERR_PTR is
+ * returned.
+ */
+static struct gpio_desc *acpi_get_gpiod_from_data(struct fwnode_handle *fwnode,
+						  const char *propname,
+						  int index,
+						  struct acpi_gpio_info *info)
+{
+	struct acpi_gpio_lookup lookup;
+	int ret;
+
+	if (!is_acpi_data_node(fwnode))
+		return ERR_PTR(-ENODEV);
+
+	if (!propname)
+		return ERR_PTR(-EINVAL);
+
+	lookup.index = index;
+
+	ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = acpi_gpio_resource_lookup(&lookup, info);
+	return ret ? ERR_PTR(ret) : lookup.desc;
+}
+
 static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
 				     const char *con_id)
 {
@@ -912,16 +951,12 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 enum gpiod_flags *dflags,
 				 unsigned long *lookupflags)
 {
-	struct acpi_device *adev;
+	struct acpi_device *adev = to_acpi_device_node(fwnode);
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
 	char propname[32];
 	int i;
 
-	adev = to_acpi_device_node(fwnode);
-	if (!adev)
-		return ERR_PTR(-ENODEV);
-
 	/* Try first from _DSD */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
 		if (con_id) {
@@ -932,7 +967,12 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 gpio_suffixes[i]);
 		}
 
-		desc = acpi_get_gpiod_by_index(adev, propname, idx, &info);
+		if (adev)
+			desc = acpi_get_gpiod_by_index(adev,
+						       propname, idx, &info);
+		else
+			desc = acpi_get_gpiod_from_data(fwnode,
+						        propname, idx, &info);
 		if (!IS_ERR(desc))
 			break;
 		if (PTR_ERR(desc) == -EPROBE_DEFER)
@@ -941,7 +981,7 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 
 	/* Then from plain _CRS GPIOs */
 	if (IS_ERR(desc)) {
-		if (!acpi_can_fallback_to_crs(adev, con_id))
+		if (!adev || !acpi_can_fallback_to_crs(adev, con_id))
 			return ERR_PTR(-ENOENT);
 
 		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
@@ -979,29 +1019,13 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
 				      const char *propname, int index,
 				      struct acpi_gpio_info *info)
 {
-	struct acpi_gpio_lookup lookup;
 	struct acpi_device *adev;
-	int ret;
 
 	adev = to_acpi_device_node(fwnode);
 	if (adev)
 		return acpi_get_gpiod_by_index(adev, propname, index, info);
 
-	if (!is_acpi_data_node(fwnode))
-		return ERR_PTR(-ENODEV);
-
-	if (!propname)
-		return ERR_PTR(-EINVAL);
-
-	memset(&lookup, 0, sizeof(lookup));
-	lookup.index = index;
-
-	ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
-	if (ret)
-		return ERR_PTR(ret);
-
-	ret = acpi_gpio_resource_lookup(&lookup, info);
-	return ret ? ERR_PTR(ret) : lookup.desc;
+	return acpi_get_gpiod_from_data(fwnode, propname, index, info);
 }
 
 /**

-- 
b4 0.11.0-dev-28747

[PATCH v2 4/6] gpiolib: acpi: avoid leaking ACPI details into upper gpiolib layers

[Dmitry Torokhov]

There is no need for the generic parts of GPIOLIB to be aware of
implementation details of ACPI-bases lookups.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-acpi.c | 51 ++++++++++++++++++++++++++++++++++++++-------
 drivers/gpio/gpiolib-acpi.h | 46 +++-------------------------------------
 drivers/gpio/gpiolib.c      |  8 ++-----
 3 files changed, 48 insertions(+), 57 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d51bf2a3203d..1bc386032ca8 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -89,6 +89,30 @@ struct acpi_gpio_chip {
 	struct list_head deferred_req_irqs_list_entry;
 };
 
+/**
+ * struct acpi_gpio_info - ACPI GPIO specific information
+ * @adev: reference to ACPI device which consumes GPIO resource
+ * @flags: GPIO initialization flags
+ * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
+ * @pin_config: pin bias as provided by ACPI
+ * @polarity: interrupt polarity as provided by ACPI
+ * @triggering: triggering type as provided by ACPI
+ * @wake_capable: wake capability as provided by ACPI
+ * @debounce: debounce timeout as provided by ACPI
+ * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
+ */
+struct acpi_gpio_info {
+	struct acpi_device *adev;
+	enum gpiod_flags flags;
+	bool gpioint;
+	int pin_config;
+	int polarity;
+	int triggering;
+	bool wake_capable;
+	unsigned int debounce;
+	unsigned int quirks;
+};
+
 /*
  * For GPIO chips which call acpi_gpiochip_request_interrupts() before late_init
  * (so builtin drivers) we register the ACPI GpioInt IRQ handlers from a
@@ -670,8 +694,8 @@ __acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
 	return ret;
 }
 
-int
-acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *info)
+static int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
+				        struct acpi_gpio_info *info)
 {
 	struct device *dev = &info->adev->dev;
 	enum gpiod_flags old = *flags;
@@ -690,8 +714,8 @@ acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *inf
 	return ret;
 }
 
-int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
-					struct acpi_gpio_info *info)
+static int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
+					       struct acpi_gpio_info *info)
 {
 	switch (info->pin_config) {
 	case ACPI_PIN_CONFIG_PULLUP:
@@ -1005,7 +1029,8 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
  * @fwnode: pointer to an ACPI firmware node to get the GPIO information from
  * @propname: Property name of the GPIO
  * @index: index of GpioIo/GpioInt resource (starting from %0)
- * @info: info pointer to fill in (optional)
+ * @lflags: bitmask of gpio_lookup_flags GPIO_* values
+ * @dflags: gpiod initialization flags
  *
  * If @fwnode is an ACPI device object, call acpi_get_gpiod_by_index() for it.
  * Otherwise (i.e. it is a data-only non-device object), use the property-based
@@ -1017,15 +1042,25 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
  */
 struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
 				      const char *propname, int index,
-				      struct acpi_gpio_info *info)
+				      unsigned long *lflags,
+				      enum gpiod_flags *dflags)
 {
+	struct acpi_gpio_info info;
 	struct acpi_device *adev;
+	struct gpio_desc *desc;
 
 	adev = to_acpi_device_node(fwnode);
 	if (adev)
-		return acpi_get_gpiod_by_index(adev, propname, index, info);
+		desc = acpi_get_gpiod_by_index(adev, propname, index, &info);
+	else
+		desc = acpi_get_gpiod_from_data(fwnode, propname, index, &info);
 
-	return acpi_get_gpiod_from_data(fwnode, propname, index, info);
+	if (!IS_ERR(desc)) {
+		acpi_gpio_update_gpiod_flags(dflags, &info);
+		acpi_gpio_update_gpiod_lookup_flags(lflags, &info);
+	}
+
+	return desc;
 }
 
 /**
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index bd1f9b92ea9e..8880615327ac 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -22,30 +22,6 @@ struct gpio_chip;
 struct gpio_desc;
 struct gpio_device;
 
-/**
- * struct acpi_gpio_info - ACPI GPIO specific information
- * @adev: reference to ACPI device which consumes GPIO resource
- * @flags: GPIO initialization flags
- * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
- * @pin_config: pin bias as provided by ACPI
- * @polarity: interrupt polarity as provided by ACPI
- * @triggering: triggering type as provided by ACPI
- * @wake_capable: wake capability as provided by ACPI
- * @debounce: debounce timeout as provided by ACPI
- * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
- */
-struct acpi_gpio_info {
-	struct acpi_device *adev;
-	enum gpiod_flags flags;
-	bool gpioint;
-	int pin_config;
-	int polarity;
-	int triggering;
-	bool wake_capable;
-	unsigned int debounce;
-	unsigned int quirks;
-};
-
 #ifdef CONFIG_ACPI
 void acpi_gpiochip_add(struct gpio_chip *chip);
 void acpi_gpiochip_remove(struct gpio_chip *chip);
@@ -55,11 +31,6 @@ void acpi_gpio_dev_init(struct gpio_chip *gc, struct gpio_device *gdev);
 void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
-int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
-				 struct acpi_gpio_info *info);
-int acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
-					struct acpi_gpio_info *info);
-
 struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 const char *con_id,
 				 unsigned int idx,
@@ -67,7 +38,8 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 unsigned long *lookupflags);
 struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
 				      const char *propname, int index,
-				      struct acpi_gpio_info *info);
+				      unsigned long *lflags,
+				      enum gpiod_flags *dflags);
 
 int acpi_gpio_count(struct device *dev, const char *con_id);
 #else
@@ -82,18 +54,6 @@ acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
 static inline void
 acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
 
-static inline int
-acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, struct acpi_gpio_info *info)
-{
-	return 0;
-}
-static inline int
-acpi_gpio_update_gpiod_lookup_flags(unsigned long *lookupflags,
-				    struct acpi_gpio_info *info)
-{
-	return 0;
-}
-
 static inline struct gpio_desc *
 acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id,
 	       unsigned int idx, enum gpiod_flags *dflags,
@@ -103,7 +63,7 @@ acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id,
 }
 static inline struct gpio_desc *
 acpi_node_get_gpiod(struct fwnode_handle *fwnode, const char *propname,
-		    int index, struct acpi_gpio_info *info)
+		    int index, unsigned long *lflags, enum gpiod_flags *dflags)
 {
 	return ERR_PTR(-ENXIO);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eebcdaca5e06..f0a7a59ac630 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3838,14 +3838,10 @@ static struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 					      label);
 		return desc;
 	} else if (is_acpi_node(fwnode)) {
-		struct acpi_gpio_info info;
-
-		desc = acpi_node_get_gpiod(fwnode, propname, index, &info);
+		desc = acpi_node_get_gpiod(fwnode, propname, index,
+					   &lflags, &dflags);
 		if (IS_ERR(desc))
 			return desc;
-
-		acpi_gpio_update_gpiod_flags(&dflags, &info);
-		acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
 	} else {
 		return ERR_PTR(-EINVAL);
 	}

-- 
b4 0.11.0-dev-28747

[PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Dmitry Torokhov]

Ensure that all paths to obtain/look up GPIOD from generic
consumer-visible APIs go through the new gpiod_find_and_request()
helper, so that we can easily extend it with support for new firmware
mechanisms.

The only exception is OF-specific [devm_]gpiod_get_from_of_node() API
that is still being used by a couple of drivers and will be removed as
soon as patches converting them to use generic fwnode/device APIs are
accepted.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-acpi.c |  39 ---------
 drivers/gpio/gpiolib-acpi.h |  10 ---
 drivers/gpio/gpiolib.c      | 207 +++++++++++++++++---------------------------
 3 files changed, 81 insertions(+), 175 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 1bc386032ca8..bed0380c5136 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1024,45 +1024,6 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 	return desc;
 }
 
-/**
- * acpi_node_get_gpiod() - get a GPIO descriptor from ACPI resources
- * @fwnode: pointer to an ACPI firmware node to get the GPIO information from
- * @propname: Property name of the GPIO
- * @index: index of GpioIo/GpioInt resource (starting from %0)
- * @lflags: bitmask of gpio_lookup_flags GPIO_* values
- * @dflags: gpiod initialization flags
- *
- * If @fwnode is an ACPI device object, call acpi_get_gpiod_by_index() for it.
- * Otherwise (i.e. it is a data-only non-device object), use the property-based
- * GPIO lookup to get to the GPIO resource with the relevant information and use
- * that to obtain the GPIO descriptor to return.
- *
- * If the GPIO cannot be translated or there is an error an ERR_PTR is
- * returned.
- */
-struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
-				      const char *propname, int index,
-				      unsigned long *lflags,
-				      enum gpiod_flags *dflags)
-{
-	struct acpi_gpio_info info;
-	struct acpi_device *adev;
-	struct gpio_desc *desc;
-
-	adev = to_acpi_device_node(fwnode);
-	if (adev)
-		desc = acpi_get_gpiod_by_index(adev, propname, index, &info);
-	else
-		desc = acpi_get_gpiod_from_data(fwnode, propname, index, &info);
-
-	if (!IS_ERR(desc)) {
-		acpi_gpio_update_gpiod_flags(dflags, &info);
-		acpi_gpio_update_gpiod_lookup_flags(lflags, &info);
-	}
-
-	return desc;
-}
-
 /**
  * acpi_dev_gpio_irq_wake_get_by() - Find GpioInt and translate it to Linux IRQ number
  * @adev: pointer to a ACPI device to get IRQ from
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index 8880615327ac..9475f99a9694 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -36,10 +36,6 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 				 unsigned int idx,
 				 enum gpiod_flags *dflags,
 				 unsigned long *lookupflags);
-struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
-				      const char *propname, int index,
-				      unsigned long *lflags,
-				      enum gpiod_flags *dflags);
 
 int acpi_gpio_count(struct device *dev, const char *con_id);
 #else
@@ -61,12 +57,6 @@ acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id,
 {
 	return ERR_PTR(-ENOENT);
 }
-static inline struct gpio_desc *
-acpi_node_get_gpiod(struct fwnode_handle *fwnode, const char *propname,
-		    int index, unsigned long *lflags, enum gpiod_flags *dflags)
-{
-	return ERR_PTR(-ENXIO);
-}
 static inline int acpi_gpio_count(struct device *dev, const char *con_id)
 {
 	return -ENODEV;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f0a7a59ac630..c33ffdd47667 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3801,58 +3801,89 @@ static int platform_gpio_count(struct device *dev, const char *con_id)
 	return count;
 }
 
-/**
- * fwnode_get_named_gpiod - obtain a GPIO from firmware node
- * @fwnode:	handle of the firmware node
- * @propname:	name of the firmware property representing the GPIO
- * @index:	index of the GPIO to obtain for the consumer
- * @dflags:	GPIO initialization flags
- * @label:	label to attach to the requested GPIO
- *
- * This function can be used for drivers that get their configuration
- * from opaque firmware.
- *
- * The function properly finds the corresponding GPIO using whatever is the
- * underlying firmware interface and then makes sure that the GPIO
- * descriptor is requested before it is returned to the caller.
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-static struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
-						const char *propname, int index,
-						enum gpiod_flags dflags,
-						const char *label)
+static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
+					      struct device *consumer,
+					      const char *con_id,
+					      unsigned int idx,
+					      enum gpiod_flags *flags,
+					      unsigned long *lookupflags)
 {
-	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
-	struct gpio_desc *desc = ERR_PTR(-ENODEV);
-	int ret;
+	struct gpio_desc *desc;
 
+	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%pfw'\n",
+		con_id, fwnode);
+
+	/* Using device tree? */
 	if (is_of_node(fwnode)) {
-		desc = gpiod_get_from_of_node(to_of_node(fwnode),
-					      propname, index,
-					      dflags,
-					      label);
-		return desc;
+		dev_dbg(consumer, "using device tree for GPIO lookup\n");
+		desc = of_find_gpio(to_of_node(fwnode),
+				    con_id, idx, lookupflags);
 	} else if (is_acpi_node(fwnode)) {
-		desc = acpi_node_get_gpiod(fwnode, propname, index,
-					   &lflags, &dflags);
-		if (IS_ERR(desc))
-			return desc;
+		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
+		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
 	} else {
-		return ERR_PTR(-EINVAL);
+		desc = ERR_PTR(-ENOENT);
 	}
 
-	/* Currently only ACPI takes this path */
+	return desc;
+}
+
+static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
+						struct fwnode_handle *fwnode,
+						const char *con_id,
+						unsigned int idx,
+						enum gpiod_flags flags,
+						const char *label,
+						bool platform_lookup_allowed)
+{
+	struct gpio_desc *desc = ERR_PTR(-ENOENT);
+	unsigned long lookupflags;
+	int ret;
+
+	if (!IS_ERR_OR_NULL(fwnode))
+		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
+					    &flags, &lookupflags);
+
+	if (gpiod_not_found(desc) && platform_lookup_allowed) {
+		/*
+		 * Either we are not using DT or ACPI, or their lookup did not
+		 * return a result. In that case, use platform lookup as a
+		 * fallback.
+		 */
+		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
+		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
+	}
+
+	if (IS_ERR(desc)) {
+		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
+		return desc;
+	}
+
+	/*
+	 * If a connection label was passed use that, else attempt to use
+	 * the device name as label
+	 */
 	ret = gpiod_request(desc, label);
-	if (ret)
-		return ERR_PTR(ret);
+	if (ret) {
+		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
+			return ERR_PTR(ret);
+
+		/*
+		 * This happens when there are several consumers for
+		 * the same GPIO line: we just return here without
+		 * further initialization. It is a bit of a hack.
+		 * This is necessary to support fixed regulators.
+		 *
+		 * FIXME: Make this more sane and safe.
+		 */
+		dev_info(consumer,
+			 "nonexclusive access to GPIO for %s\n", con_id);
+		return desc;
+	}
 
-	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
+	ret = gpiod_configure_flags(desc, con_id, lookupflags, flags);
 	if (ret < 0) {
+		dev_dbg(consumer, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
 		return ERR_PTR(ret);
 	}
@@ -3885,29 +3916,13 @@ static struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
  * In case of error an ERR_PTR() is returned.
  */
 struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
-					 const char *con_id, int index,
+					 const char *con_id,
+					 int index,
 					 enum gpiod_flags flags,
 					 const char *label)
 {
-	struct gpio_desc *desc;
-	char prop_name[32]; /* 32 is max size of property name */
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
-		if (con_id)
-			snprintf(prop_name, sizeof(prop_name), "%s-%s",
-					    con_id, gpio_suffixes[i]);
-		else
-			snprintf(prop_name, sizeof(prop_name), "%s",
-					    gpio_suffixes[i]);
-
-		desc = fwnode_get_named_gpiod(fwnode, prop_name, index, flags,
-					      label);
-		if (!gpiod_not_found(desc))
-			break;
-	}
-
-	return desc;
+	return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label,
+				      false);
 }
 EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
 
@@ -4061,72 +4076,12 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 					       unsigned int idx,
 					       enum gpiod_flags flags)
 {
-	unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT;
-	struct gpio_desc *desc = NULL;
-	int ret;
-	/* Maybe we have a device name, maybe not */
-	const char *devname = dev ? dev_name(dev) : "?";
 	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
+	const char *devname = dev ? dev_name(dev) : "?";
+	const char *label = con_id ?: devname;
 
-	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
-
-	/* Using device tree? */
-	if (is_of_node(fwnode)) {
-		dev_dbg(dev, "using device tree for GPIO lookup\n");
-		desc = of_find_gpio(to_of_node(fwnode),
-				    con_id, idx, &lookupflags);
-	} else if (is_acpi_node(fwnode)) {
-		dev_dbg(dev, "using ACPI for GPIO lookup\n");
-		desc = acpi_find_gpio(fwnode,
-				      con_id, idx, &flags, &lookupflags);
-	}
-
-	/*
-	 * Either we are not using DT or ACPI, or their lookup did not return
-	 * a result. In that case, use platform lookup as a fallback.
-	 */
-	if (!desc || gpiod_not_found(desc)) {
-		dev_dbg(dev, "using lookup tables for GPIO lookup\n");
-		desc = gpiod_find(dev, con_id, idx, &lookupflags);
-	}
-
-	if (IS_ERR(desc)) {
-		dev_dbg(dev, "No GPIO consumer %s found\n", con_id);
-		return desc;
-	}
-
-	/*
-	 * If a connection label was passed use that, else attempt to use
-	 * the device name as label
-	 */
-	ret = gpiod_request(desc, con_id ?: devname);
-	if (ret) {
-		if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
-			return ERR_PTR(ret);
-
-		/*
-		 * This happens when there are several consumers for
-		 * the same GPIO line: we just return here without
-		 * further initialization. It is a bit of a hack.
-		 * This is necessary to support fixed regulators.
-		 *
-		 * FIXME: Make this more sane and safe.
-		 */
-		dev_info(dev, "nonexclusive access to GPIO for %s\n", con_id ?: devname);
-		return desc;
-	}
-
-	ret = gpiod_configure_flags(desc, con_id, lookupflags, flags);
-	if (ret < 0) {
-		dev_dbg(dev, "setup of GPIO %s failed\n", con_id);
-		gpiod_put(desc);
-		return ERR_PTR(ret);
-	}
-
-	blocking_notifier_call_chain(&desc->gdev->notifier,
-				     GPIOLINE_CHANGED_REQUESTED, desc);
-
-	return desc;
+	return gpiod_find_and_request(dev, fwnode, con_id, idx, flags, label,
+				      true);
 }
 EXPORT_SYMBOL_GPL(gpiod_get_index);
 

-- 
b4 0.11.0-dev-28747

Re: [PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote:
> Ensure that all paths to obtain/look up GPIOD from generic
> consumer-visible APIs go through the new gpiod_find_and_request()
> helper, so that we can easily extend it with support for new firmware
> mechanisms.
> 
> The only exception is OF-specific [devm_]gpiod_get_from_of_node() API
> that is still being used by a couple of drivers and will be removed as
> soon as patches converting them to use generic fwnode/device APIs are
> accepted.

...

> +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
> +					      struct device *consumer,
> +					      const char *con_id,
> +					      unsigned int idx,
> +					      enum gpiod_flags *flags,
> +					      unsigned long *lookupflags)
>  {
> -	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;

> -	struct gpio_desc *desc = ERR_PTR(-ENODEV);

Not sure why this is needed. Now I see that else branch has been changed,
but looking closer to it, we can drop it completely, while leaving this
line untouched, correct?

> -	int ret;
> +	struct gpio_desc *desc;
>  
> +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%pfw'\n",
> +		con_id, fwnode);
> +
> +	/* Using device tree? */
>  	if (is_of_node(fwnode)) {
> -		desc = gpiod_get_from_of_node(to_of_node(fwnode),
> -					      propname, index,
> -					      dflags,
> -					      label);
> -		return desc;
> +		dev_dbg(consumer, "using device tree for GPIO lookup\n");
> +		desc = of_find_gpio(to_of_node(fwnode),
> +				    con_id, idx, lookupflags);

At least con_id can be placed on the previous line.

>  	} else if (is_acpi_node(fwnode)) {
> -		desc = acpi_node_get_gpiod(fwnode, propname, index,
> -					   &lflags, &dflags);
> -		if (IS_ERR(desc))
> -			return desc;
> +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
>  	} else {
> -		return ERR_PTR(-EINVAL);
> +		desc = ERR_PTR(-ENOENT);
>  	}
>  
> -	/* Currently only ACPI takes this path */
> +	return desc;
> +}

...

> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> +	unsigned long lookupflags;
> +	int ret;

> +	if (!IS_ERR_OR_NULL(fwnode))

I think this is superfluous check.

Now in the form of this series, you have only a single dev_dbg() that tries to
dereference it. Do we really need to have it there, since every branch has its
own dev_dbg() anyway?

> +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> +					    &flags, &lookupflags);

> +

This blank line can be dropped after addressing above.

> +	if (gpiod_not_found(desc) && platform_lookup_allowed) {
> +		/*
> +		 * Either we are not using DT or ACPI, or their lookup did not
> +		 * return a result. In that case, use platform lookup as a
> +		 * fallback.
> +		 */
> +		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> +		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> +	}
> +
> +	if (IS_ERR(desc)) {
> +		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> +		return desc;
> +	}

...

> +	return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label,
> +				      false);

One line?

...

> +	return gpiod_find_and_request(dev, fwnode, con_id, idx, flags, label,
> +				      true);

One line?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Dmitry Torokhov]

On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote:
> > Ensure that all paths to obtain/look up GPIOD from generic
> > consumer-visible APIs go through the new gpiod_find_and_request()
> > helper, so that we can easily extend it with support for new firmware
> > mechanisms.
> > 
> > The only exception is OF-specific [devm_]gpiod_get_from_of_node() API
> > that is still being used by a couple of drivers and will be removed as
> > soon as patches converting them to use generic fwnode/device APIs are
> > accepted.
> 
> ...
> 
> > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
> > +					      struct device *consumer,
> > +					      const char *con_id,
> > +					      unsigned int idx,
> > +					      enum gpiod_flags *flags,
> > +					      unsigned long *lookupflags)
> >  {
> > -	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> 
> > -	struct gpio_desc *desc = ERR_PTR(-ENODEV);
> 
> Not sure why this is needed. Now I see that else branch has been changed,
> but looking closer to it, we can drop it completely, while leaving this
> line untouched, correct?

Yes. I believe removing an initializer and doing a series of if/else
if/else was discussed and [soft] agreed-on in the previous review cycle,
but I can change it back.

I think we still need to have it return -ENOENT and not -ENODEV/-EINVAL
so that we can fall back to GPIO lookup tables when dealing with an
unsupported node type.

> 
> > -	int ret;
> > +	struct gpio_desc *desc;
> >  
> > +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%pfw'\n",
> > +		con_id, fwnode);
> > +
> > +	/* Using device tree? */
> >  	if (is_of_node(fwnode)) {
> > -		desc = gpiod_get_from_of_node(to_of_node(fwnode),
> > -					      propname, index,
> > -					      dflags,
> > -					      label);
> > -		return desc;
> > +		dev_dbg(consumer, "using device tree for GPIO lookup\n");
> > +		desc = of_find_gpio(to_of_node(fwnode),
> > +				    con_id, idx, lookupflags);
> 
> At least con_id can be placed on the previous line.

OK, I made it all 1 line.

> 
> >  	} else if (is_acpi_node(fwnode)) {
> > -		desc = acpi_node_get_gpiod(fwnode, propname, index,
> > -					   &lflags, &dflags);
> > -		if (IS_ERR(desc))
> > -			return desc;
> > +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> > +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
> >  	} else {
> > -		return ERR_PTR(-EINVAL);
> > +		desc = ERR_PTR(-ENOENT);
> >  	}
> >  
> > -	/* Currently only ACPI takes this path */
> > +	return desc;
> > +}
> 
> ...
> 
> > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > +	unsigned long lookupflags;
> > +	int ret;
> 
> > +	if (!IS_ERR_OR_NULL(fwnode))
> 
> I think this is superfluous check.
> 
> Now in the form of this series, you have only a single dev_dbg() that tries to
> dereference it. Do we really need to have it there, since every branch has its
> own dev_dbg() anyway?

As I mentioned, I like to keep this check to show the reader that we
should only descend into gpiod_find_by_fwnode() if we have a valid
fwnode. It is less about code generation and more about the intent.

I did change the logging to remove extra dev_dbg(). We will lose message
when dealing with unsupported node type, but that should not really
happen in practice.

> 
> > +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> > +					    &flags, &lookupflags);
> 
> > +
> 
> This blank line can be dropped after addressing above.
> 
> > +	if (gpiod_not_found(desc) && platform_lookup_allowed) {
> > +		/*
> > +		 * Either we are not using DT or ACPI, or their lookup did not
> > +		 * return a result. In that case, use platform lookup as a
> > +		 * fallback.
> > +		 */
> > +		dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> > +		desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> > +	}
> > +
> > +	if (IS_ERR(desc)) {
> > +		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> > +		return desc;
> > +	}
> 
> ...
> 
> > +	return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label,
> > +				      false);
> 
> One line?

OK :)

> 
> ...
> 
> > +	return gpiod_find_and_request(dev, fwnode, con_id, idx, flags, label,
> > +				      true);
> 
> One line?

OK.

Thanks,

-- 
Dmitry

Re: [PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote:

...

> > > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
> > > +					      struct device *consumer,
> > > +					      const char *con_id,
> > > +					      unsigned int idx,
> > > +					      enum gpiod_flags *flags,
> > > +					      unsigned long *lookupflags)
> > >  {
> > > -	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> > 
> > > -	struct gpio_desc *desc = ERR_PTR(-ENODEV);
> > 
> > Not sure why this is needed. Now I see that else branch has been changed,
> > but looking closer to it, we can drop it completely, while leaving this
> > line untouched, correct?
> 
> Yes. I believe removing an initializer and doing a series of if/else
> if/else was discussed and [soft] agreed-on in the previous review cycle,
> but I can change it back.
> 
> I think we still need to have it return -ENOENT and not -ENODEV/-EINVAL
> so that we can fall back to GPIO lookup tables when dealing with an
> unsupported node type.

Right, okay, let's go with whatever variant you find better.

...

> > > +	if (!IS_ERR_OR_NULL(fwnode))
> > 
> > I think this is superfluous check.
> > 
> > Now in the form of this series, you have only a single dev_dbg() that tries to
> > dereference it. Do we really need to have it there, since every branch has its
> > own dev_dbg() anyway?
> 
> As I mentioned, I like to keep this check to show the reader that we
> should only descend into gpiod_find_by_fwnode() if we have a valid
> fwnode. It is less about code generation and more about the intent.

Yes, but if fwnode is not found, we have a next check for that. I really don't
think we lose anything by dropping the check and gaining the code generation as
a side effect.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Dmitry Torokhov]

On Thu, Nov 10, 2022 at 03:42:40PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > +static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
> > > > +					      struct device *consumer,
> > > > +					      const char *con_id,
> > > > +					      unsigned int idx,
> > > > +					      enum gpiod_flags *flags,
> > > > +					      unsigned long *lookupflags)
> > > >  {
> > > > -	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
> > > 
> > > > -	struct gpio_desc *desc = ERR_PTR(-ENODEV);
> > > 
> > > Not sure why this is needed. Now I see that else branch has been changed,
> > > but looking closer to it, we can drop it completely, while leaving this
> > > line untouched, correct?
> > 
> > Yes. I believe removing an initializer and doing a series of if/else
> > if/else was discussed and [soft] agreed-on in the previous review cycle,
> > but I can change it back.
> > 
> > I think we still need to have it return -ENOENT and not -ENODEV/-EINVAL
> > so that we can fall back to GPIO lookup tables when dealing with an
> > unsupported node type.
> 
> Right, okay, let's go with whatever variant you find better.
> 
> ...
> 
> > > > +	if (!IS_ERR_OR_NULL(fwnode))
> > > 
> > > I think this is superfluous check.
> > > 
> > > Now in the form of this series, you have only a single dev_dbg() that tries to
> > > dereference it. Do we really need to have it there, since every branch has its
> > > own dev_dbg() anyway?
> > 
> > As I mentioned, I like to keep this check to show the reader that we
> > should only descend into gpiod_find_by_fwnode() if we have a valid
> > fwnode. It is less about code generation and more about the intent.
> 
> Yes, but if fwnode is not found, we have a next check for that.

No, the check you are talking about is for the GPIO not being located.
It does not have anything to do with fwnode validity. You are relying on
intimate knowledge of gpiod_find_by_fwnode() implementation and the fact
that in the current form it will withstand ERR_PTR-encoded or NULL
fwnode.

I want to have the source code so clear in its intent so that I can be
woken up in the middle of the night with a huge hangover and still be
able to tell how it is supposed to behave.

> I really don't
> think we lose anything by dropping the check and gaining the code generation as
> a side effect.

This is cold path, happening only on startup. I am not saying that we
want to make it slow unnecessarily, but a condition branch that might
even get optimized out is not something we should be concerned here.

Thanks.

-- 
Dmitry

Re: [PATCH v2 5/6] gpiolib: consolidate GPIO lookups

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 09:21:59AM -0800, Dmitry Torokhov wrote:
> On Thu, Nov 10, 2022 at 03:42:40PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 09, 2022 at 11:00:29AM -0800, Dmitry Torokhov wrote:
> > > On Wed, Nov 09, 2022 at 01:25:06PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Nov 08, 2022 at 04:26:50PM -0800, Dmitry Torokhov wrote:

...

> > > > > +	if (!IS_ERR_OR_NULL(fwnode))
> > > > 
> > > > I think this is superfluous check.
> > > > 
> > > > Now in the form of this series, you have only a single dev_dbg() that tries to
> > > > dereference it. Do we really need to have it there, since every branch has its
> > > > own dev_dbg() anyway?
> > > 
> > > As I mentioned, I like to keep this check to show the reader that we
> > > should only descend into gpiod_find_by_fwnode() if we have a valid
> > > fwnode. It is less about code generation and more about the intent.
> > 
> > Yes, but if fwnode is not found, we have a next check for that.
> 
> No, the check you are talking about is for the GPIO not being located.
> It does not have anything to do with fwnode validity. You are relying on
> intimate knowledge of gpiod_find_by_fwnode() implementation and the fact
> that in the current form it will withstand ERR_PTR-encoded or NULL
> fwnode.
> 
> I want to have the source code so clear in its intent so that I can be
> woken up in the middle of the night with a huge hangover and still be
> able to tell how it is supposed to behave.

As you said let's leave it to Bart and Linus.

> > I really don't
> > think we lose anything by dropping the check and gaining the code generation as
> > a side effect.
> 
> This is cold path, happening only on startup. I am not saying that we
> want to make it slow unnecessarily, but a condition branch that might
> even get optimized out is not something we should be concerned here.

Agree, that's why I called it "side effect".

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 6/6] gpiolib: add support for software nodes

[Dmitry Torokhov]

Now that static device properties understand notion of child nodes and
references, let's teach gpiolib to handle them:

- GPIOs are represented as a references to software nodes representing
  gpiochip
- references must have 2 arguments - GPIO number within the chip and
  GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc)
- a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
- name of the software node representing gpiochip must match label of
  the gpiochip, as we use it to locate gpiochip structure at runtime

The following illustrates use of software nodes to describe a "System"
button that is currently specified via use of gpio_keys_platform_data
in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
Documentation/devicetree/bindings/input/gpio-keys.yaml.

static const struct software_node mxt1_gpiochip2_node = {
	.name = "alchemy-gpio2",
};

static const struct property_entry mtx1_gpio_button_props[] = {
	PROPERTY_ENTRY_U32("linux,code", BTN_0),
	PROPERTY_ENTRY_STRING("label", "System button"),
	PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
	{ }
};

Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:

static const struct software_node tegra_gpiochip_node = {
	.name = "tegra-gpio",
};

static struct property_entry wifi_rfkill_prop[] __initdata = {
	PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
	PROPERTY_ENTRY_STRING("type", "wlan"),
	PROPERTY_ENTRY_GPIO("reset-gpios",
			    &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
	PROPERTY_ENTRY_GPIO("shutdown-gpios",
			    &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
	{ },
};

static struct platform_device wifi_rfkill_device = {
	.name	= "rfkill_gpio",
	.id	= -1,
};

...

	software_node_register(&tegra_gpiochip_node);
	device_create_managed_software_node(&wifi_rfkill_device.dev,
					    wifi_rfkill_prop, NULL);

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpiolib-swnode.c | 128 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib-swnode.h |  14 +++++
 drivers/gpio/gpiolib.c        |  12 +++-
 include/linux/gpio/property.h |  11 ++++
 5 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 8629e9eaf79e..010587025fc8 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
 obj-$(CONFIG_GPIO_CDEV)		+= gpiolib-cdev.o
 obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
+obj-$(CONFIG_GPIOLIB)		+= gpiolib-swnode.o
 
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_REGMAP)	+= gpio-regmap.o
diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
new file mode 100644
index 000000000000..4cf41234bfc6
--- /dev/null
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Software Node helpers for the GPIO API
+ *
+ * Copyright 2022 Google LLC
+ */
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/property.h>
+#include <linux/string.h>
+
+#include "gpiolib.h"
+#include "gpiolib-swnode.h"
+
+static void swnode_format_propname(const char *con_id, char *propname,
+				   size_t max_size)
+{
+	/*
+	 * Note we do not need to try both -gpios and -gpio suffixes,
+	 * as, unlike OF and ACPI, we can fix software nodes to conform
+	 * to the proper binding.
+	 */
+	if (con_id)
+		snprintf(propname, max_size, "%s-gpios", con_id);
+	else
+		strscpy(propname, "gpios", max_size);
+}
+
+static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
+{
+	return !strcmp(chip->label, data);
+}
+
+static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
+{
+	const struct software_node *chip_node;
+	struct gpio_chip *chip;
+
+	chip_node = to_software_node(fwnode);
+	if (!chip_node || !chip_node->name)
+		return ERR_PTR(-EINVAL);
+
+	chip = gpiochip_find((void *)chip_node->name,
+			     swnode_gpiochip_match_name);
+	if (!chip)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	return chip;
+}
+
+struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
+				   const char *con_id, unsigned int idx,
+				   unsigned long *flags)
+{
+	const struct software_node *swnode;
+	struct fwnode_reference_args args;
+	struct gpio_chip *chip;
+	struct gpio_desc *desc;
+	char propname[32]; /* 32 is max size of property name */
+	int error;
+
+	swnode = to_software_node(fwnode);
+	if (!swnode)
+		return ERR_PTR(-EINVAL);
+
+	swnode_format_propname(con_id, propname, sizeof(propname));
+
+	/*
+	 * We expect all swnode-described GPIOs have GPIO number and
+	 * polarity arguments, hence nargs is set to 2.
+	 */
+	error = fwnode_property_get_reference_args(fwnode, propname, NULL,
+						   2, idx, &args);
+	if (error) {
+		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
+			__func__, propname, fwnode, idx);
+		return ERR_PTR(error);
+	}
+
+	chip = swnode_get_chip(args.fwnode);
+	fwnode_handle_put(args.fwnode);
+	if (IS_ERR(chip))
+		return ERR_CAST(chip);
+
+	desc = gpiochip_get_desc(chip, args.args[0]);
+	*flags = args.args[1]; /* We expect native GPIO flags */
+
+	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
+		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
+
+	return desc;
+}
+
+/**
+ * swnode_gpio_count - count the GPIOs associated with a device / function
+ * @fwnode:	firmware node of the GPIO consumer, can be %NULL for
+ *		system-global GPIOs
+ * @con_id:	function within the GPIO consumer
+ *
+ * Return:
+ * The number of GPIOs associated with a device / function or %-ENOENT,
+ * if no GPIO has been assigned to the requested function.
+ */
+int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id)
+{
+	struct fwnode_reference_args args;
+	char propname[32];
+	int count;
+
+	swnode_format_propname(con_id, propname, sizeof(propname));
+
+	/*
+	 * This is not very efficient, but GPIO lists usually have only
+	 * 1 or 2 entries.
+	 */
+	count = 0;
+	while (fwnode_property_get_reference_args(fwnode, propname, NULL,
+						  0, count, &args) == 0) {
+		fwnode_handle_put(args.fwnode);
+		count++;
+	}
+
+	return count ?: -ENOENT;
+}
diff --git a/drivers/gpio/gpiolib-swnode.h b/drivers/gpio/gpiolib-swnode.h
new file mode 100644
index 000000000000..4e07be56ed9f
--- /dev/null
+++ b/drivers/gpio/gpiolib-swnode.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef GPIOLIB_SWNODE_H
+#define GPIOLIB_SWNODE_H
+
+struct fwnode_handle;
+struct gpio_desc;
+
+struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
+				   const char *con_id, unsigned int idx,
+				   unsigned long *flags);
+int swnode_gpio_count(struct fwnode_handle *fwnode, const char *con_id);
+
+#endif /* GPIOLIB_SWNODE_H */
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c33ffdd47667..bf5c072206da 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -26,6 +26,7 @@
 #include "gpiolib.h"
 #include "gpiolib-of.h"
 #include "gpiolib-acpi.h"
+#include "gpiolib-swnode.h"
 #include "gpiolib-cdev.h"
 #include "gpiolib-sysfs.h"
 
@@ -3821,6 +3822,9 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 	} else if (is_acpi_node(fwnode)) {
 		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
 		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
+	} else if (is_software_node(fwnode)) {
+		dev_dbg(consumer, "using software node for GPIO lookup\n");
+		desc = swnode_find_gpio(fwnode, con_id, idx, lookupflags);
 	} else {
 		desc = ERR_PTR(-ENOENT);
 	}
@@ -3934,13 +3938,17 @@ EXPORT_SYMBOL_GPL(fwnode_gpiod_get_index);
  */
 int gpiod_count(struct device *dev, const char *con_id)
 {
-	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
-	int count = -ENOENT;
+	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
+	int count;
 
 	if (is_of_node(fwnode))
 		count = of_gpio_get_count(dev, con_id);
 	else if (is_acpi_node(fwnode))
 		count = acpi_gpio_count(dev, con_id);
+	else if (is_software_node(fwnode))
+		count = swnode_gpio_count(fwnode, con_id);
+	else
+		count = -ENOENT;
 
 	if (count < 0)
 		count = platform_gpio_count(dev, con_id);
diff --git a/include/linux/gpio/property.h b/include/linux/gpio/property.h
new file mode 100644
index 000000000000..f76a46469f66
--- /dev/null
+++ b/include/linux/gpio/property.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0+
+#ifndef __LINUX_GPIO_PROPERTY_H
+#define __LINUX_GPIO_PROPERTY_H
+
+#include <dt-bindings/gpio/gpio.h>
+#include <linux/property.h>
+
+#define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
+	PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)
+
+#endif /* __LINUX_GPIO_PROPERTY_H */

-- 
b4 0.11.0-dev-28747

Re: [PATCH v2 6/6] gpiolib: add support for software nodes

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 04:26:51PM -0800, Dmitry Torokhov wrote:
> Now that static device properties understand notion of child nodes and
> references, let's teach gpiolib to handle them:
> 
> - GPIOs are represented as a references to software nodes representing
>   gpiochip
> - references must have 2 arguments - GPIO number within the chip and
>   GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc)
> - a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
> - name of the software node representing gpiochip must match label of
>   the gpiochip, as we use it to locate gpiochip structure at runtime
> 
> The following illustrates use of software nodes to describe a "System"
> button that is currently specified via use of gpio_keys_platform_data
> in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
> Documentation/devicetree/bindings/input/gpio-keys.yaml.
> 
> static const struct software_node mxt1_gpiochip2_node = {
> 	.name = "alchemy-gpio2",
> };
> 
> static const struct property_entry mtx1_gpio_button_props[] = {
> 	PROPERTY_ENTRY_U32("linux,code", BTN_0),
> 	PROPERTY_ENTRY_STRING("label", "System button"),
> 	PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
> 	{ }
> };
> 
> Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:
> 
> static const struct software_node tegra_gpiochip_node = {
> 	.name = "tegra-gpio",
> };
> 
> static struct property_entry wifi_rfkill_prop[] __initdata = {
> 	PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
> 	PROPERTY_ENTRY_STRING("type", "wlan"),
> 	PROPERTY_ENTRY_GPIO("reset-gpios",
> 			    &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
> 	PROPERTY_ENTRY_GPIO("shutdown-gpios",
> 			    &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
> 	{ },
> };
> 
> static struct platform_device wifi_rfkill_device = {
> 	.name	= "rfkill_gpio",
> 	.id	= -1,
> };
> 
> ...
> 
> 	software_node_register(&tegra_gpiochip_node);
> 	device_create_managed_software_node(&wifi_rfkill_device.dev,
> 					    wifi_rfkill_prop, NULL);

...

> +static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> +{
> +	const struct software_node *chip_node;
> +	struct gpio_chip *chip;
> +
> +	chip_node = to_software_node(fwnode);
> +	if (!chip_node || !chip_node->name)
> +		return ERR_PTR(-EINVAL);

> +	chip = gpiochip_find((void *)chip_node->name,
> +			     swnode_gpiochip_match_name);

One line?

> +	if (!chip)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	return chip;

As below you can use Elvis here as well, up to you.

	return chip ?: ERR_PTR(...);

> +}

...

> +	desc = gpiochip_get_desc(chip, args.args[0]);
> +	*flags = args.args[1]; /* We expect native GPIO flags */
> +
> +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> +		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));

%pe ?

> +	return desc;

...

> +	while (fwnode_property_get_reference_args(fwnode, propname, NULL,
> +						  0, count, &args) == 0) {

I would move 0 to the previous line.

> +		fwnode_handle_put(args.fwnode);
> +		count++;
> +	}

...

>  int gpiod_count(struct device *dev, const char *con_id)
>  {
> -	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> -	int count = -ENOENT;
> +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;

Why dropping const?

> +	int count;

Why this change is needed?

>  	if (is_of_node(fwnode))
>  		count = of_gpio_get_count(dev, con_id);
>  	else if (is_acpi_node(fwnode))
>  		count = acpi_gpio_count(dev, con_id);
> +	else if (is_software_node(fwnode))
> +		count = swnode_gpio_count(fwnode, con_id);
> +	else
> +		count = -ENOENT;

...

> +#include <dt-bindings/gpio/gpio.h>

Not sure why we have this here.

> +#include <linux/property.h>
> +
> +#define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
> +	PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] gpiolib: add support for software nodes

[Dmitry Torokhov]

On Wed, Nov 09, 2022 at 01:20:46PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 08, 2022 at 04:26:51PM -0800, Dmitry Torokhov wrote:
> > Now that static device properties understand notion of child nodes and
> > references, let's teach gpiolib to handle them:
> > 
> > - GPIOs are represented as a references to software nodes representing
> >   gpiochip
> > - references must have 2 arguments - GPIO number within the chip and
> >   GPIO flags (GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, etc)
> > - a new PROPERTY_ENTRY_GPIO() macro is supplied to ensure the above
> > - name of the software node representing gpiochip must match label of
> >   the gpiochip, as we use it to locate gpiochip structure at runtime
> > 
> > The following illustrates use of software nodes to describe a "System"
> > button that is currently specified via use of gpio_keys_platform_data
> > in arch/mips/alchemy/board-mtx1.c. It follows bindings specified in
> > Documentation/devicetree/bindings/input/gpio-keys.yaml.
> > 
> > static const struct software_node mxt1_gpiochip2_node = {
> > 	.name = "alchemy-gpio2",
> > };
> > 
> > static const struct property_entry mtx1_gpio_button_props[] = {
> > 	PROPERTY_ENTRY_U32("linux,code", BTN_0),
> > 	PROPERTY_ENTRY_STRING("label", "System button"),
> > 	PROPERTY_ENTRY_GPIO("gpios", &mxt1_gpiochip2_node, 7, GPIO_ACTIVE_LOW),
> > 	{ }
> > };
> > 
> > Similarly, arch/arm/mach-tegra/board-paz00.c can be converted to:
> > 
> > static const struct software_node tegra_gpiochip_node = {
> > 	.name = "tegra-gpio",
> > };
> > 
> > static struct property_entry wifi_rfkill_prop[] __initdata = {
> > 	PROPERTY_ENTRY_STRING("name", "wifi_rfkill"),
> > 	PROPERTY_ENTRY_STRING("type", "wlan"),
> > 	PROPERTY_ENTRY_GPIO("reset-gpios",
> > 			    &tegra_gpiochip_node, 25, GPIO_ACTIVE_HIGH);
> > 	PROPERTY_ENTRY_GPIO("shutdown-gpios",
> > 			    &tegra_gpiochip_node, 85, GPIO_ACTIVE_HIGH);
> > 	{ },
> > };
> > 
> > static struct platform_device wifi_rfkill_device = {
> > 	.name	= "rfkill_gpio",
> > 	.id	= -1,
> > };
> > 
> > ...
> > 
> > 	software_node_register(&tegra_gpiochip_node);
> > 	device_create_managed_software_node(&wifi_rfkill_device.dev,
> > 					    wifi_rfkill_prop, NULL);
> 
> ...
> 
> > +static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
> > +{
> > +	const struct software_node *chip_node;
> > +	struct gpio_chip *chip;
> > +
> > +	chip_node = to_software_node(fwnode);
> > +	if (!chip_node || !chip_node->name)
> > +		return ERR_PTR(-EINVAL);
> 
> > +	chip = gpiochip_find((void *)chip_node->name,
> > +			     swnode_gpiochip_match_name);
> 
> One line?

OK.

> 
> > +	if (!chip)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	return chip;
> 
> As below you can use Elvis here as well, up to you.
> 
> 	return chip ?: ERR_PTR(...);

OK.

> 
> > +}
> 
> ...
> 
> > +	desc = gpiochip_get_desc(chip, args.args[0]);
> > +	*flags = args.args[1]; /* We expect native GPIO flags */
> > +
> > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > +		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> 
> %pe ?

"/* %pe with a non-ERR_PTR gets treated as plain %p */".

I do not think users are interested in the address on success.


> 
> > +	return desc;
> 
> ...
> 
> > +	while (fwnode_property_get_reference_args(fwnode, propname, NULL,
> > +						  0, count, &args) == 0) {
> 
> I would move 0 to the previous line.

OK.

> 
> > +		fwnode_handle_put(args.fwnode);
> > +		count++;
> > +	}
> 
> ...
> 
> >  int gpiod_count(struct device *dev, const char *con_id)
> >  {
> > -	const struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> > -	int count = -ENOENT;
> > +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> 
> Why dropping const?

Restored.

> 
> > +	int count;
> 
> Why this change is needed?

Restored.

> 
> >  	if (is_of_node(fwnode))
> >  		count = of_gpio_get_count(dev, con_id);
> >  	else if (is_acpi_node(fwnode))
> >  		count = acpi_gpio_count(dev, con_id);
> > +	else if (is_software_node(fwnode))
> > +		count = swnode_gpio_count(fwnode, con_id);
> > +	else
> > +		count = -ENOENT;
> 
> ...
> 
> > +#include <dt-bindings/gpio/gpio.h>
> 
> Not sure why we have this here.

For convenience - so that users have access to GPIO_ACTIVE_HIGH/
GPIO_ACTIVE_LOW and other flags.

> 
> > +#include <linux/property.h>
> > +
> > +#define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
> > +	PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2 6/6] gpiolib: add support for software nodes

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 11:08:07AM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 09, 2022 at 01:20:46PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 08, 2022 at 04:26:51PM -0800, Dmitry Torokhov wrote:

...

> > > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > > +		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> > 
> > %pe ?
> 
> "/* %pe with a non-ERR_PTR gets treated as plain %p */".
> 
> I do not think users are interested in the address on success.

Hmm... Perhaps we can teach %pe to behave differently with, e.g. %pe0,
modification. But this is another story. So, let's go with your variant.

...

> > > +#include <dt-bindings/gpio/gpio.h>
> > 
> > Not sure why we have this here.
> 
> For convenience - so that users have access to GPIO_ACTIVE_HIGH/
> GPIO_ACTIVE_LOW and other flags.

Okay, would we make this as a guarantee then?

In such case a comment before this inclusion should be added to explain why
we do that without any actual user to be present in the header file.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] gpiolib: add support for software nodes

[Dmitry Torokhov]

On Thu, Nov 10, 2022 at 03:48:02PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 09, 2022 at 11:08:07AM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 09, 2022 at 01:20:46PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 08, 2022 at 04:26:51PM -0800, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > > > +		 __func__, propname, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> > > 
> > > %pe ?
> > 
> > "/* %pe with a non-ERR_PTR gets treated as plain %p */".
> > 
> > I do not think users are interested in the address on success.
> 
> Hmm... Perhaps we can teach %pe to behave differently with, e.g. %pe0,
> modification.

Yes, and maybe we could even have %e for normal errors ;)

> But this is another story. So, let's go with your variant.
> 
> ...
> 
> > > > +#include <dt-bindings/gpio/gpio.h>
> > > 
> > > Not sure why we have this here.
> > 
> > For convenience - so that users have access to GPIO_ACTIVE_HIGH/
> > GPIO_ACTIVE_LOW and other flags.
> 
> Okay, would we make this as a guarantee then?
> 
> In such case a comment before this inclusion should be added to explain why
> we do that without any actual user to be present in the header file.

Just to close the loop - I added a comment reflecting this in v3.

Thanks.

-- 
Dmitry

Re: [PATCH v2 0/6] Add support for software nodes to gpiolib

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 04:26:45PM -0800, Dmitry Torokhov wrote:
> This series attempts to add support for software nodes to gpiolib, using
> software node references. This allows us to convert more drivers to the
> generic device properties and drop support for custom platform data.
> 
> To describe a GPIO via software nodes we can create the following data
> items:
> 
> /* Node representing the GPIO controller/GPIO bank */
> static const struct software_node gpio_bank_b_node = {
>         .name = "B",
> };
> 
> /*
>  * Properties that will be assigned to a software node assigned to
>  * the device that used platform data.
>  */
> static const struct property_entry simone_key_enter_props[] = {
>         PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
>         PROPERTY_ENTRY_STRING("label", "enter"),
>         PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
>         { }
> };
> 
> The code in gpiolib handling software nodes uses the name in the
> software node representing GPIO controller to locate the actual instance
> of GPIO controller.

Thank for an update!

I have almost nothing serious except two nit-picks I think we can address:
- dropping const qualifier for no (?) reason
- having a superfluous check and extra dev_dbg()

If you are are going to address them, feel free to add my Rb tag to
the patches 5 & 6.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/6] Add support for software nodes to gpiolib

[Dmitry Torokhov]

On Wed, Nov 09, 2022 at 01:29:41PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 08, 2022 at 04:26:45PM -0800, Dmitry Torokhov wrote:
> > This series attempts to add support for software nodes to gpiolib, using
> > software node references. This allows us to convert more drivers to the
> > generic device properties and drop support for custom platform data.
> > 
> > To describe a GPIO via software nodes we can create the following data
> > items:
> > 
> > /* Node representing the GPIO controller/GPIO bank */
> > static const struct software_node gpio_bank_b_node = {
> >         .name = "B",
> > };
> > 
> > /*
> >  * Properties that will be assigned to a software node assigned to
> >  * the device that used platform data.
> >  */
> > static const struct property_entry simone_key_enter_props[] = {
> >         PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> >         PROPERTY_ENTRY_STRING("label", "enter"),
> >         PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> >         { }
> > };
> > 
> > The code in gpiolib handling software nodes uses the name in the
> > software node representing GPIO controller to locate the actual instance
> > of GPIO controller.
> 
> Thank for an update!
> 
> I have almost nothing serious except two nit-picks I think we can address:
> - dropping const qualifier for no (?) reason
> - having a superfluous check and extra dev_dbg()
> 
> If you are are going to address them, feel free to add my Rb tag to
> the patches 5 & 6.

Thank you for the reviews. I addressed most of your comments, but
because I still left the check you were concerned about in I'd like an
explicit reviewed-by on the v3 if you OK giving it.

Thank you.

-- 
Dmitry

Re: [PATCH v2 0/6] Add support for software nodes to gpiolib

[Andy Shevchenko]

On Wed, Nov 09, 2022 at 11:32:31AM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 09, 2022 at 01:29:41PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 08, 2022 at 04:26:45PM -0800, Dmitry Torokhov wrote:

...

> > Thank for an update!
> > 
> > I have almost nothing serious except two nit-picks I think we can address:
> > - dropping const qualifier for no (?) reason
> > - having a superfluous check and extra dev_dbg()
> > 
> > If you are are going to address them, feel free to add my Rb tag to
> > the patches 5 & 6.
> 
> Thank you for the reviews. I addressed most of your comments, but
> because I still left the check you were concerned about in I'd like an
> explicit reviewed-by on the v3 if you OK giving it.

I gave for patch 6 and for patch 5 I still think we can get rid of the check.

-- 
With Best Regards,
Andy Shevchenko