linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] ASoC: tegra: improve goto error label
@ 2018-07-20  8:04 Marcel Ziswiler
  2018-07-20  8:04 ` [PATCH 2/2] ASoC: tegra: probe deferral error reporting Marcel Ziswiler
  2018-07-20 12:19 ` Applied "ASoC: tegra: improve goto error label" to the asoc tree Mark Brown
  0 siblings, 2 replies; 13+ messages in thread
From: Marcel Ziswiler @ 2018-07-20  8:04 UTC (permalink / raw)
  To: alsa-devel, linux-tegra
  Cc: Marcel Ziswiler, Jaroslav Kysela, Thierry Reding,
	Jonathan Hunter, Mark Brown, linux-kernel, Takashi Iwai,
	Liam Girdwood

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

While the two error labels "err" and "err_clk_put" goto the same place
it is rather confusing that the earlier one is certainly used later
again.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

---

 sound/soc/tegra/tegra20_ac97.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index affad46bf188..682ef33afb5f 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -377,7 +377,7 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(ac97->clk_ac97);
 	if (ret) {
 		dev_err(&pdev->dev, "clk_enable failed: %d\n", ret);
-		goto err;
+		goto err_clk_put;
 	}
 
 	ret = snd_soc_set_ac97_ops(&tegra20_ac97_ops);
-- 
2.14.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-20  8:04 [PATCH 1/2] ASoC: tegra: improve goto error label Marcel Ziswiler
@ 2018-07-20  8:04 ` Marcel Ziswiler
  2018-07-20 12:16   ` Mark Brown
  2018-07-26  8:34   ` Stefan Agner
  2018-07-20 12:19 ` Applied "ASoC: tegra: improve goto error label" to the asoc tree Mark Brown
  1 sibling, 2 replies; 13+ messages in thread
From: Marcel Ziswiler @ 2018-07-20  8:04 UTC (permalink / raw)
  To: alsa-devel, linux-tegra
  Cc: Marcel Ziswiler, Jaroslav Kysela, Thierry Reding,
	Jonathan Hunter, Mark Brown, linux-kernel, Takashi Iwai,
	Liam Girdwood

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

Actually report the error codes from of_get_named_gpio() resp.
devm_gpio_request_one() upon trying to get the codec reset resp. sync
GPIOs which may as well just be a probe deferrals.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

---

 sound/soc/tegra/tegra20_ac97.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index 682ef33afb5f..4875512f0732 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -351,18 +351,21 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 		ret = devm_gpio_request_one(&pdev->dev, ac97->reset_gpio,
 					    GPIOF_OUT_INIT_HIGH, "codec-reset");
 		if (ret) {
-			dev_err(&pdev->dev, "could not get codec-reset GPIO\n");
+			dev_err(&pdev->dev, "could not get codec-reset GPIO: "
+					    "%d\n", ret);
 			goto err_clk_put;
 		}
 	} else {
-		dev_err(&pdev->dev, "no codec-reset GPIO supplied\n");
+		ret = ac97->reset_gpio;
+		dev_err(&pdev->dev, "no codec-reset GPIO supplied: %d\n", ret);
 		goto err_clk_put;
 	}
 
 	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
 					    "nvidia,codec-sync-gpio", 0);
 	if (!gpio_is_valid(ac97->sync_gpio)) {
-		dev_err(&pdev->dev, "no codec-sync GPIO supplied\n");
+		ret = ac97->sync_gpio;
+		dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n", ret);
 		goto err_clk_put;
 	}
 
-- 
2.14.4


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-20  8:04 ` [PATCH 2/2] ASoC: tegra: probe deferral error reporting Marcel Ziswiler
@ 2018-07-20 12:16   ` Mark Brown
  2018-07-20 12:31     ` Marcel Ziswiler
  2018-07-26  8:34   ` Stefan Agner
  1 sibling, 1 reply; 13+ messages in thread
From: Mark Brown @ 2018-07-20 12:16 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: alsa-devel, linux-tegra, Marcel Ziswiler, Jaroslav Kysela,
	Thierry Reding, Jonathan Hunter, linux-kernel, Takashi Iwai,
	Liam Girdwood

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Fri, Jul 20, 2018 at 10:04:24AM +0200, Marcel Ziswiler wrote:

