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

[PATCH 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 devicei 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.

Note that kbuild robot is likely to complain about this patchset because
it depends on patches removing [devm_]gpiod_get_from_of_node() and
devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending.
I pushed them to

git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib

for your reference.

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

---
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     |  52 +--------
 drivers/gpio/gpiolib-of.h     |  16 +--
 drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++
 drivers/gpio/gpiolib-swnode.h |  13 +++
 drivers/gpio/gpiolib.c        | 239 ++++++++++++++++++++----------------------
 8 files changed, 316 insertions(+), 297 deletions(-)
---
base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d
change-id: 20221031-gpiolib-swnode-948203f49b23

-- 
Dmitry

[PATCH 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.

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 012e66344b56..607bf4358ff7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -605,7 +605,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 */
@@ -623,8 +623,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;
@@ -632,7 +631,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 22d314229bbd..c44be0f285f3 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -17,7 +17,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);
@@ -31,7 +31,7 @@ struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
 					 enum gpiod_flags dflags,
 					 const char *label);
 #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 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.

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 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().

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 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.

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 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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib-acpi.c |  39 ---------
 drivers/gpio/gpiolib-acpi.h |  10 ---
 drivers/gpio/gpiolib-of.c   |  45 ----------
 drivers/gpio/gpiolib-of.h   |  12 ---
 drivers/gpio/gpiolib.c      | 206 +++++++++++++++++---------------------------
 5 files changed, 80 insertions(+), 232 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-of.c b/drivers/gpio/gpiolib-of.c
index 607bf4358ff7..7508628b9f6e 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -390,51 +390,6 @@ static unsigned long of_convert_gpio_flags(enum of_gpio_flags flags)
 	return lflags;
 }
 
-/**
- * gpiod_get_from_of_node() - obtain a GPIO from an OF node
- * @node:	handle of the OF node
- * @propname:	name of the DT 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
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label)
-{
-	unsigned long lflags;
-	struct gpio_desc *desc;
-	enum of_gpio_flags of_flags;
-	int ret;
-
-	desc = of_get_named_gpiod_flags(node, propname, index, &of_flags);
-	if (!desc || IS_ERR(desc))
-		return desc;
-
-	ret = gpiod_request(desc, label);
-	if (ret == -EBUSY && (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
-		return desc;
-	if (ret)
-		return ERR_PTR(ret);
-
-	lflags = of_convert_gpio_flags(of_flags);
-
-	ret = gpiod_configure_flags(desc, propname, lflags, dflags);
-	if (ret < 0) {
-		gpiod_put(desc);
-		return ERR_PTR(ret);
-	}
-
-	return desc;
-}
-
 static struct gpio_desc *of_find_gpio_rename(struct device_node *np,
 					     const char *con_id,
 					     unsigned int idx,
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index c44be0f285f3..16c0848c9a25 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -26,10 +26,6 @@ void of_gpiochip_remove(struct gpio_chip *gc);
 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);
-struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label);
 #else
 static inline struct gpio_desc *of_find_gpio(struct device_node *np,
 					     const char *con_id,
@@ -52,14 +48,6 @@ static inline void of_gpio_dev_init(struct gpio_chip *gc,
 				    struct gpio_device *gdev)
 {
 }
-static inline
-struct gpio_desc *gpiod_get_from_of_node(const struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label)
-{
-	return ERR_PTR(-ENOSYS);
-}
 #endif /* CONFIG_OF_GPIO */
 
 extern struct notifier_block gpio_of_notifier;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f0a7a59ac630..79aaba693c4f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3801,58 +3801,87 @@ 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 = ERR_PTR(-ENOENT);
 
+	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
+		con_id, fwnode_get_name(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;
-	} else {
-		return ERR_PTR(-EINVAL);
+		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
+		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
 	}
 
-	/* 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 (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 +3914,14 @@ 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 +4075,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

[PATCH 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).
- name of the software node representing gpiochip must match label of
  the gpiochip, as we use it to locate gpiochip structure at runtime.

const struct software_node gpio_bank_b_node = {
	.name = "B",
};

const struct property_entry simone_key_enter_props[] __initconst = {
	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
	PROPERTY_ENTRY_STRING("label", "enter"),
	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
	{ }
};

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpiolib-swnode.c | 106 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib-swnode.h |  13 ++++++
 drivers/gpio/gpiolib.c        |  35 +++++++++++++-
 4 files changed, 153 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..d005ce0b986d
--- /dev/null
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -0,0 +1,106 @@
+// 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 "gpiolib.h"
+#include "gpiolib-swnode.h"
+
+static int swnode_gpiochip_match_name(struct gpio_chip *chip, void *data)
+{
+	return !strcmp(chip->label, data);
+}
+
+struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
+				   const char *con_id, unsigned int idx,
+				   unsigned long *flags)
+{
+	const struct software_node *chip_node;
+	const struct software_node *swnode;
+	struct fwnode_reference_args args;
+	struct gpio_chip *chip;
+	struct gpio_desc *desc;
+	char prop_name[32]; /* 32 is max size of property name */
+	int error;
+
+	swnode = to_software_node(fwnode);
+	if (!swnode)
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * 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(prop_name, sizeof(prop_name), "%s-gpios", con_id);
+	else
+		strscpy(prop_name, "gpios", sizeof(prop_name));
+
+	/*
+	 * 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, prop_name, NULL,
+						   2, idx, &args);
+	if (error) {
+		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
+			__func__, prop_name, fwnode, idx);
+		return ERR_PTR(error);
+	}
+
+	chip_node = to_software_node(args.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);
+
+	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__, prop_name, 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 prop_name[32];
+	int count;
+
+	if (con_id)
+		snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id);
+	else
+		strscpy(prop_name, "gpios", sizeof(prop_name));
+
+	/*
+	 * This is not very efficient, but GPIO lists usually have only
+	 * 1 or 2 entries.
+	 */
+	count = 0;
+	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
+						  0, count, &args) == 0)
+		count++;
+
+	return count ? count : -ENOENT;
+}
diff --git a/drivers/gpio/gpiolib-swnode.h b/drivers/gpio/gpiolib-swnode.h
new file mode 100644
index 000000000000..afd51c9b6e34
--- /dev/null
+++ b/drivers/gpio/gpiolib-swnode.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef GPIOLIB_SWNODE_H
+#define GPIOLIB_SWNODE_H
+
+struct fwnode_handle;
+
+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 79aaba693c4f..b9976485587d 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"
 
@@ -3813,6 +3814,19 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
 		con_id, fwnode_get_name(fwnode));
 
