Re: [PATCH v3 03/10] gpiolib: implement low-level, shared GPIO support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Oct 29, 2025 at 12:20:39PM +0100, Bartosz Golaszewski wrote:
> 
> This module scans the device tree (for now only OF nodes are supported
> but care is taken to make other fwnode implementations easy to
> integrate) and determines which GPIO lines are shared by multiple users.
> It stores that information in memory. When the GPIO chip exposing shared
> lines is registered, the shared GPIO descriptors it exposes are marked
> as shared and virtual "proxy" devices that mediate access to the shared
> lines are created. When a consumer of a shared GPIO looks it up, its
> fwnode lookup is redirected to a just-in-time machine lookup that points
> to this proxy device.
> 
> This code can be compiled out on platforms which don't use shared GPIOs.

Besides strcmp_suffix() that already exists in OF core, there are also some
existing pieces that seems being repeated here (again). Can we reduce amount
of duplication?

...

> +#if IS_ENABLED(CONFIG_OF)
> +static int gpio_shared_of_traverse(struct device_node *curr)
> +{

I believe parts of this code may be resided somewhere in drivers/of/property.c
or nearby as it has the similar parsing routines.

> +	struct gpio_shared_entry *entry;
> +	size_t con_id_len, suffix_len;
> +	struct fwnode_handle *fwnode;
> +	struct of_phandle_args args;
> +	struct property *prop;
> +	unsigned int offset;
> +	const char *suffix;
> +	int ret, count, i;
> +
> +	for_each_property_of_node(curr, prop) {
> +		/*
> +		 * The standard name for a GPIO property is "foo-gpios"
> +		 * or "foo-gpio". Some bindings also use "gpios" or "gpio".
> +		 * There are some legacy device-trees which have a different
> +		 * naming convention and for which we have rename quirks in
> +		 * place in gpiolib-of.c. I don't think any of them require
> +		 * support for shared GPIOs so for now let's just ignore
> +		 * them. We can always just export the quirk list and
> +		 * iterate over it here.
> +		 */
> +		if (!strends(prop->name, "-gpios") &&
> +		    !strends(prop->name, "-gpio") &&
> +		    strcmp(prop->name, "gpios") != 0 &&
> +		    strcmp(prop->name, "gpio") != 0)
> +			continue;
> +
> +		count = of_count_phandle_with_args(curr, prop->name,
> +						   "#gpio-cells");
> +		if (count <= 0)
> +			continue;
> +
> +		for (i = 0; i < count; i++) {
> +			struct device_node *np __free(device_node) = NULL;
> +
> +			ret = of_parse_phandle_with_args(curr, prop->name,
> +							 "#gpio-cells", i,
> +							 &args);
> +			if (ret)
> +				continue;
> +
> +			np = args.np;
> +
> +			if (!of_property_present(np, "gpio-controller"))
> +				continue;
> +
> +			/*
> +			 * We support 1, 2 and 3 cell GPIO bindings in the
> +			 * kernel currently. There's only one old MIPS dts that
> +			 * has a one-cell binding but there's no associated
> +			 * consumer so it may as well be an error. There don't
> +			 * seem to be any 3-cell users of non-exclusive GPIOs,
> +			 * so we can skip this as well. Let's occupy ourselves
> +			 * with the predominant 2-cell binding with the first
> +			 * cell indicating the hardware offset of the GPIO and
> +			 * the second defining the GPIO flags of the request.
> +			 */
> +			if (args.args_count != 2)
> +				continue;
> +
> +			fwnode = of_fwnode_handle(args.np);
> +			offset = args.args[0];
> +
> +			entry = gpio_shared_find_entry(fwnode, offset);
> +			if (!entry) {
> +				entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +				if (!entry)
> +					return -ENOMEM;
> +
> +				entry->fwnode = fwnode_handle_get(fwnode);
> +				entry->offset = offset;
> +				entry->index = count;
> +				INIT_LIST_HEAD(&entry->refs);
> +
> +				list_add_tail(&entry->list, &gpio_shared_list);
> +			}
> +
> +			struct gpio_shared_ref *ref __free(kfree) =
> +					kzalloc(sizeof(*ref), GFP_KERNEL);
> +			if (!ref)
> +				return -ENOMEM;
> +
> +			ref->fwnode = fwnode_handle_get(of_fwnode_handle(curr));
> +			ref->flags = args.args[1];
> +
> +			if (strends(prop->name, "gpios"))
> +				suffix = "-gpios";
> +			else if (strends(prop->name, "gpio"))
> +				suffix = "-gpio";
> +			else
> +				suffix = NULL;
> +			if (!suffix)
> +				continue;
> +
> +			/* We only set con_id if there's actually one. */
> +			if (strcmp(prop->name, "gpios") && strcmp(prop->name, "gpio")) {
> +				ref->con_id = kstrdup(prop->name, GFP_KERNEL);
> +				if (!ref->con_id)
> +					return -ENOMEM;
> +
> +				con_id_len = strlen(ref->con_id);
> +				suffix_len = strlen(suffix);
> +
> +				ref->con_id[con_id_len - suffix_len] = '\0';
> +			}
> +
> +			ref->dev_id = ida_alloc(&gpio_shared_ida, GFP_KERNEL);
> +			if (ref->dev_id < 0) {
> +				kfree(ref->con_id);
> +				return -ENOMEM;
> +			}
> +
> +			if (!list_empty(&entry->refs))
> +				pr_debug("GPIO %u at %s is shared by multiple firmware nodes\n",
> +					 entry->offset, fwnode_get_name(entry->fwnode));
> +
> +			list_add_tail(&no_free_ptr(ref)->list, &entry->refs);
> +		}
> +	}
> +
> +	for_each_child_of_node_scoped(curr, child) {
> +		ret = gpio_shared_of_traverse(child);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko