1. Patchew
  2. linux
  3. View series

[PATCH] reset: gpio: add a devlink between reset-gpio and its consumer

Bartosz Golaszewski posted 1 patch 6 days, 10 hours ago 
drivers/reset/core.c | 73 +++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 58 insertions(+), 15 deletions(-)
Re: [PATCH] reset: gpio: add a devlink between reset-gpio and its consumer
Posted by Philipp Zabel 6 days, 9 hours ago 
Hi Bartosz,

On Di, 2025-11-25 at 13:55 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> The device that requests the reset control managed by the reset-gpio
> device is effectively its consumer but the devlink is only established
> between it and the GPIO controller exposing the reset pin. Add a devlink
> between the consumer of the reset control and its supplier. This will
> allow us to simplify the GPIOLIB code managing shared GPIOs when
> handling the corner case of reset-gpio and gpiolib-shared interacting.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> This change will allow us to simplify the code in gpiolib-shared.c in
> the next cycle, so it would be awesome if it could make v6.19.
> 
> Val: I'm Cc'ing you as you're already playing with audio on hamoa. If v2
> of the reset-gpios fix works for you, could you please also test this
> and make sure it doesn't break anything with soundwire?
> 
> Thanks in advance,
> Bart
> ---
>  drivers/reset/core.c | 73 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 0135dd0ae20498396fdb5a682f913b6048cb5750..15b8da9ebcf196f7d5ce7921e4f8aba0ea6b0de4 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -20,6 +20,7 @@
>  #include <linux/kref.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/reset.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
> @@ -82,6 +83,7 @@ struct reset_gpio_lookup {
>  	struct of_phandle_args of_args;
>  	struct fwnode_handle *swnode;
>  	struct list_head list;
> +	struct auxiliary_device *adev;
>  };
>  
>  static const char *rcdev_name(struct reset_controller_dev *rcdev)
> @@ -829,16 +831,16 @@ static void reset_gpio_aux_device_release(struct device *dev)
>  	kfree(adev);
>  }
>  
> -static int reset_add_gpio_aux_device(struct device *parent,
> -				     struct fwnode_handle *swnode,
> -				     int id, void *pdata)
> +static struct auxiliary_device *
> +reset_add_gpio_aux_device(struct device *parent, struct fwnode_handle *swnode,
> +			  int id, void *pdata)

This function grows ever more similar to auxiliary_device_create().

s/add/create/?

