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

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

On Wed, Oct 22, 2025 at 03:10:42PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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.

...

> +		if (!strends(prop->name, "-gpios") &&
> +		    !strends(prop->name, "-gpio") &&

> +		    strcmp(prop->name, "gpios") != 0 &&
> +		    strcmp(prop->name, "gpio") != 0)

We have gpio_suffixes for a reason (also refer to for_each_gpio_property_name()
implementation, and yes I understand the difference, this is just a reference
for an example of use of the existing list of suffixes).

> +			continue;

...

> +			if (strends(prop->name, "gpios"))
> +				suffix = "-gpios";
> +			else if (strends(prop->name, "gpio"))
> +				suffix = "-gpio";
> +			else
> +				suffix = NULL;
> +			if (!suffix)
> +				continue;

In a similar way.

> +			/* We only set con_id if there's actually one. */
> +			if (strcmp(prop->name, "gpios") && strcmp(prop->name, "gpio")) {

And again...

> +				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';
> +			}

...

> +	adev->dev.parent = gdev->dev.parent;
> +	/* No need to dev->release() anything. */

And is it okay?

See drivers/base/core.c:2567

WARN(1, KERN_ERR "Device '%s' does not have a release() function, it is broken and must be fixed. See Documentation/core-api/kobject.rst.\n",

...

> +	pr_debug("Created an auxiliary GPIO proxy %s for GPIO device %s\n",
> +		 dev_name(&adev->dev), gpio_device_get_label(gdev));

Are you expecting dev_name() to be NULL in some cases? Otherwise why is this
not a dev_dbg() call?


> +	return 0;
> +}

...

> +			char *key __free(kfree) =
> +				kasprintf(GFP_KERNEL,
> +					  KBUILD_MODNAME ".proxy.%u",
> +					  ref->adev.id);

This looks awful. Just allow a bit longer line

> +			if (!key)
> +				return -ENOMEM;

...

> +static void gpio_shared_remove_adev(struct auxiliary_device *adev)
> +{
> +	lockdep_assert_held(&gpio_shared_lock);
> +
> +	auxiliary_device_uninit(adev);
> +	auxiliary_device_delete(adev);

_destroy() ? Esp. taking into account the (wrong?) ordering.

> +}

...

> +		set_bit(GPIOD_FLAG_SHARED, flags);

Do you need this to be atomic?

> +		/*
> +		 * Shared GPIOs are not requested via the normal path. Make
> +		 * them inaccessible to anyone even before we register the
> +		 * chip.
> +		 */
> +		set_bit(GPIOD_FLAG_REQUESTED, flags);

Ditto.

...

> +struct gpio_shared_desc *devm_gpiod_shared_get(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +	struct gpio_shared_desc *shared_desc;
> +	struct gpio_shared_entry *entry;
> +	struct gpio_shared_ref *ref;
> +	struct gpio_device *gdev;
> +	int ret;

> + +	scoped_guard(mutex, &gpio_shared_lock) {

Much better to split the below to a helper and make it run under a
scoped_guard() here or call a guard()() there.

> +		list_for_each_entry(entry, &gpio_shared_list, list) {
> +			list_for_each_entry(ref, &entry->refs, list) {
> +				if (adev != &ref->adev)
> +					continue;
> +
> +				if (entry->shared_desc) {
> +					kref_get(&entry->ref);
> +					shared_desc = entry->shared_desc;
> +					break;
> +				}
> +
> +				shared_desc = kzalloc(sizeof(*shared_desc), GFP_KERNEL);
> +				if (!shared_desc)
> +					return ERR_PTR(-ENOMEM);
> +
> +				gdev = gpio_device_find_by_fwnode(entry->fwnode);
> +				if (!gdev) {
> +					kfree(shared_desc);
> +					return ERR_PTR(-EPROBE_DEFER);
> +				}
> +
> +				shared_desc->desc = &gdev->descs[entry->offset];
> +				shared_desc->can_sleep = gpiod_cansleep(shared_desc->desc);
> +				if (shared_desc->can_sleep)
> +					mutex_init(&shared_desc->mutex);
> +				else
> +					spin_lock_init(&shared_desc->spinlock);
> +
> +				kref_init(&entry->ref);
> +				entry->shared_desc = shared_desc;
> +
> +				pr_debug("Device %s acquired a reference to the shared GPIO %u owned by %s\n",
> +					 dev_name(dev), desc_to_gpio(shared_desc->desc),
> +					 gpio_device_get_label(gdev));
> +				break;
> +			}
> +		}
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, gpiod_shared_put, entry);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return shared_desc;
> +}

...

> +/*
> + * This is only called if gpio_shared_init() fails so it's in fact __init and
> + * not __exit.
> + */

Fine. Have you checked if there are any section mismatch warnings during kernel
(post)build?

-- 
With Best Regards,
Andy Shevchenko