Hi Morimoto-san,
On Mon, Sep 11, 2023 at 3:44 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> +-- ULCB -------------------+
> |+--------+ +--------+|
> || SSI0| <---> |ak4613 ||
> || SSI1| <---> | ||
> || | +--------+|
> || | +--------+|
> || SSI2| <---> |HDMI ||
> || | +--------+|
> || SSI3| <--+ |
> || SSI4| <-+| |
> |+--------+ || |
> +-------------||------------+
> +-- Kingfisher -------------+
> | || +--------+|
> | |+->|pcm3168a||
> | +-->| ||
> | +--------+|
> +---------------------------+
>
> On UCLB/KF, we intuitively think we want to handle these
> as "2 Sound Cards".
>
> card0,0: 1st sound of ULCB (SSI0 - ak4613)
> card0,1: 2nd sound of ULCB (SSI2 - HDMI)
> card1,0: 1st sound of KF (SSI3 - pcm3168a)
> ^ ^
>
> But, we needed to handle it as "1 big Sound Card",
> because of ASoC Component vs Card framwork limitation.
>
> card0,0: 1st sound of ULCB/KF (SSI0 - ak4613)
> card0,1: 2nd sound of ULCB/KF (SSI2 - HDMI)
> card0,2: 3rd sound of ULCB/KF (SSI3 - pcm3168a)
> ^ ^
>
> Now ASoC support multi Component which allow us to handle "2 Sound Cards"
> such as "ULCB Sound Card" and "Kingfisher Sound Card".
>
> This patch updates all ULCB/KF Audio dtsi.
> One note is that Sound Card specification method from userland will be
> changed, especially for Kingfisher Sound.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
Thanks for your patch!
> --- a/arch/arm64/boot/dts/renesas/ulcb-kf-audio-graph-card-mix+split.dtsi
> +++ b/arch/arm64/boot/dts/renesas/ulcb-kf-audio-graph-card-mix+split.dtsi
> @@ -19,32 +19,31 @@
> *
> * (A) aplay -D plughw:0,0 xxx.wav (MIX-0)
> * (B) aplay -D plughw:0,1 xxx.wav (MIX-1)
> - * (C) aplay -D plughw:0,2 xxx.wav (TDM-0)
> - * (D) aplay -D plughw:0,3 xxx.wav (TDM-1)
> - * (E) aplay -D plughw:0,4 xxx.wav (TDM-2)
> - * (F) aplay -D plughw:0,5 xxx.wav (TDM-3)
> + * (C) aplay -D plughw:1,0 xxx.wav (TDM-0)
> + * (D) aplay -D plughw:1,1 xxx.wav (TDM-1)
> + * (E) aplay -D plughw:1,2 xxx.wav (TDM-2)
> + * (F) aplay -D plughw:1,3 xxx.wav (TDM-3)
> *
> * (A) arecord -D plughw:0,0 xxx.wav
> - * (G) arecord -D plughw:0,6 xxx.wav
> + * (G) arecord -D plughw:1,4 xxx.wav
> */
> +/ {
> + sound_card_kf: expand_sound {
Please no underscores in (new) node names (everywhere)
expand-sound?
> + compatible = "audio-graph-scu-card";
> + label = "snd-kf-split";
>
> -&sound_card {
> - routing = "ak4613 Playback", "DAI0 Playback",
> - "ak4613 Playback", "DAI1 Playback",
> - "DAI0 Capture", "ak4613 Capture",
> - "pcm3168a Playback", "DAI2 Playback",
> - "pcm3168a Playback", "DAI3 Playback",
> - "pcm3168a Playback", "DAI4 Playback",
> - "pcm3168a Playback", "DAI5 Playback";
> + routing = "pcm3168a Playback", "DAI2 Playback",
> + "pcm3168a Playback", "DAI3 Playback",
> + "pcm3168a Playback", "DAI4 Playback",
> + "pcm3168a Playback", "DAI5 Playback";
>
> - dais = <&rsnd_port0 /* (A) CPU0 */
> - &rsnd_port1 /* (B) CPU1 */
> - &rsnd_port2 /* (C) CPU2 */
> - &rsnd_port3 /* (D) CPU3 */
> - &rsnd_port4 /* (E) CPU4 */
> - &rsnd_port5 /* (F) CPU5 */
> - &rsnd_port6 /* (G) GPU6 */
> - >;
> + dais = <&snd_kf1 /* (C) CPU2 */
> + &snd_kf2 /* (D) CPU3 */
> + &snd_kf3 /* (E) CPU4 */
> + &snd_kf4 /* (F) CPU5 */
> + &snd_kf5 /* (G) GPU6 */
> + >;
> + };
> };
>
> &pcm3168a {
> @@ -103,13 +102,14 @@ pcm3168a_endpoint_c: endpoint {
> };
>
> &rcar_sound {
> - ports {
> - /* rsnd_port0-1 are defined in ulcb.dtsi */
Don't you need to add
#address-cells = <1>;
#size-cells = <0>;
like in the other files?
> + ports@1 {
So now you end up with a "ports" node without a unit address, and a
"ports@1" node with a unit address, which looks very strange to me...
According to the example in the description for commit 547b02f74e4ac1e7
("ASoC: rsnd: enable multi Component support for Audio Graph
Card/Card2"), the first node should be named "ports@0":
Ex) Audio Graph Card/Card2
rcar_sound {
/* Component0 */
ports@0 {
...
};
/* Component1 */
ports@1 {
...
};
};
This comment is valid for the other .dtsi files, too.
> + #address-cells = <1>;
> + #size-cells = <0>;
>
> /*
> * (C) CPU2
> */
> - rsnd_port2: port@2 {
> + snd_kf1: port@2 {
> reg = <2>;
> rsnd_for_pcm3168a_play1: endpoint {
> remote-endpoint = <&pcm3168a_endpoint_p1>;
> --- a/arch/arm64/boot/dts/renesas/ulcb-kf-simple-audio-card-mix+split.dtsi
> +++ b/arch/arm64/boot/dts/renesas/ulcb-kf-simple-audio-card-mix+split.dtsi
> @@ -115,7 +118,11 @@ &pcm3168a {
> };
>
> &rcar_sound {
> - rcar_sound,dai {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rcar_sound,dai@1 {
Same here: "rcar_sound,dai" node without a unit address, and
"rcar_sound,dai@1" node with a unit address.
According to the example in the description for commit 547b02f74e4ac1e7
("ASoC: rsnd: enable multi Component support for Audio Graph
Card/Card2"), the first node should be named "rcar_sound,dai@0":
Ex) Simple Card
rcar_sound {
...
/* Component0 */
rcar_sound,dai@0 {
...
};
/* Component1 */
rcar_sound,dai@1 {
...
};
};
This comment is valid for the other .dtsi files, too.
> + reg = <1>;
>
> /* dai0-1 are defined in ulcb.dtsi */
>
The rest LGTM (for a sound-illiterate reviewer like me ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
