Hi Geert,
Thanks for the feedback.
> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx
>
> 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