> -			dev_err(&pdev->dev, "could not get codec-reset GPIO\n");
> +			dev_err(&pdev->dev, "could not get codec-reset GPIO: "
> +					    "%d\n", ret);

Don't split strings over lines like this, it causes problems when people
grep for errors.  It's better to go over 80 columns.

>  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
>  					    "nvidia,codec-sync-gpio", 0);
>  	if (!gpio_is_valid(ac97->sync_gpio)) {
> -		dev_err(&pdev->dev, "no codec-sync GPIO supplied\n");
> +		ret = ac97->sync_gpio;
> +		dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n", ret);
>  		goto err_clk_put;
>  	}

This isn't reporting an error code associated with the attempt to find a
codec-sync GPIO, it's the result of some other operation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Applied "ASoC: tegra: improve goto error label" to the asoc tree
  2018-07-20  8:04 [PATCH 1/2] ASoC: tegra: improve goto error label Marcel Ziswiler
  2018-07-20  8:04 ` [PATCH 2/2] ASoC: tegra: probe deferral error reporting Marcel Ziswiler
@ 2018-07-20 12:19 ` Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2018-07-20 12:19 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: Mark Brown, alsa-devel, linux-tegra, Liam Girdwood, Takashi Iwai,
	linux-kernel, Jonathan Hunter, Mark Brown, Thierry Reding,
	alsa-devel

The patch

   ASoC: tegra: improve goto error label

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 2ec42486358f63c4a426514c395d13f4b9de5da8 Mon Sep 17 00:00:00 2001
From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Date: Fri, 20 Jul 2018 10:04:23 +0200
Subject: [PATCH] ASoC: tegra: improve goto error label

While the two error labels "err" and "err_clk_put" goto the same place
it is rather confusing that the earlier one is certainly used later
again.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/tegra/tegra20_ac97.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index affad46bf188..682ef33afb5f 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -377,7 +377,7 @@ static int tegra20_ac97_platform_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(ac97->clk_ac97);
 	if (ret) {
 		dev_err(&pdev->dev, "clk_enable failed: %d\n", ret);
-		goto err;
+		goto err_clk_put;
 	}
 
 	ret = snd_soc_set_ac97_ops(&tegra20_ac97_ops);
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-20 12:16   ` Mark Brown
@ 2018-07-20 12:31     ` Marcel Ziswiler
  2018-07-21  9:56       ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Marcel Ziswiler @ 2018-07-20 12:31 UTC (permalink / raw)
  To: broonie
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	alsa-devel, linux-tegra

