Message-ID: <0aa11ef6-4166-41d8-98bc-6c7687d10b11@gmail.com>
Date: Fri, 16 May 2025 15:50:02 +0300
From: Mihalcea Laurentiu <laurentiumihalcea111@...il.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc: Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, Liam Girdwood <lgirdwood@...il.com>,
 linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH RFC 2/3] ASoC: audio-graph-card2: support explicitly
 disabled links


On 16.05.2025 04:36, Kuninori Morimoto wrote:
> Hi Laurentiu
>
> Thank you for the patch
>
>> An explicitly disabled link is a DAI link in which one of its device
>> endpoints (e.g: codec or CPU) has been disabled in the DTS via the
>> "status" property. Formally speaking:
>>
>> 	OF_LINK_IS_DISABLED(lnk) = OF_NODE_IS_DISABLED(dev0) ||
>> 	                           OF_NODE_IS_DISABLED(dev1);
>>
>> where dev0 and dev1 are the two devices (CPU/codec) that make up the
>> link.
>>
>> If at least one link was explicitly disabled that means DAPM routes
>> passed through the OF property "routing" can fail as some widgets might
>> not exist. Consider the following example:
>>
>> 	CODEC A has widgets A0, A1.
>> 	CODEC B has widgets B0, B1.
>>
>> 	my-card {
>> 		compatible = "audio-graph-card2":
>> 		label = "my-label";
>> 		links = <&cpu0_port>, <&cpu1_port>;
>> 		routing = "A0", "A1",
>> 		          "B0", "B1";
>> 	};
>>
>> 	CODEC A's DT node was disabled.
>> 	CODEC B's DT node is enabled.
>> 	CPU0's DT node is enabled.
>> 	CPU1's DT node is enabled.
>>
>> If CODEC A's DT node is disabled via the 'status = "disabled"' property
>> that means the A0 -> A1 route cannot be created. This doesn't affect the
>> B0 -> B1 route though as CODEC B was never disabled in the DT.
>>
>> This is why, if any explicitly disabled link is discovered, the
>> "disable_of_route_checks" flag is turned on.
>>
>> If all links were explicitly disabled the sound card creation will fail.
>> Otherwise, if there's at least one link which wasn't explicitly disabled
>> then the sound card creation will succeed.
>>
>> Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@....com>
>> ---
> I think I could understand your situation, but the solution (= this patch) is
> too much complicated for me. Indeed it might detect disabled device, but some
> board might want to detect it as error, unlike your case.


that is true. One problem with this solution is the fact that you can't

really distinguish between links that were intentionally disabled (see example

from RFC's cover letter) and links that were accidentally disabled (e.g. user

forgot to set 'status = "okay"' for one of the link's devices)


>
> You want to add "disable_of_route_checks" flag to the card, right ?
> How about to add new property, like "force detect xxx", or
> "DAI might not be detected", etc, etc, etc...
>
> If we can have such property, it will be more simple code.
>
> 	if (it_has_flag("force_detect_xxx")) {
> 		dev_info(dev, "xxx");
> 		card->disable_of_route_checks = 1;
> 	}


well, I think the solution is made up of 2 parts:

    1) the part that allows link devices to be disabled

    2) the part that allows routes to fails


what you're suggesting is dropping the first part and going with just the second one.

The problem I see with this (let's assume we have the BASE-PLUGIN example presented

in the RFC's cover letter) is that:

    1) You'll have to specify the links inside of PLUGIN's DT overlay (instead of BASE's DTS)

    2) You'll have to do the link connection part inside PLUGIN's DT overlay.


So, our example DTS and DT overlay would look like this:


[snippet from base.dts]

	my_card: card {
		compatible = "audio-graph-card2";
		links = <&l2>; /* here, we're only allowed to specify links that exist */
		routing = "Headphones", "C20",
			  "Headphones", "C21",
			  "Line", "C01";
	};

	d0: cpu@0 {
		l0: port { l0_ep: endpoint { /* connected when plugin.dtbo is applied */ } };
	};

	d1: cpu@1 {
		l1: port { l1_ep: endpoint { /* connected when plugin.dtbo is applied */ } };
	};

	d2: cpu@2 {
		l2: port { l2_ep: endpoint { remote-endpoint = <&c2_ep>; } };
	};

	c2: codec@2 {
		port { c2_ep: endpoint { remote-endpoint = <&l2_ep>; } };
	};


[snippet from plugin.dtso]

&my_card {
	/* here we're forced to also specify l2 even if this is already done
         * in base.dts. This is because DT overlays don't support appending to
	 * properties.
         */
	remote-endpoint = <&l0>, <&l1>, <&l2>;
};

&l0_ep {
	remote-endpoint = <&c1_ep>;
};

&l1_ep {
	remote-endpoint = <&c2_ep>;
};

c0: codec@0 {
	port { c0_ep: endpoint { remote-endpoint = <&l0_ep>; } };
};

c1: codec@1 {
	port { c1_ep: endpoint { remote-endpoint = <&l1_ep>; } };
};

Assume that we also have BASE1 that is compatible with PLUGIN but
C0 and C1 are connected to BASE1's D0 and D5. Since there's no D1-C1
connection that means you'll have to create another DT overlay. Thus,
the scalability of plugin.dtso decreases.

Now, for our particular case, we have BASE0 and BASE1 with the following
DAIs and CODECS (these are physically present on the base boards):

BASE0 has DAIs D0, D1 and CODEC C0
BASE1 has DAIs D0, D1 and CODEC C1

Both of these boards are compatible with plugin PLUGIN that has codec C2.
The possible DAI links are:

For BASE0:

D0 <----> C0
D1 <----> C2 (only possible with PLUGIN connected)

For BASE1:

D0 <----> C1
D1 <----> C2 (only possible with PLUGIN connected)

Since the D1 <---> C2 connection is valid for both BASE0-PLUGIN and
BASE1-PLUGIN combinations I think we can make do without the support
for explicitly disabled links. But I don't think this is ideal because:

1) If we ever need to support board BASE3 that is compatible with PLUGIN
and uses Dn != D1 to connect with PLUGIN's C2 codec then we're going to
either modify our devicetrees (to make plugin.dtso applicable to BASE3
as well) or add another DT overlay (not really desirable because we already
have a lot of devicetrees). (note: more of an _if_ situation. Don't think we
can actually use this as an argument. I just wanted to have this out in the
open)

2) People might get confused when looking at the "links" and "routing"
properties. Now, "links" will only contain links that actually exist, while
"routing" can contain widgets from codecs that don't exist on the base board.
(note: perhaps not a deal breaker? again, just wanted to have this out in the
open)


Either way, I believe your concern is valid. This new feature adds a lot of
extra code and validating it is a pain. Still, even if we go with just the
extra DT property as you suggested I believe this is a step forward so I think
we can go with that for now and see how well this scales.


BTW, thanks a lot for taking the time to review this huge series!!

> Thank you for your help !!
>
> Best regards
> ---
> Kuninori Morimoto

Powered by blists - more mailing lists