>  {
>  	struct auxiliary_device *adev;
>  	int ret;
>  
>  	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>  	if (!adev)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	adev->id = id;
>  	adev->name = "gpio";
> @@ -850,23 +852,55 @@ static int reset_add_gpio_aux_device(struct device *parent,
>  	ret = auxiliary_device_init(adev);
>  	if (ret) {
>  		kfree(adev);
> -		return ret;
> +		return ERR_PTR(ret);
>  	}
>  
>  	ret = __auxiliary_device_add(adev, "reset");
>  	if (ret) {
>  		auxiliary_device_uninit(adev);
>  		kfree(adev);
> -		return ret;
> +		return ERR_PTR(ret);
>  	}
>  
> -	return ret;
> +	return adev;
> +}
> +
> +static void reset_gpio_add_devlink(struct device_node *np,
> +				   struct reset_gpio_lookup *rgpio_dev)
> +{
> +	struct device *consumer;
> +
> +	/*
> +	 * We must use get_dev_from_fwnode() and not of_find_device_by_node()
> +	 * because the latter only considers the platform bus while we want to
> +	 * get consumers of any kind that can be associated with firmware
> +	 * nodes: auxiliary, soundwire, etc.
> +	 */
> +	consumer = get_dev_from_fwnode(of_fwnode_handle(np));

If called via __reset_control_get(), this just reconstructs the device
from dev->of_node. I think it would be better to move the linking into
__reset_control_get() and use the passed in consumer device directly.

> +	if (consumer) {
> +		if (!device_link_add(consumer, &rgpio_dev->adev->dev, DL_FLAG_STATELESS))

Who removes this link when the consumer puts the reset control, or if
we error out later?

> +			pr_warn("Failed to create a device link between reset-gpio and its consumer");
> +
> +		put_device(consumer);
> +	}
> +	/*
> +	 * else { }
> +	 *
> +	 * TODO: If ever there's a case where we need to support shared
> +	 * reset-gpios retrieved from a device node for which there's no
> +	 * device present yet, this is where we'd set up a notifier waiting
> +	 * for the device to appear in the system. This would be a lot of code
> +	 * that would go unused for now so let's cross that bridge when and if
> +	 * we get there.
> +	 */
>  }
>  
>  /*
> - * @args:	phandle to the GPIO provider with all the args like GPIO number
> + * @np: OF-node associated with the consumer
> + * @args: phandle to the GPIO provider with all the args like GPIO number
>   */
> -static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> +static int __reset_add_reset_gpio_device(struct device_node *np,
> +					 const struct of_phandle_args *args)
>  {
>  	struct property_entry properties[2] = { };
>  	unsigned int offset, of_flags, lflags;
> @@ -916,8 +950,14 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  
>  	list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) {
>  		if (args->np == rgpio_dev->of_args.np) {
> -			if (of_phandle_args_equal(args, &rgpio_dev->of_args))
> -				return 0; /* Already on the list, done */
> +			if (of_phandle_args_equal(args, &rgpio_dev->of_args)) {
> +				/*
> +				 * Already on the list, create the device link
> +				 * and stop here.
> +				 */
> +				reset_gpio_add_devlink(np, rgpio_dev);

I think instead of linking here ...

> +				return 0;
> +			}
>  		}
>  	}
>  
> @@ -950,11 +990,14 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  		goto err_put_of_node;
>  	}
>  
> -	ret = reset_add_gpio_aux_device(parent, rgpio_dev->swnode, id,
> -					&rgpio_dev->of_args);
> -	if (ret)
> +	rgpio_dev->adev = reset_add_gpio_aux_device(parent, rgpio_dev->swnode,
> +						    id, &rgpio_dev->of_args);
> +	if (IS_ERR(rgpio_dev->adev)) {
> +		ret = PTR_ERR(rgpio_dev->adev);
>  		goto err_del_swnode;
> +	}
>  
> +	reset_gpio_add_devlink(np, rgpio_dev);

... and here, we could just create a device link between the passed in
consumer dev and rstc->rcdev->dev in __reset_control_get().

regards
Philipp
Re: [PATCH] reset: gpio: add a devlink between reset-gpio and its consumer
Posted by Bartosz Golaszewski 5 days, 14 hours ago 
On Tue, Nov 25, 2025 at 3:33 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Bartosz,
>
> On Di, 2025-11-25 at 13:55 +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > The device that requests the reset control managed by the reset-gpio
> > device is effectively its consumer but the devlink is only established
> > between it and the GPIO controller exposing the reset pin. Add a devlink
> > between the consumer of the reset control and its supplier. This will
> > allow us to simplify the GPIOLIB code managing shared GPIOs when
> > handling the corner case of reset-gpio and gpiolib-shared interacting.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> > This change will allow us to simplify the code in gpiolib-shared.c in
> > the next cycle, so it would be awesome if it could make v6.19.
> >
> > Val: I'm Cc'ing you as you're already playing with audio on hamoa. If v2
> > of the reset-gpios fix works for you, could you please also test this
> > and make sure it doesn't break anything with soundwire?
> >
> > Thanks in advance,
> > Bart
> > ---
> >  drivers/reset/core.c | 73 +++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 58 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> >
> > -static int reset_add_gpio_aux_device(struct device *parent,
> > -                                  struct fwnode_handle *swnode,
> > -                                  int id, void *pdata)
> > +static struct auxiliary_device *
> > +reset_add_gpio_aux_device(struct device *parent, struct fwnode_handle *swnode,
> > +                       int id, void *pdata)
>
> This function grows ever more similar to auxiliary_device_create().
>

I could have used it if not for the fact that it calls
device_set_of_node_from_dev() and we don't want it.

> s/add/create/?

I don't mind but would prefer to not overload this patch.

