| 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 |
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
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
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
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 --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);
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(-)