diff mbox series

[7/9] reset: make the provider of reset-gpios the parent of the reset device

Message ID 20251006-reset-gpios-swnodes-v1-7-6d3325b9af42@linaro.org
State New
Headers show
Series reset: rework reset-gpios handling | expand

Commit Message

Bartosz Golaszewski Oct. 6, 2025, 1 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Auxiliary devices really do need a parent so ahead of converting the
reset-gpios driver to registering on the auxiliary bus, make the GPIO
device that provides the reset GPIO the parent of the reset-gpio device.
To that end move the lookup of the GPIO device by fwnode to the
beginning of __reset_add_reset_gpio_device() which has the added benefor
of bailing out earlier, before allocating resources for the virtual
device, if the chip is not up yet.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Philipp Zabel Oct. 6, 2025, 3:19 p.m. UTC | #1
Hi Bartosz,

On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Auxiliary devices really do need a parent so ahead of converting the
> reset-gpios driver to registering on the auxiliary bus, make the GPIO
> device that provides the reset GPIO the parent of the reset-gpio device.
> To that end move the lookup of the GPIO device by fwnode to the
> beginning of __reset_add_reset_gpio_device() which has the added benefor

Typo: benefit.

> of bailing out earlier, before allocating resources for the virtual
> device, if the chip is not up yet.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/reset/core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 5a696e2dbcc224a633e2b321da53b7bc699cb5f3..ad85ddc8dd9fcf8b512cb09168586e0afca257f1 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -849,11 +849,11 @@ static void __reset_control_put_internal(struct reset_control *rstc)
>  	kref_put(&rstc->refcnt, __reset_control_release);
>  }
>  
> -static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> +static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
> +					 struct device_node *np,
>  					 unsigned int gpio,
>  					 unsigned int of_flags)
>  {
> -	const struct fwnode_handle *fwnode = of_fwnode_handle(np);
>  	unsigned int lookup_flags;
>  	const char *label_tmp;
>  
> @@ -868,10 +868,6 @@ static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
>  		return -EINVAL;
>  	}
>  
> -	struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode);
> -	if (!gdev)
> -		return -EPROBE_DEFER;
> -
>  	label_tmp = gpio_device_get_label(gdev);

This is the only remaining use of gdev in
__reset_add_reset_gpio_lookup().
It would make sense to move this as well and only pass the label.

Given that all this is removed in patch 9, this is not super important.

>  	if (!label_tmp)
>  		return -EINVAL;
> @@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  	if (args->args_count != 2)
>  		return -ENOENT;
>  
> +	struct gpio_device *gdev __free(gpio_device_put) =
> +		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));

We are mixing cleanup helpers with gotos in this function, which the
documentation in cleanup.h explicitly advises against.

I know the current code is already guilty, but could you take this
opportunity to prepend a patch that splits the part under guard() into
a separate function?

I'd also move this block after the lockdep_assert_not_held() below.


regards
Philipp
Bartosz Golaszewski Oct. 20, 2025, 3:25 p.m. UTC | #2
On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> >       if (!label_tmp)
> >               return -EINVAL;
> > @@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> >       if (args->args_count != 2)
> >               return -ENOENT;
> >
> > +     struct gpio_device *gdev __free(gpio_device_put) =
> > +             gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
>
> We are mixing cleanup helpers with gotos in this function, which the
> documentation in cleanup.h explicitly advises against.
>
> I know the current code is already guilty, but could you take this
> opportunity to prepend a patch that splits the part under guard() into
> a separate function?
>

If I'm being honest, I'd just make everything else use __free() as
well. Except for IDA, it's possible.

That being said: I have another thing in the works, namely converting
the OF code to fwnode in reset core. I may address this there as I'll
be moving stuff around. Does this make sense?

> I'd also move this block after the lockdep_assert_not_held() below.
>

Yeah lockdep asserts should be at the top of the function.