> > +
> > +static void reset_gpio_add_devlink(struct device_node *np,
> > +                                struct reset_gpio_lookup *rgpio_dev)
> > +{
> > +     struct device *consumer;
> > +
> > +     /*
> > +      * We must use get_dev_from_fwnode() and not of_find_device_by_node()
> > +      * because the latter only considers the platform bus while we want to
> > +      * get consumers of any kind that can be associated with firmware
> > +      * nodes: auxiliary, soundwire, etc.
> > +      */
> > +     consumer = get_dev_from_fwnode(of_fwnode_handle(np));
>
> If called via __reset_control_get(), this just reconstructs the device
> from dev->of_node. I think it would be better to move the linking into
> __reset_control_get() and use the passed in consumer device directly.
>

That would affect all users, do we want this? In most cases they will
already have a link with different flags. I don't think this is
correct.

> > +     if (consumer) {
> > +             if (!device_link_add(consumer, &rgpio_dev->adev->dev, DL_FLAG_STATELESS))
>
> Who removes this link when the consumer puts the reset control, or if
> we error out later?
>

Nobody. Here's why: the reset-gpio device never gets removed. If we're
here, this means it's already registered. If the consumer is unbound,
the devlink stays in place. When we rebind, device_link_add() will do
nothing. If the consumer device is released, we purge all links
anyway. Let me know if I'm wrong, but seems to me, it's fine to leave
it and make it stateless.

Bart
Re: [PATCH] reset: gpio: add a devlink between reset-gpio and its consumer
Posted by Philipp Zabel 5 days, 12 hours ago 
On Mi, 2025-11-26 at 09:56 +0100, Bartosz Golaszewski wrote:

[...]
> > > -static int reset_add_gpio_aux_device(struct device *parent,
> > > -                                  struct fwnode_handle *swnode,
> > > -                                  int id, void *pdata)
> > > +static struct auxiliary_device *
> > > +reset_add_gpio_aux_device(struct device *parent, struct fwnode_handle *swnode,
> > > +                       int id, void *pdata)
> > 
> > This function grows ever more similar to auxiliary_device_create().
> > 
> 
> I could have used it if not for the fact that it calls
> device_set_of_node_from_dev() and we don't want it.
> 
> > s/add/create/?
> 
> I don't mind but would prefer to not overload this patch.

Ok.

[...]
> > > +     consumer = get_dev_from_fwnode(of_fwnode_handle(np));
> > 
> > If called via __reset_control_get(), this just reconstructs the device
> > from dev->of_node. I think it would be better to move the linking into
> > __reset_control_get() and use the passed in consumer device directly.
> > 
> 
> That would affect all users, do we want this? In most cases they will
> already have a link with different flags. I don't think this is
> correct.

Right. We could also pass an optional dev into __of_reset_control_get()
and then just go

	if (gpio_fallback)
		device_link_add(dev, rcdev->dev, DL_FLAG_STATELESS);

at the end.

> > > +     if (consumer) {
> > > +             if (!device_link_add(consumer, &rgpio_dev->adev->dev, DL_FLAG_STATELESS))
> > 
> > Who removes this link when the consumer puts the reset control, or if
> > we error out later?
> > 
> 
> Nobody. Here's why: the reset-gpio device never gets removed. If we're
> here, this means it's already registered. If the consumer is unbound,
> the devlink stays in place. When we rebind, device_link_add() will do
> nothing. If the consumer device is released, we purge all links
> anyway. Let me know if I'm wrong, but seems to me, it's fine to leave
> it and make it stateless.

I think you are right. Still, the device_link_add() documentation says
that the caller is expected to call device_link_del/remove() and we
don't, so warrants a comment.

regards
Philipp
Re: [PATCH] reset: gpio: add a devlink between reset-gpio and its consumer
Posted by Bartosz Golaszewski 5 days, 12 hours ago 
On Wed, Nov 26, 2025 at 11:38 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> [...]
> > > > +     consumer = get_dev_from_fwnode(of_fwnode_handle(np));
> > >
> > > If called via __reset_control_get(), this just reconstructs the device
> > > from dev->of_node. I think it would be better to move the linking into
> > > __reset_control_get() and use the passed in consumer device directly.
> > >
> >
> > That would affect all users, do we want this? In most cases they will
> > already have a link with different flags. I don't think this is
> > correct.
>
> Right. We could also pass an optional dev into __of_reset_control_get()
> and then just go
>
>         if (gpio_fallback)
>                 device_link_add(dev, rcdev->dev, DL_FLAG_STATELESS);
>
> at the end.

I think I'll do the conversion to fwnode first and see about adding
this next. I'm facing a different issue that may make it harder to use
devlinks in gpiolib-shared than I expected.

Let's leave this for now.

Bart

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.