+	/*
+	 * First look up GPIO in the secondary software node in case
+	 * it was used to store updated properties.
+	 */
+	if (is_software_node(fwnode->secondary)) {
+		dev_dbg(consumer,
+			"using secondary software node for GPIO lookup\n");
+		desc = swnode_find_gpio(fwnode->secondary,
+					con_id, idx, lookupflags);
+		if (!gpiod_not_found(desc))
+			return desc;
+	}
+
 	/* Using device tree? */
 	if (is_of_node(fwnode)) {
 		dev_dbg(consumer, "using device tree for GPIO lookup\n");
@@ -3821,6 +3835,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);
 	}
 
 	return desc;
@@ -3933,13 +3950,27 @@ 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;
+
+	/*
+	 * First look up GPIO in the secondary software node in case
+	 * it was used to store updated properties.
+	 */
+	if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) {
+		count = swnode_gpio_count(fwnode->secondary, con_id);
+		if (count > 0)
+			return 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);

-- 
b4 0.11.0-dev-28747

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

[Bartosz Golaszewski]

On Fri, Nov 4, 2022 at 7:10 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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 devicei 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.
>
> Note that kbuild robot is likely to complain about this patchset because
> it depends on patches removing [devm_]gpiod_get_from_of_node() and
> devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending.
> I pushed them to
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib
>
> for your reference.
>
> 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
>
> ---
> 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     |  52 +--------
>  drivers/gpio/gpiolib-of.h     |  16 +--
>  drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++
>  drivers/gpio/gpiolib-swnode.h |  13 +++
>  drivers/gpio/gpiolib.c        | 239 ++++++++++++++++++++----------------------
>  8 files changed, 316 insertions(+), 297 deletions(-)
> ---
> base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d
> change-id: 20221031-gpiolib-swnode-948203f49b23
>
> --
> Dmitry
>

This is great work. I'll wait for Andy to Ack the ACPI patches and
let's get it in.

Bartosz

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

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 11:10:15PM -0700, 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.

...

> +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)
>  {

> +	struct gpio_desc *desc = ERR_PTR(-ENOENT);

No need, just return directly.

> +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
> +		con_id, fwnode_get_name(fwnode));

%pfwP ?

> +
> +	/* Using device tree? */
>  	if (is_of_node(fwnode)) {
> +		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)) {

With direct return, no need for 'else' here.

> +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
>  	}
>  
> +	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);

We can get rid of the assignment, see below.


> +	unsigned long lookupflags;
> +	int ret;

> +	if (fwnode)

Do we need this check?

Debug message above (when %pfw is used) would be even useful when
fwnode == NULL.

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

> +

The blank line can be removed after above comments being addressed.

> +	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) {
> +		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);

Cam be one line.

> +		return desc;
> +	}
>  
> +	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);
>  	}

...

>  struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
> +					 const char *con_id,
> +					 int index,
>  					 enum gpiod_flags flags,
>  					 const char *label)
>  {
>  

Unnecessary blank line?

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

Can be one line.

>  }

...


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

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 11:10:10PM -0700, 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 devicei 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.
> 
> Note that kbuild robot is likely to complain about this patchset because
> it depends on patches removing [devm_]gpiod_get_from_of_node() and
> devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending.
> I pushed them to
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib
> 
> for your reference.

I agree with Bart, this is nice work!
So, for the patches 1-4:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 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
> 
> ---
> 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     |  52 +--------
>  drivers/gpio/gpiolib-of.h     |  16 +--
>  drivers/gpio/gpiolib-swnode.c | 106 +++++++++++++++++++
>  drivers/gpio/gpiolib-swnode.h |  13 +++
>  drivers/gpio/gpiolib.c        | 239 ++++++++++++++++++++----------------------
>  8 files changed, 316 insertions(+), 297 deletions(-)
> ---
> base-commit: dc04f5ab1b1114aa19b9026f816fc01ca9c9941d
> change-id: 20221031-gpiolib-swnode-948203f49b23
> 
> -- 
> Dmitry
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 11:10:16PM -0700, 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).
> - name of the software node representing gpiochip must match label of
>   the gpiochip, as we use it to locate gpiochip structure at runtime.
> 
> const struct software_node gpio_bank_b_node = {
> 	.name = "B",
> };
> 
> const struct property_entry simone_key_enter_props[] __initconst = {
> 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),

> 	PROPERTY_ENTRY_STRING("label", "enter"),
> 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),

Okay, can we have an example for something like reset-gpios? Because from
the above I can't easily get what label is and how in the `gpioinfo` tool
the requested line will look like.

> 	{ }
> };

...

> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>

It seems you are using much more that these ones.

...

> +	char prop_name[32]; /* 32 is max size of property name */

Why is it not defined then?

...

> +	/*
> +	 * 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.
> +	 */

True, but for the sake of consistency between providers perhaps it makes sense
to check that as well. Dunno, up to Linus and Bart to decide.

...

> +	/*
> +	 * We expect all swnode-described GPIOs have GPIO number and
> +	 * polarity arguments, hence nargs is set to 2.
> +	 */

Maybe instead you can provide a custom macro wrapper that will check the number
of arguments at compile time?

...

> +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> +			__func__, prop_name, fwnode, idx);

__func__ is not needed. Dynamic Debug can automatically add it.
Since you have an fwnode, use that as a marker.

...

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

One line?

...

> +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> +		 __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc));

Same as above.

...

> +	char prop_name[32];

> +	if (con_id)
> +		snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id);
> +	else
> +		strscpy(prop_name, "gpios", sizeof(prop_name));

I saw this code, please deduplicate.

...

> +	/*
> +	 * This is not very efficient, but GPIO lists usually have only
> +	 * 1 or 2 entries.
> +	 */
> +	count = 0;
> +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> +						  0, count, &args) == 0)

I would put it into for loop (and looking into property.h I think propname
is fine variable name):

	for (count = 0; ; count++) {
		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
			break;
	}

Btw, what about reference counting? Do we need to care about it?

> +	return count ? count : -ENOENT;

Elvis would work as well.

	return count ?: -ENOENT;


...

> +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);

...

> +	/*
> +	 * First look up GPIO in the secondary software node in case
> +	 * it was used to store updated properties.

Why this is done first? We don't try secondary before we have checked primary.

> +	 */

> +	if (is_software_node(fwnode->secondary)) {

With the previous comments it would become

	if (fwnode && is_...)

> +		dev_dbg(consumer,
> +			"using secondary software node for GPIO lookup\n");
> +		desc = swnode_find_gpio(fwnode->secondary,
> +					con_id, idx, lookupflags);
> +		if (!gpiod_not_found(desc))
> +			return desc;
> +	}

...