Bart
Bartosz Golaszewski Oct. 20, 2025, 3:56 p.m. UTC | #3
On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> >
> > @@ -868,10 +868,6 @@ static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> >               return -EINVAL;
> >       }
> >
> > -     struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode);
> > -     if (!gdev)
> > -             return -EPROBE_DEFER;
> > -
> >       label_tmp = gpio_device_get_label(gdev);
>
> This is the only remaining use of gdev in
> __reset_add_reset_gpio_lookup().
> It would make sense to move this as well and only pass the label.
>
> Given that all this is removed in patch 9, this is not super important.
>

I'll allow myself to skip it then because it causes a surprising
number of conflicts later into the series.

Bartosz
Philipp Zabel Oct. 21, 2025, 9:17 a.m. UTC | #4
On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > [...] could you take this
> > opportunity to prepend a patch that splits the part under guard() into
> > a separate function?
> 
> If I'm being honest, I'd just make everything else use __free() as
> well. Except for IDA, it's possible.
> 
> That being said: I have another thing in the works, namely converting
> the OF code to fwnode in reset core. I may address this there as I'll
> be moving stuff around. Does this make sense?

Yes. There was already a previous attempt at fwnode support [1], but we
abandoned that when there was no use case anymore.

[1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com

> 
regards
Philipp
Bartosz Golaszewski Oct. 21, 2025, 9:27 a.m. UTC | #5
On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > [...] could you take this
> > > opportunity to prepend a patch that splits the part under guard() into
> > > a separate function?
> >
> > If I'm being honest, I'd just make everything else use __free() as
> > well. Except for IDA, it's possible.
> >
> > That being said: I have another thing in the works, namely converting
> > the OF code to fwnode in reset core. I may address this there as I'll
> > be moving stuff around. Does this make sense?
>
> Yes. There was already a previous attempt at fwnode support [1], but we
> abandoned that when there was no use case anymore.
>
> [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
>

Ah, what was the exact reason for abandoning this? It's not clear from
the email thread.

To be clear: I think that we can convert the core reset code to fwnode
without necessarily converting all the drivers right away.

Bart
Philipp Zabel Oct. 21, 2025, 9:31 a.m. UTC | #6
On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > [...] could you take this
> > > > opportunity to prepend a patch that splits the part under guard() into
> > > > a separate function?
> > > 
> > > If I'm being honest, I'd just make everything else use __free() as
> > > well. Except for IDA, it's possible.
> > > 
> > > That being said: I have another thing in the works, namely converting
> > > the OF code to fwnode in reset core. I may address this there as I'll
> > > be moving stuff around. Does this make sense?
> > 
> > Yes. There was already a previous attempt at fwnode support [1], but we
> > abandoned that when there was no use case anymore.
> > 
> > [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
> > 
> 
> Ah, what was the exact reason for abandoning this? It's not clear from
> the email thread.
> 
> To be clear: I think that we can convert the core reset code to fwnode
> without necessarily converting all the drivers right away.

The use case vanished in patch review.

No need to convert all existing drivers right away, but I'd like to see
a user that benefits from the conversion.

regards
Philipp
Bartosz Golaszewski Oct. 21, 2025, 9:39 a.m. UTC | #7
On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > [...] could you take this
> > > > > opportunity to prepend a patch that splits the part under guard() into
> > > > > a separate function?
> > > >
> > > > If I'm being honest, I'd just make everything else use __free() as
> > > > well. Except for IDA, it's possible.
> > > >
> > > > That being said: I have another thing in the works, namely converting
> > > > the OF code to fwnode in reset core. I may address this there as I'll
> > > > be moving stuff around. Does this make sense?
> > >
> > > Yes. There was already a previous attempt at fwnode support [1], but we
> > > abandoned that when there was no use case anymore.
> > >
> > > [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
> > >
> >
> > Ah, what was the exact reason for abandoning this? It's not clear from
> > the email thread.
> >
> > To be clear: I think that we can convert the core reset code to fwnode
> > without necessarily converting all the drivers right away.
>
> The use case vanished in patch review.
>
> No need to convert all existing drivers right away, but I'd like to see
> a user that benefits from the conversion.
>

The first obvious user will be the reset-gpio driver which will see
its core code simplified as we won't need to cast between OF and
fwnodes.

Bartosz
Andy Shevchenko Oct. 21, 2025, 2:55 p.m. UTC | #8
On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > No need to convert all existing drivers right away, but I'd like to see
> > a user that benefits from the conversion.
> >
> 
> The first obvious user will be the reset-gpio driver which will see
> its core code simplified as we won't need to cast between OF and
> fwnodes.

+1 to Bart's work. reset-gpio in current form is useless in all my cases
(it's OF-centric in 2025! We should not do that in a new code).

More over, conversion to reset-gpio from open coded GPIO APIs is a clear
regression and I want to NAK all those changes (if any already done) for
the discrete components that may be used outside of certainly OF-only niche of
the platforms.
Andy Shevchenko Oct. 21, 2025, 3:03 p.m. UTC | #9
On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > No need to convert all existing drivers right away, but I'd like to see
> > > a user that benefits from the conversion.
> > >
> > 
> > The first obvious user will be the reset-gpio driver which will see
> > its core code simplified as we won't need to cast between OF and
> > fwnodes.
> 
> +1 to Bart's work. reset-gpio in current form is useless in all my cases
> (it's OF-centric in 2025! We should not do that in a new code).
> 
> More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> regression and I want to NAK all those changes (if any already done) for
> the discrete components that may be used outside of certainly OF-only niche of
> the platforms.

To be clear, the conversion that's done while reset-gpio is kept OF-centric.
I'm in favour of using it, but we need to make it agnostic.
Bartosz Golaszewski Oct. 21, 2025, 3:23 p.m. UTC | #10
On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
>
> [...]
>
> > > > No need to convert all existing drivers right away, but I'd like to see
> > > > a user that benefits from the conversion.
> > > >
> > >
> > > The first obvious user will be the reset-gpio driver which will see
> > > its core code simplified as we won't need to cast between OF and
> > > fwnodes.
> >
> > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > (it's OF-centric in 2025! We should not do that in a new code).
> >
> > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > regression and I want to NAK all those changes (if any already done) for
> > the discrete components that may be used outside of certainly OF-only niche of
> > the platforms.
>
> To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> I'm in favour of using it, but we need to make it agnostic.
>

As of now, the whole reset framework is completely OF-centric, I don't
know what good blocking any such conversions would bring? I intend to
convert the reset core but not individual drivers.

Bart
Andy Shevchenko Oct. 21, 2025, 3:47 p.m. UTC | #11
On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > a user that benefits from the conversion.
> > > > >
> > > >
> > > > The first obvious user will be the reset-gpio driver which will see
> > > > its core code simplified as we won't need to cast between OF and
> > > > fwnodes.
> > >
> > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > (it's OF-centric in 2025! We should not do that in a new code).
> > >
> > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > regression and I want to NAK all those changes (if any already done) for
> > > the discrete components that may be used outside of certainly OF-only niche of
> > > the platforms.
> >
> > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > I'm in favour of using it, but we need to make it agnostic.
> 
> As of now, the whole reset framework is completely OF-centric, I don't
> know what good blocking any such conversions would bring? I intend to
> convert the reset core but not individual drivers.

Blocking making new regressions?

Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
much fine with the idea and conversion.
Philipp Zabel Oct. 22, 2025, 8:39 a.m. UTC | #12
On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> 
> [...]
> 
> > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > a user that benefits from the conversion.
> > > > > > 
> > > > > 
> > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > its core code simplified as we won't need to cast between OF and
> > > > > fwnodes.
> > > > 
> > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > 
> > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > regression and I want to NAK all those changes (if any already done) for
> > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > the platforms.
> > > 
> > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > I'm in favour of using it, but we need to make it agnostic.
> > 
> > As of now, the whole reset framework is completely OF-centric, I don't
> > know what good blocking any such conversions would bring? I intend to
> > convert the reset core but not individual drivers.
> 
> Blocking making new regressions?
> 
> Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> much fine with the idea and conversion.

I think we might be talking about different "conversions" and different
"blocking" here?

1) Conversion of the reset core from of_node to fwnode.
2) Conversion of reset controller drivers from of_node to fwnode.
3) Conversion of consumer drivers from gpiod to reset_control API.

My understanding is:

Bartosz would like to convert the reset core to fwnode (1) but not
convert all the individual reset controller drivers (2). He doesn't
like blocking (1) - this statement was partially in reaction to me
bringing up a previous attempt that didn't go through.

Andy would like to block consumer driver conversions from gpiod to
reset_control API (3) while the reset-gpio driver only works on OF
platforms.

Please correct me if and where I misunderstood.

I think fwnode conversion of the reset controller framework core is a
good idea, I'd just like to see a use case accompanying the conversion.
It seems like enabling the reset-gpio driver to be used on non-OF
platforms could be that. Andy, do you have an actual case in mind?

Certainly dropping the gpiod reset handling from consumer drivers
should not be done while it introduces regressions.

regards
Philipp
Bartosz Golaszewski Oct. 22, 2025, 12:17 p.m. UTC | #13
On Wed, Oct 22, 2025 at 10:39 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> >
> > [...]
> >
> > > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > > a user that benefits from the conversion.
> > > > > > >
> > > > > >
> > > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > > its core code simplified as we won't need to cast between OF and
> > > > > > fwnodes.
> > > > >
> > > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > >
> > > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > > regression and I want to NAK all those changes (if any already done) for
> > > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > > the platforms.
> > > >
> > > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > > I'm in favour of using it, but we need to make it agnostic.
> > >
> > > As of now, the whole reset framework is completely OF-centric, I don't
> > > know what good blocking any such conversions would bring? I intend to
> > > convert the reset core but not individual drivers.
> >
> > Blocking making new regressions?
> >
> > Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> > much fine with the idea and conversion.
>
> I think we might be talking about different "conversions" and different
> "blocking" here?
>
> 1) Conversion of the reset core from of_node to fwnode.
> 2) Conversion of reset controller drivers from of_node to fwnode.
> 3) Conversion of consumer drivers from gpiod to reset_control API.
>
> My understanding is:
>
> Bartosz would like to convert the reset core to fwnode (1) but not
> convert all the individual reset controller drivers (2). He doesn't
> like blocking (1) - this statement was partially in reaction to me
> bringing up a previous attempt that didn't go through.
>
> Andy would like to block consumer driver conversions from gpiod to
> reset_control API (3) while the reset-gpio driver only works on OF
> platforms.
>
> Please correct me if and where I misunderstood.
>

I think Andy is afraid that people will convert drivers that are used
in the fwnode world to reset-gpio which only works with OF. I don't
think that anyone's trying to do it though.

> I think fwnode conversion of the reset controller framework core is a
> good idea, I'd just like to see a use case accompanying the conversion.
> It seems like enabling the reset-gpio driver to be used on non-OF
> platforms could be that. Andy, do you have an actual case in mind?
>

I'd say converting the reset core to fwnode has merits on its own. We
should typically use the highest available abstraction layer (which is
fwnode in this case) unless we absolutely have no choice (for
instance: using some very OF-specific APIs).

That being said: the reset-gpio driver will be able to work with any
firmware node once we do the conversion which is a good first
use-case.

Bartosz
Andy Shevchenko Oct. 22, 2025, 4:11 p.m. UTC | #14
On Wed, Oct 22, 2025 at 02:17:53PM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 22, 2025 at 10:39 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > > > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > > > a user that benefits from the conversion.
> > > > > > > >
> > > > > > >
> > > > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > > > its core code simplified as we won't need to cast between OF and
> > > > > > > fwnodes.
> > > > > >
> > > > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > > >
> > > > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > > > regression and I want to NAK all those changes (if any already done) for
> > > > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > > > the platforms.
> > > > >
> > > > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > > > I'm in favour of using it, but we need to make it agnostic.
> > > >
> > > > As of now, the whole reset framework is completely OF-centric, I don't
> > > > know what good blocking any such conversions would bring? I intend to
> > > > convert the reset core but not individual drivers.
> > >
> > > Blocking making new regressions?
> > >
> > > Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> > > much fine with the idea and conversion.
> >
> > I think we might be talking about different "conversions" and different
> > "blocking" here?
> >
> > 1) Conversion of the reset core from of_node to fwnode.
> > 2) Conversion of reset controller drivers from of_node to fwnode.
> > 3) Conversion of consumer drivers from gpiod to reset_control API.
> >
> > My understanding is:
> >
> > Bartosz would like to convert the reset core to fwnode (1) but not
> > convert all the individual reset controller drivers (2). He doesn't
> > like blocking (1) - this statement was partially in reaction to me
> > bringing up a previous attempt that didn't go through.
> >
> > Andy would like to block consumer driver conversions from gpiod to
> > reset_control API (3) while the reset-gpio driver only works on OF
> > platforms.
> >
> > Please correct me if and where I misunderstood.
> 
> I think Andy is afraid that people will convert drivers that are used
> in the fwnode world to reset-gpio which only works with OF. I don't
> think that anyone's trying to do it though.

