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

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

On Thu, Oct 23, 2025 at 08:55:27PM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 22, 2025 at 7:34 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Oct 22, 2025 at 03:10:42PM +0200, Bartosz Golaszewski wrote:

...

> > > +             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).
> 
> And how would you use them here - when you also need the hyphen -
> without multiple dynamic allocations instead of static strings?

Something like

	char suffix[6];
	bool found = false;

	for_each_gpio_property_name(suffix, "")
		found = found || strends();
	for_each_gpio_property_name(suffix, NULL)
		found = found || (strcmp() == 0);
	if (!found)
		continue;

Of course with more thinking this may be optimized to avoid snprintf()
(probably with a new helper macro or so).

But see my next reply, I found something more interesting.

...

> > > +     /* 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",
> 
> Huh... you're not wrong but I haven't seen this warning. Do people
> just use empty functions in this case?

I dunno. Maybe something applies a default release in you case? Can you
investigate that?

...

> > > +     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?
> 
> It's the low-level code saying: "I created device X for Y", not "Hey,
> here's X, I'm here for Y".

OK.

> > > +     return 0;
> > > +}

...

> > > +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.
> >
> 
> You're right about the order but if you _add() then you should
> _delete(). You typically _destroy() if you earlier _create().

Maybe, but as we noticed above my suggestion would make the code less error
prone to begin with.

> > > +}

...

> > > +     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.
> 
> I'm not following, please rephrase.

	scoped_guard() {
		call_my_new_helper_which_is_easier to read();
	}

	ret = devm_add_...

OR

call_my_new_helper_which_is_easier to read()
{
	guard()()

	...
}

	call_my_new_helper_which_is_easier to read();

	ret = devm_add_...


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

-- 
With Best Regards,
Andy Shevchenko