>  int gpiod_count(struct device *dev, const char *con_id)
>  {
> +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> +	int count;
> +
> +	/*
> +	 * First look up GPIO in the secondary software node in case
> +	 * it was used to store updated properties.
> +	 */

Same question as above.

> +	if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) {
> +		count = swnode_gpio_count(fwnode->secondary, con_id);
> +		if (count > 0)
> +			return 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);

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

Hi Andy,

On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 03, 2022 at 11:10:15PM -0700, 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.
> 
> ...
> 
> > +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)
> >  {
> 
> > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> 
> No need, just return directly.
> 
> > +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
> > +		con_id, fwnode_get_name(fwnode));
> 
> %pfwP ?

OK. Although, I think I like %pfw (without 'P') better as it gives
results like:

	/soc/i2c@11007000/edp-bridge@8

or

	\_SB.PCI0.I2C1.D010

which should help identifying the exact node.

> 
> > +
> > +	/* Using device tree? */
> >  	if (is_of_node(fwnode)) {
> > +		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)) {
> 
> With direct return, no need for 'else' here.

When we have several branches of equal weight I prefer not to have
early/inline returns, but I can add:

	} else {
		desc = ERR_PTR(-ENOENT);
	}

at the end, what do you think?

> 
> > +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> > +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
> >  	}
> >  
> > +	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);
> 
> We can get rid of the assignment, see below.
> 
> 
> > +	unsigned long lookupflags;
> > +	int ret;
> 
> > +	if (fwnode)
> 
> Do we need this check?

Yes, I would prefer to have it as it clearly informs the reader that we
are only doing lookup by node if we actually have a node.

gpiod_find_and_request() expects that it gets a valid node and in the
followup change it will be dereferencing fwnode without checking for
NULL-ness.

> 
> Debug message above (when %pfw is used) would be even useful when
> fwnode == NULL.
> 
> > +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> > +					    &flags, &lookupflags);
> 
> > +
> 
> The blank line can be removed after above comments being addressed.
> 
> > +	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) {
> > +		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);
> 
> Cam be one line.

I still have not embraced the new 100 columns limit. Linus, Bart, are
you OK with moving to 100 or do you want to stay with 80 for a while?

> 
> > +		return desc;
> > +	}
> >  
> > +	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);
> >  	}
> 
> ...
> 
> >  struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
> > +					 const char *con_id,
> > +					 int index,
> >  					 enum gpiod_flags flags,
> >  					 const char *label)
> >  {
> >  
> 
> Unnecessary blank line?

Indeed, I'll fix it.

> 
> > +	return gpiod_find_and_request(NULL, fwnode, con_id, index, flags, label,
> > +				      false);
> 
> Can be one line.

Yep, depending on 80/100 column answer.

Thanks for the review!


-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 03, 2022 at 11:10:16PM -0700, 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).
> > - name of the software node representing gpiochip must match label of
> >   the gpiochip, as we use it to locate gpiochip structure at runtime.
> > 
> > const struct software_node gpio_bank_b_node = {
> > 	.name = "B",
> > };
> > 
> > const struct property_entry simone_key_enter_props[] __initconst = {
> > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> 
> > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> 
> Okay, can we have an example for something like reset-gpios? Because from
> the above I can't easily get what label is and how in the `gpioinfo` tool
> the requested line will look like.

The label is something unrelated to gpio. The example was supposed to
match gpio-keys binding found in
Documentation/devicetree/bindings/input/gpio-keys.yaml

> 
> > 	{ }
> > };
> 
> ...
> 
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/gpio/consumer.h>
> 
> It seems you are using much more that these ones.

Yeah, you are right.

> 
> ...
> 
> > +	char prop_name[32]; /* 32 is max size of property name */
> 
> Why is it not defined then?

Not sure. 32 is the limit used throughout gpiolib (see the main
gpiolib.c, gpiolib-acpi.c and gpiolib-of.c). We could add a private
gpiolib define. Or we could dynamically allocate strings if we belive
this is an issue.

I'd like to do it as a followup if we decide this needs changing.


> 
> ...
> 
> > +	/*
> > +	 * 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.
> > +	 */
> 
> True, but for the sake of consistency between providers perhaps it makes sense
> to check that as well. Dunno, up to Linus and Bart to decide.

I hear you, however we had to have this fallback for OF and ACPI because
of concerns of separate DT/firmware and keeping compatibility. Here we
do not have this problem, so I think we should require properly named
properties.

> 
> ...
> 
> > +	/*
> > +	 * We expect all swnode-described GPIOs have GPIO number and
> > +	 * polarity arguments, hence nargs is set to 2.
> > +	 */
> 
> Maybe instead you can provide a custom macro wrapper that will check the number
> of arguments at compile time?

We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
that enforces needed arguments.


> 
> ...
> 
> > +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> > +			__func__, prop_name, fwnode, idx);
> 
> __func__ is not needed. Dynamic Debug can automatically add it.
> Since you have an fwnode, use that as a marker.

I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can
guess the function from other log messages we emit, but does it hurt
having it?

> 
> ...
> 
> > +	chip = gpiochip_find((void *)chip_node->name,
> > +			     swnode_gpiochip_match_name);
> 
> One line?
> 
> ...
> 
> > +	pr_debug("%s: parsed '%s' property of node '%pfwP[%d]' - status (%d)\n",
> > +		 __func__, prop_name, fwnode, idx, PTR_ERR_OR_ZERO(desc));
> 
> Same as above.
> 
> ...
> 
> > +	char prop_name[32];
> 
> > +	if (con_id)
> > +		snprintf(prop_name, sizeof(prop_name), "%s-gpios", con_id);
> > +	else
> > +		strscpy(prop_name, "gpios", sizeof(prop_name));
> 
> I saw this code, please deduplicate.

OK.

> 
> ...
> 
> > +	/*
> > +	 * This is not very efficient, but GPIO lists usually have only
> > +	 * 1 or 2 entries.
> > +	 */
> > +	count = 0;
> > +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> > +						  0, count, &args) == 0)
> 
> I would put it into for loop (and looking into property.h I think propname
> is fine variable name):
> 
> 	for (count = 0; ; count++) {
> 		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
> 			break;
> 	}

OK on name, but I like explicit counting with the "while" loop as it
shows the purpose of the code.

> 
> Btw, what about reference counting? Do we need to care about it?

Yes, indeed, we need to drop the reference, thank you for noticing!
> 
> > +	return count ? count : -ENOENT;
> 
> Elvis would work as well.
> 
> 	return count ?: -ENOENT;

OK, I like Elvis.

