diff mbox series

[v3,14/25] ASoC: renesas: rz-ssi: Use goto label names that specify their actions

Message ID 20241113133540.2005850-15-claudiu.beznea.uj@bp.renesas.com
State New
Headers show
Series Add audio support for the Renesas RZ/G3S SoC | expand

Commit Message

Claudiu Beznea Nov. 13, 2024, 1:35 p.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use goto label names that specify their action. In this way we can have
a better understanding of what is the action associated with the label
by just reading the label name.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v3:
- s/sh/renesas in patch title

Changes in v2:
- none

 sound/soc/renesas/rz-ssi.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Geert Uytterhoeven Dec. 9, 2024, 1:51 p.m. UTC | #1 
Hi Claudiu,

On Wed, Nov 13, 2024 at 2:36 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Use goto label names that specify their action. In this way we can have
> a better understanding of what is the action associated with the label
> by just reading the label name.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/sound/soc/renesas/rz-ssi.c
> +++ b/sound/soc/renesas/rz-ssi.c
> @@ -1084,15 +1084,15 @@ static int rz_ssi_probe(struct platform_device *pdev)
>         /* Error Interrupt */
>         ssi->irq_int = platform_get_irq_byname(pdev, "int_req");
>         if (ssi->irq_int < 0) {
> -               rz_ssi_release_dma_channels(ssi);
> -               return ssi->irq_int;
> +               ret = ssi->irq_int;
> +               goto err_release_dma_chs;
>         }
>
>         ret = devm_request_irq(dev, ssi->irq_int, &rz_ssi_interrupt,
>                                0, dev_name(dev), ssi);
>         if (ret < 0) {
> -               rz_ssi_release_dma_channels(ssi);
> -               return dev_err_probe(dev, ret, "irq request error (int_req)\n");
> +               dev_err_probe(dev, ret, "irq request error (int_req)\n");
> +               goto err_release_dma_chs;
>         }
>
>         if (!rz_ssi_is_dma_enabled(ssi)) {

Inside this block there are several return statements.
As we know DMA is not available when we get here, these do not
need to call rz_ssi_release_dma_channels() hence do not use
"goto err_release_dma_chs".
However, this may be missed when making future changes.
So perhaps it may be prudent to make this safer, by moving this inside
the failure branch of the rz_ssi_dma_request() check above?

Gr{oetje,eeting}s,

                        Geert
Claudiu Beznea Dec. 10, 2024, 9:56 a.m. UTC | #2 
Hi, Geert,

On 09.12.2024 15:51, Geert Uytterhoeven wrote:
> Inside this block there are several return statements.
> As we know DMA is not available when we get here, these do not
> need to call rz_ssi_release_dma_channels() hence do not use
> "goto err_release_dma_chs".
> However, this may be missed when making future changes.
> So perhaps it may be prudent to make this safer, by moving this inside
> the failure branch of the rz_ssi_dma_request() check above?

I agree! As this series is already big enough I would prefer to handle it
after it is integrated. Keeping it like this doesn't impact the RZ/G3S support.

Are you OK with this approach?

Thank you for your review,
Claudiu
Geert Uytterhoeven Dec. 10, 2024, 10:01 a.m. UTC | #3 
Hi Claudiu,

On Tue, Dec 10, 2024 at 10:56 AM Claudiu Beznea
<claudiu.beznea@tuxon.dev> wrote:
> On 09.12.2024 15:51, Geert Uytterhoeven wrote:
> > Inside this block there are several return statements.
> > As we know DMA is not available when we get here, these do not
> > need to call rz_ssi_release_dma_channels() hence do not use
> > "goto err_release_dma_chs".
> > However, this may be missed when making future changes.
> > So perhaps it may be prudent to make this safer, by moving this inside
> > the failure branch of the rz_ssi_dma_request() check above?
>
> I agree! As this series is already big enough I would prefer to handle it
> after it is integrated. Keeping it like this doesn't impact the RZ/G3S support.
>
> Are you OK with this approach?

Fine for me!

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
index 2f56c63582e7..4af381f6d470 100644
--- a/sound/soc/renesas/rz-ssi.c
+++ b/sound/soc/renesas/rz-ssi.c
@@ -1084,15 +1084,15 @@  static int rz_ssi_probe(struct platform_device *pdev)
 	/* Error Interrupt */
 	ssi->irq_int = platform_get_irq_byname(pdev, "int_req");
 	if (ssi->irq_int < 0) {
-		rz_ssi_release_dma_channels(ssi);
-		return ssi->irq_int;
+		ret = ssi->irq_int;
+		goto err_release_dma_chs;
 	}
 
 	ret = devm_request_irq(dev, ssi->irq_int, &rz_ssi_interrupt,
 			       0, dev_name(dev), ssi);
 	if (ret < 0) {
-		rz_ssi_release_dma_channels(ssi);
-		return dev_err_probe(dev, ret, "irq request error (int_req)\n");
+		dev_err_probe(dev, ret, "irq request error (int_req)\n");
+		goto err_release_dma_chs;
 	}
 
 	if (!rz_ssi_is_dma_enabled(ssi)) {
@@ -1136,7 +1136,7 @@  static int rz_ssi_probe(struct platform_device *pdev)
 	ssi->rstc = devm_reset_control_get_exclusive(dev, NULL);
 	if (IS_ERR(ssi->rstc)) {
 		ret = PTR_ERR(ssi->rstc);
-		goto err_reset;
+		goto err_release_dma_chs;
 	}
 
 	reset_control_deassert(ssi->rstc);
@@ -1152,17 +1152,17 @@  static int rz_ssi_probe(struct platform_device *pdev)
 					      ARRAY_SIZE(rz_ssi_soc_dai));
 	if (ret < 0) {
 		dev_err(dev, "failed to register snd component\n");
-		goto err_snd_soc;
+		goto err_pm_put;
 	}
 
 	return 0;
 
-err_snd_soc:
+err_pm_put:
 	pm_runtime_put(dev);
 err_pm:
 	pm_runtime_disable(dev);
 	reset_control_assert(ssi->rstc);
-err_reset:
+err_release_dma_chs:
 	rz_ssi_release_dma_channels(ssi);
 
 	return ret;