[PATCH v11 0/6] gpio-sim: configfs-based GPIO simulator

[PATCH v11 0/6] gpio-sim: configfs-based GPIO simulator

[Bartosz Golaszewski]

Here's the eleventh revision of the simulator.

As there was no reasoning with configfs maintainers for many months,
this time the whole concept of committable items has been dropped. Instead,
each configfs chip item (or rather a group - more on that later) exposes a new
attribute called 'live'. Writing 1 to it brings the chip on-line (registers
the platform device) and writing 0 tears it down.

There are some caveats to that approach - for example: we can't block
the user-space from deleting chip items when chips are live but is just
handled by silently destroying the chip device in the background.

In v11 the configfs structure has been deepened to allow creating
multiple banks per platform device. The sysfs interface has changed so
that the gpio_simX attributes are now under the bank's device node and
not the platform device's.

v1 -> v2:
- add selftests for gpio-sim
- add helper programs for selftests
- update the configfs rename callback to work with the new API introduced in
  v5.11
- fix a missing quote in the documentation
- use !! whenever using bits operation that are required to return 0 or 1
- use provided bitmap API instead of reimplementing copy or fill operations
- fix a deadlock in gpio_sim_direction_output()
- add new read-only configfs attributes for mapping of configfs items to GPIO
  device names
- and address other minor issues pointed out in reviews of v1

v2 -> v3:
- use devm_bitmap_alloc() instead of the zalloc variant if we're initializing
  the bitmap with 1s
- drop the patch exporting device_is_bound()
- don't return -ENODEV from dev_nam and chip_name configfs attributes, return
  a string indicating that the device is not available yet ('n/a')
- fix indentation where it makes sense
- don't protect IDA functions which use their own locking and where it's not
  needed
- use kmemdup() instead of kzalloc() + memcpy()
- collected review tags
- minor coding style fixes

v3 -> v4:
- return 'none' instead of 'n/a' from dev_name and chip_name before the device
  is registered
- use sysfs_emit() instead of s*printf()
- drop GPIO_SIM_MAX_PROP as it's only used in an array's definition where it's
  fine to hardcode the value

v4 -> v5:
- drop lib patches that are already upstream
- use BIT() instead of (1UL << bit) for flags
- fix refcounting for the configfs_dirent in rename()
- drop d_move() from the rename() callback
- free memory allocated for the live and pending groups in configfs_d_iput()
  and not in detach_groups()
- make sure that if a group of some name is in the live directory, a new group
  with the same name cannot be created in the pending directory

v5 -> v6:
- go back to using (1UL << bit) instead of BIT()
- if the live group dentry doesn't exist for whatever reason at the time when
  mkdir() in the pending group is called (would be a BUG()), return -ENOENT
  instead of -EEXIST which should only be returned if given subsystem already
  exists in either live or pending group

v6 -> v7:
- as detailed by Andy in commit 6fda593f3082 ("gpio: mockup: Convert to use
  software nodes") removing device properties after the platform device is
  removed but before the GPIO device gets dropped can lead to a use-after-free
  bug - use software nodes to manually control the freeing of the properties

v7 -> v8:
- fixed some minor coding style issues as pointed out by Andy

v8 -> v9:
- dropped the patches implementing committable-items and reworked the
  driver to not use them
- reworked the gpio-line-names property and configuring specific lines
  in general
- many smaller tweaks here and there

v9 -> v10:
- make writing to 'live' wait for the probe to finish and report an
  error to user-space if it failed
- add the ability to hog lines from the kernel-space
- rework locking (drop separate locks for line context objects)
- rework the sysfs interface (create a separate group for each line with
  a constant number of attributes instead of going the other way around)

v10 -> v11:
- rework the configfs structure to represent a deeper hierarchy that
  gpiolib supports, namely: multiple banks per platform device

Bartosz Golaszewski (6):
  gpiolib: provide gpiod_remove_hogs()
  gpiolib: allow to specify the firmware node in struct gpio_chip
  gpio: sim: new testing module
  selftests: gpio: provide a helper for reading chip info
  selftests: gpio: add a helper for reading GPIO line names
  selftests: gpio: add test cases for gpio-sim

 Documentation/admin-guide/gpio/gpio-sim.rst   |   93 +
 drivers/gpio/Kconfig                          |    8 +
 drivers/gpio/Makefile                         |    1 +
 drivers/gpio/gpio-sim.c                       | 1592 +++++++++++++++++
 drivers/gpio/gpiolib.c                        |   26 +-
 include/linux/gpio/driver.h                   |    2 +
 include/linux/gpio/machine.h                  |    2 +
 tools/testing/selftests/gpio/.gitignore       |    2 +
 tools/testing/selftests/gpio/Makefile         |    4 +-
 tools/testing/selftests/gpio/config           |    1 +
 tools/testing/selftests/gpio/gpio-chip-info.c |   57 +
 tools/testing/selftests/gpio/gpio-line-name.c |   55 +
 tools/testing/selftests/gpio/gpio-sim.sh      |  396 ++++
 13 files changed, 2236 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c
 create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c
 create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c
 create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh

-- 
2.25.1

[PATCH v11 1/6] gpiolib: provide gpiod_remove_hogs()

[Bartosz Golaszewski]

Currently all users of gpiod_add_hogs() call it only once at system
init so there never was any need for a mechanism allowing to remove
them. Now the upcoming gpio-sim will need to tear down chips with hogged
lines so provide a function that allows to remove hogs.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c       | 11 +++++++++++
 include/linux/gpio/machine.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index abfbf546d159..22b98a590a88 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3540,6 +3540,17 @@ void gpiod_add_hogs(struct gpiod_hog *hogs)
 }
 EXPORT_SYMBOL_GPL(gpiod_add_hogs);
 
+void gpiod_remove_hogs(struct gpiod_hog *hogs)
+{
+	struct gpiod_hog *hog;
+
+	mutex_lock(&gpio_machine_hogs_mutex);
+	for (hog = &hogs[0]; hog->chip_label; hog++)
+		list_del(&hog->list);
+	mutex_unlock(&gpio_machine_hogs_mutex);
+}
+EXPORT_SYMBOL_GPL(gpiod_remove_hogs);
+
 static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index d755e529c1e3..2647dd10b541 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -100,6 +100,7 @@ void gpiod_add_lookup_table(struct gpiod_lookup_table *table);
 void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n);
 void gpiod_remove_lookup_table(struct gpiod_lookup_table *table);
 void gpiod_add_hogs(struct gpiod_hog *hogs);
+void gpiod_remove_hogs(struct gpiod_hog *hogs);
 #else /* ! CONFIG_GPIOLIB */
 static inline
 void gpiod_add_lookup_table(struct gpiod_lookup_table *table) {}
@@ -108,6 +109,7 @@ void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n) {}
 static inline
 void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) {}
 static inline void gpiod_add_hogs(struct gpiod_hog *hogs) {}
+static inline void gpiod_remove_hogs(struct gpiod_hog *hogs) {}
 #endif /* CONFIG_GPIOLIB */
 
 #endif /* __LINUX_GPIO_MACHINE_H */
-- 
2.25.1

[PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

Software nodes allow us to represent hierarchies for device components
that don't have their struct device representation yet - for instance:
banks of GPIOs under a common GPIO expander. The core gpiolib core
however doesn't offer any way of passing this information from the
drivers.

This extends struct gpio_chip with a pointer to fwnode that can be set
by the driver and used to pass device properties for child nodes.

This is similar to how we handle device-tree sub-nodes with
CONFIG_OF_GPIO enabled.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c      | 15 ++++++++++++++-
 include/linux/gpio/driver.h |  2 ++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 22b98a590a88..2a877b4ccdee 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -593,13 +593,26 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *lock_key,
 			       struct lock_class_key *request_key)
 {
-	struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
+	struct fwnode_handle *fwnode = NULL;
 	unsigned long	flags;
 	int		ret = 0;
 	unsigned	i;
 	int		base = gc->base;
 	struct gpio_device *gdev;
 
+#if IS_ENABLED(CONFIG_OF_GPIO)
+	if (gc->of_node && gc->fwnode) {
+		pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
+		       __func__);
+		return -EINVAL;
+	}
+#endif /* CONFIG_OF_GPIO */
+
+	if (gc->fwnode)
+		fwnode = gc->fwnode;
+	else if (gc->parent)
+		fwnode = dev_fwnode(gc->parent);
+
 	/*
 	 * First: allocate and populate the internal stat container, and
 	 * set up the struct device.
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a673a359e20b..b0728c8ad90c 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -289,6 +289,7 @@ struct gpio_irq_chip {
  *	number or the name of the SoC IP-block implementing it.
  * @gpiodev: the internal state holder, opaque struct
  * @parent: optional parent device providing the GPIOs
+ * @fwnode: optional fwnode providing this controller's properties
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @request: optional hook for chip-specific activation, such as
  *	enabling module power and clock; may sleep
@@ -377,6 +378,7 @@ struct gpio_chip {
 	const char		*label;
 	struct gpio_device	*gpiodev;
 	struct device		*parent;
+	struct fwnode_handle	*fwnode;
 	struct module		*owner;
 
 	int			(*request)(struct gpio_chip *gc,
-- 
2.25.1

[PATCH v11 3/6] gpio: sim: new testing module

[Bartosz Golaszewski]

Implement a new, modern GPIO testing module controlled by configfs
attributes instead of module parameters. The goal of this driver is
to provide a replacement for gpio-mockup that will be easily extensible
with new features and doesn't require reloading the module to change
the setup.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 Documentation/admin-guide/gpio/gpio-sim.rst |   93 ++
 drivers/gpio/Kconfig                        |    8 +
 drivers/gpio/Makefile                       |    1 +
 drivers/gpio/gpio-sim.c                     | 1592 +++++++++++++++++++
 4 files changed, 1694 insertions(+)
 create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
 create mode 100644 drivers/gpio/gpio-sim.c

diff --git a/Documentation/admin-guide/gpio/gpio-sim.rst b/Documentation/admin-guide/gpio/gpio-sim.rst
new file mode 100644
index 000000000000..ec28e9ac9b9c
--- /dev/null
+++ b/Documentation/admin-guide/gpio/gpio-sim.rst
@@ -0,0 +1,93 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Configfs GPIO Simulator
+=======================
+
+The configfs GPIO Simulator (gpio-sim) provides a way to create simulated GPIO
+chips for testing purposes. The lines exposed by these chips can be accessed
+using the standard GPIO character device interface as well as manipulated
+using sysfs attributes.
+
+Creating simulated chips
+------------------------
+
+The gpio-sim module registers a configfs subsystem called 'gpio-sim'. For
+details of the configfs filesystem, please refer to the configfs documentaion.
+
+The user can create a hierarchy of configfs groups and items as well as modify
+values of exposed attributes. Once the chip is instantiated, this hierarchy
+will be translated to appropriate device properties. The general structure is:
+
+Group: /config/gpio-sim
+
+This is the top directory of the gpio-sim configfs tree.
+
+Group: /config/gpio-sim/gpio-device
+Attribute: /config/gpio-sim/gpio-device/dev_name
+Attribute: /config/gpio-sim/gpio-device/live
+
+This is a directory representing a GPIO platform device. The 'dev_name'
+attribute is read-only and allows the user-space to read the platform device
+name (e.g. 'gpio-sim.0'). The 'live' attribute allows to trigger the actual
+creation of the device once it's fully configured. The accepted values are:
+'1' to enable the simulated device and '0' to disable and tear it down.
+
+Group: /config/gpio-sim/gpio-device/gpio-bankX/
+Attribute: /config/gpio-sim/gpio-device/gpio-bankX/chip_name
+Attribute: /config/gpio-sim/gpio-device/gpio-bankX/num_lines
+
+This group represents a bank of GPIOs under the top platform device. The
+'chip_name' attribute is read-only and allows the user-space to read the
+device name of the bank device. The 'num_lines' attribute allows to specify
+the number of lines exposed by this bank.
+
+Group: /config/gpio-sim/gpio-device/gpio-bankX/lineY
+Attribute: /config/gpio-sim/gpio-device/gpio-bankX/lineY/name
+
+This group represents a single line at the offset Y. The 'name' attribute
+allows to set the line name as represented by the 'gpio-line-names' property.
+
+Item: /config/gpio-sim/gpio-device/gpio-bankX/lineY/hog
+Attribute: /config/gpio-sim/gpio-device/gpio-bankX/lineY/hog/name
+Attribute: /config/gpio-sim/gpio-device/gpio-bankX/lineY/hog/direction
+
+This item makes the gpio-sim module hog the associated line. The 'name'
+attribute specifies the in-kernel consumer name to use. The 'direction'
+attribute specifies the hog direction and must be one of: 'input',
+'output-high' and 'output-low'.
+
+Inside each bank directory, there's a set of attributes that can be used to
+configure the new chip. Additionally the user can mkdir() subdirectories inside
+the chip's directory that allow to pass additional configuration for specific
+lines. The name of those subdirectories must take the form of: 'line<offset>'
+(e.g. 'line0', 'line20', etc.) as the name will be used by the module to assign
+the config to the specific line at given offset.
+
+Once the confiuration is complete, the 'live' attribute must be set to 1 in
+order to instantiate the chip. It can be set back to 0 to destroy the simulated
+chip. The module will synchronously wait for the new simulated device to be
+successfully probed and if this doesn't happen, writing to 'live' will result
+in an error.
+
+Simulated GPIO chips can also be defined in device-tree. The compatible string
+must be: "gpio-simulator". Supported properties are:
+
+  "gpio-sim,label" - chip label
+
+Other standard GPIO properties (like "gpio-line-names", "ngpios" or gpio-hog)
+are also supported.
+
+Manipulating simulated lines
+----------------------------
+
+Each simulated GPIO chip creates a separate sysfs group under its device
+directory for each exposed line
+(e.g. /sys/devices/platform/gpio-sim.X/gpiochipY/). The name of each group
+is of the form: 'sim_gpioX' where X is the offset of the line. Inside each
+group there are two attibutes:
+
+  pull - allows to read and set the current simulated pull setting for every
+         line, when writing the value must be one of: 'pull-up', 'pull-down'
+
+  value - allows to read the current value of the line which may be different
+          from the pull if the line is being driven from user-space
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 60d9374c72c0..9acdb4d1047b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1694,6 +1694,14 @@ config GPIO_VIRTIO
 	  These virtual GPIOs can be routed to real GPIOs or attached to
 	  simulators on the host (like QEMU).
 
+config GPIO_SIM
+	tristate "GPIO Simulator Module"
+	select IRQ_SIM
+	select CONFIGFS_FS
+	help
+	  This enables the GPIO simulator - a configfs-based GPIO testing
+	  driver.
+
 endmenu
 
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 71ee9fc2ff83..f21577de2474 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -133,6 +133,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
+obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
new file mode 100644
index 000000000000..0785b1b9053f
--- /dev/null
+++ b/drivers/gpio/gpio-sim.c
@@ -0,0 +1,1592 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO testing driver based on configfs.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/bitmap.h>
+#include <linux/completion.h>
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/machine.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irq_sim.h>
+#include <linux/list.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/string_helpers.h>
+#include <linux/sysfs.h>
+
+#include "gpiolib.h"
+
+#define GPIO_SIM_PROP_MAX	4 /* Max 3 properties + sentinel. */
+#define GPIO_SIM_NUM_ATTRS	3 /* value, pull and sentinel */
+
+static DEFINE_IDA(gpio_sim_ida);
+
+struct gpio_sim_chip {
+	struct gpio_chip gc;
+	unsigned long *direction_map;
+	unsigned long *value_map;
+	unsigned long *pull_map;
+	struct irq_domain *irq_sim;
+	struct mutex lock;
+	const struct attribute_group **attr_groups;
+};
+
+struct gpio_sim_attribute {
+	struct device_attribute dev_attr;
+	unsigned int offset;
+};
+
+static struct gpio_sim_attribute *
+to_gpio_sim_attr(struct device_attribute *dev_attr)
+{
+	return container_of(dev_attr, struct gpio_sim_attribute, dev_attr);
+}
+
+static int gpio_sim_apply_pull(struct gpio_sim_chip *chip,
+			       unsigned int offset, int value)
+{
+	int curr_val, irq, irq_type, ret;
+	struct gpio_desc *desc;
+	struct gpio_chip *gc;
+
+	gc = &chip->gc;
+	desc = &gc->gpiodev->descs[offset];
+
+	mutex_lock(&chip->lock);
+
+	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
+		curr_val = !!test_bit(offset, chip->value_map);
+		if (curr_val == value)
+			goto set_pull;
+
+		/*
+		 * This is fine - it just means, nobody is listening
+		 * for interrupts on this line, otherwise
+		 * irq_create_mapping() would have been called from
+		 * the to_irq() callback.
+		 */
+		irq = irq_find_mapping(chip->irq_sim, offset);
+		if (!irq)
+			goto set_value;
+
+		irq_type = irq_get_trigger_type(irq);
+
+		if ((value && (irq_type & IRQ_TYPE_EDGE_RISING)) ||
+		    (!value && (irq_type & IRQ_TYPE_EDGE_FALLING))) {
+			ret = irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING,
+						    true);
+			if (ret)
+				goto set_pull;
+		}
+	}
+
+set_value:
+	/* Change the value unless we're actively driving the line. */
+	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
+	    !test_bit(FLAG_IS_OUT, &desc->flags))
+		__assign_bit(offset, chip->value_map, value);
+
+set_pull:
+	__assign_bit(offset, chip->pull_map, value);
+	mutex_unlock(&chip->lock);
+	return 0;
+}
+
+static int gpio_sim_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = !!test_bit(offset, chip->value_map);
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static void gpio_sim_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__assign_bit(offset, chip->value_map, value);
+	mutex_unlock(&chip->lock);
+}
+
+static int gpio_sim_get_multiple(struct gpio_chip *gc,
+				 unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	bitmap_copy(bits, chip->value_map, gc->ngpio);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static void gpio_sim_set_multiple(struct gpio_chip *gc,
+				  unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	bitmap_copy(chip->value_map, bits, gc->ngpio);
+	mutex_unlock(&chip->lock);
+}
+
+static int gpio_sim_direction_output(struct gpio_chip *gc,
+				     unsigned int offset, int value)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__clear_bit(offset, chip->direction_map);
+	__assign_bit(offset, chip->value_map, value);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static int gpio_sim_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__set_bit(offset, chip->direction_map);
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+static int gpio_sim_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+	int direction;
+
+	mutex_lock(&chip->lock);
+	direction = !!test_bit(offset, chip->direction_map);
+	mutex_unlock(&chip->lock);
+
+	return direction ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int gpio_sim_set_config(struct gpio_chip *gc,
+				  unsigned int offset, unsigned long config)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_UP:
+		return gpio_sim_apply_pull(chip, offset, 1);
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		return gpio_sim_apply_pull(chip, offset, 0);
+	default:
+		break;
+	}
+
+	return -ENOTSUPP;
+}
+
+static int gpio_sim_to_irq(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	return irq_create_mapping(chip->irq_sim, offset);
+}
+
+static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__assign_bit(offset, chip->value_map, !!test_bit(offset, chip->pull_map));
+	mutex_unlock(&chip->lock);
+}
+
+static ssize_t gpio_sim_sysfs_val_show(struct device *dev,
+				       struct device_attribute *attr, char *buf)
+{
+	struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+	int val;
+
+	mutex_lock(&chip->lock);
+	val = !!test_bit(line_attr->offset, chip->value_map);
+	mutex_unlock(&chip->lock);
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t gpio_sim_sysfs_val_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	/*
+	 * Not assigning this function will result in write() returning -EIO
+	 * which is confusing. Return -EPERM explicitly.
+	 */
+	return -EPERM;
+}
+
+static ssize_t gpio_sim_sysfs_pull_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+	char *repr;
+	int pull;
+
+	mutex_lock(&chip->lock);
+	pull = !!test_bit(line_attr->offset, chip->pull_map);
+	mutex_unlock(&chip->lock);
+
+	if (pull)
+		repr = "pull-up";
+	else
+		repr = "pull-down";
+
+	return sysfs_emit(buf, "%s\n", repr);
+}
+
+static ssize_t gpio_sim_sysfs_pull_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t len)
+{
+	struct gpio_sim_attribute *line_attr = to_gpio_sim_attr(attr);
+	struct gpio_sim_chip *chip = dev_get_drvdata(dev);
+	int ret, pull;
+
+	if (sysfs_streq(buf, "pull-down"))
+		pull = 0;
+	else if (sysfs_streq(buf, "pull-up"))
+		pull = 1;
+	else
+		return -EINVAL;
+
+	ret = gpio_sim_apply_pull(chip, line_attr->offset, pull);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static void gpio_sim_mutex_destroy(void *data)
+{
+	struct mutex *lock = data;
+
+	mutex_destroy(lock);
+}
+
+static void gpio_sim_sysfs_remove(void *data)
+{
+	struct gpio_sim_chip *chip = data;
+
+	sysfs_remove_groups(&chip->gc.gpiodev->dev.kobj, chip->attr_groups);
+}
+
+static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
+{
+	struct device_attribute *val_dev_attr, *pull_dev_attr;
+	struct gpio_sim_attribute *val_attr, *pull_attr;
+	unsigned int num_lines = chip->gc.ngpio;
+	struct device *dev = chip->gc.parent;
+	struct attribute_group *attr_group;
+	struct attribute **attrs;
+	int i, ret;
+
+	chip->attr_groups = devm_kcalloc(dev, sizeof(*chip->attr_groups),
+					 num_lines + 1, GFP_KERNEL);
+	if (!chip->attr_groups)
+		return -ENOMEM;
+
+	for (i = 0; i < num_lines; i++) {
+		attr_group = devm_kzalloc(dev, sizeof(*attr_group), GFP_KERNEL);
+		attrs = devm_kcalloc(dev, sizeof(*attrs),
+				     GPIO_SIM_NUM_ATTRS, GFP_KERNEL);
+		val_attr = devm_kzalloc(dev, sizeof(*val_attr), GFP_KERNEL);
+		pull_attr = devm_kzalloc(dev, sizeof(*pull_attr), GFP_KERNEL);
+		if (!attr_group || !attrs || !val_attr || !pull_attr)
+			return -ENOMEM;
+
+		attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
+						  "sim_gpio%u", i);
+		if (!attr_group->name)
+			return -ENOMEM;
+
+		val_attr->offset = pull_attr->offset = i;
+
+		val_dev_attr = &val_attr->dev_attr;
+		pull_dev_attr = &pull_attr->dev_attr;
+
+		sysfs_attr_init(&val_dev_attr->attr);
+		sysfs_attr_init(&pull_dev_attr->attr);
+
+		val_dev_attr->attr.name = "value";
+		pull_dev_attr->attr.name = "pull";
+
+		val_dev_attr->attr.mode = pull_dev_attr->attr.mode = 0644;
+
+		val_dev_attr->show = gpio_sim_sysfs_val_show;
+		val_dev_attr->store = gpio_sim_sysfs_val_store;
+		pull_dev_attr->show = gpio_sim_sysfs_pull_show;
+		pull_dev_attr->store = gpio_sim_sysfs_pull_store;
+
+		attrs[0] = &val_dev_attr->attr;
+		attrs[1] = &pull_dev_attr->attr;
+
+		attr_group->attrs = attrs;
+		chip->attr_groups[i] = attr_group;
+	}
+
+	ret = sysfs_create_groups(&chip->gc.gpiodev->dev.kobj,
+				  chip->attr_groups);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, gpio_sim_sysfs_remove, chip);
+}
+
+static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
+{
+	struct gpio_sim_chip *chip;
+	struct gpio_chip *gc;
+	const char *label;
+	u32 num_lines;
+	int ret;
+
+	ret = fwnode_property_read_u32(swnode, "ngpios", &num_lines);
+	if (ret)
+		return ret;
+
+	ret = fwnode_property_read_string(swnode, "gpio-sim,label", &label);
+	if (ret) {
+		label = devm_kasprintf(dev, GFP_KERNEL, "%s-%s",
+				       dev_name(dev), fwnode_get_name(swnode));
+		if (!label)
+			return -ENOMEM;
+	}
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->direction_map = devm_bitmap_alloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->direction_map)
+		return -ENOMEM;
+
+	/* Default to input mode. */
+	bitmap_fill(chip->direction_map, num_lines);
+
+	chip->value_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->value_map)
+		return -ENOMEM;
+
+	chip->pull_map = devm_bitmap_zalloc(dev, num_lines, GFP_KERNEL);
+	if (!chip->pull_map)
+		return -ENOMEM;
+
+	chip->irq_sim = devm_irq_domain_create_sim(dev, NULL, num_lines);
+	if (IS_ERR(chip->irq_sim))
+		return PTR_ERR(chip->irq_sim);
+
+	mutex_init(&chip->lock);
+	ret = devm_add_action_or_reset(dev, gpio_sim_mutex_destroy,
+				       &chip->lock);
+	if (ret)
+		return ret;
+
+	gc = &chip->gc;
+	gc->base = -1;
+	gc->ngpio = num_lines;
+	gc->label = label;
+	gc->owner = THIS_MODULE;
+	gc->parent = dev;
+	gc->fwnode = swnode;
+	gc->get = gpio_sim_get;
+	gc->set = gpio_sim_set;
+	gc->get_multiple = gpio_sim_get_multiple;
+	gc->set_multiple = gpio_sim_set_multiple;
+	gc->direction_output = gpio_sim_direction_output;
+	gc->direction_input = gpio_sim_direction_input;
+	gc->get_direction = gpio_sim_get_direction;
+	gc->set_config = gpio_sim_set_config;
+	gc->to_irq = gpio_sim_to_irq;
+	gc->free = gpio_sim_free;
+
+	ret = devm_gpiochip_add_data(dev, gc, chip);
+	if (ret)
+		return ret;
+
+	/* Used by sysfs and configfs callbacks. */
+	dev_set_drvdata(&gc->gpiodev->dev, chip);
+
+	return gpio_sim_setup_sysfs(chip);
+}
+
+static int gpio_sim_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *swnode;
+	int ret;
+
+	device_for_each_child_node(dev, swnode) {
+		ret = gpio_sim_add_bank(swnode, dev);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id gpio_sim_of_match[] = {
+	{ .compatible = "gpio-simulator" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpio_sim_of_match);
+
+static struct platform_driver gpio_sim_driver = {
+	.driver = {
+		.name = "gpio-sim",
+		.of_match_table = gpio_sim_of_match,
+	},
+	.probe = gpio_sim_probe,
+};
+
+struct gpio_sim_device {
+	struct config_group group;
+
+	/*
+	 * If pdev is NULL, the device is 'pending' (waiting for configuration).
+	 * Once the pointer is assigned, the device has been created and the
+	 * item is 'live'.
+	 */
+	struct platform_device *pdev;
+	int id;
+
+	/*
+	 * Each configfs filesystem operation is protected with the subsystem
+	 * mutex. Each separate attribute is protected with the buffer mutex.
+	 * This structure however can be modified by callbacks of different
+	 * attributes so we need another lock.
+	 *
+	 * We use this lock fo protecting all data structures owned by this
+	 * object too.
+	 */
+	struct mutex lock;
+
+	/*
+	 * This is used to synchronously wait for the driver's probe to complete
+	 * and notify the user-space about any errors.
+	 */
+	struct notifier_block bus_notifier;
+	struct completion probe_completion;
+	bool driver_bound;
+	
+	struct gpiod_hog *hogs;
+
+	struct list_head bank_list;
+};
+
+/* This is called with dev->lock already taken. */
+static int gpio_sim_bus_notifier_call(struct notifier_block *nb,
+				      unsigned long action, void *data)
+{
+	struct gpio_sim_device *simdev = container_of(nb,
+						      struct gpio_sim_device,
+						      bus_notifier);
+	struct device *dev = data;
+	char devname[32];
+
+	snprintf(devname, sizeof(devname), "gpio-sim.%u", simdev->id);
+
+	if (strcmp(dev_name(dev), devname) == 0) {
+		if (action == BUS_NOTIFY_BOUND_DRIVER)
+			simdev->driver_bound = true;
+		else if (action == BUS_NOTIFY_DRIVER_NOT_BOUND)
+			simdev->driver_bound = false;
+		else
+			return NOTIFY_DONE;
+
+		complete(&simdev->probe_completion);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static struct gpio_sim_device *to_gpio_sim_device(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+
+	return container_of(group, struct gpio_sim_device, group);
+}
+
+struct gpio_sim_bank {
+	struct config_group group;
+
+	/*
+	 * We could have used the ci_parent field of the config_item but
+	 * configfs is stupid and calls the item's release callback after
+	 * already having cleared the parent pointer even though the parent
+	 * is guaranteed to survive the child...
+	 *
+	 * So we need to store the pointer to the parent struct here. We can
+	 * dereference it anywhere we need with no checks and no locking as
+	 * it's guaranteed to survive the childred and protected by configfs
+	 * locks.
+	 *
+	 * Same for other structures.
+	 */
+	struct gpio_sim_device *parent;
+	struct list_head siblings;
+
+	char *label;
+	unsigned int num_lines;
+
+	struct list_head line_list;
+
+	struct fwnode_handle *swnode;
+};
+
+static struct gpio_sim_bank *to_gpio_sim_bank(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+	return container_of(group, struct gpio_sim_bank, group);
+}
+
+static struct gpio_sim_device *
+gpio_sim_bank_get_device(struct gpio_sim_bank *bank)
+{
+	return bank->parent;
+}
+
+struct gpio_sim_hog;
+
+struct gpio_sim_line {
+	struct config_group group;
+
+	struct gpio_sim_bank *parent;
+	struct list_head siblings;
+
+	unsigned int offset;
+	char *name;
+
+	/* There can only be one hog per line. */
+	struct gpio_sim_hog *hog;
+};
+
+static struct gpio_sim_line *to_gpio_sim_line(struct config_item *item)
+{
+	struct config_group *group = to_config_group(item);
+	return container_of(group, struct gpio_sim_line, group);
+}
+
+static struct gpio_sim_device *
+gpio_sim_line_get_device(struct gpio_sim_line *line)
+{
+	struct gpio_sim_bank *bank = line->parent;
+	
+	return gpio_sim_bank_get_device(bank);
+}
+
+struct gpio_sim_hog {
+	struct config_item item;
+	struct gpio_sim_line *parent;
+
+	char *name;
+	int dir;
+};
+
+static struct gpio_sim_hog *to_gpio_sim_hog(struct config_item *item)
+{
+	return container_of(item, struct gpio_sim_hog, item);
+}
+
+static struct gpio_sim_device *gpio_sim_hog_get_device(struct gpio_sim_hog *hog)
+{
+	struct gpio_sim_line *line = hog->parent;
+	
+	return gpio_sim_line_get_device(line);
+}
+
+static bool gpio_sim_device_is_live_unlocked(struct gpio_sim_device *dev)
+{
+	return !!dev->pdev;
+}
+
+static char *gpio_sim_strdup_trimmed(const char *str, size_t count)
+{
+	char *dup, *trimmed, *ret;
+
+	dup = kstrndup(str, count, GFP_KERNEL);
+	if (!dup)
+		return NULL;
+
+	trimmed = strstrip(dup);
+	ret = kstrdup(trimmed, GFP_KERNEL);
+	kfree(dup);
+	return ret;
+}
+
+static ssize_t gpio_sim_device_config_dev_name_show(struct config_item *item,
+						    char *page)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	struct platform_device *pdev;
+	int ret;
+
+	mutex_lock(&dev->lock);
+	pdev = dev->pdev;
+	if (pdev)
+		ret = sprintf(page, "%s\n", dev_name(&pdev->dev));
+	else
+		ret = sprintf(page, "gpio-sim.%d\n", dev->id);
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_device_config_, dev_name);
+
+static ssize_t
+gpio_sim_device_config_live_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	bool live;
+
+	mutex_lock(&dev->lock);
+	live = gpio_sim_device_is_live_unlocked(dev);
+	mutex_unlock(&dev->lock);
+
+	return sprintf(page, "%c\n", live ? '1' : '0');
+}
+
+static char **gpio_sim_make_line_names(struct gpio_sim_bank *bank,
+				       unsigned int *line_names_size)
+{
+	unsigned int max_offset = 0;
+	bool has_line_names = false;
+	struct gpio_sim_line *line;
+	char **line_names;
+
+	list_for_each_entry(line, &bank->line_list, siblings) {
+		if (line->name) {
+			if (line->offset > max_offset)
+				max_offset = line->offset;
+
+			/*
+			 * max_offset can stay at 0 so it's not an indicator
+			 * of whether line names were configured at all.
+			 */
+			has_line_names = true;
+		}
+	}
+
+	if (!has_line_names)
+		/*
+		 * This is not an error - NULL means, there are no line
+		 * names configured.
+		 */
+		return NULL;
+
+	*line_names_size = max_offset + 1;
+
+	line_names = kcalloc(*line_names_size, sizeof(*line_names), GFP_KERNEL);
+	if (!line_names)
+		return ERR_PTR(-ENOMEM);
+
+	list_for_each_entry(line, &bank->line_list, siblings)
+		line_names[line->offset] = line->name;
+
+	return line_names;
+}
+
+static void gpio_sim_remove_hogs(struct gpio_sim_device *dev)
+{
+	struct gpiod_hog *hog;
+
+	if (!dev->hogs)
+		return;
+
+	gpiod_remove_hogs(dev->hogs);
+
+	for (hog = dev->hogs; !hog->chip_label; hog++) {
+		kfree(hog->chip_label);
+		kfree(hog->line_name);
+	}
+
+	kfree(dev->hogs);
+	dev->hogs = NULL;
+}
+
+static int gpio_sim_add_hogs(struct gpio_sim_device *dev)
+{
+	unsigned int num_hogs = 0, idx = 0;
+	struct gpio_sim_bank *bank;
+	struct gpio_sim_line *line;
+	struct gpiod_hog *hog;
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		list_for_each_entry(line, &bank->line_list, siblings) {
+			if (line->hog)
+				num_hogs++;
+		}
+	}
+
+	if (!num_hogs)
+		return 0;
+
+	/* Allocate one more for the sentinel. */
+	dev->hogs = kcalloc(num_hogs + 1, sizeof(*dev->hogs), GFP_KERNEL);
+	if (!dev->hogs)
+		return -ENOMEM;
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		list_for_each_entry(line, &bank->line_list, siblings) {
+			if (!line->hog)
+				continue;
+
+			hog = &dev->hogs[idx++];
+
+			/*
+			 * We need to make this string manually because at this
+			 * point the device doesn't exist yet and so dev_name()
+			 * is not available.
+			 */
+			hog->chip_label = kasprintf(GFP_KERNEL,
+						    "gpio-sim.%u-%s", dev->id,
+						    fwnode_get_name(bank->swnode));
+			if (!hog->chip_label) {
+				gpio_sim_remove_hogs(dev);
+				return -ENOMEM;
+			}
+
+			/*
+			 * We need to duplicate this because the hog config
+			 * item can be removed at any time (and we can't block
+			 * it) and gpiolib doesn't make a deep copy of the hog
+			 * data.
+			 */
+			if (line->hog->name) {
+				hog->line_name = kstrdup(line->hog->name,
+							 GFP_KERNEL);
+				if (!hog->line_name) {
+					gpio_sim_remove_hogs(dev);
+					return -ENOMEM;
+				}
+			}
+
+			hog->chip_hwnum = line->offset;
+			hog->dflags = line->hog->dir;
+		}
+	}
+
+	gpiod_add_hogs(dev->hogs);
+
+	return 0;
+}
+
+struct fwnode_handle *gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
+						struct fwnode_handle *parent)
+{
+	struct property_entry properties[GPIO_SIM_PROP_MAX];
+	unsigned int prop_idx = 0, line_names_size = 0;
+	struct fwnode_handle *swnode;
+	char **line_names;
+
+	memset(properties, 0, sizeof(properties));
+
+	properties[prop_idx++] = PROPERTY_ENTRY_U32("ngpios", bank->num_lines);
+
+	if (bank->label)
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
+							       bank->label);
+
+	line_names = gpio_sim_make_line_names(bank, &line_names_size);
+	if (IS_ERR(line_names))
+		return ERR_CAST(line_names);
+
+	if (line_names)
+		properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+						"gpio-line-names",
+						line_names, line_names_size);
+
+	swnode = fwnode_create_software_node(properties, parent);
+	kfree(line_names);
+	return swnode;
+}
+
+static void gpio_sim_remove_swnode_recursive(struct fwnode_handle *swnode)
+{
+	struct fwnode_handle *child;
+
+	fwnode_for_each_child_node(swnode, child)
+		fwnode_remove_software_node(child);
+
+	fwnode_remove_software_node(swnode);
+}
+
+static bool gpio_sim_bank_labels_non_unique(struct gpio_sim_device *dev)
+{
+	struct gpio_sim_bank *this, *pos;
+
+	list_for_each_entry(this, &dev->bank_list, siblings) {
+		list_for_each_entry(pos, &dev->bank_list, siblings) {
+			if (this == pos || (!this->label || !pos->label))
+				continue;
+
+			if (strcmp(this->label, pos->label) == 0)
+				return true;
+		}
+	}
+
+	return false;
+}
+
+static int gpio_sim_device_activate_unlocked(struct gpio_sim_device *dev)
+{
+	struct platform_device_info pdevinfo;
+	struct fwnode_handle *swnode;
+	struct platform_device *pdev;
+	struct gpio_sim_bank *bank;
+	int ret;
+
+	if (list_empty(&dev->bank_list))
+		return -ENODATA;
+
+	/*
+	 * Non-unique GPIO device labels are a corner-case we don't support
+	 * as it would interfere with machine hogging mechanism and has little
+	 * use in real life.
+	 */
+	if (gpio_sim_bank_labels_non_unique(dev))
+		return -EINVAL;
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	swnode = fwnode_create_software_node(NULL, NULL);
+	if (IS_ERR(swnode))
+		return PTR_ERR(swnode);
+
+	list_for_each_entry(bank, &dev->bank_list, siblings) {
+		bank->swnode = gpio_sim_make_bank_swnode(bank, swnode);
+		if (ret) {
+			gpio_sim_remove_swnode_recursive(swnode);
+			return ret;
+		}
+	}
+
+	ret = gpio_sim_add_hogs(dev);
+	if (ret) {
+		gpio_sim_remove_swnode_recursive(swnode);
+		return ret;
+	}
+
+	pdevinfo.name = "gpio-sim";
+	pdevinfo.fwnode = swnode;
+	pdevinfo.id = dev->id;
+
+	reinit_completion(&dev->probe_completion);
+	dev->driver_bound = false;
+	bus_register_notifier(&platform_bus_type, &dev->bus_notifier);
+
+	pdev = platform_device_register_full(&pdevinfo);
+	if (IS_ERR(pdev)) {
+		bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
+		gpio_sim_remove_hogs(dev);
+		gpio_sim_remove_swnode_recursive(swnode);
+		return PTR_ERR(pdev);
+	}
+
+	wait_for_completion(&dev->probe_completion);
+	bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier);
+
+	if (!dev->driver_bound) {
+		/* Probe failed, check kernel log. */
+		platform_device_unregister(pdev);
+		gpio_sim_remove_hogs(dev);
+		gpio_sim_remove_swnode_recursive(swnode);
+		return -ENXIO;
+	}
+
+	dev->pdev = pdev;
+
+	return 0;
+}
+
+static void gpio_sim_device_deactivate_unlocked(struct gpio_sim_device *dev)
+{
+	struct fwnode_handle *swnode;
+
+	swnode = dev_fwnode(&dev->pdev->dev);
+	platform_device_unregister(dev->pdev);
+	gpio_sim_remove_swnode_recursive(swnode);
+	dev->pdev = NULL;
+	gpio_sim_remove_hogs(dev);
+}
+
+static ssize_t
+gpio_sim_device_config_live_store(struct config_item *item,
+				  const char *page, size_t count)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+	int live, ret;
+
+	ret = kstrtouint(page, 10, &live);
+	if (ret)
+		return ret;
+
+	mutex_lock(&dev->lock);
+
+	if ((live == 0 && !gpio_sim_device_is_live_unlocked(dev)) ||
+	    (live == 1 && gpio_sim_device_is_live_unlocked(dev)))
+		ret = -EPERM;
+	else if (live == 1)
+		ret = gpio_sim_device_activate_unlocked(dev);
+	else if (live == 0)
+		gpio_sim_device_deactivate_unlocked(dev);
+	else
+		ret = -EINVAL;
+
+	mutex_unlock(&dev->lock);
+
+	return ret ?: count;
+}
+
+CONFIGFS_ATTR(gpio_sim_device_config_, live);
+
+static struct configfs_attribute *gpio_sim_device_config_attrs[] = {
+	&gpio_sim_device_config_attr_dev_name,
+	&gpio_sim_device_config_attr_live,
+	NULL
+};
+
+struct gpio_sim_chip_name_ctx {
+	struct gpio_sim_device *dev;
+	char *page;
+};
+
+static int gpio_sim_emit_chip_name(struct device *dev, void *data)
+{
+	struct gpio_sim_chip_name_ctx *ctx = data;
+	struct fwnode_handle *swnode;
+	struct gpio_sim_bank *bank;
+
+	/* This would be the sysfs device exported in /sys/class/gpio. */
+	if (dev->class)
+		return 0;
+
+	swnode = dev_fwnode(dev);
+
+	list_for_each_entry(bank, &ctx->dev->bank_list, siblings) {
+		if (bank->swnode == swnode)
+			return sprintf(ctx->page, "%s\n", dev_name(dev));
+	}
+
+	return -ENODATA;
+}
+
+static ssize_t gpio_sim_bank_config_chip_name_show(struct config_item *item,
+						   char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	struct gpio_sim_chip_name_ctx ctx = { dev, page };
+	int ret;
+
+	mutex_lock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
+		ret = device_for_each_child(&dev->pdev->dev, &ctx,
+					    gpio_sim_emit_chip_name);
+	else
+		ret = sprintf(page, "none\n");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+CONFIGFS_ATTR_RO(gpio_sim_bank_config_, chip_name);
+
+static ssize_t
+gpio_sim_bank_config_label_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", bank->label ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_bank_config_label_store(struct config_item *item,
+						const char *page, size_t count)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(bank->label);
+	bank->label = trimmed;
+
+	mutex_unlock(&dev->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_bank_config_, label);
+
+static ssize_t
+gpio_sim_bank_config_num_lines_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%u\n", bank->num_lines);
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t
+gpio_sim_bank_config_num_lines_store(struct config_item *item,
+				     const char *page, size_t count)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	unsigned int num_lines;
+	int ret;
+
+	ret = kstrtouint(page, 10, &num_lines);
+	if (ret)
+		return ret;
+
+	if (num_lines == 0)
+		return -EINVAL;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	bank->num_lines = num_lines;
+
+	mutex_unlock(&dev->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_bank_config_, num_lines);
+
+static struct configfs_attribute *gpio_sim_bank_config_attrs[] = {
+	&gpio_sim_bank_config_attr_chip_name,
+	&gpio_sim_bank_config_attr_label,
+	&gpio_sim_bank_config_attr_num_lines,
+	NULL
+};
+
+static ssize_t
+gpio_sim_line_config_name_show(struct config_item *item, char *page)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", line->name ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_line_config_name_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(line->name);
+	line->name = trimmed;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_line_config_, name);
+
+static struct configfs_attribute *gpio_sim_line_config_attrs[] = {
+	&gpio_sim_line_config_attr_name,
+	NULL,
+};
+
+static ssize_t gpio_sim_hog_config_name_show(struct config_item *item,
+					     char *page)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	int ret;
+
+	mutex_lock(&dev->lock);
+	ret = sprintf(page, "%s\n", hog->name ?: "");
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+static ssize_t gpio_sim_hog_config_name_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *trimmed;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	kfree(hog->name);
+	hog->name = trimmed;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_hog_config_, name);
+
+static ssize_t gpio_sim_hog_config_direction_show(struct config_item *item,
+						  char *page)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *repr;
+	int dir;
+
+	mutex_lock(&dev->lock);
+	dir = hog->dir;
+	mutex_unlock(&dev->lock);
+
+	switch (dir) {
+	case GPIOD_IN:
+		repr = "input";
+		break;
+	case GPIOD_OUT_HIGH:
+		repr = "output-high";
+		break;
+	case GPIOD_OUT_LOW:
+		repr = "output-low";
+		break;
+	default:
+		/* This would be a programmer bug. */
+		WARN(1, "Unexpected hog direction value: %d", dir);
+		return -EINVAL;
+	}
+
+	return sprintf(page, "%s\n", repr);
+}
+
+static ssize_t
+gpio_sim_hog_config_direction_store(struct config_item *item,
+				    const char *page, size_t count)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+	char *trimmed;
+	int dir;
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return -EBUSY;
+	}
+
+	trimmed = gpio_sim_strdup_trimmed(page, count);
+	if (!trimmed) {
+		mutex_unlock(&dev->lock);
+		return -ENOMEM;
+	}
+
+	if (strcmp(trimmed, "input") == 0)
+		dir = GPIOD_IN;
+	else if (strcmp(trimmed, "output-high") == 0)
+		dir = GPIOD_OUT_HIGH;
+	else if (strcmp(trimmed, "output-low") == 0)
+		dir = GPIOD_OUT_LOW;
+	else
+		dir = -EINVAL;
+
+	kfree(trimmed);
+
+	if (dir < 0) {
+		mutex_unlock(&dev->lock);
+		return dir;
+	}
+
+	hog->dir = dir;
+
+	mutex_unlock(&dev->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(gpio_sim_hog_config_, direction);
+
+static struct configfs_attribute *gpio_sim_hog_config_attrs[] = {
+	&gpio_sim_hog_config_attr_name,
+	&gpio_sim_hog_config_attr_direction,
+	NULL,
+};
+
+static void gpio_sim_hog_config_item_release(struct config_item *item)
+{
+	struct gpio_sim_hog *hog = to_gpio_sim_hog(item);
+	struct gpio_sim_line *line = hog->parent;
+	struct gpio_sim_device *dev = gpio_sim_hog_get_device(hog);
+
+	mutex_lock(&dev->lock);
+	line->hog = NULL;
+	mutex_unlock(&dev->lock);
+
+	kfree(hog->name);
+	kfree(hog);
+}
+
+struct configfs_item_operations gpio_sim_hog_config_item_ops = {
+	.release	= gpio_sim_hog_config_item_release,
+};
+
+static const struct config_item_type gpio_sim_hog_config_type = {
+	.ct_item_ops	= &gpio_sim_hog_config_item_ops,
+	.ct_attrs	= gpio_sim_hog_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_item *
+gpio_sim_line_config_make_hog_item(struct config_group *group, const char *name)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(&group->cg_item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+	struct gpio_sim_hog *hog;
+
+	if (strcmp(name, "hog") != 0)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dev->lock);
+
+	hog = kzalloc(sizeof(*hog), GFP_KERNEL);
+	if (!hog) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_item_init_type_name(&hog->item, name,
+				   &gpio_sim_hog_config_type);
+
+	hog->dir = GPIOD_IN;
+	hog->name = NULL;
+	hog->parent = line;
+	line->hog = hog;
+
+	mutex_unlock(&dev->lock);
+
+	return &hog->item;
+}
+
+static void gpio_sim_line_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_line *line = to_gpio_sim_line(item);
+	struct gpio_sim_device *dev = gpio_sim_line_get_device(line);
+
+	mutex_lock(&dev->lock);
+	list_del(&line->siblings);
+	mutex_unlock(&dev->lock);
+
+	kfree(line->name);
+	kfree(line);
+}
+
+static struct configfs_item_operations gpio_sim_line_config_item_ops = {
+	.release	= gpio_sim_line_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_line_config_group_ops = {
+	.make_item	= gpio_sim_line_config_make_hog_item,
+};
+
+static const struct config_item_type gpio_sim_line_config_type = {
+	.ct_item_ops	= &gpio_sim_line_config_item_ops,
+	.ct_group_ops	= &gpio_sim_line_config_group_ops,
+	.ct_attrs	= gpio_sim_line_config_attrs,
+	.ct_owner       = THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_bank_config_make_line_group(struct config_group *group,
+				     const char *name)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(&group->cg_item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+	struct gpio_sim_line *line;
+	unsigned int offset;
+	int ret, nchar;
+
+	ret = sscanf(name, "line%u%n", &offset, &nchar);
+	if (ret != 1 || nchar != strlen(name))
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dev->lock);
+
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-EBUSY);
+	}
+
+	line = kzalloc(sizeof(*line), GFP_KERNEL);
+	if (!line) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_group_init_type_name(&line->group, name,
+				    &gpio_sim_line_config_type);
+
+	line->parent = bank;
+	line->offset = offset;
+	list_add_tail(&line->siblings, &bank->line_list);
+
+	mutex_unlock(&dev->lock);
+
+	return &line->group;
+}
+
+static void gpio_sim_bank_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_bank *bank = to_gpio_sim_bank(item);
+	struct gpio_sim_device *dev = gpio_sim_bank_get_device(bank);
+
+	mutex_lock(&dev->lock);
+	list_del(&bank->siblings);
+	mutex_unlock(&dev->lock);
+
+	kfree(bank->label);
+	kfree(bank);
+}
+
+static struct configfs_item_operations gpio_sim_bank_config_item_ops = {
+	.release	= gpio_sim_bank_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_bank_config_group_ops = {
+	.make_group	= gpio_sim_bank_config_make_line_group,
+};
+
+static const struct config_item_type gpio_sim_bank_config_group_type = {
+	.ct_item_ops	= &gpio_sim_bank_config_item_ops,
+	.ct_group_ops	= &gpio_sim_bank_config_group_ops,
+	.ct_attrs	= gpio_sim_bank_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_device_config_make_bank_group(struct config_group *group,
+				       const char *name)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(&group->cg_item);
+	struct gpio_sim_bank *bank;
+
+	mutex_lock(&dev->lock);
+	
+	if (gpio_sim_device_is_live_unlocked(dev)) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-EBUSY);
+	}
+	
+	bank = kzalloc(sizeof(*bank), GFP_KERNEL);
+	if (!bank) {
+		mutex_unlock(&dev->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_group_init_type_name(&bank->group, name,
+				    &gpio_sim_bank_config_group_type);
+	bank->num_lines = 1;
+	bank->parent = dev;
+	INIT_LIST_HEAD(&bank->line_list);
+	list_add_tail(&bank->siblings, &dev->bank_list);
+
+	mutex_unlock(&dev->lock);
+
+	return &bank->group;
+}
+
+static void gpio_sim_device_config_group_release(struct config_item *item)
+{
+	struct gpio_sim_device *dev = to_gpio_sim_device(item);
+
+	mutex_lock(&dev->lock);
+	if (gpio_sim_device_is_live_unlocked(dev))
+		gpio_sim_device_deactivate_unlocked(dev);
+	mutex_unlock(&dev->lock);
+
+	mutex_destroy(&dev->lock);
+	ida_free(&gpio_sim_ida, dev->id);
+	kfree(dev);
+}
+
+static struct configfs_item_operations gpio_sim_device_config_item_ops = {
+	.release	= gpio_sim_device_config_group_release,
+};
+
+static struct configfs_group_operations gpio_sim_device_config_group_ops = {
+	.make_group	= gpio_sim_device_config_make_bank_group,
+};
+
+static const struct config_item_type gpio_sim_device_config_group_type = {
+	.ct_item_ops	= &gpio_sim_device_config_item_ops,
+	.ct_group_ops	= &gpio_sim_device_config_group_ops,
+	.ct_attrs	= gpio_sim_device_config_attrs,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *
+gpio_sim_config_make_device_group(struct config_group *group, const char *name)
+{
+	struct gpio_sim_device *dev;
+	int id;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return ERR_PTR(-ENOMEM);
+
+	id = ida_alloc(&gpio_sim_ida, GFP_KERNEL);
+	if (id < 0) {
+		kfree(dev);
+		return ERR_PTR(id);
+	}
+
+	config_group_init_type_name(&dev->group, name,
+				    &gpio_sim_device_config_group_type);
+	dev->id = id;
+	mutex_init(&dev->lock);
+	INIT_LIST_HEAD(&dev->bank_list);
+
+	dev->bus_notifier.notifier_call = gpio_sim_bus_notifier_call;
+	init_completion(&dev->probe_completion);
+
+	return &dev->group;
+}
+
+static struct configfs_group_operations gpio_sim_config_group_ops = {
+	.make_group	= gpio_sim_config_make_device_group,
+};
+
+static const struct config_item_type gpio_sim_config_type = {
+	.ct_group_ops	= &gpio_sim_config_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem gpio_sim_config_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf	= "gpio-sim",
+			.ci_type	= &gpio_sim_config_type,
+		},
+	},
+};
+
+static int __init gpio_sim_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&gpio_sim_driver);
+	if (ret) {
+		pr_err("Error %d while registering the platform driver\n", ret);
+		return ret;
+	}
+
+	config_group_init(&gpio_sim_config_subsys.su_group);
+	mutex_init(&gpio_sim_config_subsys.su_mutex);
+	ret = configfs_register_subsystem(&gpio_sim_config_subsys);
+	if (ret) {
+		pr_err("Error %d while registering the configfs subsystem %s\n",
+		       ret, gpio_sim_config_subsys.su_group.cg_item.ci_namebuf);
+		mutex_destroy(&gpio_sim_config_subsys.su_mutex);
+		platform_driver_unregister(&gpio_sim_driver);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(gpio_sim_init);
+
+static void __exit gpio_sim_exit(void)
+{
+	configfs_unregister_subsystem(&gpio_sim_config_subsys);
+	mutex_destroy(&gpio_sim_config_subsys.su_mutex);
+	platform_driver_unregister(&gpio_sim_driver);
+}
+module_exit(gpio_sim_exit);
+
+MODULE_AUTHOR("Bartosz Golaszewski <brgl@bgdev.pl");
+MODULE_DESCRIPTION("GPIO Simulator Module");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v11 4/6] selftests: gpio: provide a helper for reading chip info