> 
> 
> ...
> 
> > +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);
> 
> ...
> 
> > +	/*
> > +	 * First look up GPIO in the secondary software node in case
> > +	 * it was used to store updated properties.
> 
> Why this is done first? We don't try secondary before we have checked primary.

I believe we should check secondary first, so that secondaries can be
used not only to add missing properties, but also to override existing
ones in case they are incorrect.

> 
> > +	 */
> 
> > +	if (is_software_node(fwnode->secondary)) {
> 
> With the previous comments it would become
> 
> 	if (fwnode && is_...)

Right, I prefer if we could trust that fwnode passed here is not NULL.

> 
> > +		dev_dbg(consumer,
> > +			"using secondary software node for GPIO lookup\n");
> > +		desc = swnode_find_gpio(fwnode->secondary,
> > +					con_id, idx, lookupflags);
> > +		if (!gpiod_not_found(desc))
> > +			return desc;
> > +	}
> 
> ...
> 
> >  int gpiod_count(struct device *dev, const char *con_id)
> >  {
> > +	struct fwnode_handle *fwnode = dev ? dev_fwnode(dev) : NULL;
> > +	int count;
> > +
> > +	/*
> > +	 * First look up GPIO in the secondary software node in case
> > +	 * it was used to store updated properties.
> > +	 */
> 
> Same question as above.
> 
> > +	if (!IS_ERR_OR_NULL(fwnode) && is_software_node(fwnode->secondary)) {
> > +		count = swnode_gpio_count(fwnode->secondary, con_id);
> > +		if (count > 0)
> > +			return 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);
> 

Thanks for the review!

-- 
Dmitry

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

[Andy Shevchenko]

On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:

...

> > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > 
> > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > 
> > Okay, can we have an example for something like reset-gpios? Because from
> > the above I can't easily get what label is and how in the `gpioinfo` tool
> > the requested line will look like.
> 
> The label is something unrelated to gpio. The example was supposed to
> match gpio-keys binding found in
> Documentation/devicetree/bindings/input/gpio-keys.yaml

Yes, but what would be output of `gpioinfo` for the above  example and
if GPIO is named properly (with con_id)?

> > > 	{ }
> > > };

...

> > > +	/*
> > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > +	 * polarity arguments, hence nargs is set to 2.
> > > +	 */
> > 
> > Maybe instead you can provide a custom macro wrapper that will check the number
> > of arguments at compile time?
> 
> We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> that enforces needed arguments.

Yes, that's what I meant.

...

> > > +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> > > +			__func__, prop_name, fwnode, idx);
> > 
> > __func__ is not needed. Dynamic Debug can automatically add it.
> > Since you have an fwnode, use that as a marker.
> 
> I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can
> guess the function from other log messages we emit, but does it hurt
> having it?

I think it's redundant. You can modify message itself to improve its
uniqueness.

...

> > > +	/*
> > > +	 * This is not very efficient, but GPIO lists usually have only
> > > +	 * 1 or 2 entries.
> > > +	 */
> > > +	count = 0;
> > > +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> > > +						  0, count, &args) == 0)
> > 
> > I would put it into for loop (and looking into property.h I think propname
> > is fine variable name):
> > 
> > 	for (count = 0; ; count++) {
> > 		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
> > 			break;
> > 	}
> 
> OK on name, but I like explicit counting with the "while" loop as it
> shows the purpose of the code.

OK, let's see how it will look like with the proper dropped reference.

> > Btw, what about reference counting? Do we need to care about it?
> 
> Yes, indeed, we need to drop the reference, thank you for noticing!

...

> > > +	/*
> > > +	 * First look up GPIO in the secondary software node in case
> > > +	 * it was used to store updated properties.
> > 
> > Why this is done first? We don't try secondary before we have checked primary.
> 
> I believe we should check secondary first, so that secondaries can be
> used not only to add missing properties, but also to override existing
> ones in case they are incorrect.

It contradicts all code we have in the kernel regarding the use of software
nodes, you need very strong argument to justify that.

Personally I think this must be fixed.

> > > +	 */

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote:
> On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 03, 2022 at 11:10:15PM -0700, 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)
> > >  {
> > 
> > > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > 
> > No need, just return directly.
> > 
> > > +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
> > > +		con_id, fwnode_get_name(fwnode));
> > 
> > %pfwP ?
> 
> OK. Although, I think I like %pfw (without 'P') better as it gives
> results like:
> 
> 	/soc/i2c@11007000/edp-bridge@8
> 
> or
> 
> 	\_SB.PCI0.I2C1.D010
> 
> which should help identifying the exact node.

I agree.

> > > +	/* Using device tree? */
> > >  	if (is_of_node(fwnode)) {
> > > +		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)) {
> > 
> > With direct return, no need for 'else' here.
> 
> When we have several branches of equal weight I prefer not to have
> early/inline returns, but I can add:
> 
> 	} else {
> 		desc = ERR_PTR(-ENOENT);
> 	}
> 
> at the end, what do you think?

No strong opinion here.

> > > +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> > > +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
> > >  	}
> > >  
> > > +	return desc;
> > > +}

...

> > > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > 
> > We can get rid of the assignment, see below.

Still below another thought which affects this.

> > > +	if (fwnode)
> > 
> > Do we need this check?
> 
> Yes, I would prefer to have it as it clearly informs the reader that we
> are only doing lookup by node if we actually have a node.
> 
> gpiod_find_and_request() expects that it gets a valid node and in the
> followup change it will be dereferencing fwnode without checking for
> NULL-ness.

But most of the code will check for the NULL anyway. The exceptions are
dev_dbg() and accessing to the secondary fwnode.

> > Debug message above (when %pfw is used) would be even useful when
> > fwnode == NULL.

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

Looking into drivers/base/property.c makes me realize that you might need to
test for error pointer as well.

Perhaps something like

	if (IS_ERR_OR_NULL(fwnode))
		return ERR_PTR(-ENOENT);

in the gpiod_find_by_fwnode() needs to be added. Can you check this?

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > > 
> > > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > > 
> > > Okay, can we have an example for something like reset-gpios? Because from
> > > the above I can't easily get what label is and how in the `gpioinfo` tool
> > > the requested line will look like.
> > 
> > The label is something unrelated to gpio. The example was supposed to
> > match gpio-keys binding found in
> > Documentation/devicetree/bindings/input/gpio-keys.yaml
> 
> Yes, but what would be output of `gpioinfo` for the above  example and
> if GPIO is named properly (with con_id)?

Same as if I am using device tree, or ACPI, etc. I am not changing how
labeling is done, so whatever rules were before adding swnode support
they will be used with swnodes.