On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> On Fri, Jul 20, 2018 at 10:04:24AM +0200, Marcel Ziswiler wrote:
> 
> > -			dev_err(&pdev->dev, "could not get codec-
> > reset GPIO\n");
> > +			dev_err(&pdev->dev, "could not get codec-
> > reset GPIO: "
> > +					    "%d\n", ret);
> 
> Don't split strings over lines like this, it causes problems when
> people
> grep for errors.  It's better to go over 80 columns.

OK, will do. However, in this case I did not actually split anything
searchable anyway.

> >  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
> >  					    "nvidia,codec-sync-
> > gpio", 0);
> >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > supplied\n");
> > +		ret = ac97->sync_gpio;
> > +		dev_err(&pdev->dev, "no codec-sync GPIO supplied:
> > %d\n", ret);
> >  		goto err_clk_put;
> >  	}
> 
> This isn't reporting an error code associated with the attempt to
> find a
> codec-sync GPIO, it's the result of some other operation.

What exactly is then the of_get_named_gpio() above please doing if
not getting the codec sync GPIO? I am not following you, sorry.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-20 12:31     ` Marcel Ziswiler
@ 2018-07-21  9:56       ` Mark Brown
  2018-07-21 10:06         ` Marcel Ziswiler
  2018-07-21 11:17         ` Dmitry Osipenko
  0 siblings, 2 replies; 13+ messages in thread
From: Mark Brown @ 2018-07-21  9:56 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	alsa-devel, linux-tegra

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:

> > >  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
> > >  					    "nvidia,codec-sync-
> > > gpio", 0);
> > >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > supplied\n");
> > > +		ret = ac97->sync_gpio;
> > > +		dev_err(&pdev->dev, "no codec-sync GPIO supplied:
> > > %d\n", ret);
> > >  		goto err_clk_put;
> > >  	}

> > This isn't reporting an error code associated with the attempt to
> > find a
> > codec-sync GPIO, it's the result of some other operation.

> What exactly is then the of_get_named_gpio() above please doing if
> not getting the codec sync GPIO? I am not following you, sorry.

It's not in any way involved in setting the value of ret, whatever value
that has it's nothing to do with that operation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21  9:56       ` Mark Brown
@ 2018-07-21 10:06         ` Marcel Ziswiler
  2018-07-23 10:25           ` Mark Brown
  2018-07-21 11:17         ` Dmitry Osipenko
  1 sibling, 1 reply; 13+ messages in thread
From: Marcel Ziswiler @ 2018-07-21 10:06 UTC (permalink / raw)
  To: broonie
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	linux-tegra, alsa-devel

On Sat, 2018-07-21 at 10:56 +0100, Mark Brown wrote:
> On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> > On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> > > >  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
> > > >  					    "nvidia,codec-
> > > > sync-
> > > > gpio", 0);

Above ac97->sync_gpio gets return value from of_get_named_gpio(),
right?

> > > >  	if (!gpio_is_valid(ac97->sync_gpio)) {

Here it goes if ac97->sync_gpio is not a valid GPIO e.g. in an error
case as reported by above of_get_named_gpio(), right?

> > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > supplied\n");
> > > > +		ret = ac97->sync_gpio;

And here I assign ret with that return value from of_get_named_gpio(),
right?

> > > > +		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > supplied:
> > > > %d\n", ret);

And then I actually report what error it was (e.g. probe deferral).

> > > >  		goto err_clk_put;
> > > >  	}
> > > This isn't reporting an error code associated with the attempt to
> > > find a
> > > codec-sync GPIO, it's the result of some other operation.
> > What exactly is then the of_get_named_gpio() above please doing if
> > not getting the codec sync GPIO? I am not following you, sorry.
> 
> It's not in any way involved in setting the value of ret, whatever
> value
> that has it's nothing to do with that operation.

I really do not understand what you are trying to get at, sorry.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21  9:56       ` Mark Brown
  2018-07-21 10:06         ` Marcel Ziswiler
@ 2018-07-21 11:17         ` Dmitry Osipenko
  2018-07-21 11:55           ` Marcel Ziswiler
  1 sibling, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2018-07-21 11:17 UTC (permalink / raw)
  To: Mark Brown, Marcel Ziswiler
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	alsa-devel, linux-tegra

On Saturday, 21 July 2018 12:56:15 MSK Mark Brown wrote:
> On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> > On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> > > >  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
> > > >  	
> > > >  					    "nvidia,codec-sync-
> > > > 
> > > > gpio", 0);
> > > > 
> > > >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > > > 
> > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > supplied\n");
> > > > +		ret = ac97->sync_gpio;
> > > > +		dev_err(&pdev->dev, "no codec-sync GPIO supplied:
> > > > %d\n", ret);
> > > > 
> > > >  		goto err_clk_put;
> > > >  	
> > > >  	}
> > > 
> > > This isn't reporting an error code associated with the attempt to
> > > find a
> > > codec-sync GPIO, it's the result of some other operation.
> > 
> > What exactly is then the of_get_named_gpio() above please doing if
> > not getting the codec sync GPIO? I am not following you, sorry.
> 
> It's not in any way involved in setting the value of ret, whatever value
> that has it's nothing to do with that operation.

The comment to gpio_is_valid() says that it "Returns GPIO number to use with 
Linux generic GPIO API, or one of the errno value on the error condition". 
Comment doesn't explicitly states that the returned GPIO number is always 
valid, but it is kinda implied.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21 11:17         ` Dmitry Osipenko
@ 2018-07-21 11:55           ` Marcel Ziswiler
  2018-07-21 12:03             ` Dmitry Osipenko
  0 siblings, 1 reply; 13+ messages in thread
From: Marcel Ziswiler @ 2018-07-21 11:55 UTC (permalink / raw)
  To: digetx, broonie
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	linux-tegra, alsa-devel

On Sat, 2018-07-21 at 14:17 +0300, Dmitry Osipenko wrote:
> On Saturday, 21 July 2018 12:56:15 MSK Mark Brown wrote:
> > On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> > > On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> > > > >  	ac97->sync_gpio = of_get_named_gpio(pdev-
> > > > > >dev.of_node,
> > > > >  	
> > > > >  					    "nvidia,codec-
> > > > > sync-
> > > > > 
> > > > > gpio", 0);
> > > > > 
> > > > >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > > > > 
> > > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > supplied\n");
> > > > > +		ret = ac97->sync_gpio;
> > > > > +		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > supplied:
> > > > > %d\n", ret);
> > > > > 
> > > > >  		goto err_clk_put;
> > > > >  	
> > > > >  	}
> > > > 
> > > > This isn't reporting an error code associated with the attempt
> > > > to
> > > > find a
> > > > codec-sync GPIO, it's the result of some other operation.
> > > 
> > > What exactly is then the of_get_named_gpio() above please doing
> > > if
> > > not getting the codec sync GPIO? I am not following you, sorry.
> > 
> > It's not in any way involved in setting the value of ret, whatever
> > value
> > that has it's nothing to do with that operation.
> 
> The comment to gpio_is_valid() says that it "Returns GPIO number to
> use with 
> Linux generic GPIO API, or one of the errno value on the error
> condition". 
> Comment doesn't explicitly states that the returned GPIO number is
> always 
> valid, but it is kinda implied.

Do you mean I should be assigning the return value of gpio_is_valid()
to ret and use that instead?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21 11:55           ` Marcel Ziswiler
@ 2018-07-21 12:03             ` Dmitry Osipenko
  2018-07-21 12:15               ` Dmitry Osipenko
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2018-07-21 12:03 UTC (permalink / raw)
  To: Marcel Ziswiler, broonie
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	linux-tegra, alsa-devel

On Saturday, 21 July 2018 14:55:21 MSK Marcel Ziswiler wrote:
> On Sat, 2018-07-21 at 14:17 +0300, Dmitry Osipenko wrote:
> 
> > On Saturday, 21 July 2018 12:56:15 MSK Mark Brown wrote:
> > 
> > > On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> > > 
> > > > On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> > > > 
> > > > > >  	ac97->sync_gpio = of_get_named_gpio(pdev-
> > > > > > >
> > > > > > >dev.of_node,
> > > > > > >
> > > > > >  	
> > > > > >  	
> > > > > >  					    "nvidia,codec-
> > > > > > 
> > > > > > sync-
> > > > > > 
> > > > > > gpio", 0);
> > > > > > 
> > > > > > 
> > > > > >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > > > > > 
> > > > > > 
> > > > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > > supplied\n");
> > > > > > +		ret = ac97->sync_gpio;
> > > > > > +		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > > supplied:
> > > > > > %d\n", ret);
> > > > > > 
> > > > > > 
> > > > > >  		goto err_clk_put;
> > > > > >  	
> > > > > >  	
> > > > > >  	}
> > > > > 
> > > > > 
> > > > > This isn't reporting an error code associated with the attempt
> > > > > to
> > > > > find a
> > > > > codec-sync GPIO, it's the result of some other operation.
> > > > 
> > > > 
> > > > What exactly is then the of_get_named_gpio() above please doing
> > > > if
> > > > not getting the codec sync GPIO? I am not following you, sorry.
> > > 
> > > 
> > > It's not in any way involved in setting the value of ret, whatever
> > > value
> > > that has it's nothing to do with that operation.
> > 
> > 
> > The comment to gpio_is_valid() says that it "Returns GPIO number to
> > use with 
> > Linux generic GPIO API, or one of the errno value on the error
> > condition". 
> > Comment doesn't explicitly states that the returned GPIO number is
> > always 
> > valid, but it is kinda implied.
> 
> 
> Do you mean I should be assigning the return value of gpio_is_valid()
> to ret and use that instead?

No, gpio_is_valid() returns a boolean. I think your patch is fine as it is is.

Probably Mark meant something like this:

        ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
                                            "nvidia,codec-sync-gpio", 0);
        if (ac97->sync_gpio < 0) {
                ret = ac97->sync_gpio;
                dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n", ret);
                goto err_clk_put;
        }

        if (!gpio_is_valid(ac97->sync_gpio)) {
                ret = -EINVAL;
                goto err_clk_put;
       }

But that is not needed because of_get_named_gpio() returns either a valid GPIO 
number or a error code.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21 12:03             ` Dmitry Osipenko
@ 2018-07-21 12:15               ` Dmitry Osipenko
  0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2018-07-21 12:15 UTC (permalink / raw)
  To: Marcel Ziswiler, broonie
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	linux-tegra, alsa-devel

On Saturday, 21 July 2018 15:03:57 MSK Dmitry Osipenko wrote:
> On Saturday, 21 July 2018 14:55:21 MSK Marcel Ziswiler wrote:
> > On Sat, 2018-07-21 at 14:17 +0300, Dmitry Osipenko wrote:
> > > On Saturday, 21 July 2018 12:56:15 MSK Mark Brown wrote:
> > > > On Fri, Jul 20, 2018 at 12:31:07PM +0000, Marcel Ziswiler wrote:
> > > > > On Fri, 2018-07-20 at 13:16 +0100, Mark Brown wrote:
> > > > > > >  	ac97->sync_gpio = of_get_named_gpio(pdev-
> > > > > > > >
> > > > > > > >dev.of_node,
> > > > > > > >
> > > > > > >  					    "nvidia,codec-
> > > > > > > 
> > > > > > > sync-
> > > > > > > 
> > > > > > > gpio", 0);
> > > > > > > 
> > > > > > >  	if (!gpio_is_valid(ac97->sync_gpio)) {
> > > > > > > 
> > > > > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > > > supplied\n");
> > > > > > > +		ret = ac97->sync_gpio;
> > > > > > > +		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > > > supplied:
> > > > > > > %d\n", ret);
> > > > > > > 
> > > > > > >  		goto err_clk_put;
> > > > > > >  	
> > > > > > >  	}
> > > > > > 
> > > > > > This isn't reporting an error code associated with the attempt
> > > > > > to
> > > > > > find a
> > > > > > codec-sync GPIO, it's the result of some other operation.
> > > > > 
> > > > > What exactly is then the of_get_named_gpio() above please doing
> > > > > if
> > > > > not getting the codec sync GPIO? I am not following you, sorry.
> > > > 
> > > > It's not in any way involved in setting the value of ret, whatever
> > > > value
> > > > that has it's nothing to do with that operation.
> > > 
> > > The comment to gpio_is_valid() says that it "Returns GPIO number to
> > > use with
> > > Linux generic GPIO API, or one of the errno value on the error
> > > condition".
> > > Comment doesn't explicitly states that the returned GPIO number is
> > > always
> > > valid, but it is kinda implied.
> > 
> > Do you mean I should be assigning the return value of gpio_is_valid()
> > to ret and use that instead?
> 
> No, gpio_is_valid() returns a boolean. I think your patch is fine as it is
> is.
> 
> Probably Mark meant something like this:
> 
>         ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
>                                             "nvidia,codec-sync-gpio", 0);
>         if (ac97->sync_gpio < 0) {
>                 ret = ac97->sync_gpio;
>                 dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n",
> ret); goto err_clk_put;
>         }
> 
>         if (!gpio_is_valid(ac97->sync_gpio)) {
>                 ret = -EINVAL;
>                 goto err_clk_put;
>        }
> 
> But that is not needed because of_get_named_gpio() returns either a valid
> GPIO number or a error code.

Also note that tegra20_ac97 code doesn't check the returned value of:

	gpio_request(workdata->sync_gpio, "codec-sync");

That is a bit fragile. Probably would be better to move the GPIO requesting to 
the drivers probe function using the devm_gpio_request_one() and fail the 
driver probing if requesting fails. That should be a distinct patch if you'll 
want to implement that.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-21 10:06         ` Marcel Ziswiler
@ 2018-07-23 10:25           ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2018-07-23 10:25 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: linux-kernel, jonathanh, thierry.reding, tiwai, lgirdwood, perex,
	linux-tegra, alsa-devel

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Sat, Jul 21, 2018 at 10:06:23AM +0000, Marcel Ziswiler wrote:
> On Sat, 2018-07-21 at 10:56 +0100, Mark Brown wrote:

> > > > > -		dev_err(&pdev->dev, "no codec-sync GPIO
> > > > > supplied\n");
> > > > > +		ret = ac97->sync_gpio;

> And here I assign ret with that return value from of_get_named_gpio(),
> right?

> > It's not in any way involved in setting the value of ret, whatever
> > value
> > that has it's nothing to do with that operation.

> I really do not understand what you are trying to get at, sorry.

Didn't see the above assignment you'd added, sorry.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] ASoC: tegra: probe deferral error reporting
  2018-07-20  8:04 ` [PATCH 2/2] ASoC: tegra: probe deferral error reporting Marcel Ziswiler
  2018-07-20 12:16   ` Mark Brown
@ 2018-07-26  8:34   ` Stefan Agner
  1 sibling, 0 replies; 13+ messages in thread
From: Stefan Agner @ 2018-07-26  8:34 UTC (permalink / raw)
  To: Marcel Ziswiler
  Cc: alsa-devel, linux-tegra, Marcel Ziswiler, Jaroslav Kysela,
	Thierry Reding, Jonathan Hunter, Mark Brown, linux-kernel,
	Takashi Iwai, Liam Girdwood, linux-tegra-owner

On 20.07.2018 10:04, Marcel Ziswiler wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> Actually report the error codes from of_get_named_gpio() resp.
> devm_gpio_request_one() upon trying to get the codec reset resp. sync
> GPIOs which may as well just be a probe deferrals.
> 
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
>  sound/soc/tegra/tegra20_ac97.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
> index 682ef33afb5f..4875512f0732 100644
> --- a/sound/soc/tegra/tegra20_ac97.c
> +++ b/sound/soc/tegra/tegra20_ac97.c
> @@ -351,18 +351,21 @@ static int tegra20_ac97_platform_probe(struct
> platform_device *pdev)
>  		ret = devm_gpio_request_one(&pdev->dev, ac97->reset_gpio,
>  					    GPIOF_OUT_INIT_HIGH, "codec-reset");
>  		if (ret) {
> -			dev_err(&pdev->dev, "could not get codec-reset GPIO\n");
> +			dev_err(&pdev->dev, "could not get codec-reset GPIO: "
> +					    "%d\n", ret);
>  			goto err_clk_put;
>  		}
>  	} else {
> -		dev_err(&pdev->dev, "no codec-reset GPIO supplied\n");
> +		ret = ac97->reset_gpio;
> +		dev_err(&pdev->dev, "no codec-reset GPIO supplied: %d\n", ret);
>  		goto err_clk_put;
>  	}
>  
>  	ac97->sync_gpio = of_get_named_gpio(pdev->dev.of_node,
>  					    "nvidia,codec-sync-gpio", 0);
>  	if (!gpio_is_valid(ac97->sync_gpio)) {
> -		dev_err(&pdev->dev, "no codec-sync GPIO supplied\n");
> +		ret = ac97->sync_gpio;
> +		dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n", ret);
>  		goto err_clk_put;

This will still print an error on defer, which is not really nice. I
suggest to suppress defer errors completely, e.g. by:

		if (ret != -EPROBE_DEFER)
			dev_err(&pdev->dev, "no codec-sync GPIO supplied: %d\n", ret);

The driver framework provides debug level prints if debugging of
deferred probing is required.

--
Stefan


>  	}

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-07-26  8:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-20  8:04 [PATCH 1/2] ASoC: tegra: improve goto error label Marcel Ziswiler
2018-07-20  8:04 ` [PATCH 2/2] ASoC: tegra: probe deferral error reporting Marcel Ziswiler
2018-07-20 12:16   ` Mark Brown
2018-07-20 12:31     ` Marcel Ziswiler
2018-07-21  9:56       ` Mark Brown
2018-07-21 10:06         ` Marcel Ziswiler
2018-07-23 10:25           ` Mark Brown
2018-07-21 11:17         ` Dmitry Osipenko
2018-07-21 11:55           ` Marcel Ziswiler
2018-07-21 12:03             ` Dmitry Osipenko
2018-07-21 12:15               ` Dmitry Osipenko
2018-07-26  8:34   ` Stefan Agner
2018-07-20 12:19 ` Applied "ASoC: tegra: improve goto error label" to the asoc tree Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).