| Message ID | 20220404172322.32578-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | mmc: renesas: Trivial fixes | expand |
Hi Prabhakar and Pavel, Thanks for the patch. > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of > returning directly > > Jump to error path "edisclk" instead of returning directly in case of > devm_reset_control_get_optional_exclusive() failure. > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible") > Reported-by: Pavel Machek <pavel@denx.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/mmc/host/renesas_sdhi_core.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c > b/drivers/mmc/host/renesas_sdhi_core.c > index 2797a9c0f17d..cddb0185f5fb 100644 > --- a/drivers/mmc/host/renesas_sdhi_core.c > +++ b/drivers/mmc/host/renesas_sdhi_core.c > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device > *pdev, > goto efree; > > priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, > NULL); > - if (IS_ERR(priv->rstc)) > - return PTR_ERR(priv->rstc); > + if (IS_ERR(priv->rstc)) { > + ret = PTR_ERR(priv->rstc); > + goto edisclk; > + } Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get? Cheers, Biju > > ver = sd_ctrl_read16(host, CTL_VERSION); > /* GEN2_SDR104 is first known SDHI to use 32bit block count */ > -- > 2.17.1
Hi Biju, Thank you for the review. On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhakar and Pavel, > > Thanks for the patch. > > > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of > > returning directly > > > > Jump to error path "edisclk" instead of returning directly in case of > > devm_reset_control_get_optional_exclusive() failure. > > > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible") > > Reported-by: Pavel Machek <pavel@denx.de> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/mmc/host/renesas_sdhi_core.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c > > b/drivers/mmc/host/renesas_sdhi_core.c > > index 2797a9c0f17d..cddb0185f5fb 100644 > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device > > *pdev, > > goto efree; > > > > priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, > > NULL); > > - if (IS_ERR(priv->rstc)) > > - return PTR_ERR(priv->rstc); > > + if (IS_ERR(priv->rstc)) { > > + ret = PTR_ERR(priv->rstc); > > + goto edisclk; > > + } > > Why can't devm_reset_control_get_optional_exclusive to be moved up before devm_clk_get? > In that case we will have to jump to the "efree" label Or if you don't want goto at all this can be moved to the very beginning of the probe. Wolfram, what is your preference on the above? Cheers, Prabhakar
Hi Prabhkar, > Subject: Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of > returning directly > > Hi Biju, > > Thank you for the review. > > On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > Hi Prabhakar and Pavel, > > > > Thanks for the patch. > > > > > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead > > > of returning directly > > > > > > Jump to error path "edisclk" instead of returning directly in case > > > of > > > devm_reset_control_get_optional_exclusive() failure. > > > > > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if > > > possible") > > > Reported-by: Pavel Machek <pavel@denx.de> > > > Signed-off-by: Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/mmc/host/renesas_sdhi_core.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c > > > b/drivers/mmc/host/renesas_sdhi_core.c > > > index 2797a9c0f17d..cddb0185f5fb 100644 > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device > > > *pdev, > > > goto efree; > > > > > > priv->rstc = > > > devm_reset_control_get_optional_exclusive(&pdev->dev, > > > NULL); > > > - if (IS_ERR(priv->rstc)) > > > - return PTR_ERR(priv->rstc); > > > + if (IS_ERR(priv->rstc)) { > > > + ret = PTR_ERR(priv->rstc); > > > + goto edisclk; > > > + } > > > > Why can't devm_reset_control_get_optional_exclusive to be moved up > before devm_clk_get? > > > In that case we will have to jump to the "efree" label Or if you don't > want goto at all this can be moved to the very beginning of the probe. I guess it has to move up, first get reset handle and clock handle and return error directly in case of error, Then do clk/reset ops. > > Wolfram, what is your preference on the above? > > Cheers, > Prabhakar
Hi Biju, On Mon, Apr 4, 2022 at 7:12 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Hi Prabhkar, > > > Subject: Re: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead of > > returning directly > > > > Hi Biju, > > > > Thank you for the review. > > > > On Mon, Apr 4, 2022 at 7:02 PM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > > > Hi Prabhakar and Pavel, > > > > > > Thanks for the patch. > > > > > > > Subject: [PATCH 1/2] mmc: renesas_sdhi: Jump to error path instead > > > > of returning directly > > > > > > > > Jump to error path "edisclk" instead of returning directly in case > > > > of > > > > devm_reset_control_get_optional_exclusive() failure. > > > > > > > > Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if > > > > possible") > > > > Reported-by: Pavel Machek <pavel@denx.de> > > > > Signed-off-by: Lad Prabhakar > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > drivers/mmc/host/renesas_sdhi_core.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mmc/host/renesas_sdhi_core.c > > > > b/drivers/mmc/host/renesas_sdhi_core.c > > > > index 2797a9c0f17d..cddb0185f5fb 100644 > > > > --- a/drivers/mmc/host/renesas_sdhi_core.c > > > > +++ b/drivers/mmc/host/renesas_sdhi_core.c > > > > @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device > > > > *pdev, > > > > goto efree; > > > > > > > > priv->rstc = > > > > devm_reset_control_get_optional_exclusive(&pdev->dev, > > > > NULL); > > > > - if (IS_ERR(priv->rstc)) > > > > - return PTR_ERR(priv->rstc); > > > > + if (IS_ERR(priv->rstc)) { > > > > + ret = PTR_ERR(priv->rstc); > > > > + goto edisclk; > > > > + } > > > > > > Why can't devm_reset_control_get_optional_exclusive to be moved up > > before devm_clk_get? > > > > > In that case we will have to jump to the "efree" label Or if you don't > > want goto at all this can be moved to the very beginning of the probe. > > I guess it has to move up, first get reset handle and clock handle and return error > directly in case of error, Then do clk/reset ops. > Fine by me. Cheers, Prabhakar > > > > Wolfram, what is your preference on the above? > > > > Cheers, > > Prabhakar
> I guess it has to move up, first get reset handle and clock handle and return error > directly in case of error, Then do clk/reset ops. > > > > > Wolfram, what is your preference on the above? Yes, moving up makes sense. First check all the handles before we actually initialize the hardware. Thanks for pointing all this out.
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 2797a9c0f17d..cddb0185f5fb 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -1033,8 +1033,10 @@ int renesas_sdhi_probe(struct platform_device *pdev, goto efree; priv->rstc = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL); - if (IS_ERR(priv->rstc)) - return PTR_ERR(priv->rstc); + if (IS_ERR(priv->rstc)) { + ret = PTR_ERR(priv->rstc); + goto edisclk; + } ver = sd_ctrl_read16(host, CTL_VERSION); /* GEN2_SDR104 is first known SDHI to use 32bit block count */
Jump to error path "edisclk" instead of returning directly in case of devm_reset_control_get_optional_exclusive() failure. Fixes: b4d86f37eacb7 ("mmc: renesas_sdhi: do hard reset if possible") Reported-by: Pavel Machek <pavel@denx.de> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/mmc/host/renesas_sdhi_core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)