With the hack patch to gpio-keys.c below and device using the following
DT fragment I see the following from gpioinfo:

        gpio_keys: gpio-keys {
                status = "okay";

                compatible = "gpio-keys";
                pinctrl-names = "default";
                pinctrl-0 = <&pen_eject>;

                pen_insert: pen-insert {
                        label = "Pen Insert";
                        /* Insert = low, eject = high */
                        /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */
                        linux,code = <SW_PEN_INSERTED>;
                        linux,input-type = <EV_SW>;
                        wakeup-event-action = <EV_ACT_DEASSERTED>;
                        wakeup-source;
                };
        };

Just "gpios" (con_id == NULL):

        line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]

With "key-gpios" (con_id == "key") it is exactly the same:

        line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]

Ah, I guess you wonder how it will look like if we do not pass this
"label" into devm_fwnode_gpiod_get() and instead use NULL?

	line  18: "PEN_EJECT_OD" "?" input active-low [used]

If the driver used gpiod_get() or similar it would either have the
"con_id" label or device name (produced with dev_name(dev) if con_id is
NULL. Still, not changes from using swnodes compared to ACPI or DT.

> 
> > > > 	{ }
> > > > };
> 
> ...
> 
> > > > +	/*
> > > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > > +	 * polarity arguments, hence nargs is set to 2.
> > > > +	 */
> > > 
> > > Maybe instead you can provide a custom macro wrapper that will check the number
> > > of arguments at compile time?
> > 
> > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> > that enforces needed arguments.
> 
> Yes, that's what I meant.

Where do you think it should go? Not sure if I want to pollute
property.h, I guess linux/gpio/matchine.h will need to include
property.h?

> 
> ...
> 
> > > > +		pr_debug("%s: can't parse '%s' property of node '%pfwP[%d]'\n",
> > > > +			__func__, prop_name, fwnode, idx);
> > > 
> > > __func__ is not needed. Dynamic Debug can automatically add it.
> > > Since you have an fwnode, use that as a marker.
> > 
> > I was mimicking gpiolib-of.c::of_get_named_gpiod_flags(). I guess we can
> > guess the function from other log messages we emit, but does it hurt
> > having it?
> 
> I think it's redundant. You can modify message itself to improve its
> uniqueness.

¯\_(ツ)_/¯ I think we are moving into extreme bikeshedding direction
here.

> 
> ...
> 
> > > > +	/*
> > > > +	 * This is not very efficient, but GPIO lists usually have only
> > > > +	 * 1 or 2 entries.
> > > > +	 */
> > > > +	count = 0;
> > > > +	while (fwnode_property_get_reference_args(fwnode, prop_name, NULL,
> > > > +						  0, count, &args) == 0)
> > > 
> > > I would put it into for loop (and looking into property.h I think propname
> > > is fine variable name):
> > > 
> > > 	for (count = 0; ; count++) {
> > > 		if (fwnode_property_get_reference_args(fwnode, propname, NULL, 0, count, &args))
> > > 			break;
> > > 	}
> > 
> > OK on name, but I like explicit counting with the "while" loop as it
> > shows the purpose of the code.
> 
> OK, let's see how it will look like with the proper dropped reference.
> 
> > > Btw, what about reference counting? Do we need to care about it?
> > 
> > Yes, indeed, we need to drop the reference, thank you for noticing!
> 
> ...
> 
> > > > +	/*
> > > > +	 * First look up GPIO in the secondary software node in case
> > > > +	 * it was used to store updated properties.
> > > 
> > > Why this is done first? We don't try secondary before we have checked primary.
> > 
> > I believe we should check secondary first, so that secondaries can be
> > used not only to add missing properties, but also to override existing
> > ones in case they are incorrect.
> 
> It contradicts all code we have in the kernel regarding the use of software
> nodes, you need very strong argument to justify that.
> 
> Personally I think this must be fixed.

I agree, the rest of the code should be fixed ;) I'll put it on my TODO
list.

I gave my argument above already: swnodes should not only be useful to
add missing properties, but also allow fixing up existing ones. If I
implemented what you are suggesting then I would not be able to create
this concise example and would need to model entire DT node for GPIO
keys.

Thanks.

-- 
Dmitry


diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 22a91db645b8f..5fe51c5baa6bb 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -30,6 +30,17 @@
 #include <linux/spinlock.h>
 #include <dt-bindings/input/gpio-keys.h>
 
+#include <linux/property.h>
+#include <linux/gpio/machine.h>
+const struct software_node gpio_bank_node = {
+	.name = "pinctrl_paris",
+};
+
+const struct property_entry pen_insert_props[] = {
+	PROPERTY_ENTRY_REF("key-gpios", &gpio_bank_node, 18, GPIO_ACTIVE_LOW),
+	{ }
+};
+
 struct gpio_button_data {
 	const struct gpio_keys_button *button;
 	struct input_dev *input;
@@ -519,8 +530,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
 	spin_lock_init(&bdata->lock);
 
 	if (child) {
+		if (!strcmp(fwnode_get_name(child), "pen-insert"))
+			child->secondary = fwnode_create_software_node(pen_insert_props, NULL);
+
 		bdata->gpiod = devm_fwnode_gpiod_get(dev, child,
-						     NULL, GPIOD_IN, desc);
+						     "key", GPIOD_IN, desc);
 		if (IS_ERR(bdata->gpiod)) {
 			error = PTR_ERR(bdata->gpiod);
 			if (error == -ENOENT) {
@@ -1056,14 +1070,18 @@ static struct platform_driver gpio_keys_device_driver = {
 	}
 };
 
+
+
 static int __init gpio_keys_init(void)
 {
+	software_node_register(&gpio_bank_node);
 	return platform_driver_register(&gpio_keys_device_driver);
 }
 
 static void __exit gpio_keys_exit(void)
 {
 	platform_driver_unregister(&gpio_keys_device_driver);
+	software_node_unregister(&gpio_bank_node);
 }
 
 late_initcall(gpio_keys_init);

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

[Dmitry Torokhov]

On Fri, Nov 04, 2022 at 11:06:58PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote:
> > On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote:
> > > On Thu, Nov 03, 2022 at 11:10:15PM -0700, 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)
> > > >  {
> > > 
> > > > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > > 
> > > No need, just return directly.
> > > 
> > > > +	dev_dbg(consumer, "GPIO lookup for consumer %s in node '%s'\n",
> > > > +		con_id, fwnode_get_name(fwnode));
> > > 
> > > %pfwP ?
> > 
> > OK. Although, I think I like %pfw (without 'P') better as it gives
> > results like:
> > 
> > 	/soc/i2c@11007000/edp-bridge@8
> > 
> > or
> > 
> > 	\_SB.PCI0.I2C1.D010
> > 
> > which should help identifying the exact node.
> 
> I agree.
> 
> > > > +	/* Using device tree? */
> > > >  	if (is_of_node(fwnode)) {
> > > > +		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)) {
> > > 
> > > With direct return, no need for 'else' here.
> > 
> > When we have several branches of equal weight I prefer not to have
> > early/inline returns, but I can add:
> > 
> > 	} else {
> > 		desc = ERR_PTR(-ENOENT);
> > 	}
> > 
> > at the end, what do you think?
> 
> No strong opinion here.
> 
> > > > +		dev_dbg(consumer, "using ACPI for GPIO lookup\n");
> > > > +		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
> > > >  	}
> > > >  
> > > > +	return desc;
> > > > +}
> 
> ...
> 
> > > > +	struct gpio_desc *desc = ERR_PTR(-ENOENT);
> > > 
> > > We can get rid of the assignment, see below.
> 
> Still below another thought which affects this.
> 
> > > > +	if (fwnode)
> > > 
> > > Do we need this check?
> > 
> > Yes, I would prefer to have it as it clearly informs the reader that we
> > are only doing lookup by node if we actually have a node.
> > 
> > gpiod_find_and_request() expects that it gets a valid node and in the
> > followup change it will be dereferencing fwnode without checking for
> > NULL-ness.
> 
> But most of the code will check for the NULL anyway. The exceptions are
> dev_dbg() and accessing to the secondary fwnode.