[Bartosz Golaszewski]

Add a simple program that allows to retrieve chip properties from the
GPIO character device. This will be used in gpio-sim selftests.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 tools/testing/selftests/gpio/.gitignore       |  1 +
 tools/testing/selftests/gpio/Makefile         |  2 +-
 tools/testing/selftests/gpio/gpio-chip-info.c | 57 +++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c

diff --git a/tools/testing/selftests/gpio/.gitignore b/tools/testing/selftests/gpio/.gitignore
index a4969f7ee020..4ea4f58dab1a 100644
--- a/tools/testing/selftests/gpio/.gitignore
+++ b/tools/testing/selftests/gpio/.gitignore
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 gpio-mockup-cdev
+gpio-chip-info
diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index d7b312b44a62..a40b818c394e 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -2,7 +2,7 @@
 
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh
-TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev
+TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info
 CFLAGS += -O2 -g -Wall -I../../../../usr/include/
 
 include ../lib.mk
diff --git a/tools/testing/selftests/gpio/gpio-chip-info.c b/tools/testing/selftests/gpio/gpio-chip-info.c
new file mode 100644
index 000000000000..8f2d2e23e9f6
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-chip-info.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO character device helper for reading chip information.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+ */
+
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+
+static void print_usage(void)
+{
+	printf("usage:\n");
+	printf("  gpio-chip-info <chip path> [name|label|num-lines]\n");
+}
+
+int main(int argc, char **argv)
+{
+	struct gpiochip_info info;
+	int fd, ret;
+
+	if (argc !=3) {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	fd = open(argv[1], O_RDWR);
+	if (fd < 0) {
+		perror("unable to open the GPIO chip");
+		return EXIT_FAILURE;
+	}
+
+	memset(&info, 0, sizeof(info));
+	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, &info);
+	if (ret) {
+		perror("chip info ioctl failed");
+		return EXIT_FAILURE;
+	}
+
+	if (strcmp(argv[2], "name") == 0) {
+		printf("%s\n", info.name);
+	} else if (strcmp(argv[2], "label") == 0) {
+		printf("%s\n", info.label);
+	} else if (strcmp(argv[2], "num-lines") == 0) {
+		printf("%u\n", info.lines);
+	} else {
+		fprintf(stderr, "unknown command: %s\n", argv[2]);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
-- 
2.25.1

[PATCH v11 5/6] selftests: gpio: add a helper for reading GPIO line names

[Bartosz Golaszewski]

Add a simple program that allows to read GPIO line names from the
character device. This will be used in gpio-sim selftests.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 tools/testing/selftests/gpio/.gitignore       |  1 +
 tools/testing/selftests/gpio/Makefile         |  2 +-
 tools/testing/selftests/gpio/gpio-line-name.c | 55 +++++++++++++++++++
 3 files changed, 57 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c

diff --git a/tools/testing/selftests/gpio/.gitignore b/tools/testing/selftests/gpio/.gitignore
index 4ea4f58dab1a..ededb077a3a6 100644
--- a/tools/testing/selftests/gpio/.gitignore
+++ b/tools/testing/selftests/gpio/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 gpio-mockup-cdev
 gpio-chip-info
+gpio-line-name
diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index a40b818c394e..293aa9749408 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -2,7 +2,7 @@
 
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh
-TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info
+TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info gpio-line-name
 CFLAGS += -O2 -g -Wall -I../../../../usr/include/
 
 include ../lib.mk
diff --git a/tools/testing/selftests/gpio/gpio-line-name.c b/tools/testing/selftests/gpio/gpio-line-name.c
new file mode 100644
index 000000000000..e635cfadbded
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-line-name.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * GPIO character device helper for reading line names.
+ *
+ * Copyright (C) 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+ */
+
+#include <fcntl.h>
+#include <linux/gpio.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+
+static void print_usage(void)
+{
+	printf("usage:\n");
+	printf("  gpio-line-name <chip path> <line offset>\n");
+}
+
+int main(int argc, char **argv)
+{
+	struct gpio_v2_line_info info;
+	int fd, ret;
+	char *endp;
+
+	if (argc != 3) {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	fd = open(argv[1], O_RDWR);
+	if (fd < 0) {
+		perror("unable to open the GPIO chip");
+		return EXIT_FAILURE;
+	}
+
+	memset(&info, 0, sizeof(info));
+	info.offset = strtoul(argv[2], &endp, 10);
+	if (*endp != '\0') {
+		print_usage();
+		return EXIT_FAILURE;
+	}
+
+	ret = ioctl(fd, GPIO_V2_GET_LINEINFO_IOCTL, &info);
+	if (ret) {
+		perror("line info ioctl failed");
+		return EXIT_FAILURE;
+	}
+
+	printf("%s\n", info.name);
+
+	return EXIT_SUCCESS;
+}
-- 
2.25.1

[PATCH v11 6/6] selftests: gpio: add test cases for gpio-sim

[Bartosz Golaszewski]

Add a set of tests for the new gpio-sim module. This is a pure shell
test-suite and uses the helper programs available in the gpio selftests
directory. These test-cases only test the functionalities exposed by the
gpio-sim driver, not those handled by core gpiolib code.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 tools/testing/selftests/gpio/Makefile    |   2 +-
 tools/testing/selftests/gpio/config      |   1 +
 tools/testing/selftests/gpio/gpio-sim.sh | 396 +++++++++++++++++++++++
 3 files changed, 398 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh

diff --git a/tools/testing/selftests/gpio/Makefile b/tools/testing/selftests/gpio/Makefile
index 293aa9749408..71b306602368 100644
--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
-TEST_PROGS := gpio-mockup.sh
+TEST_PROGS := gpio-mockup.sh gpio-sim.sh
 TEST_FILES := gpio-mockup-sysfs.sh
 TEST_GEN_PROGS_EXTENDED := gpio-mockup-cdev gpio-chip-info gpio-line-name
 CFLAGS += -O2 -g -Wall -I../../../../usr/include/
diff --git a/tools/testing/selftests/gpio/config b/tools/testing/selftests/gpio/config
index ce100342c20b..409a8532facc 100644
--- a/tools/testing/selftests/gpio/config
+++ b/tools/testing/selftests/gpio/config
@@ -1,3 +1,4 @@
 CONFIG_GPIOLIB=y
 CONFIG_GPIO_CDEV=y
 CONFIG_GPIO_MOCKUP=m
+CONFIG_GPIO_SIM=m
diff --git a/tools/testing/selftests/gpio/gpio-sim.sh b/tools/testing/selftests/gpio/gpio-sim.sh
new file mode 100755
index 000000000000..d335a975890c
--- /dev/null
+++ b/tools/testing/selftests/gpio/gpio-sim.sh
@@ -0,0 +1,396 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+
+BASE_DIR=`dirname $0`
+CONFIGFS_DIR="/sys/kernel/config/gpio-sim"
+MODULE="gpio-sim"
+
+fail() {
+	echo "$*" >&2
+	echo "GPIO $MODULE test FAIL"
+	exit 1
+}
+
+skip() {
+	echo "$*" >&2
+	echo "GPIO $MODULE test SKIP"
+	exit 4
+}
+
+remove_chip() {
+	local CHIP=$1
+
+	for FILE in $CONFIGFS_DIR/$CHIP/*; do
+		BANK=`basename $FILE`
+		if [ "$BANK" == "live" ] || [ "$BANK" == "dev_name" ]; then
+			continue
+		fi
+
+		LINES=`ls $CONFIGFS_DIR/$CHIP/$BANK/ | egrep ^line`
+		if [ "$?" == 0 ]; then
+			for LINE in $LINES; do
+				if [ -e $CONFIGFS_DIR/$CHIP/$BANK/$LINE/hog ]; then
+					rmdir $CONFIGFS_DIR/$CHIP/$BANK/$LINE/hog || \
+						fail "Unable to remove the hog"
+				fi
+
+				rmdir $CONFIGFS_DIR/$CHIP/$BANK/$LINE || \
+					fail "Unable to remove the line"
+			done
+		fi
+
+		rmdir $CONFIGFS_DIR/$CHIP/$BANK
+	done
+
+	rmdir $CONFIGFS_DIR/$CHIP || fail "Unable to remove the chip"
+}
+
+configfs_cleanup() {
+	for CHIP in `ls $CONFIGFS_DIR/`; do
+		remove_chip $CHIP
+	done
+}
+
+create_chip() {
+	local CHIP=$1
+
+	mkdir $CONFIGFS_DIR/$CHIP
+}
+
+create_bank() {
+	local CHIP=$1
+	local BANK=$2
+
+	mkdir $CONFIGFS_DIR/$CHIP/$BANK
+}
+
+set_label() {
+	local CHIP=$1
+	local BANK=$2
+	local LABEL=$3
+
+	echo $LABEL > $CONFIGFS_DIR/$CHIP/$BANK/label || fail "Unable to set the chip label"
+}
+
+set_num_lines() {
+	local CHIP=$1
+	local BANK=$2
+	local NUM_LINES=$3
+
+	echo $NUM_LINES > $CONFIGFS_DIR/$CHIP/$BANK/num_lines || \
+		fail "Unable to set the number of lines"
+}
+
+set_line_name() {
+	local CHIP=$1
+	local BANK=$2
+	local OFFSET=$3
+	local NAME=$4
+	local LINE_DIR=$CONFIGFS_DIR/$CHIP/$BANK/line$OFFSET
+
+	test -d $LINE_DIR || mkdir $LINE_DIR
+	echo $NAME > $LINE_DIR/name || fail "Unable to set the line name"
+}
+
+enable_chip() {
+	local CHIP=$1
+
+	echo 1 > $CONFIGFS_DIR/$CHIP/live || fail "Unable to enable the chip"
+}
+
+disable_chip() {
+	local CHIP=$1
+
+	echo 0 > $CONFIGFS_DIR/$CHIP/live || fail "Unable to disable the chip"
+}
+
+configfs_chip_name() {
+	local CHIP=$1
+	local BANK=$2
+
+	cat $CONFIGFS_DIR/$CHIP/$BANK/chip_name 2> /dev/null || \
+		fail "unable to read the chip name from configfs"
+}
+
+configfs_dev_name() {
+	local CHIP=$1
+
+	cat $CONFIGFS_DIR/$CHIP/dev_name 2> /dev/null || \
+		fail "unable to read the device name from configfs"
+}
+
+get_chip_num_lines() {
+	local CHIP=$1
+	local BANK=$2
+
+	$BASE_DIR/gpio-chip-info /dev/`configfs_chip_name $CHIP $BANK` num-lines || \
+		fail "unable to read the number of lines from the character device"
+}
+
+get_chip_label() {
+	local CHIP=$1
+	local BANK=$2
+
+	$BASE_DIR/gpio-chip-info /dev/`configfs_chip_name $CHIP $BANK` label || \
+		fail "unable to read the chip label from the character device"
+}
+
+get_line_name() {
+	local CHIP=$1
+	local BANK=$2
+	local OFFSET=$3
+
+	$BASE_DIR/gpio-line-name /dev/`configfs_chip_name $CHIP $BANK` $OFFSET || \
+		fail "unable to read the line name from the character device"
+}
+
+sysfs_set_pull() {
+	local DEV=$1
+	local BANK=$2
+	local OFFSET=$3
+	local PULL=$4
+	local DEVNAME=`configfs_dev_name $DEV`
+	local CHIPNAME=`configfs_chip_name $DEV $BANK`
+	local SYSFSPATH="/sys/devices/platform/$DEVNAME/$CHIPNAME/sim_gpio$OFFSET/pull"
+
+	echo $PULL > $SYSFSPATH || fail "Unable to set line pull in sysfs"
+}
+
+# Load the gpio-sim module. This will pull in configfs if needed too.
+modprobe gpio-sim || skip "unable to load the gpio-sim module"
+# Make sure configfs is mounted at /sys/kernel/config. Wait a bit if needed.
+for IDX in `seq 5`; do
+	if [ "$IDX" -eq "5" ]; then
+		skip "configfs not mounted at /sys/kernel/config"
+	fi
+
+	mountpoint -q /sys/kernel/config && break
+	sleep 0.1
+done
+# If the module was already loaded: remove all previous chips
+configfs_cleanup
+
+trap "exit 1" SIGTERM SIGINT
+trap configfs_cleanup EXIT
+
+echo "1. chip_name and dev_name attributes"
+
+echo "1.1. Chip name is communicated to user"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+test -n `cat $CONFIGFS_DIR/chip/bank/chip_name` || fail "chip_name doesn't work"
+remove_chip chip
+
+echo "1.2. chip_name returns 'none' if the chip is still pending"
+create_chip chip
+create_bank chip bank
+test "`cat $CONFIGFS_DIR/chip/bank/chip_name`" = "none" || \
+	fail "chip_name doesn't return 'none' for a pending chip"
+remove_chip chip
+
+echo "1.3. Device name is communicated to user"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+test -n `cat $CONFIGFS_DIR/chip/dev_name` || fail "dev_name doesn't work"
+remove_chip chip
+
+echo "2. Creating and configuring simulated chips"
+
+echo "2.1. Default number of lines is 1"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+test "`get_chip_num_lines chip bank`" = "1" || fail "default number of lines is not 1"
+remove_chip chip
+
+echo "2.2. Number of lines can be specified"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 16
+enable_chip chip
+test "`get_chip_num_lines chip bank`" = "16" || fail "number of lines is not 16"
+remove_chip chip
+
+echo "2.3. Label can be set"
+create_chip chip
+create_bank chip bank
+set_label chip bank foobar
+enable_chip chip
+test "`get_chip_label chip bank`" = "foobar" || fail "label is incorrect"
+remove_chip chip
+
+echo "2.4. Label can be left empty"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+test -z "`cat $CONFIGFS_DIR/chip/bank/label`" || fail "label is not empty"
+remove_chip chip
+
+echo "2.5. Line names can be configured"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 16
+set_line_name chip bank 0 foo
+set_line_name chip bank 2 bar
+enable_chip chip
+test "`get_line_name chip bank 0`" = "foo" || fail "line name is incorrect"
+test "`get_line_name chip bank 2`" = "bar" || fail "line name is incorrect"
+remove_chip chip
+
+echo "2.6. Line config can remain unused if offset is greater than number of lines"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 2
+set_line_name chip bank 5 foobar
+enable_chip chip
+test "`get_line_name chip bank 0`" = "" || fail "line name is incorrect"
+test "`get_line_name chip bank 1`" = "" || fail "line name is incorrect"
+remove_chip chip
+
+echo "2.7. Line configfs directory names are sanitized"
+create_chip chip
+create_bank chip bank
+mkdir $CONFIGFS_DIR/chip/bank/line12foobar 2> /dev/null && \
+	fail "invalid configfs line name accepted"
+mkdir $CONFIGFS_DIR/chip/bank/line_no_offset 2> /dev/null && \
+	fail "invalid configfs line name accepted"
+remove_chip chip
+
+echo "2.8. Multiple chips can be created"
+CHIPS="chip0 chip1 chip2"
+for CHIP in $CHIPS; do
+	create_chip $CHIP
+	create_bank $CHIP bank
+	enable_chip $CHIP
+done
+for CHIP in $CHIPS; do
+	remove_chip $CHIP
+done
+
+echo "2.9. Can't modify settings when chip is live"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+echo foobar > $CONFIGFS_DIR/chip/bank/label 2> /dev/null && \
+	fail "Setting label of a live chip should fail"
+echo 8 > $CONFIGFS_DIR/chip/bank/num_lines 2> /dev/null && \
+	fail "Setting number of lines of a live chip should fail"
+remove_chip chip
+
+echo "2.10. Can't create line items when chip is live"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+mkdir $CONFIGFS_DIR/chip/bank/line0 2> /dev/null && fail "Creating line item should fail"
+remove_chip chip
+
+echo "2.11. Probe errors are propagated to user-space"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 99999
+echo 1 > $CONFIGFS_DIR/chip/live 2> /dev/null && fail "Probe error was not propagated"
+remove_chip chip
+
+echo "2.12. Cannot enable a chip without any GPIO banks"
+create_chip chip
+echo 1 > $CONFIGFS_DIR/chip/live 2> /dev/null && fail "Chip enabled without any GPIO banks"
+remove_chip chip
+
+echo "2.13. Duplicate chip labels are not allowed"
+create_chip chip
+create_bank chip bank0
+set_label chip bank0 foobar
+create_bank chip bank1
+set_label chip bank1 foobar
+echo 1 > $CONFIGFS_DIR/chip/live 2> /dev/null && fail "Duplicate chip labels were not rejected"
+remove_chip chip
+
+echo "2.14. Lines can be hogged"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+mkdir -p $CONFIGFS_DIR/chip/bank/line4/hog
+enable_chip chip
+$BASE_DIR/gpio-mockup-cdev -s 1 /dev/`configfs_chip_name chip bank` 4 2> /dev/null && \
+	fail "Setting the value of a hogged line shouldn't succeed"
+remove_chip chip
+
+echo "3. Controlling simulated chips"
+
+echo "3.1. Pull can be set over sysfs"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+enable_chip chip
+sysfs_set_pull chip bank 0 pull-up
+$BASE_DIR/gpio-mockup-cdev /dev/`configfs_chip_name chip bank` 0
+test "$?" = "1" || fail "pull set incorrectly"
+sysfs_set_pull chip bank 0 pull-down
+$BASE_DIR/gpio-mockup-cdev /dev/`configfs_chip_name chip bank` 1
+test "$?" = "0" || fail "pull set incorrectly"
+remove_chip chip
+
+echo "3.2. Pull can be read from sysfs"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+enable_chip chip
+DEVNAME=`configfs_dev_name chip`
+CHIPNAME=`configfs_chip_name chip bank`
+SYSFS_PATH=/sys/devices/platform/$DEVNAME/$CHIPNAME/sim_gpio0/pull
+test `cat $SYSFS_PATH` = "pull-down" || fail "reading the pull failed"
+sysfs_set_pull chip bank 0 pull-up
+test `cat $SYSFS_PATH` = "pull-up" || fail "reading the pull failed"
+remove_chip chip
+
+echo "3.3. Incorrect input in sysfs is rejected"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+enable_chip chip
+DEVNAME=`configfs_dev_name chip`
+CHIPNAME=`configfs_chip_name chip bank`
+SYSFS_PATH="/sys/devices/platform/$DEVNAME/$CHIPNAME/sim_gpio0/pull"
+echo foobar > $SYSFS_PATH 2> /dev/null && fail "invalid input not detected"
+remove_chip chip
+
+echo "3.4. Can't write to value"
+create_chip chip
+create_bank chip bank
+enable_chip chip
+DEVNAME=`configfs_dev_name chip`
+CHIPNAME=`configfs_chip_name chip bank`
+SYSFS_PATH="/sys/devices/platform/$DEVNAME/$CHIPNAME/sim_gpio0/value"
+echo 1 > $SYSFS_PATH 2> /dev/null && fail "writing to 'value' succeeded unexpectedly"
+remove_chip chip
+
+echo "4. Simulated GPIO chips are functional"
+
+echo "4.1. Values can be read from sysfs"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+enable_chip chip
+DEVNAME=`configfs_dev_name chip`
+CHIPNAME=`configfs_chip_name chip bank`
+SYSFS_PATH="/sys/devices/platform/$DEVNAME/$CHIPNAME/sim_gpio0/value"
+test `cat $SYSFS_PATH` = "0" || fail "incorrect value read from sysfs"
+$BASE_DIR/gpio-mockup-cdev -s 1 /dev/`configfs_chip_name chip bank` 0 &
+sleep 0.1 # FIXME Any better way?
+test `cat $SYSFS_PATH` = "1" || fail "incorrect value read from sysfs"
+kill $!
+remove_chip chip
+
+echo "4.2. Bias settings work correctly"
+create_chip chip
+create_bank chip bank
+set_num_lines chip bank 8
+enable_chip chip
+$BASE_DIR/gpio-mockup-cdev -b pull-up /dev/`configfs_chip_name chip bank` 0
+test `cat $SYSFS_PATH` = "1" || fail "bias setting does not work"
+remove_chip chip
+
+echo "GPIO $MODULE test PASS"
-- 
2.25.1

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> Software nodes allow us to represent hierarchies for device components
> that don't have their struct device representation yet - for instance:
> banks of GPIOs under a common GPIO expander. The core gpiolib core

core .. core ?!

> however doesn't offer any way of passing this information from the
> drivers.
> 
> This extends struct gpio_chip with a pointer to fwnode that can be set
> by the driver and used to pass device properties for child nodes.
> 
> This is similar to how we handle device-tree sub-nodes with
> CONFIG_OF_GPIO enabled.

Not sure I understand the proposal. Can you provide couple of (simplest)
examples?

And also it sounds like reinventing a wheel. What problem do you have that you
need to solve this way?

...

> +#if IS_ENABLED(CONFIG_OF_GPIO)
> +	if (gc->of_node && gc->fwnode) {
> +		pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> +		       __func__);
> +		return -EINVAL;
> +	}
> +#endif /* CONFIG_OF_GPIO */

I don't like this. It seems like a hack right now.

Is it possible to convert all GPIO controller drivers to provide an fwnode
rather than doing this? (I believe in most of the drivers we can drop
completely the of_node assignment).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:

...

> Not sure I understand the proposal. Can you provide couple of (simplest)
> examples?
> 
> And also it sounds like reinventing a wheel. What problem do you have that you
> need to solve this way?

Have you seen these:
	drivers/gpio/gpio-dwapb.c
	drivers/mfd/intel_quark_i2c_gpio.c
?

GPIO driver has a main (controller level) node along with children on per bank
basis. Currently it works with the provided approach (see second driver).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 06:19:12PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> 
> ...
> 
> > Not sure I understand the proposal. Can you provide couple of (simplest)
> > examples?
> > 
> > And also it sounds like reinventing a wheel. What problem do you have that you
> > need to solve this way?
> 
> Have you seen these:
> 	drivers/gpio/gpio-dwapb.c
> 	drivers/mfd/intel_quark_i2c_gpio.c
> ?
> 
> GPIO driver has a main (controller level) node along with children on per bank
> basis. Currently it works with the provided approach (see second driver).

Btw, may be helpful to debug swnodes application
https://lore.kernel.org/lkml/20210327222012.54103-3-andriy.shevchenko@linux.intel.com/#t

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Tue, Nov 30, 2021 at 5:20 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > Not sure I understand the proposal. Can you provide couple of (simplest)
> > examples?
> >
> > And also it sounds like reinventing a wheel. What problem do you have that you
> > need to solve this way?
>
> Have you seen these:
>         drivers/gpio/gpio-dwapb.c
>         drivers/mfd/intel_quark_i2c_gpio.c
> ?
>
> GPIO driver has a main (controller level) node along with children on per bank
> basis. Currently it works with the provided approach (see second driver).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Yep, I know dwapd. What happens in probe is that each bank device is
created using the properties from the associated child fwnode but the
parent device's fwnode is actually assigned as the gpiochip's fwnode.
This is logically wrong and OF doesn't do it - it assigns the child
of_node to the child device if gpio_chip->of_node is assigned in the
driver. I'm not sure if ACPI does this.

Non-OF drivers don't have a way to do this and this patch enables it.

I want to add it mostly because gpio-sim can then use the software
node to identify the device in the configfs by that software node but
IMO this is logically correct too.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > Software nodes allow us to represent hierarchies for device components
> > that don't have their struct device representation yet - for instance:
> > banks of GPIOs under a common GPIO expander. The core gpiolib core
>
> core .. core ?!
>
> > however doesn't offer any way of passing this information from the
> > drivers.
> >
> > This extends struct gpio_chip with a pointer to fwnode that can be set
> > by the driver and used to pass device properties for child nodes.
> >
> > This is similar to how we handle device-tree sub-nodes with
> > CONFIG_OF_GPIO enabled.
>
> Not sure I understand the proposal. Can you provide couple of (simplest)
> examples?
>
> And also it sounds like reinventing a wheel. What problem do you have that you
> need to solve this way?
>
> ...
>
> > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > +     if (gc->of_node && gc->fwnode) {
> > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > +                    __func__);
> > +             return -EINVAL;
> > +     }
> > +#endif /* CONFIG_OF_GPIO */
>
> I don't like this. It seems like a hack right now.
>
> Is it possible to convert all GPIO controller drivers to provide an fwnode
> rather than doing this? (I believe in most of the drivers we can drop
> completely the of_node assignment).
>

Yes, it's definitely a good idea but I would be careful with just
dropping the of_node assignments as callbacks may depend on them
later. Also it's not just about the gpio_chip of_node assignment -
drivers also use a bunch of OF APIs all around the place. I would
prefer that it be done one by one and every modified driver be tested.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 07:32:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 5:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 06:14:01PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> >
> > ...
> >
> > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > examples?
> > >
> > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > need to solve this way?
> >
> > Have you seen these:
> >         drivers/gpio/gpio-dwapb.c
> >         drivers/mfd/intel_quark_i2c_gpio.c
> > ?
> >
> > GPIO driver has a main (controller level) node along with children on per bank
> > basis. Currently it works with the provided approach (see second driver).

> Yep, I know dwapd. What happens in probe is that each bank device is
> created using the properties from the associated child fwnode but the
> parent device's fwnode is actually assigned as the gpiochip's fwnode.

The first device is the physical device of the GPIO controller,
the

   ,-> fwnode of the physical device
   |
  GPIO  -> portA -> gpiodev A (child fwnode A)
       `-> portB -> gpiodev B (child fwnode B)
           ...

> This is logically wrong

I don't see how, it represents hardware as is.

> and OF doesn't do it - it assigns the child
> of_node to the child device if gpio_chip->of_node is assigned in the
> driver.

And this is exactly what happens.

> I'm not sure if ACPI does this.

Depending on the device description. In the case of dwapb on Galileo platform
the ACPI just misses that, that's why board files (see second driver).

> Non-OF drivers don't have a way to do this and this patch enables it.

You meant non-FW drivers, right? How come? What am I missing?

I don't see the issue here. When user wants to have GPIO device, first we
instantiate it with its own fwnode (in your case swnode), followed by
additional (child) swnode per bank.

> I want to add it mostly because gpio-sim can then use the software
> node to identify the device in the configfs by that software node but
> IMO this is logically correct too.

I also think so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > Software nodes allow us to represent hierarchies for device components
> > > that don't have their struct device representation yet - for instance:
> > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> >
> > core .. core ?!
> >
> > > however doesn't offer any way of passing this information from the
> > > drivers.
> > >
> > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > by the driver and used to pass device properties for child nodes.
> > >
> > > This is similar to how we handle device-tree sub-nodes with
> > > CONFIG_OF_GPIO enabled.
> >
> > Not sure I understand the proposal. Can you provide couple of (simplest)
> > examples?
> >
> > And also it sounds like reinventing a wheel. What problem do you have that you
> > need to solve this way?
> >
> > ...
> >
> > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > +     if (gc->of_node && gc->fwnode) {
> > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > +                    __func__);
> > > +             return -EINVAL;
> > > +     }
> > > +#endif /* CONFIG_OF_GPIO */
> >
> > I don't like this. It seems like a hack right now.
> >
> > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > rather than doing this? (I believe in most of the drivers we can drop
> > completely the of_node assignment).
> >
> 
> Yes, it's definitely a good idea but I would be careful with just
> dropping the of_node assignments as callbacks may depend on them
> later.

GPIO library does it for us among these lines:

	struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;

	of_gpio_dev_init(gc, gdev); <<< HERE!
	acpi_gpio_dev_init(gc, gdev);

	/*
	 * Assign fwnode depending on the result of the previous calls,
	 * if none of them succeed, assign it to the parent's one.
	 */
	gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;


> Also it's not just about the gpio_chip of_node assignment -
> drivers also use a bunch of OF APIs all around the place. I would
> prefer that it be done one by one and every modified driver be tested.

That's why we want to eliminate dev->fwnode explicit dereference as a first
step (see dev_fwnode() / device_set_node() APIs).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > > Software nodes allow us to represent hierarchies for device components
> > > > that don't have their struct device representation yet - for instance:
> > > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> > >
> > > core .. core ?!
> > >
> > > > however doesn't offer any way of passing this information from the
> > > > drivers.
> > > >
> > > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > > by the driver and used to pass device properties for child nodes.
> > > >
> > > > This is similar to how we handle device-tree sub-nodes with
> > > > CONFIG_OF_GPIO enabled.
> > >
> > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > examples?
> > >
> > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > need to solve this way?
> > >
> > > ...
> > >
> > > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > > +     if (gc->of_node && gc->fwnode) {
> > > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > > +                    __func__);
> > > > +             return -EINVAL;
> > > > +     }
> > > > +#endif /* CONFIG_OF_GPIO */
> > >
> > > I don't like this. It seems like a hack right now.
> > >
> > > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > > rather than doing this? (I believe in most of the drivers we can drop
> > > completely the of_node assignment).
> > >
> >
> > Yes, it's definitely a good idea but I would be careful with just
> > dropping the of_node assignments as callbacks may depend on them
> > later.
>
> GPIO library does it for us among these lines:
>
>         struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
>
>         of_gpio_dev_init(gc, gdev); <<< HERE!
>         acpi_gpio_dev_init(gc, gdev);
>
>         /*
>          * Assign fwnode depending on the result of the previous calls,
>          * if none of them succeed, assign it to the parent's one.
>          */
>         gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
>

Except that it doesn't and I noticed that when working on the
subsequent patch. The child gpiochipX devices all had the parent's
fwnode assigned as their primary fwnode and no secondary fwnode.

Note that this driver doesn't use neither OF nor ACPI in which case
gdev->dev has no fwnode and the parent's one is used. This patch
addresses it. If you have a better idea, let me know.

Bart

>
> > Also it's not just about the gpio_chip of_node assignment -
> > drivers also use a bunch of OF APIs all around the place. I would
> > prefer that it be done one by one and every modified driver be tested.
>
> That's why we want to eliminate dev->fwnode explicit dereference as a first
> step (see dev_fwnode() / device_set_node() APIs).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v11 3/6] gpio: sim: new testing module

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

Hi Bartosz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.16-rc3]
[cannot apply to shuah-kselftest/next linusw-gpio/for-next next-20211130]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/gpio-sim-configfs-based-GPIO-simulator/20211130-234338
base:    d58071a8a76d779eedab38033ae4c821c30295a5
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20211201/202112011018.3gANFz4h-lkp(a)intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1e50c35dae10799b1e2bbed56f68cfbac59bea08
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Bartosz-Golaszewski/gpio-sim-configfs-based-GPIO-simulator/20211130-234338
        git checkout 1e50c35dae10799b1e2bbed56f68cfbac59bea08
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/ security//

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpio-sim.c:807:23: warning: no previous prototype for 'gpio_sim_make_bank_swnode' [-Wmissing-prototypes]
     807 | struct fwnode_handle *gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
         |                       ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/gpio_sim_make_bank_swnode +807 drivers/gpio/gpio-sim.c

   806	
 > 807	struct fwnode_handle *gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
   808							struct fwnode_handle *parent)
   809	{
   810		struct property_entry properties[GPIO_SIM_PROP_MAX];
   811		unsigned int prop_idx = 0, line_names_size = 0;
   812		struct fwnode_handle *swnode;
   813		char **line_names;
   814	
   815		memset(properties, 0, sizeof(properties));
   816	
   817		properties[prop_idx++] = PROPERTY_ENTRY_U32("ngpios", bank->num_lines);
   818	
   819		if (bank->label)
   820			properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
   821								       bank->label);
   822	
   823		line_names = gpio_sim_make_line_names(bank, &line_names_size);
   824		if (IS_ERR(line_names))
   825			return ERR_CAST(line_names);
   826	
   827		if (line_names)
   828			properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
   829							"gpio-line-names",
   830							line_names, line_names_size);
   831	
   832		swnode = fwnode_create_software_node(properties, parent);
   833		kfree(line_names);
   834		return swnode;
   835	}
   836	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v11 3/6] gpio: sim: new testing module

[Bartosz Golaszewski]

[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]

On Wed, Dec 1, 2021 at 3:56 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Bartosz,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on v5.16-rc3]
> [cannot apply to shuah-kselftest/next linusw-gpio/for-next next-20211130]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url:    https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/gpio-sim-configfs-based-GPIO-simulator/20211130-234338
> base:    d58071a8a76d779eedab38033ae4c821c30295a5
> config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20211201/202112011018.3gANFz4h-lkp(a)intel.com/config)
> compiler: sh4-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/1e50c35dae10799b1e2bbed56f68cfbac59bea08
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Bartosz-Golaszewski/gpio-sim-configfs-based-GPIO-simulator/20211130-234338
>         git checkout 1e50c35dae10799b1e2bbed56f68cfbac59bea08
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/ security//
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpio/gpio-sim.c:807:23: warning: no previous prototype for 'gpio_sim_make_bank_swnode' [-Wmissing-prototypes]
>      807 | struct fwnode_handle *gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
>          |                       ^~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> vim +/gpio_sim_make_bank_swnode +807 drivers/gpio/gpio-sim.c
>
>    806
>  > 807  struct fwnode_handle *gpio_sim_make_bank_swnode(struct gpio_sim_bank *bank,
>    808                                                  struct fwnode_handle *parent)
>    809  {

Will statify it in v12.

Bart

>    810          struct property_entry properties[GPIO_SIM_PROP_MAX];
>    811          unsigned int prop_idx = 0, line_names_size = 0;
>    812          struct fwnode_handle *swnode;
>    813          char **line_names;
>    814
>    815          memset(properties, 0, sizeof(properties));
>    816
>    817          properties[prop_idx++] = PROPERTY_ENTRY_U32("ngpios", bank->num_lines);
>    818
>    819          if (bank->label)
>    820                  properties[prop_idx++] = PROPERTY_ENTRY_STRING("gpio-sim,label",
>    821                                                                 bank->label);
>    822
>    823          line_names = gpio_sim_make_line_names(bank, &line_names_size);
>    824          if (IS_ERR(line_names))
>    825                  return ERR_CAST(line_names);
>    826
>    827          if (line_names)
>    828                  properties[prop_idx++] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
>    829                                                  "gpio-line-names",
>    830                                                  line_names, line_names_size);
>    831
>    832          swnode = fwnode_create_software_node(properties, parent);
>    833          kfree(line_names);
>    834          return swnode;
>    835  }
>    836
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > > > Software nodes allow us to represent hierarchies for device components
> > > > > that don't have their struct device representation yet - for instance:
> > > > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> > > >
> > > > core .. core ?!
> > > >
> > > > > however doesn't offer any way of passing this information from the
> > > > > drivers.
> > > > >
> > > > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > > > by the driver and used to pass device properties for child nodes.
> > > > >
> > > > > This is similar to how we handle device-tree sub-nodes with
> > > > > CONFIG_OF_GPIO enabled.
> > > >
> > > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > > examples?
> > > >
> > > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > > need to solve this way?
> > > >
> > > > ...
> > > >
> > > > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > > > +     if (gc->of_node && gc->fwnode) {
> > > > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > > > +                    __func__);
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +#endif /* CONFIG_OF_GPIO */
> > > >
> > > > I don't like this. It seems like a hack right now.
> > > >
> > > > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > > > rather than doing this? (I believe in most of the drivers we can drop
> > > > completely the of_node assignment).
> > > >
> > >
> > > Yes, it's definitely a good idea but I would be careful with just
> > > dropping the of_node assignments as callbacks may depend on them
> > > later.
> >
> > GPIO library does it for us among these lines:
> >
> >         struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
> >
> >         of_gpio_dev_init(gc, gdev); <<< HERE!
> >         acpi_gpio_dev_init(gc, gdev);
> >
> >         /*
> >          * Assign fwnode depending on the result of the previous calls,
> >          * if none of them succeed, assign it to the parent's one.
> >          */
> >         gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> >
>
> Except that it doesn't and I noticed that when working on the
> subsequent patch. The child gpiochipX devices all had the parent's
> fwnode assigned as their primary fwnode and no secondary fwnode.
>
> Note that this driver doesn't use neither OF nor ACPI in which case
> gdev->dev has no fwnode and the parent's one is used. This patch
> addresses it. If you have a better idea, let me know.
>
> Bart

Let me maybe rephrase the problem: currently, for GPIO devices
instantiating multiple banks created outside of the OF or ACPI
frameworks (e.g. instantiated manually and configured using a
hierarchy of software nodes with a single parent swnode and a number
of child swnodes representing the children), it is impossible to
assign firmware nodes other than the one representing the top GPIO
device to the gpiochip child devices.

In fact if we want to drop the OF APIs entirely from gpiolib - this
would be the right first step as for gpio-sim it actually replaces the
gc->of_node = some_of_node; assignment that OF-based drivers do for
sub-nodes defining banks and it does work with device-tree (I verified
that too) thanks to the fwnode abstraction layer.

Linus: Do you have anything against this change?

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > > > > Software nodes allow us to represent hierarchies for device components
> > > > > > that don't have their struct device representation yet - for instance:
> > > > > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> > > > >
> > > > > core .. core ?!
> > > > >
> > > > > > however doesn't offer any way of passing this information from the
> > > > > > drivers.
> > > > > >
> > > > > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > > > > by the driver and used to pass device properties for child nodes.
> > > > > >
> > > > > > This is similar to how we handle device-tree sub-nodes with
> > > > > > CONFIG_OF_GPIO enabled.
> > > > >
> > > > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > > > examples?
> > > > >
> > > > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > > > need to solve this way?
> > > > >
> > > > > ...
> > > > >
> > > > > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > > > > +     if (gc->of_node && gc->fwnode) {
> > > > > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > > > > +                    __func__);
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > > +#endif /* CONFIG_OF_GPIO */
> > > > >
> > > > > I don't like this. It seems like a hack right now.
> > > > >
> > > > > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > > > > rather than doing this? (I believe in most of the drivers we can drop
> > > > > completely the of_node assignment).
> > > > >
> > > >
> > > > Yes, it's definitely a good idea but I would be careful with just
> > > > dropping the of_node assignments as callbacks may depend on them
> > > > later.
> > >
> > > GPIO library does it for us among these lines:
> > >
> > >         struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
> > >
> > >         of_gpio_dev_init(gc, gdev); <<< HERE!
> > >         acpi_gpio_dev_init(gc, gdev);
> > >
> > >         /*
> > >          * Assign fwnode depending on the result of the previous calls,
> > >          * if none of them succeed, assign it to the parent's one.
> > >          */
> > >         gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> > >
> >
> > Except that it doesn't and I noticed that when working on the
> > subsequent patch. The child gpiochipX devices all had the parent's
> > fwnode assigned as their primary fwnode and no secondary fwnode.
> >
> > Note that this driver doesn't use neither OF nor ACPI in which case
> > gdev->dev has no fwnode and the parent's one is used. This patch
> > addresses it. If you have a better idea, let me know.
> >
> > Bart
> 
> Let me maybe rephrase the problem: currently, for GPIO devices
> instantiating multiple banks created outside of the OF or ACPI
> frameworks (e.g. instantiated manually and configured using a
> hierarchy of software nodes with a single parent swnode and a number
> of child swnodes representing the children), it is impossible to
> assign firmware nodes other than the one representing the top GPIO
> device to the gpiochip child devices.
> 
> In fact if we want to drop the OF APIs entirely from gpiolib - this
> would be the right first step as for gpio-sim it actually replaces the
> gc->of_node = some_of_node; assignment that OF-based drivers do for
> sub-nodes defining banks and it does work with device-tree (I verified
> that too) thanks to the fwnode abstraction layer.

I still don't see how you set up hierarchy of primary/secondary fwnodes.

And I don't like this change. It seems it band-aids some issue with fwnode
usage. What the easiest way to reproduce the issue with your series applied
(without this change)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Tue, Nov 30, 2021 at 10:00 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Tue, Nov 30, 2021 at 09:25:35PM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 30, 2021 at 5:15 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > On Tue, Nov 30, 2021 at 04:41:23PM +0100, Bartosz Golaszewski wrote:
> > > > > > > Software nodes allow us to represent hierarchies for device components
> > > > > > > that don't have their struct device representation yet - for instance:
> > > > > > > banks of GPIOs under a common GPIO expander. The core gpiolib core
> > > > > >
> > > > > > core .. core ?!
> > > > > >
> > > > > > > however doesn't offer any way of passing this information from the
> > > > > > > drivers.
> > > > > > >
> > > > > > > This extends struct gpio_chip with a pointer to fwnode that can be set
> > > > > > > by the driver and used to pass device properties for child nodes.
> > > > > > >
> > > > > > > This is similar to how we handle device-tree sub-nodes with
> > > > > > > CONFIG_OF_GPIO enabled.
> > > > > >
> > > > > > Not sure I understand the proposal. Can you provide couple of (simplest)
> > > > > > examples?
> > > > > >
> > > > > > And also it sounds like reinventing a wheel. What problem do you have that you
> > > > > > need to solve this way?
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > > > > > > +     if (gc->of_node && gc->fwnode) {
> > > > > > > +             pr_err("%s: tried to set both the of_node and fwnode in gpio_chip\n",
> > > > > > > +                    __func__);
> > > > > > > +             return -EINVAL;
> > > > > > > +     }
> > > > > > > +#endif /* CONFIG_OF_GPIO */
> > > > > >
> > > > > > I don't like this. It seems like a hack right now.
> > > > > >
> > > > > > Is it possible to convert all GPIO controller drivers to provide an fwnode
> > > > > > rather than doing this? (I believe in most of the drivers we can drop
> > > > > > completely the of_node assignment).
> > > > > >
> > > > >
> > > > > Yes, it's definitely a good idea but I would be careful with just
> > > > > dropping the of_node assignments as callbacks may depend on them
> > > > > later.
> > > >
> > > > GPIO library does it for us among these lines:
> > > >
> > > >         struct fwnode_handle *fwnode = gc->parent ? dev_fwnode(gc->parent) : NULL;
> > > >
> > > >         of_gpio_dev_init(gc, gdev); <<< HERE!
> > > >         acpi_gpio_dev_init(gc, gdev);
> > > >
> > > >         /*
> > > >          * Assign fwnode depending on the result of the previous calls,
> > > >          * if none of them succeed, assign it to the parent's one.
> > > >          */
> > > >         gdev->dev.fwnode = dev_fwnode(&gdev->dev) ?: fwnode;
> > > >
> > >
> > > Except that it doesn't and I noticed that when working on the
> > > subsequent patch. The child gpiochipX devices all had the parent's
> > > fwnode assigned as their primary fwnode and no secondary fwnode.
> > >
> > > Note that this driver doesn't use neither OF nor ACPI in which case
> > > gdev->dev has no fwnode and the parent's one is used. This patch
> > > addresses it. If you have a better idea, let me know.
> > >
> > > Bart
> >
> > Let me maybe rephrase the problem: currently, for GPIO devices
> > instantiating multiple banks created outside of the OF or ACPI
> > frameworks (e.g. instantiated manually and configured using a
> > hierarchy of software nodes with a single parent swnode and a number
> > of child swnodes representing the children), it is impossible to
> > assign firmware nodes other than the one representing the top GPIO
> > device to the gpiochip child devices.
> >
> > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > would be the right first step as for gpio-sim it actually replaces the
> > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > sub-nodes defining banks and it does work with device-tree (I verified
> > that too) thanks to the fwnode abstraction layer.
>
> I still don't see how you set up hierarchy of primary/secondary fwnodes.
>
> And I don't like this change. It seems it band-aids some issue with fwnode
> usage. What the easiest way to reproduce the issue with your series applied
> (without this change)?
>

Drop this patch and drop the line where the fwnode is assigned in
gpio-sim.c. Then probe the device and print the addresses of the
parent and child swnodes. See how they are the same and don't match
the swnode hierarchy we created. You can then apply this patch and see
how it becomes correct.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > instantiating multiple banks created outside of the OF or ACPI
> > > frameworks (e.g. instantiated manually and configured using a
> > > hierarchy of software nodes with a single parent swnode and a number
> > > of child swnodes representing the children), it is impossible to
> > > assign firmware nodes other than the one representing the top GPIO
> > > device to the gpiochip child devices.
> > >
> > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > would be the right first step as for gpio-sim it actually replaces the
> > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > sub-nodes defining banks and it does work with device-tree (I verified
> > > that too) thanks to the fwnode abstraction layer.
> >
> > I still don't see how you set up hierarchy of primary/secondary fwnodes.
> >
> > And I don't like this change. It seems it band-aids some issue with fwnode
> > usage. What the easiest way to reproduce the issue with your series applied
> > (without this change)?
> 
> Drop this patch and drop the line where the fwnode is assigned in
> gpio-sim.c. Then probe the device and print the addresses of the
> parent and child swnodes. See how they are the same and don't match
> the swnode hierarchy we created. You can then apply this patch and see
> how it becomes correct.

Thanks. I will give a spin.

Note, it seems I have to revert your older code first...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Wed, Dec 01, 2021 at 04:28:19PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> ...
> 
> > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > instantiating multiple banks created outside of the OF or ACPI
> > > > frameworks (e.g. instantiated manually and configured using a
> > > > hierarchy of software nodes with a single parent swnode and a number
> > > > of child swnodes representing the children), it is impossible to
> > > > assign firmware nodes other than the one representing the top GPIO
> > > > device to the gpiochip child devices.
> > > >
> > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > would be the right first step as for gpio-sim it actually replaces the
> > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > that too) thanks to the fwnode abstraction layer.
> > >
> > > I still don't see how you set up hierarchy of primary/secondary fwnodes.
> > >
> > > And I don't like this change. It seems it band-aids some issue with fwnode
> > > usage. What the easiest way to reproduce the issue with your series applied
> > > (without this change)?
> > 
> > Drop this patch and drop the line where the fwnode is assigned in
> > gpio-sim.c. Then probe the device and print the addresses of the
> > parent and child swnodes. See how they are the same and don't match
> > the swnode hierarchy we created. You can then apply this patch and see
> > how it becomes correct.
> 
> Thanks. I will give a spin.
> 
> Note, it seems I have to revert your older code first...

Okay, I have to postpone because simple revert doesn't work for me.
Can you clean up the next, please and I can use it starting from tomorrow?


$ git tag --contains 5065e08e4ef3
DONT-USE-next-20211105
next-20211101
next-20211102
next-20211103
next-20211104
next-20211105
next-20211106
next-20211108
next-20211109
next-20211110
next-20211111
next-20211112
next-20211115
next-20211116
next-20211117
next-20211118
next-20211123
next-20211124
next-20211125
next-20211126
next-20211129
next-20211130
next-20211201

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Wed, Dec 1, 2021 at 3:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 01, 2021 at 04:28:19PM +0200, Andy Shevchenko wrote:
> > On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > ...
> >
> > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > of child swnodes representing the children), it is impossible to
> > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > device to the gpiochip child devices.
> > > > >
> > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > that too) thanks to the fwnode abstraction layer.
> > > >
> > > > I still don't see how you set up hierarchy of primary/secondary fwnodes.
> > > >
> > > > And I don't like this change. It seems it band-aids some issue with fwnode
> > > > usage. What the easiest way to reproduce the issue with your series applied
> > > > (without this change)?
> > >
> > > Drop this patch and drop the line where the fwnode is assigned in
> > > gpio-sim.c. Then probe the device and print the addresses of the
> > > parent and child swnodes. See how they are the same and don't match
> > > the swnode hierarchy we created. You can then apply this patch and see
> > > how it becomes correct.
> >
> > Thanks. I will give a spin.
> >
> > Note, it seems I have to revert your older code first...
>
> Okay, I have to postpone because simple revert doesn't work for me.
> Can you clean up the next, please and I can use it starting from tomorrow?
>
>
> $ git tag --contains 5065e08e4ef3
> DONT-USE-next-20211105
> next-20211101
> next-20211102
> next-20211103
> next-20211104
> next-20211105
> next-20211106
> next-20211108
> next-20211109
> next-20211110
> next-20211111
> next-20211112
> next-20211115
> next-20211116
> next-20211117
> next-20211118
> next-20211123
> next-20211124
> next-20211125
> next-20211126
> next-20211129
> next-20211130
> next-20211201
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

None of this is in next, please use:
https://github.com/brgl/linux/tree/topic/gpio-sim-v12 if you want a
branch.

I just thought you were going to simply apply these patches.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Wed, Dec 01, 2021 at 03:36:29PM +0100, Bartosz Golaszewski wrote:
> On Wed, Dec 1, 2021 at 3:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Dec 01, 2021 at 04:28:19PM +0200, Andy Shevchenko wrote:
> > > On Wed, Dec 01, 2021 at 02:53:42PM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Dec 1, 2021 at 2:40 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> > > > Drop this patch and drop the line where the fwnode is assigned in
> > > > gpio-sim.c. Then probe the device and print the addresses of the
> > > > parent and child swnodes. See how they are the same and don't match
> > > > the swnode hierarchy we created. You can then apply this patch and see
> > > > how it becomes correct.
> > >
> > > Thanks. I will give a spin.
> > >
> > > Note, it seems I have to revert your older code first...
> >
> > Okay, I have to postpone because simple revert doesn't work for me.
> > Can you clean up the next, please and I can use it starting from tomorrow?
> >
> >
> > $ git tag --contains 5065e08e4ef3
> > DONT-USE-next-20211105
> > next-20211101
> > next-20211102
> > next-20211103
> > next-20211104
> > next-20211105
> > next-20211106
> > next-20211108
> > next-20211109
> > next-20211110
> > next-20211111
> > next-20211112
> > next-20211115
> > next-20211116
> > next-20211117
> > next-20211118
> > next-20211123
> > next-20211124
> > next-20211125
> > next-20211126
> > next-20211129
> > next-20211130
> > next-20211201

> None of this is in next, please use:
> https://github.com/brgl/linux/tree/topic/gpio-sim-v12 if you want a
> branch.

It the old version that prevents me to apply (my branches based on next).

> I just thought you were going to simply apply these patches.

That's correct, but see above.

Anyway, I'm already switched tasks for today. Maybe I will have some time
later on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

...

> Let me maybe rephrase the problem: currently, for GPIO devices
> instantiating multiple banks created outside of the OF or ACPI
> frameworks (e.g. instantiated manually and configured using a
> hierarchy of software nodes with a single parent swnode and a number
> of child swnodes representing the children), it is impossible to
> assign firmware nodes other than the one representing the top GPIO
> device to the gpiochip child devices.
> 
> In fact if we want to drop the OF APIs entirely from gpiolib - this
> would be the right first step as for gpio-sim it actually replaces the
> gc->of_node = some_of_node; assignment that OF-based drivers do for
> sub-nodes defining banks and it does work with device-tree (I verified
> that too) thanks to the fwnode abstraction layer.

In exchange of acknowledgements I confirm that I understood the issue
you are describing. What I still don't like is this band-aid:ish approach.
What we really need is to replace of_node by fwnode in GPIO library once
for all. But it can be done later after your simulation series (or before,
i.o.w. independently), hence I propose to update TODO and do it separately.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> ...
>
> > Let me maybe rephrase the problem: currently, for GPIO devices
> > instantiating multiple banks created outside of the OF or ACPI
> > frameworks (e.g. instantiated manually and configured using a
> > hierarchy of software nodes with a single parent swnode and a number
> > of child swnodes representing the children), it is impossible to
> > assign firmware nodes other than the one representing the top GPIO
> > device to the gpiochip child devices.
> >
> > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > would be the right first step as for gpio-sim it actually replaces the
> > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > sub-nodes defining banks and it does work with device-tree (I verified
> > that too) thanks to the fwnode abstraction layer.
>
> In exchange of acknowledgements I confirm that I understood the issue
> you are describing. What I still don't like is this band-aid:ish approach.
> What we really need is to replace of_node by fwnode in GPIO library once
> for all. But it can be done later after your simulation series (or before,
> i.o.w. independently), hence I propose to update TODO and do it separately.
>

But this is what we already do for OF. How would the core gpiolib know
how the firmware nodes represent the banks? It's the driver's job to
tell the framework which node corresponds with what. If anything, we
should start replacing of_nodes with fwnodes in drivers and eventually
we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
the fwnode pointer I added as the driver still needs to assign it
itself.

Again: I may be missing something here but I've been going through
this on and on and can't figure out any other way. Looking at
gpiolib-acpi.c I don't see it correctly assigning fwnodes to
sub-devices either but I don't have any HW to test it.

As for this series: I can't really drop this patch as gpio-sim relies
on swnodes being correctly associated with gpio_chips to identify the
gpiodevs from configfs callbacks.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > ...
> >
> > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > instantiating multiple banks created outside of the OF or ACPI
> > > frameworks (e.g. instantiated manually and configured using a
> > > hierarchy of software nodes with a single parent swnode and a number
> > > of child swnodes representing the children), it is impossible to
> > > assign firmware nodes other than the one representing the top GPIO
> > > device to the gpiochip child devices.
> > >
> > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > would be the right first step as for gpio-sim it actually replaces the
> > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > sub-nodes defining banks and it does work with device-tree (I verified
> > > that too) thanks to the fwnode abstraction layer.
> >
> > In exchange of acknowledgements I confirm that I understood the issue
> > you are describing. What I still don't like is this band-aid:ish approach.
> > What we really need is to replace of_node by fwnode in GPIO library once
> > for all. But it can be done later after your simulation series (or before,
> > i.o.w. independently), hence I propose to update TODO and do it separately.
> >
> 
> But this is what we already do for OF. How would the core gpiolib know
> how the firmware nodes represent the banks? It's the driver's job to
> tell the framework which node corresponds with what. If anything, we
> should start replacing of_nodes with fwnodes in drivers and eventually
> we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> the fwnode pointer I added as the driver still needs to assign it
> itself.
> 
> Again: I may be missing something here but I've been going through
> this on and on and can't figure out any other way. Looking at
> gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> sub-devices either but I don't have any HW to test it.
> 
> As for this series: I can't really drop this patch as gpio-sim relies
> on swnodes being correctly associated with gpio_chips to identify the
> gpiodevs from configfs callbacks.

Then we need to replace of_node by fwnode as a first step. I have looked
briefly into the list of drivers that may have been cleaned up and it doesn't
look too long.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > ...
> > >
> > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > instantiating multiple banks created outside of the OF or ACPI
> > > > frameworks (e.g. instantiated manually and configured using a
> > > > hierarchy of software nodes with a single parent swnode and a number
> > > > of child swnodes representing the children), it is impossible to
> > > > assign firmware nodes other than the one representing the top GPIO
> > > > device to the gpiochip child devices.
> > > >
> > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > would be the right first step as for gpio-sim it actually replaces the
> > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > that too) thanks to the fwnode abstraction layer.
> > >
> > > In exchange of acknowledgements I confirm that I understood the issue
> > > you are describing. What I still don't like is this band-aid:ish approach.
> > > What we really need is to replace of_node by fwnode in GPIO library once
> > > for all. But it can be done later after your simulation series (or before,
> > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > >
> > 
> > But this is what we already do for OF. How would the core gpiolib know
> > how the firmware nodes represent the banks? It's the driver's job to
> > tell the framework which node corresponds with what. If anything, we
> > should start replacing of_nodes with fwnodes in drivers and eventually
> > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > the fwnode pointer I added as the driver still needs to assign it
> > itself.
> > 
> > Again: I may be missing something here but I've been going through
> > this on and on and can't figure out any other way. Looking at
> > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > sub-devices either but I don't have any HW to test it.
> > 
> > As for this series: I can't really drop this patch as gpio-sim relies
> > on swnodes being correctly associated with gpio_chips to identify the
> > gpiodevs from configfs callbacks.
> 
> Then we need to replace of_node by fwnode as a first step. I have looked
> briefly into the list of drivers that may have been cleaned up and it doesn't
> look too long.

Let me kick this off by sending couple of patches.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > ...
> > > >
> > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > of child swnodes representing the children), it is impossible to
> > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > device to the gpiochip child devices.
> > > > >
> > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > that too) thanks to the fwnode abstraction layer.
> > > >
> > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > for all. But it can be done later after your simulation series (or before,
> > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > >
> > >
> > > But this is what we already do for OF. How would the core gpiolib know
> > > how the firmware nodes represent the banks? It's the driver's job to
> > > tell the framework which node corresponds with what. If anything, we
> > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > the fwnode pointer I added as the driver still needs to assign it
> > > itself.
> > >
> > > Again: I may be missing something here but I've been going through
> > > this on and on and can't figure out any other way. Looking at
> > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > sub-devices either but I don't have any HW to test it.
> > >
> > > As for this series: I can't really drop this patch as gpio-sim relies
> > > on swnodes being correctly associated with gpio_chips to identify the
> > > gpiodevs from configfs callbacks.
> >
> > Then we need to replace of_node by fwnode as a first step. I have looked
> > briefly into the list of drivers that may have been cleaned up and it doesn't
> > look too long.
>
> Let me kick this off by sending couple of patches.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Are you fine with merging this in the meantime to get gpio-sim into mainline?

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > of child swnodes representing the children), it is impossible to
> > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > device to the gpiochip child devices.
> > > > > >
> > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > that too) thanks to the fwnode abstraction layer.
> > > > >
> > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > for all. But it can be done later after your simulation series (or before,
> > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > >
> > > >
> > > > But this is what we already do for OF. How would the core gpiolib know
> > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > tell the framework which node corresponds with what. If anything, we
> > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > the fwnode pointer I added as the driver still needs to assign it
> > > > itself.
> > > >
> > > > Again: I may be missing something here but I've been going through
> > > > this on and on and can't figure out any other way. Looking at
> > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > sub-devices either but I don't have any HW to test it.
> > > >
> > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > gpiodevs from configfs callbacks.
> > >
> > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > look too long.
> >
> > Let me kick this off by sending couple of patches.
> 
> Are you fine with merging this in the meantime to get gpio-sim into mainline?

gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
etc) but not this patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > >
> > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > device to the gpiochip child devices.
> > > > > > >
> > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > >
> > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > >
> > > > >
> > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > tell the framework which node corresponds with what. If anything, we
> > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > itself.
> > > > >
> > > > > Again: I may be missing something here but I've been going through
> > > > > this on and on and can't figure out any other way. Looking at
> > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > sub-devices either but I don't have any HW to test it.
> > > > >
> > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > gpiodevs from configfs callbacks.
> > > >
> > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > look too long.
> > >
> > > Let me kick this off by sending couple of patches.
> >
> > Are you fine with merging this in the meantime to get gpio-sim into mainline?
>
> gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> etc) but not this patch.
>

There's no way around it though AFAIK. First - the 'gpio-line-names'
property will not work for banks. 'ngpios' will only work because we
read it manually in probe() to figure out the number of sysfs groups.
And also configfs callbacks will not be able to associate bank devices
with configfs groups. I would really like to hear an alternative -
even if it's just an idea and not actual implementation.

I'm really curious to see how you'll remove the of_node pointer and
not introduce the corresponding fwnode pointer actually.

Bart

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > >
> > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > device to the gpiochip child devices.
> > > > > > > >
> > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > >
> > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > >
> > > > > >
> > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > itself.
> > > > > >
> > > > > > Again: I may be missing something here but I've been going through
> > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > sub-devices either but I don't have any HW to test it.
> > > > > >
> > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > gpiodevs from configfs callbacks.
> > > > >
> > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > look too long.
> > > >
> > > > Let me kick this off by sending couple of patches.
> > >
> > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> >
> > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > etc) but not this patch.
> >
> 
> There's no way around it though AFAIK. First - the 'gpio-line-names'
> property will not work for banks. 'ngpios' will only work because we
> read it manually in probe() to figure out the number of sysfs groups.
> And also configfs callbacks will not be able to associate bank devices
> with configfs groups. I would really like to hear an alternative -
> even if it's just an idea and not actual implementation.
> 
> I'm really curious to see how you'll remove the of_node pointer and
> not introduce the corresponding fwnode pointer actually.

Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
having of_node and fwnode at the same time in the struct gpio_chip.

Yes, we may modify this patch to work without that ugly ifdeffery and with both
in the structure, but I don't think it's a good solution.

Now clearly we have to clean up of_node first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Bartosz Golaszewski]

On Thu, Dec 2, 2021 at 4:41 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > >
> > > > > > > > ...
> > > > > > > >
> > > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > > device to the gpiochip child devices.
> > > > > > > > >
> > > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > > >
> > > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > > >
> > > > > > >
> > > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > > itself.
> > > > > > >
> > > > > > > Again: I may be missing something here but I've been going through
> > > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > > sub-devices either but I don't have any HW to test it.
> > > > > > >
> > > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > > gpiodevs from configfs callbacks.
> > > > > >
> > > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > > look too long.
> > > > >
> > > > > Let me kick this off by sending couple of patches.
> > > >
> > > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> > >
> > > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > > etc) but not this patch.
> > >
> >
> > There's no way around it though AFAIK. First - the 'gpio-line-names'
> > property will not work for banks. 'ngpios' will only work because we
> > read it manually in probe() to figure out the number of sysfs groups.
> > And also configfs callbacks will not be able to associate bank devices
> > with configfs groups. I would really like to hear an alternative -
> > even if it's just an idea and not actual implementation.
> >
> > I'm really curious to see how you'll remove the of_node pointer and
> > not introduce the corresponding fwnode pointer actually.
>
> Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
> having of_node and fwnode at the same time in the struct gpio_chip.
>
> Yes, we may modify this patch to work without that ugly ifdeffery and with both
> in the structure, but I don't think it's a good solution.
>

It may not be the best solution but we can't simply convert all the
drivers to fwnode and pray they work. I would like every converted
driver to be well tested because there can be some issues lurking in
the fwnode <-> of_node conversion. That will take time.

Meanwhile, this would block gpio-sim for months again. I don't believe
this patch is wrong as it fixes a real issue and as you said: fwnode
will most likely stay in gpio_chip.

IMO we should introduce fwnode, convert gpiolib and drivers to using
it gradually, remove of_node once there are no more users.

Bart

> Now clearly we have to clean up of_node first.
>

Re: [PATCH v11 2/6] gpiolib: allow to specify the firmware node in struct gpio_chip

[Andy Shevchenko]

On Thu, Dec 02, 2021 at 06:00:05PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 4:41 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Dec 02, 2021 at 02:52:55PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Dec 2, 2021 at 2:45 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Dec 02, 2021 at 02:06:57PM +0100, Bartosz Golaszewski wrote:
> > > > > On Thu, Dec 2, 2021 at 12:38 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Thu, Dec 02, 2021 at 01:35:01PM +0200, Andy Shevchenko wrote:
> > > > > > > On Thu, Dec 02, 2021 at 12:24:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Thu, Dec 2, 2021 at 11:58 AM Andy Shevchenko
> > > > > > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > > > > On Wed, Dec 01, 2021 at 02:11:28PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > > > On Tue, Nov 30, 2021 at 10:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > Let me maybe rephrase the problem: currently, for GPIO devices
> > > > > > > > > > instantiating multiple banks created outside of the OF or ACPI
> > > > > > > > > > frameworks (e.g. instantiated manually and configured using a
> > > > > > > > > > hierarchy of software nodes with a single parent swnode and a number
> > > > > > > > > > of child swnodes representing the children), it is impossible to
> > > > > > > > > > assign firmware nodes other than the one representing the top GPIO
> > > > > > > > > > device to the gpiochip child devices.
> > > > > > > > > >
> > > > > > > > > > In fact if we want to drop the OF APIs entirely from gpiolib - this
> > > > > > > > > > would be the right first step as for gpio-sim it actually replaces the
> > > > > > > > > > gc->of_node = some_of_node; assignment that OF-based drivers do for
> > > > > > > > > > sub-nodes defining banks and it does work with device-tree (I verified
> > > > > > > > > > that too) thanks to the fwnode abstraction layer.
> > > > > > > > >
> > > > > > > > > In exchange of acknowledgements I confirm that I understood the issue
> > > > > > > > > you are describing. What I still don't like is this band-aid:ish approach.
> > > > > > > > > What we really need is to replace of_node by fwnode in GPIO library once
> > > > > > > > > for all. But it can be done later after your simulation series (or before,
> > > > > > > > > i.o.w. independently), hence I propose to update TODO and do it separately.
> > > > > > > > >
> > > > > > > >
> > > > > > > > But this is what we already do for OF. How would the core gpiolib know
> > > > > > > > how the firmware nodes represent the banks? It's the driver's job to
> > > > > > > > tell the framework which node corresponds with what. If anything, we
> > > > > > > > should start replacing of_nodes with fwnodes in drivers and eventually
> > > > > > > > we'd drop the of_node pointer from gpio_chip entirely, but we'd keep
> > > > > > > > the fwnode pointer I added as the driver still needs to assign it
> > > > > > > > itself.
> > > > > > > >
> > > > > > > > Again: I may be missing something here but I've been going through
> > > > > > > > this on and on and can't figure out any other way. Looking at
> > > > > > > > gpiolib-acpi.c I don't see it correctly assigning fwnodes to
> > > > > > > > sub-devices either but I don't have any HW to test it.
> > > > > > > >
> > > > > > > > As for this series: I can't really drop this patch as gpio-sim relies
> > > > > > > > on swnodes being correctly associated with gpio_chips to identify the
> > > > > > > > gpiodevs from configfs callbacks.
> > > > > > >
> > > > > > > Then we need to replace of_node by fwnode as a first step. I have looked
> > > > > > > briefly into the list of drivers that may have been cleaned up and it doesn't
> > > > > > > look too long.
> > > > > >
> > > > > > Let me kick this off by sending couple of patches.
> > > > >
> > > > > Are you fine with merging this in the meantime to get gpio-sim into mainline?
> > > >
> > > > gpio-sim, yes, (though I may bikeshed about naming of the configfs attributes,
> > > > etc) but not this patch.
> > > >
> > >
> > > There's no way around it though AFAIK. First - the 'gpio-line-names'
> > > property will not work for banks. 'ngpios' will only work because we
> > > read it manually in probe() to figure out the number of sysfs groups.
> > > And also configfs callbacks will not be able to associate bank devices
> > > with configfs groups. I would really like to hear an alternative -
> > > even if it's just an idea and not actual implementation.
> > >
> > > I'm really curious to see how you'll remove the of_node pointer and
> > > not introduce the corresponding fwnode pointer actually.
> >
> > Seems I was unclear, fwnode pointer will be needed, but what I'm against of is
> > having of_node and fwnode at the same time in the struct gpio_chip.
> >
> > Yes, we may modify this patch to work without that ugly ifdeffery and with both
> > in the structure, but I don't think it's a good solution.
> >
> 
> It may not be the best solution but we can't simply convert all the
> drivers to fwnode and pray they work. I would like every converted
> driver to be well tested because there can be some issues lurking in
> the fwnode <-> of_node conversion. That will take time.

> Meanwhile, this would block gpio-sim for months again. I don't believe
> this patch is wrong as it fixes a real issue and as you said: fwnode
> will most likely stay in gpio_chip.o

It doesn't strictly speaking "fix". But it allows to get things right.

> IMO we should introduce fwnode, convert gpiolib and drivers to using
> it gradually, remove of_node once there are no more users.

I may accept the change after some amendments done:
 - get rid of ifdeffery (remove that block completely)
 - add TODO entry
 - add "deprecated" keyword to of_node
 - ...I hope I haven't miss anything else...

> > Now clearly we have to clean up of_node first.

-- 
With Best Regards,
Andy Shevchenko