| Message ID | 87wq9vmyyc.wl%kuninori.morimoto.gx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 26, 2014 at 01:34:21AM -0700, Kuninori Morimoto wrote: > This patch expand route setting by using DAI name. > You can select "ak4642-hifi" side "Playback" by below. > DT > simple-audio-card,routing = > "ak4642-hifi Playback", "DAI0 Playback"; > non DT > struct snd_soc_dapm_route route[] = { > { "ak4642-hifi Playback", NULL, "DAI0 Playback"}, > } If we're already specifying the DAI links for the (D)PCM code it seems like we shouldn't also have to put DAPM routes for them in DT as well.
On 08/26/2014 11:11 AM, Mark Brown wrote: > On Tue, Aug 26, 2014 at 01:34:21AM -0700, Kuninori Morimoto wrote: > >> This patch expand route setting by using DAI name. >> You can select "ak4642-hifi" side "Playback" by below. > >> DT >> simple-audio-card,routing = >> "ak4642-hifi Playback", "DAI0 Playback"; > >> non DT >> struct snd_soc_dapm_route route[] = { >> { "ak4642-hifi Playback", NULL, "DAI0 Playback"}, >> } > > If we're already specifying the DAI links for the (D)PCM code it seems > like we shouldn't also have to put DAPM routes for them in DT as well. > Yes, and I think we shouldn't use anything except for datahsheet pin names in the devicetree routing, because otherwise we are leaking driver implementation details.
On Tue, Aug 26, 2014 at 11:19:01AM +0200, Lars-Peter Clausen wrote: > On 08/26/2014 11:11 AM, Mark Brown wrote: > >If we're already specifying the DAI links for the (D)PCM code it seems > >like we shouldn't also have to put DAPM routes for them in DT as well. > Yes, and I think we shouldn't use anything except for datahsheet pin names > in the devicetree routing, because otherwise we are leaking driver > implementation details. While I agree with the sentiment for this when it comes to DAIs we probably want to use the name the interfaces get in the documentation rather than pin names since they involve multiple pins working together.
Hi Mark, Lars I'm confusing > > >If we're already specifying the DAI links for the (D)PCM code it seems > > >like we shouldn't also have to put DAPM routes for them in DT as well. > > > Yes, and I think we shouldn't use anything except for datahsheet pin names > > in the devicetree routing, because otherwise we are leaking driver > > implementation details. It came from snd_soc_of_parse_audio_routing() Do you mean this function itself is not good ? > While I agree with the sentiment for this when it comes to DAIs we > probably want to use the name the interfaces get in the documentation > rather than pin names since they involve multiple pins working together. Sorry, but what does your "interfaces get in the documentation" mean ?
On Tue, Aug 26, 2014 at 03:31:44AM -0700, Kuninori Morimoto wrote: > > > >If we're already specifying the DAI links for the (D)PCM code it seems > > > >like we shouldn't also have to put DAPM routes for them in DT as well. > > > Yes, and I think we shouldn't use anything except for datahsheet pin names > > > in the devicetree routing, because otherwise we are leaking driver > > > implementation details. > It came from snd_soc_of_parse_audio_routing() > Do you mean this function itself is not good ? That's intended to be routing analogue pins to each other, not for DAI links in DPCM - for DAI links we should be getting this information from elsewhere. > > While I agree with the sentiment for this when it comes to DAIs we > > probably want to use the name the interfaces get in the documentation > > rather than pin names since they involve multiple pins working together. > Sorry, but what does your "interfaces get in the documentation" mean ? If the documentation refers to the interface as for example "I2S0" then the DT should refer to it as I2S0 too.
Hi Mark > > > > >If we're already specifying the DAI links for the (D)PCM code it seems > > > > >like we shouldn't also have to put DAPM routes for them in DT as well. > > > > > Yes, and I think we shouldn't use anything except for datahsheet pin names > > > > in the devicetree routing, because otherwise we are leaking driver > > > > implementation details. > > > It came from snd_soc_of_parse_audio_routing() > > Do you mean this function itself is not good ? > > That's intended to be routing analogue pins to each other, not for DAI > links in DPCM - for DAI links we should be getting this information from > elsewhere. > > > > While I agree with the sentiment for this when it comes to DAIs we > > > probably want to use the name the interfaces get in the documentation > > > rather than pin names since they involve multiple pins working together. > > > Sorry, but what does your "interfaces get in the documentation" mean ? > > If the documentation refers to the interface as for example "I2S0" then > the DT should refer to it as I2S0 too. Hmm... This means we need update DPCM interface ? In my understanding, DPCM needs DAPM routing information somehow in final stage. But, we want to specify it as "DAI link" like interface. Now, I have many questions. If my understand is correct, my prev patch is OK for "DAPM final stage", but we need wrapper for "DPCM interface" ? It will exchanges "I2S0" to "ak4642 Playback" internally. (And exchanges format too ?) Is this needed as "DT interface" ? Can non-DT code use "ak4642 Playback" directly ? I'm wondering that some driver is using DPCM already (in non-DT) in upstream. If we can use "I2S0" interface, it is understandable if FE/BE was FE cpu: CPU-A codec: dummy BE cpu: dummy codec: Codec-A But, How about this case ? FE cpu: CPU-A codec: Codec-A BE cpu: CPU-B codec: Codec-B
Hi Mark again > But, How about this case ? > > FE cpu: CPU-A > codec: Codec-A > > BE cpu: CPU-B > codec: Codec-B I found 1 method. I can create it if we can assume that "simple-card doen't support above style", > If the documentation refers to the interface as for example "I2S0" then > the DT should refer to it as I2S0 too. simple-card is using "format" property now, and I remember that someone want to exchange format in DPCM. My 1st DPCM patch used "remote" property for specify FE/BE. And, we can get DAI stream_name if we can update snd_soc_of_get_dai_name() This means, we can use DPCM like below if you can accept my previous "ASoC: dapm: enable DAI name on DAPM route" What do you think ? sound { compatible = "simple-audio-card"; /* FrontEnd */ simple-audio-card,dai-link@0 { ... format = "left_j"; remote = <&endpoint>; cpu { sound-dai = <&rcar_sound 0>; }; codec { /* dummy */ }; }; /* BackEnd */ endpoint: simple-audio-card,dai-link@1 { ... format = "left_j"; cpu { /* dummy */ }; codec1: codec { sound-dai = <&ak4643>; }; }; };
Hi Lars Thank you for your comment > > sound { > > compatible = "simple-audio-card"; > > > > /* FrontEnd */ > > simple-audio-card,dai-link@0 { > > ... > > format = "left_j"; > > remote = <&endpoint>; > > > > cpu { > > sound-dai = <&rcar_sound 0>; > > }; > > codec { /* dummy */ }; > > }; > > > > /* BackEnd */ > > endpoint: simple-audio-card,dai-link@1 { > > ... > > format = "left_j"; > > > > cpu { /* dummy */ }; > > codec1: codec { > > sound-dai = <&ak4643>; > > }; > > }; > > }; > > When you try to come up with with a binding try to completely ignore that > something call DPCM exists. The binding is supposed to describe the hardware > and how the different hardware components are interconnected. So try to come > up with a binding that accurately describes the hardware connections. Once > that is done try to map the binding onto the existing software framework. > The last step may require some adjustments to the framework. Now, my system is working well with simple-card by this sound { compatible = "simple-audio-card"; ... cpu { sound-dai = <&rcar_sound 0>; }; codec { sound-dai = <&ak4643>; }; }; The reason why I'm tring to support DPCM on simple-card is "sampling rate convert". My rcar_sound can convert sampling rate, and I tried to add this feature as rcar_sound property. But, Mark rejected and requests me to use DPCM for it, since it can be common featrue. Current existing simple-card can't use it, and I'm tring. But, am I misunderstanding ? Best regards --- Kuninori Morimoto
On 08/28/2014 02:33 AM, Kuninori Morimoto wrote: > > Hi Lars > > Thank you for your comment > >>> sound { >>> compatible = "simple-audio-card"; >>> >>> /* FrontEnd */ >>> simple-audio-card,dai-link@0 { >>> ... >>> format = "left_j"; >>> remote = <&endpoint>; >>> >>> cpu { >>> sound-dai = <&rcar_sound 0>; >>> }; >>> codec { /* dummy */ }; >>> }; >>> >>> /* BackEnd */ >>> endpoint: simple-audio-card,dai-link@1 { >>> ... >>> format = "left_j"; >>> >>> cpu { /* dummy */ }; >>> codec1: codec { >>> sound-dai = <&ak4643>; >>> }; >>> }; >>> }; >> >> When you try to come up with with a binding try to completely ignore that >> something call DPCM exists. The binding is supposed to describe the hardware >> and how the different hardware components are interconnected. So try to come >> up with a binding that accurately describes the hardware connections. Once >> that is done try to map the binding onto the existing software framework. >> The last step may require some adjustments to the framework. > > Now, my system is working well with simple-card by this > > sound { > compatible = "simple-audio-card"; > ... > > cpu { > sound-dai = <&rcar_sound 0>; > }; > codec { > sound-dai = <&ak4643>; > }; > }; > > The reason why I'm tring to support DPCM on simple-card is "sampling rate convert". > My rcar_sound can convert sampling rate, and I tried to add this feature as > rcar_sound property. > But, Mark rejected and requests me to use DPCM for it, > since it can be common featrue. > Current existing simple-card can't use it, and I'm tring. > But, am I misunderstanding ? Is the sample rate converter a extra module, or is the sample rate converter just a feature of the I2S core? - Lars
On Wed, Aug 27, 2014 at 05:33:55PM -0700, Kuninori Morimoto wrote: > Now, my system is working well with simple-card by this > sound { > compatible = "simple-audio-card"; > ... > > cpu { > sound-dai = <&rcar_sound 0>; > }; > codec { > sound-dai = <&ak4643>; > }; > }; > The reason why I'm tring to support DPCM on simple-card is "sampling rate convert". > My rcar_sound can convert sampling rate, and I tried to add this feature as > rcar_sound property. > But, Mark rejected and requests me to use DPCM for it, > since it can be common featrue. > Current existing simple-card can't use it, and I'm tring. > But, am I misunderstanding ? There's two separate things here. One is how the code is implemented (which does look very much like it should be doing DPCM) and the other is how the DT binding looks - the DT binding is supposed to be a hardware neutral thing and not depend on Linux implementation details. If you've already got the hardware described well enough to discover everything then that generally indicates that the DT binding should not need to change.
Hi Lars > > The reason why I'm tring to support DPCM on simple-card is "sampling rate convert". > > My rcar_sound can convert sampling rate, and I tried to add this feature as > > rcar_sound property. > > But, Mark rejected and requests me to use DPCM for it, > > since it can be common featrue. > > Current existing simple-card can't use it, and I'm tring. > > But, am I misunderstanding ? > > Is the sample rate converter a extra module, or is the sample rate converter > just a feature of the I2S core? Does this "I2S core" = CPU ? Then, it is CPU's feature. The image is like this 48kHz ---> 48kHz 44.1kHz ---> [CPU] ---> [codec] 96kHz --->
Hi > > sound { > > compatible = "simple-audio-card"; > > ... > > > > cpu { > > sound-dai = <&rcar_sound 0>; > > }; > > codec { > > sound-dai = <&ak4643>; > > }; > > }; (snip) > There's two separate things here. One is how the code is implemented > (which does look very much like it should be doing DPCM) and the other > is how the DT binding looks - the DT binding is supposed to be a > hardware neutral thing and not depend on Linux implementation details. > If you've already got the hardware described well enough to discover > everything then that generally indicates that the DT binding should not > need to change. In my case, I need DPCM if CPU want to use "sampling rate convert", otherwise, I don't need it. So, DPCM <-> non DPCM switching happen if DTS has sampling-rate-convert = <xxxxxxx> or something like that. Does my understanding correct ?
Hi Mark, Lars > > > sound { > > > compatible = "simple-audio-card"; > > > ... > > > > > > cpu { > > > sound-dai = <&rcar_sound 0>; > > > }; > > > codec { > > > sound-dai = <&ak4643>; > > > }; > > > }; > (snip) > > There's two separate things here. One is how the code is implemented > > (which does look very much like it should be doing DPCM) and the other > > is how the DT binding looks - the DT binding is supposed to be a > > hardware neutral thing and not depend on Linux implementation details. > > If you've already got the hardware described well enough to discover > > everything then that generally indicates that the DT binding should not > > need to change. > > In my case, I need DPCM if CPU want to use "sampling rate convert", > otherwise, I don't need it. > So, DPCM <-> non DPCM switching happen if DTS has > sampling-rate-convert = <xxxxxxx> or something like that. > Does my understanding correct ? I re-considered about above, and checked current code. If my above understanding was correct, simple-card driver needs many patches to support it. And, it is not easy I want to synchronize basic agreement before starting it. Otherwise, it is easy to reject my many weeks work, and I don't want it. # 1st simple-card DT support used 1 year... My understanding is below. It is good match for my current purpose, and "sampling-rate-convert feature via DPCM" can be standard on simple-card. My concern is simple card code will be not simple... -- non DPCM ---- sound { compatible = "simple-audio-card"; ... cpu { sound-dai = <&cpu-dai>; }; codec { sound-dai = <&codec-dai>; }; }; same as current style -- DPCM ---- sound { compatible = "simple-audio-card"; ... cpu { sound-dai = <&cpu-dai>; }; codec { sound-dai = <&codec-dai>; fixed-sampling-rate = <44100>; }; }; DPCM FE/BE will be FE cpu : cpu-dai codec : dummy BE cpu : dummy codec : codec-dai required route will be created automatically but, it needs "enable DAI name on DAPM route" method anyway
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index b24f70a..084f15b 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -2395,6 +2395,30 @@ err: return ret; } +static void snd_soc_dapm_route_scan(struct snd_soc_dapm_widget *w, + struct snd_soc_dapm_widget **wsource, + struct snd_soc_dapm_widget **wsink, + struct snd_soc_dapm_widget **wtsource, + struct snd_soc_dapm_widget **wtsink, + struct snd_soc_dapm_context *dapm, + const char *sink, + const char *source, + const char *name) +{ + if (!*wsink && !(strcmp(name, sink))) { + *wtsink = w; + if (w->dapm == dapm) + *wsink = w; + return; + } + + if (!*wsource && !(strcmp(name, source))) { + *wtsource = w; + if (w->dapm == dapm) + *wsource = w; + } +} + static int snd_soc_dapm_add_route(struct snd_soc_dapm_context *dapm, const struct snd_soc_dapm_route *route) { @@ -2425,17 +2449,20 @@ static int snd_soc_dapm_add_route(struct snd_soc_dapm_context *dapm, * current DAPM context */ list_for_each_entry(w, &dapm->card->widgets, list) { - if (!wsink && !(strcmp(w->name, sink))) { - wtsink = w; - if (w->dapm == dapm) - wsink = w; - continue; - } - if (!wsource && !(strcmp(w->name, source))) { - wtsource = w; - if (w->dapm == dapm) - wsource = w; + if (w->dai) { + char w_name[80]; + + snprintf(w_name, sizeof(w_name), "%s %s", + w->dai->name, w->name); + + snd_soc_dapm_route_scan(w, &wsource, &wsink, + &wtsource, &wtsink, dapm, + sink, source, w_name); } + + snd_soc_dapm_route_scan(w, &wsource, &wsink, + &wtsource, &wtsink, dapm, + sink, source, w->name); } /* use widget from another DAPM context if not found from this */ if (!wsink)