LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ASoC: core: Don't set platform name when of_node is set
@ 2021-03-09  8:23 Daniel Baluta
  2021-03-09 15:34 ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2021-03-09  8:23 UTC (permalink / raw)
  To: broonie, alsa-devel
  Cc: lgirdwood, perex, tiwai, linux-kernel, linux-imx,
	ranjani.sridharan, pierre-louis.bossart, shengjiu.wang,
	Daniel Baluta

From: Daniel Baluta <daniel.baluta@nxp.com>

Platform may be specified by either name or OF node but not
both.

For OF node platforms (e.g i.MX) we end up with both platform name
and of_node set and sound card registration will fail with the error:

  asoc-simple-card sof-sound-wm8960: ASoC: Neither/both
  platform name/of_node are set for sai1-wm8960-hifi

Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 sound/soc/soc-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 16ba54eb8164..76ab42fa9461 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1660,7 +1660,9 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
 				dev_err(card->dev, "init platform error");
 				continue;
 			}
-			dai_link->platforms->name = component->name;
+
+			if (!dai_link->platforms->of_node)
+				dai_link->platforms->name = component->name;
 
 			/* convert non BE into BE */
 			if (!dai_link->no_pcm) {
-- 
2.27.0


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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-09  8:23 [PATCH] ASoC: core: Don't set platform name when of_node is set Daniel Baluta
@ 2021-03-09 15:34 ` Mark Brown
  2021-03-12  8:32   ` Daniel Baluta
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2021-03-09 15:34 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: alsa-devel, lgirdwood, perex, tiwai, linux-kernel, linux-imx,
	ranjani.sridharan, pierre-louis.bossart, shengjiu.wang,
	Daniel Baluta

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

On Tue, Mar 09, 2021 at 10:23:28AM +0200, Daniel Baluta wrote:
> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
> Platform may be specified by either name or OF node but not
> both.
> 
> For OF node platforms (e.g i.MX) we end up with both platform name
> and of_node set and sound card registration will fail with the error:
> 
>   asoc-simple-card sof-sound-wm8960: ASoC: Neither/both
>   platform name/of_node are set for sai1-wm8960-hifi

This doesn't actually say what the change does.

> -			dai_link->platforms->name = component->name;
> +
> +			if (!dai_link->platforms->of_node)
> +				dai_link->platforms->name = component->name;

Why would we prefer the node name over something explicitly configured?

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

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-09 15:34 ` Mark Brown
@ 2021-03-12  8:32   ` Daniel Baluta
  2021-03-12 10:49     ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2021-03-12  8:32 UTC (permalink / raw)
  To: Mark Brown
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

On Tue, Mar 9, 2021 at 5:38 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Mar 09, 2021 at 10:23:28AM +0200, Daniel Baluta wrote:
> > From: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Platform may be specified by either name or OF node but not
> > both.
> >
> > For OF node platforms (e.g i.MX) we end up with both platform name
> > and of_node set and sound card registration will fail with the error:
> >
> >   asoc-simple-card sof-sound-wm8960: ASoC: Neither/both
> >   platform name/of_node are set for sai1-wm8960-hifi
>
> This doesn't actually say what the change does.

Will send v2 with a better explanation.

>
> > -                     dai_link->platforms->name = component->name;
> > +
> > +                     if (!dai_link->platforms->of_node)
> > +                             dai_link->platforms->name = component->name;
>
> Why would we prefer the node name over something explicitly configured?

Not sure I follow your question. I think the difference stands in the
way we treat OF vs non-OF platforms.

With OF-platforms, dai_link->platforms->of_node is always set! So we
no longer need
to set dai->platforms->name.

Actually setting both of_node and name will make sound card
registration fail! In this is the case I'm trying
to fix here.

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12  8:32   ` Daniel Baluta
@ 2021-03-12 10:49     ` Mark Brown
  2021-03-12 10:59       ` Daniel Baluta
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2021-03-12 10:49 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

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

On Fri, Mar 12, 2021 at 10:32:54AM +0200, Daniel Baluta wrote:
> On Tue, Mar 9, 2021 at 5:38 PM Mark Brown <broonie@kernel.org> wrote:

> > > +                     if (!dai_link->platforms->of_node)
> > > +                             dai_link->platforms->name = component->name;

> > Why would we prefer the node name over something explicitly configured?

> Not sure I follow your question. I think the difference stands in the
> way we treat OF vs non-OF platforms.

If an explicit name has been provided why would we override it with an
autogenerated one?

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

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12 10:49     ` Mark Brown
@ 2021-03-12 10:59       ` Daniel Baluta
  2021-03-12 11:57         ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2021-03-12 10:59 UTC (permalink / raw)
  To: Mark Brown
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

On Fri, Mar 12, 2021 at 12:50 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Mar 12, 2021 at 10:32:54AM +0200, Daniel Baluta wrote:
> > On Tue, Mar 9, 2021 at 5:38 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > +                     if (!dai_link->platforms->of_node)
> > > > +                             dai_link->platforms->name = component->name;
>
> > > Why would we prefer the node name over something explicitly configured?
>
> > Not sure I follow your question. I think the difference stands in the
> > way we treat OF vs non-OF platforms.
>
> If an explicit name has been provided why would we override it with an
> autogenerated one?

Wait, are you asking why the initial code:

  dai_link->platforms->name = component->name;


is there in the initial code?

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12 10:59       ` Daniel Baluta
@ 2021-03-12 11:57         ` Mark Brown
  2021-03-12 12:37           ` Daniel Baluta
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2021-03-12 11:57 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

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

On Fri, Mar 12, 2021 at 12:59:29PM +0200, Daniel Baluta wrote:
> On Fri, Mar 12, 2021 at 12:50 PM Mark Brown <broonie@kernel.org> wrote:

> > If an explicit name has been provided why would we override it with an
> > autogenerated one?

> Wait, are you asking why the initial code:

>   dai_link->platforms->name = component->name;

> is there in the initial code?

No, just the opposite!  If there's an explict name configured why do you
want to ignore it?

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

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12 11:57         ` Mark Brown
@ 2021-03-12 12:37           ` Daniel Baluta
  2021-03-12 14:23             ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2021-03-12 12:37 UTC (permalink / raw)
  To: Mark Brown
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

On Fri, Mar 12, 2021 at 1:59 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Mar 12, 2021 at 12:59:29PM +0200, Daniel Baluta wrote:
> > On Fri, Mar 12, 2021 at 12:50 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > If an explicit name has been provided why would we override it with an
> > > autogenerated one?
>
> > Wait, are you asking why the initial code:
>
> >   dai_link->platforms->name = component->name;
>
> > is there in the initial code?
>
> No, just the opposite!  If there's an explict name configured why do you
> want to ignore it?

Because the initial assignment:

dai_link->platforms->name = component->name;

doesn't take into consideration that dai_link->platform->of_node is
also explicitly configured.

So, my change only configures the name  (dai_link->platform->name)
if the dai->platform->of_node wasn't previously explicitly configured.

Otherwise, we end up with both dai_link->platforms->name and
dai->link->platforms->of_node
configured and we hit this error:

soc_dai_link_sanity_check:
/*
 * Platform may be specified by either name or OF node, but it
 * can be left unspecified, then no components will be inserted
 * in the rtdcom list
 */
if (!!platform->name == !!platform->of_node) {
    dev_err(card->dev,
    "ASoC: Neither/both platform name/of_node are set for %s\n", link->name);
    return -EINVAL;
}

I start with a simple-audio-card node:


sof-sound-wm8960 {
    compatible = "simple-audio-card";

    simple-audio-card,dai-link {
       format = "i2s";
       cpu {
            sound-dai = <&dsp 1>;
       };
       sndcodec: codec {
            sound-dai = <&wm8960>;
       };
}

Notice that doesn't have any platform field.

But then in sound/soc/generic/simple-card-utils.c:asoc_simple_canonicalize_platform
explicitly sets dai_link->platforms->of_node like this:

»       if (!dai_link->platforms->of_node)
»       »       dai_link->platforms->of_node = dai_link->cpus->of_node;

I hope this is more clear.

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12 12:37           ` Daniel Baluta
@ 2021-03-12 14:23             ` Mark Brown
  2021-03-17  8:48               ` Daniel Baluta
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2021-03-12 14:23 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

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

On Fri, Mar 12, 2021 at 02:37:30PM +0200, Daniel Baluta wrote:
> On Fri, Mar 12, 2021 at 1:59 PM Mark Brown <broonie@kernel.org> wrote:

> > No, just the opposite!  If there's an explict name configured why do you
> > want to ignore it?

> Because the initial assignment:

> dai_link->platforms->name = component->name;

> doesn't take into consideration that dai_link->platform->of_node is
> also explicitly configured.

But why should we take that into consideration here?

> dai->link->platforms->of_node
> configured and we hit this error:
> 
> soc_dai_link_sanity_check:
> /*
>  * Platform may be specified by either name or OF node, but it
>  * can be left unspecified, then no components will be inserted
>  * in the rtdcom list
>  */
> if (!!platform->name == !!platform->of_node) {
>     dev_err(card->dev,
>     "ASoC: Neither/both platform name/of_node are set for %s\n", link->name);
>     return -EINVAL;
> }

OK, but then does this check actually make sense?  The code has been
that way since the initial DT introduction since we disallow matching by
both name and OF node in order to avoid confusion when building the card
so I think it does but it's only with this mail that I get the
information to figure out that this is something we actually check for
then go find the reason why we check.

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

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

* Re: [PATCH] ASoC: core: Don't set platform name when of_node is set
  2021-03-12 14:23             ` Mark Brown
@ 2021-03-17  8:48               ` Daniel Baluta
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Baluta @ 2021-03-17  8:48 UTC (permalink / raw)
  To: Mark Brown
  Cc: Daniel Baluta, Linux-ALSA, Liam Girdwood, Jaroslav Kysela,
	Takashi Iwai, Linux Kernel Mailing List, dl-linux-imx,
	Ranjani Sridharan, Pierre-Louis Bossart, S.j. Wang,
	Daniel Baluta

On Fri, Mar 12, 2021 at 4:24 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Mar 12, 2021 at 02:37:30PM +0200, Daniel Baluta wrote:
> > On Fri, Mar 12, 2021 at 1:59 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > No, just the opposite!  If there's an explict name configured why do you
> > > want to ignore it?
>
> > Because the initial assignment:
>
> > dai_link->platforms->name = component->name;
>
> > doesn't take into consideration that dai_link->platform->of_node is
> > also explicitly configured.
>
> But why should we take that into consideration here?
>
> > dai->link->platforms->of_node
> > configured and we hit this error:
> >
> > soc_dai_link_sanity_check:
> > /*
> >  * Platform may be specified by either name or OF node, but it
> >  * can be left unspecified, then no components will be inserted
> >  * in the rtdcom list
> >  */
> > if (!!platform->name == !!platform->of_node) {
> >     dev_err(card->dev,
> >     "ASoC: Neither/both platform name/of_node are set for %s\n", link->name);
> >     return -EINVAL;
> > }
>
> OK, but then does this check actually make sense?  The code has been
> that way since the initial DT introduction since we disallow matching by
> both name and OF node in order to avoid confusion when building the card
> so I think it does but it's only with this mail that I get the
> information to figure out that this is something we actually check for
> then go find the reason why we check.

I will enhance the commit message and send v2. Hope to catch all the
inner details.

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

end of thread, other threads:[~2021-03-17  8:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-09  8:23 [PATCH] ASoC: core: Don't set platform name when of_node is set Daniel Baluta
2021-03-09 15:34 ` Mark Brown
2021-03-12  8:32   ` Daniel Baluta
2021-03-12 10:49     ` Mark Brown
2021-03-12 10:59       ` Daniel Baluta
2021-03-12 11:57         ` Mark Brown
2021-03-12 12:37           ` Daniel Baluta
2021-03-12 14:23             ` Mark Brown
2021-03-17  8:48               ` Daniel Baluta

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).