diff mbox series

reset: rzg2l-usbphy-ctrl: Move reset_deassert after devm_*()

Message ID 20240609201622.87166-1-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series reset: rzg2l-usbphy-ctrl: Move reset_deassert after devm_*() | expand

Commit Message

Biju Das June 9, 2024, 8:16 p.m. UTC
Move reset_control_deassert after devm_reset_controller_register() to
simplify the error path in probe().

While at it, drop the blank line before devm_reset_controller_register().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/reset/reset-rzg2l-usbphy-ctrl.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Geert Uytterhoeven June 10, 2024, 11:26 a.m. UTC | #1
Hi Biju,

Thanks for your patch!

On Sun, Jun 9, 2024 at 10:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Move reset_control_deassert after devm_reset_controller_register() to
> simplify the error path in probe().

Where's the simplification?
Oh, this patch fixes the issue that the reset is not re-asserted in
case devm_reset_controller_register() fails? Please say so.

> While at it, drop the blank line before devm_reset_controller_register().

I'd rather keep that blank line.

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/reset/reset-rzg2l-usbphy-ctrl.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> index 8f6fbd978591..93c65a57686d 100644
> --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> @@ -121,20 +121,19 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
>                 return dev_err_probe(dev, PTR_ERR(priv->rstc),
>                                      "failed to get reset\n");
>
> -       error = reset_control_deassert(priv->rstc);
> -       if (error)
> -               return error;
> -
>         priv->rcdev.ops = &rzg2l_usbphy_ctrl_reset_ops;
>         priv->rcdev.of_reset_n_cells = 1;
>         priv->rcdev.nr_resets = NUM_PORTS;
>         priv->rcdev.of_node = dev->of_node;
>         priv->rcdev.dev = dev;
> -
>         error = devm_reset_controller_register(dev, &priv->rcdev);

As soon as the reset controller is registered, it could be used by a
reset consumer, right?  Unfortunately all hardware setup is only done
after this registration, so I think the registration should be moved
to the end of the function.

>         if (error)
>                 return error;
>
> +       error = reset_control_deassert(priv->rstc);
> +       if (error)
> +               return error;
> +
>         spin_lock_init(&priv->lock);
>         dev_set_drvdata(dev, priv);

Gr{oetje,eeting}s,

                        Geert
Biju Das June 10, 2024, noon UTC | #2
Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, June 10, 2024 12:26 PM
> Subject: Re: [PATCH] reset: rzg2l-usbphy-ctrl: Move reset_deassert after devm_*()
> 
> Hi Biju,
> 
> Thanks for your patch!
> 
> On Sun, Jun 9, 2024 at 10:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Move reset_control_deassert after devm_reset_controller_register() to
> > simplify the error path in probe().
> 
> Where's the simplification?
> Oh, this patch fixes the issue that the reset is not re-asserted in case
> devm_reset_controller_register() fails? Please say so.

OK.

> 
> > While at it, drop the blank line before devm_reset_controller_register().
> 
> I'd rather keep that blank line.

OK.

> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/reset/reset-rzg2l-usbphy-ctrl.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > index 8f6fbd978591..93c65a57686d 100644
> > --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
> > @@ -121,20 +121,19 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
> >                 return dev_err_probe(dev, PTR_ERR(priv->rstc),
> >                                      "failed to get reset\n");
> >
> > -       error = reset_control_deassert(priv->rstc);
> > -       if (error)
> > -               return error;
> > -
> >         priv->rcdev.ops = &rzg2l_usbphy_ctrl_reset_ops;
> >         priv->rcdev.of_reset_n_cells = 1;
> >         priv->rcdev.nr_resets = NUM_PORTS;
> >         priv->rcdev.of_node = dev->of_node;
> >         priv->rcdev.dev = dev;
> > -
> >         error = devm_reset_controller_register(dev, &priv->rcdev);
> 
> As soon as the reset controller is registered, it could be used by a reset consumer, right?

That is correct.

> Unfortunately all hardware setup is only done after this registration, so I think the registration
> should be moved to the end of the function.

OK. I will move this to the very end, after putting pll and phy into reset state.

Also, I am planning to replace pm_runtime_enable(), pm_runtime_resume_and_get()
with devm_clk_enabled() to simplify the probe()/remove() as separate patch.
I guess it is ok to you??

Cheers,
Biju

> 
> >         if (error)
> >                 return error;
> >
> > +       error = reset_control_deassert(priv->rstc);
> > +       if (error)
> > +               return error;
> > +
> >         spin_lock_init(&priv->lock);
> >         dev_set_drvdata(dev, priv);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But when I'm talking to
> journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
Geert Uytterhoeven June 10, 2024, 12:09 p.m. UTC | #3
Hi Biju,

On Mon, Jun 10, 2024 at 2:00 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Also, I am planning to replace pm_runtime_enable(), pm_runtime_resume_and_get()
> with devm_clk_enabled() to simplify the probe()/remove() as separate patch.
> I guess it is ok to you??

I don't think that is a good idea, as it will cause breakage when
extending PM Domain support on RZ/G2L (and RZ/G3S?).

BTW, devm_pm_runtime_enable() does exist,
devm_pm_runtime_resume_and_get() doesn't yet.

Gr{oetje,eeting}s,

                        Geert
Biju Das June 10, 2024, 12:16 p.m. UTC | #4
Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, June 10, 2024 1:09 PM
> Subject: Re: [PATCH] reset: rzg2l-usbphy-ctrl: Move reset_deassert after devm_*()
> 
> Hi Biju,
> 
> On Mon, Jun 10, 2024 at 2:00 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Also, I am planning to replace pm_runtime_enable(),
> > pm_runtime_resume_and_get() with devm_clk_enabled() to simplify the probe()/remove() as separate
> patch.
> > I guess it is ok to you??
> 
> I don't think that is a good idea, as it will cause breakage when extending PM Domain support on
> RZ/G2L (and RZ/G3S?).

OK, I haven’t thought about extending PM Domain support for RZ/G2L.

> BTW, devm_pm_runtime_enable() does exist,
OK.

> devm_pm_runtime_resume_and_get() doesn't yet.

Then in that case I will leave like as it is.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
index 8f6fbd978591..93c65a57686d 100644
--- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c
+++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c
@@ -121,20 +121,19 @@  static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(priv->rstc),
 				     "failed to get reset\n");
 
-	error = reset_control_deassert(priv->rstc);
-	if (error)
-		return error;
-
 	priv->rcdev.ops = &rzg2l_usbphy_ctrl_reset_ops;
 	priv->rcdev.of_reset_n_cells = 1;
 	priv->rcdev.nr_resets = NUM_PORTS;
 	priv->rcdev.of_node = dev->of_node;
 	priv->rcdev.dev = dev;
-
 	error = devm_reset_controller_register(dev, &priv->rcdev);
 	if (error)
 		return error;
 
+	error = reset_control_deassert(priv->rstc);
+	if (error)
+		return error;
+
 	spin_lock_init(&priv->lock);
 	dev_set_drvdata(dev, priv);