I think it is just a matter of what I want to express through source. I
want to show that the device might not have fwnode, and that we only
descend into gpiod_find_by_fwnode() in cases where we actually have
fwnode.

> 
> > > Debug message above (when %pfw is used) would be even useful when
> > > fwnode == NULL.
> 
> > > > +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> > > > +					    &flags, &lookupflags);
> 
> Looking into drivers/base/property.c makes me realize that you might need to
> test for error pointer as well.
> 
> Perhaps something like
> 
> 	if (IS_ERR_OR_NULL(fwnode))
> 		return ERR_PTR(-ENOENT);
> 
> in the gpiod_find_by_fwnode() needs to be added. Can you check this?

No, only fwnode->secondary pointer can be PTR_ERR()-encoded.

From comment to set_primary_fwnode() in drivers/base/core.c

 * Valid fwnode cases are:
 *  - primary --> secondary --> -ENODEV
 *  - primary --> NULL
 *  - secondary --> -ENODEV
 *  - NULL

I do not believe we should be concerned about someone passing secondary
pointers from fwnodes directly into gpiolib.

Thanks.

-- 
Dmitry

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

[Andy Shevchenko]

On Fri, Nov 04, 2022 at 09:56:57PM -0700, Dmitry Torokhov wrote:
> On Fri, Nov 04, 2022 at 11:06:58PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 04, 2022 at 11:52:26AM -0700, Dmitry Torokhov wrote:
> > > On Fri, Nov 04, 2022 at 07:17:27PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Nov 03, 2022 at 11:10:15PM -0700, Dmitry Torokhov wrote:

...

> > > > > +		desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> > > > > +					    &flags, &lookupflags);
> > 
> > Looking into drivers/base/property.c makes me realize that you might need to
> > test for error pointer as well.
> > 
> > Perhaps something like
> > 
> > 	if (IS_ERR_OR_NULL(fwnode))
> > 		return ERR_PTR(-ENOENT);
> > 
> > in the gpiod_find_by_fwnode() needs to be added. Can you check this?
> 
> No, only fwnode->secondary pointer can be PTR_ERR()-encoded.
> 
> From comment to set_primary_fwnode() in drivers/base/core.c
> 
>  * Valid fwnode cases are:
>  *  - primary --> secondary --> -ENODEV
>  *  - primary --> NULL
>  *  - secondary --> -ENODEV
>  *  - NULL
> 
> I do not believe we should be concerned about someone passing secondary
> pointers from fwnodes directly into gpiolib.

It's not only about secondary vs. primary notation, some of fwnode API calls
may return error pointer and hence entire stack should be prepared for that.