You are both right about my worries and there is of course the case.
https://patch.msgid.link/1720009575-11677-1-git-send-email-shengjiu.wang@nxp.com

The mentioned change should be reverted.

And this was just found by a couple of minutes of `git log --grep`. I am pretty
sure there are handful of a such wrong patches.

Compare to https://patch.msgid.link/20250815172353.2430981-3-mohammad.rafi.shaik@oss.qualcomm.com
which is done correctly (it doesn't  break old functionality on non-OF platforms).

> > I think fwnode conversion of the reset controller framework core is a
> > good idea, I'd just like to see a use case accompanying the conversion.
> > It seems like enabling the reset-gpio driver to be used on non-OF
> > platforms could be that. Andy, do you have an actual case in mind?
> 
> I'd say converting the reset core to fwnode has merits on its own. We
> should typically use the highest available abstraction layer (which is
> fwnode in this case) unless we absolutely have no choice (for
> instance: using some very OF-specific APIs).
> 
> That being said: the reset-gpio driver will be able to work with any
> firmware node once we do the conversion which is a good first
> use-case.

+1, as I already mentioned I am in favour of this change.
diff mbox series

Patch

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 5a696e2dbcc224a633e2b321da53b7bc699cb5f3..ad85ddc8dd9fcf8b512cb09168586e0afca257f1 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -849,11 +849,11 @@  static void __reset_control_put_internal(struct reset_control *rstc)
 	kref_put(&rstc->refcnt, __reset_control_release);
 }
 
-static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
+static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
+					 struct device_node *np,
 					 unsigned int gpio,
 					 unsigned int of_flags)
 {
-	const struct fwnode_handle *fwnode = of_fwnode_handle(np);
 	unsigned int lookup_flags;
 	const char *label_tmp;
 
@@ -868,10 +868,6 @@  static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
 		return -EINVAL;
 	}
 
-	struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode);
-	if (!gdev)
-		return -EPROBE_DEFER;
-
 	label_tmp = gpio_device_get_label(gdev);
 	if (!label_tmp)
 		return -EINVAL;
@@ -919,6 +915,11 @@  static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	if (args->args_count != 2)
 		return -ENOENT;
 
+	struct gpio_device *gdev __free(gpio_device_put) =
+		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
+	if (!gdev)
+		return -EPROBE_DEFER;
+
 	/*
 	 * Registering reset-gpio device might cause immediate
 	 * bind, resulting in its probe() registering new reset controller thus
@@ -946,7 +947,7 @@  static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 		goto err_ida_free;
 	}
 
-	ret = __reset_add_reset_gpio_lookup(id, args->np, args->args[0],
+	ret = __reset_add_reset_gpio_lookup(gdev, id, args->np, args->args[0],
 					    args->args[1]);
 	if (ret < 0)
 		goto err_kfree;
@@ -958,7 +959,8 @@  static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 * Hold reference as long as rgpio_dev memory is valid.
 	 */
 	of_node_get(rgpio_dev->of_args.np);
-	pdev = platform_device_register_data(NULL, "reset-gpio", id,
+	pdev = platform_device_register_data(gpio_device_to_device(gdev),
+					     "reset-gpio", id,
 					     &rgpio_dev->of_args,
 					     sizeof(rgpio_dev->of_args));
 	ret = PTR_ERR_OR_ZERO(pdev);