| Message ID | 20250127113723.24479-5-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Support for RZ/G3E ICU | expand |
On Mo, 2025-01-27 at 11:36 +0000, Biju Das wrote: > Use devm_reset_control_get_exclusive_deasserted() to simplify > rzv2h_icu_init(). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v1->v2: > * Split the simplification patch into two. > * Updated commit header and description. > * Replaced devm_reset_control_get_optional_exclusive_deasserted()-> > devm_reset_control_get_exclusive_deasserted(). > * Moved simplification using devm_pm_runtime_enable() to next patch. > --- > drivers/irqchip/irq-renesas-rzv2h.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c > index fd432b43506b..5c5d2e015a88 100644 > --- a/drivers/irqchip/irq-renesas-rzv2h.c > +++ b/drivers/irqchip/irq-renesas-rzv2h.c > @@ -456,15 +456,10 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node *parent) > goto put_dev; > } > > - resetn = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + resetn = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL); > if (IS_ERR(resetn)) { > ret = PTR_ERR(resetn); > - goto put_dev; > - } > - > - ret = reset_control_deassert(resetn); > - if (ret) { > - dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > + dev_err(&pdev->dev, "failed to acquire deasserted reset: %d\n", ret); > goto put_dev; > } > > @@ -495,7 +490,6 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node *parent) > pm_runtime_put(&pdev->dev); > pm_disable: > pm_runtime_disable(&pdev->dev); > - reset_control_assert(resetn); > put_dev: > put_device(&pdev->dev); This is mixing devres cleanup and manual cleanup in a way that is not obviously safe. Specifically, put_device() will be called before the reset control is asserted in the error path. To keep asserting the reset control first, put_device() could be called via devm_add_action_or_reset(). regards Philipp
Hi Philipp Zabel, Thanks for the feedback. > -----Original Message----- > From: Philipp Zabel <p.zabel@pengutronix.de> > Sent: 27 January 2025 14:04 > Subject: Re: [PATCH v2 04/12] irqchip/renesas-rzv2h: Use devm_reset_control_get_exclusive_deasserted() > > On Mo, 2025-01-27 at 11:36 +0000, Biju Das wrote: > > Use devm_reset_control_get_exclusive_deasserted() to simplify > > rzv2h_icu_init(). > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v1->v2: > > * Split the simplification patch into two. > > * Updated commit header and description. > > * Replaced devm_reset_control_get_optional_exclusive_deasserted()-> > > devm_reset_control_get_exclusive_deasserted(). > > * Moved simplification using devm_pm_runtime_enable() to next patch. > > --- > > drivers/irqchip/irq-renesas-rzv2h.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/irqchip/irq-renesas-rzv2h.c > > b/drivers/irqchip/irq-renesas-rzv2h.c > > index fd432b43506b..5c5d2e015a88 100644 > > --- a/drivers/irqchip/irq-renesas-rzv2h.c > > +++ b/drivers/irqchip/irq-renesas-rzv2h.c > > @@ -456,15 +456,10 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node > *parent) > > goto put_dev; > > } > > > > - resetn = devm_reset_control_get_exclusive(&pdev->dev, NULL); > > + resetn = devm_reset_control_get_exclusive_deasserted(&pdev->dev, > > +NULL); > > if (IS_ERR(resetn)) { > > ret = PTR_ERR(resetn); > > - goto put_dev; > > - } > > - > > - ret = reset_control_deassert(resetn); > > - if (ret) { > > - dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); > > + dev_err(&pdev->dev, "failed to acquire deasserted reset: %d\n", > > +ret); > > goto put_dev; > > } > > > > @@ -495,7 +490,6 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node *parent) > > pm_runtime_put(&pdev->dev); > > pm_disable: > > pm_runtime_disable(&pdev->dev); > > - reset_control_assert(resetn); > > put_dev: > > put_device(&pdev->dev); > > This is mixing devres cleanup and manual cleanup in a way that is not obviously safe. Specifically, > put_device() will be called before the reset control is asserted in the error path. To keep asserting > the reset control first, put_device() could be called via devm_add_action_or_reset(). OK, I will add devm_add_action_or_reset() to call put_device() in case of error in probe(). Cheers, Biju
diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c index fd432b43506b..5c5d2e015a88 100644 --- a/drivers/irqchip/irq-renesas-rzv2h.c +++ b/drivers/irqchip/irq-renesas-rzv2h.c @@ -456,15 +456,10 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node *parent) goto put_dev; } - resetn = devm_reset_control_get_exclusive(&pdev->dev, NULL); + resetn = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL); if (IS_ERR(resetn)) { ret = PTR_ERR(resetn); - goto put_dev; - } - - ret = reset_control_deassert(resetn); - if (ret) { - dev_err(&pdev->dev, "failed to deassert resetn pin, %d\n", ret); + dev_err(&pdev->dev, "failed to acquire deasserted reset: %d\n", ret); goto put_dev; } @@ -495,7 +490,6 @@ static int rzv2h_icu_init(struct device_node *node, struct device_node *parent) pm_runtime_put(&pdev->dev); pm_disable: pm_runtime_disable(&pdev->dev); - reset_control_assert(resetn); put_dev: put_device(&pdev->dev);
Use devm_reset_control_get_exclusive_deasserted() to simplify rzv2h_icu_init(). Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * Split the simplification patch into two. * Updated commit header and description. * Replaced devm_reset_control_get_optional_exclusive_deasserted()-> devm_reset_control_get_exclusive_deasserted(). * Moved simplification using devm_pm_runtime_enable() to next patch. --- drivers/irqchip/irq-renesas-rzv2h.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)