See 002752af7b89 ("device property: Allow error pointer to be passed to
fwnode APIs") as an example.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote:
> On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:

...

> > > > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > > > 
> > > > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > > > 
> > > > Okay, can we have an example for something like reset-gpios? Because from
> > > > the above I can't easily get what label is and how in the `gpioinfo` tool
> > > > the requested line will look like.
> > > 
> > > The label is something unrelated to gpio. The example was supposed to
> > > match gpio-keys binding found in
> > > Documentation/devicetree/bindings/input/gpio-keys.yaml
> > 
> > Yes, but what would be output of `gpioinfo` for the above  example and
> > if GPIO is named properly (with con_id)?
> 
> Same as if I am using device tree, or ACPI, etc. I am not changing how
> labeling is done, so whatever rules were before adding swnode support
> they will be used with swnodes.
> 
> With the hack patch to gpio-keys.c below and device using the following
> DT fragment I see the following from gpioinfo:
> 
>         gpio_keys: gpio-keys {
>                 status = "okay";
> 
>                 compatible = "gpio-keys";
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pen_eject>;
> 
>                 pen_insert: pen-insert {
>                         label = "Pen Insert";
>                         /* Insert = low, eject = high */
>                         /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */
>                         linux,code = <SW_PEN_INSERTED>;
>                         linux,input-type = <EV_SW>;
>                         wakeup-event-action = <EV_ACT_DEASSERTED>;
>                         wakeup-source;
>                 };
>         };
> 
> Just "gpios" (con_id == NULL):
> 
>         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> 
> With "key-gpios" (con_id == "key") it is exactly the same:
> 
>         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> 
> Ah, I guess you wonder how it will look like if we do not pass this
> "label" into devm_fwnode_gpiod_get() and instead use NULL?
> 
> 	line  18: "PEN_EJECT_OD" "?" input active-low [used]
> 
> If the driver used gpiod_get() or similar it would either have the
> "con_id" label or device name (produced with dev_name(dev) if con_id is
> NULL. Still, not changes from using swnodes compared to ACPI or DT.

Yes, can you add a summary of above to the commit message?

> > > > > 	{ }
> > > > > };

...

> > > > > +	/*
> > > > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > > > +	 * polarity arguments, hence nargs is set to 2.
> > > > > +	 */
> > > > 
> > > > Maybe instead you can provide a custom macro wrapper that will check the number
> > > > of arguments at compile time?
> > > 
> > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> > > that enforces needed arguments.
> > 
> > Yes, that's what I meant.
> 
> Where do you think it should go? Not sure if I want to pollute
> property.h, I guess linux/gpio/matchine.h will need to include
> property.h?

Good question. I was thinking more of a separate header for that,
because adding property.h adds tons of stuff which might be not
needed otherwise.

...

> > > > > +	/*
> > > > > +	 * First look up GPIO in the secondary software node in case
> > > > > +	 * it was used to store updated properties.
> > > > 
> > > > Why this is done first? We don't try secondary before we have checked primary.
> > > 
> > > I believe we should check secondary first, so that secondaries can be
> > > used not only to add missing properties, but also to override existing
> > > ones in case they are incorrect.
> > 
> > It contradicts all code we have in the kernel regarding the use of software
> > nodes, you need very strong argument to justify that.
> > 
> > Personally I think this must be fixed.
> 
> I agree, the rest of the code should be fixed ;) I'll put it on my TODO
> list.

I'm not sure what "rest of the code" you are referring to. The core part of
device property APIs?

> I gave my argument above already: swnodes should not only be useful to
> add missing properties, but also allow fixing up existing ones. If I
> implemented what you are suggesting then I would not be able to create
> this concise example and would need to model entire DT node for GPIO
> keys.

Why do you need that in the first place? We should not use swnodes as primary
source of the information. The auxiliary one is fine. "Fixing" via priority
inversion in current model is not good thing to have.

If you really need that you have to first do the following:
- convert fwnode to be a list node
- unembed it from struct device (leaving only head of list there
- update all necessary APIs respectively

In such implementation list_add() / list_add_tail() will define a priority
and you may have stack of properties.

Doing it in a hackish way by allowing priority inversion in _some_ APIs
is no go in my opinion.

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Torokhov]

On Mon, Nov 07, 2022 at 01:08:09PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote:
> > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:
> 
> ...
> 
> > > > > > const struct property_entry simone_key_enter_props[] __initconst = {
> > > > > > 	PROPERTY_ENTRY_U32("linux,code", KEY_ENTER),
> > > > > 
> > > > > > 	PROPERTY_ENTRY_STRING("label", "enter"),
> > > > > > 	PROPERTY_ENTRY_REF("gpios", &gpio_bank_b_node, 123, GPIO_ACTIVE_LOW),
> > > > > 
> > > > > Okay, can we have an example for something like reset-gpios? Because from
> > > > > the above I can't easily get what label is and how in the `gpioinfo` tool
> > > > > the requested line will look like.
> > > > 
> > > > The label is something unrelated to gpio. The example was supposed to
> > > > match gpio-keys binding found in
> > > > Documentation/devicetree/bindings/input/gpio-keys.yaml
> > > 
> > > Yes, but what would be output of `gpioinfo` for the above  example and
> > > if GPIO is named properly (with con_id)?
> > 
> > Same as if I am using device tree, or ACPI, etc. I am not changing how
> > labeling is done, so whatever rules were before adding swnode support
> > they will be used with swnodes.
> > 
> > With the hack patch to gpio-keys.c below and device using the following
> > DT fragment I see the following from gpioinfo:
> > 
> >         gpio_keys: gpio-keys {
> >                 status = "okay";
> > 
> >                 compatible = "gpio-keys";
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pen_eject>;
> > 
> >                 pen_insert: pen-insert {
> >                         label = "Pen Insert";
> >                         /* Insert = low, eject = high */
> >                         /* gpios = <&pio 18 GPIO_ACTIVE_LOW>; */
> >                         linux,code = <SW_PEN_INSERTED>;
> >                         linux,input-type = <EV_SW>;
> >                         wakeup-event-action = <EV_ACT_DEASSERTED>;
> >                         wakeup-source;
> >                 };
> >         };
> > 
> > Just "gpios" (con_id == NULL):
> > 
> >         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> > 
> > With "key-gpios" (con_id == "key") it is exactly the same:
> > 
> >         line  18: "PEN_EJECT_OD" "Pen Insert" input active-low [used]
> > 
> > Ah, I guess you wonder how it will look like if we do not pass this
> > "label" into devm_fwnode_gpiod_get() and instead use NULL?
> > 
> > 	line  18: "PEN_EJECT_OD" "?" input active-low [used]
> > 
> > If the driver used gpiod_get() or similar it would either have the
> > "con_id" label or device name (produced with dev_name(dev) if con_id is
> > NULL. Still, not changes from using swnodes compared to ACPI or DT.
> 
> Yes, can you add a summary of above to the commit message?
> 
> > > > > > 	{ }
> > > > > > };
> 
> ...
> 
> > > > > > +	/*
> > > > > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > > > > +	 * polarity arguments, hence nargs is set to 2.
> > > > > > +	 */
> > > > > 
> > > > > Maybe instead you can provide a custom macro wrapper that will check the number
> > > > > of arguments at compile time?
> > > > 
> > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> > > > that enforces needed arguments.
> > > 
> > > Yes, that's what I meant.
> > 
> > Where do you think it should go? Not sure if I want to pollute
> > property.h, I guess linux/gpio/matchine.h will need to include
> > property.h?
> 
> Good question. I was thinking more of a separate header for that,
> because adding property.h adds tons of stuff which might be not
> needed otherwise.

OK, I guess include/linux/gpio/property.h ?

> 
> ...
> 
> > > > > > +	/*
> > > > > > +	 * First look up GPIO in the secondary software node in case
> > > > > > +	 * it was used to store updated properties.
> > > > > 
> > > > > Why this is done first? We don't try secondary before we have checked primary.
> > > > 
> > > > I believe we should check secondary first, so that secondaries can be
> > > > used not only to add missing properties, but also to override existing
> > > > ones in case they are incorrect.
> > > 
> > > It contradicts all code we have in the kernel regarding the use of software
> > > nodes, you need very strong argument to justify that.
> > > 
> > > Personally I think this must be fixed.
> > 
> > I agree, the rest of the code should be fixed ;) I'll put it on my TODO
> > list.
> 
> I'm not sure what "rest of the code" you are referring to. The core part of
> device property APIs?

Yes.

> 
> > I gave my argument above already: swnodes should not only be useful to
> > add missing properties, but also allow fixing up existing ones. If I
> > implemented what you are suggesting then I would not be able to create
> > this concise example and would need to model entire DT node for GPIO
> > keys.
> 
> Why do you need that in the first place? We should not use swnodes as primary
> source of the information. The auxiliary one is fine. "Fixing" via priority
> inversion in current model is not good thing to have.
> 
> If you really need that you have to first do the following:
> - convert fwnode to be a list node
> - unembed it from struct device (leaving only head of list there
> - update all necessary APIs respectively
> 
> In such implementation list_add() / list_add_tail() will define a priority
> and you may have stack of properties.

Hmm, that will complicate things quite a bit. I wonder why you think
that using swnodes to fix up the "bad" firmware data is not desirable.
Swnodes are controlled by the kernel and thus we can potentially allow
users tweak them from usersoace. There is a desire to allow easier
access to various driver's parameters - see for example Hans patches to
Goodix and Silead where he adds code that intercepts reading of device
properties and instead gets data form module parameter - I would like to
have such facility in more general way.

https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/

> 
> Doing it in a hackish way by allowing priority inversion in _some_ APIs
> is no go in my opinion.

Yes, I agree that we want to have all APIs behave in the similar way. It
occurred to me that the topic of handling secondary node is actually
separate from swnode hanlding, so I will remove it from this patch and I
can start a separate thread/patches for it after we land this series.

Thanks.

-- 
Dmitry

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

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 08:12:32AM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 07, 2022 at 01:08:09PM +0200, Andy Shevchenko wrote:
> > On Fri, Nov 04, 2022 at 09:48:47PM -0700, Dmitry Torokhov wrote:
> > > On Fri, Nov 04, 2022 at 10:57:31PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Nov 04, 2022 at 12:33:06PM -0700, Dmitry Torokhov wrote:
> > > > > On Fri, Nov 04, 2022 at 08:08:03PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Nov 03, 2022 at 11:10:16PM -0700, Dmitry Torokhov wrote:

...

> > > > > > > +	/*
> > > > > > > +	 * We expect all swnode-described GPIOs have GPIO number and
> > > > > > > +	 * polarity arguments, hence nargs is set to 2.
> > > > > > > +	 */
> > > > > > 
> > > > > > Maybe instead you can provide a custom macro wrapper that will check the number
> > > > > > of arguments at compile time?
> > > > > 
> > > > > We could have PROPERTY_ENTRY_GPIO() built on top of PROPERTY_ENTRY_REF()
> > > > > that enforces needed arguments.
> > > > 
> > > > Yes, that's what I meant.
> > > 
> > > Where do you think it should go? Not sure if I want to pollute
> > > property.h, I guess linux/gpio/matchine.h will need to include
> > > property.h?
> > 
> > Good question. I was thinking more of a separate header for that,
> > because adding property.h adds tons of stuff which might be not
> > needed otherwise.
> 
> OK, I guess include/linux/gpio/property.h ?

Why not?

...

> > > > > > > +	/*
> > > > > > > +	 * First look up GPIO in the secondary software node in case
> > > > > > > +	 * it was used to store updated properties.
> > > > > > 
> > > > > > Why this is done first? We don't try secondary before we have checked primary.
> > > > > 
> > > > > I believe we should check secondary first, so that secondaries can be
> > > > > used not only to add missing properties, but also to override existing
> > > > > ones in case they are incorrect.
> > > > 
> > > > It contradicts all code we have in the kernel regarding the use of software
> > > > nodes, you need very strong argument to justify that.
> > > > 
> > > > Personally I think this must be fixed.
> > > 
> > > I agree, the rest of the code should be fixed ;) I'll put it on my TODO
> > > list.
> > 
> > I'm not sure what "rest of the code" you are referring to. The core part of
> > device property APIs?
> 
> Yes.

> > > I gave my argument above already: swnodes should not only be useful to
> > > add missing properties, but also allow fixing up existing ones. If I
> > > implemented what you are suggesting then I would not be able to create
> > > this concise example and would need to model entire DT node for GPIO
> > > keys.
> > 
> > Why do you need that in the first place? We should not use swnodes as primary
> > source of the information. The auxiliary one is fine. "Fixing" via priority
> > inversion in current model is not good thing to have.
> > 
> > If you really need that you have to first do the following:
> > - convert fwnode to be a list node
> > - unembed it from struct device (leaving only head of list there
> > - update all necessary APIs respectively
> > 
> > In such implementation list_add() / list_add_tail() will define a priority
> > and you may have stack of properties.
> 
> Hmm, that will complicate things quite a bit. I wonder why you think
> that using swnodes to fix up the "bad" firmware data is not desirable.

I put there "in current model". It means that for secondary-primary and only
them that design doesn't allow to create quirks.

> Swnodes are controlled by the kernel and thus we can potentially allow
> users tweak them from usersoace. There is a desire to allow easier
> access to various driver's parameters - see for example Hans patches to
> Goodix and Silead where he adds code that intercepts reading of device
> properties and instead gets data form module parameter - I would like to
> have such facility in more general way.
> 
> https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/

How can you guarantee that flip-flopping priority of reading properties doesn't
break things?

Moreover, what problem we are trying to hack up? The DT should be fixed in DT.
ACPI? In ACPI properties are not that common, and even that, we shouldn't unleash
vendors to make all possible abuse-like mistakes in ACPI, that's why I do not think
that allowing property quirks is a good idea at all.

> > Doing it in a hackish way by allowing priority inversion in _some_ APIs
> > is no go in my opinion.
> 
> Yes, I agree that we want to have all APIs behave in the similar way. It
> occurred to me that the topic of handling secondary node is actually
> separate from swnode hanlding, so I will remove it from this patch and I
> can start a separate thread/patches for it after we land this series.

As long as the priority is out of the picture I'm fine with this series,

Because as I said the current design seems (I wasn't the author, so I
might be mistaken) had been made with "we trust the firmware" in mind.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Nov 07, 2022 at 10:59:39PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 07, 2022 at 08:12:32AM -0800, Dmitry Torokhov wrote:

...

> > Swnodes are controlled by the kernel and thus we can potentially allow
> > users tweak them from usersoace. There is a desire to allow easier
> > access to various driver's parameters - see for example Hans patches to
> > Goodix and Silead where he adds code that intercepts reading of device
> > properties and instead gets data form module parameter - I would like to
> > have such facility in more general way.
> > 
> > https://lore.kernel.org/all/20221025122930.421377-3-hdegoede@redhat.com/
> 
> How can you guarantee that flip-flopping priority of reading properties doesn't
> break things?
> 
> Moreover, what problem we are trying to hack up? The DT should be fixed in DT.
> ACPI? In ACPI properties are not that common, and even that, we shouldn't unleash
> vendors to make all possible abuse-like mistakes in ACPI, that's why I do not think
> that allowing property quirks is a good idea at all.

To clarify. In the context when we consider the reversed priority of their
importance. That said, that the "quirk first, firmware later" is NAK by me,
while "firmware first, quirk latter" is pretty much fine.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Fri, Nov 4, 2022 at 7:10 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> 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 devicei 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.
>
> Note that kbuild robot is likely to complain about this patchset because
> it depends on patches removing [devm_]gpiod_get_from_of_node() and
> devm_fwnode_get_[index_]gpiod_from_child() APIs that are still pending.
> I pushed them to
>
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tmp-gpiolib
>
> for your reference.
>
> 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

I have waited literally years for this patch series :D

Acked-by: Linus Walleij <linus.walleij@linaro.org>

The ACPI details is Andy territory so I dare not speak about those,
but for everything else I think this is a go.

Yours,
Linus Walleij