| Message ID | 874jihlx44.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | ASoC: makes CPU/Codec channel connection map more generic | expand |
Hi Conor > > + CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the > > + numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and > > Again, relying on header files in an operating system to explain the > property is not a runner. You need to explain how to populate this > property in an operating system independent manner. Sample is not mandatory here, I will remove Linux header pointer from here in v6. Thank you for your help !! Best regards --- Kuninori Morimoto
On 10/23/23 00:35, Kuninori Morimoto wrote: > Current ASoC CPU:Codec = N:M connection is using connection mapping idea, > but it is used for N < M case only. We want to use it for any case. > > By this patch, not only N:M connection, but all existing connection > (1:1, 1:N, N:N) will use same connection mapping. Then, because it will > use default mapping, no conversion patch is needed to exising drivers. > > More over, CPU:Codec = N:M (N > M) also supported in the same time. > > ch_maps array will has CPU/Codec index by this patch. > > Image > CPU0 <---> Codec0 > CPU1 <-+-> Codec1 > CPU2 <-/ > > ch_map > ch_map[0].cpu = 0 ch_map[0].codec = 0 > ch_map[1].cpu = 1 ch_map[1].codec = 1 > ch_map[2].cpu = 2 ch_map[2].codec = 1 > > Link: https://lore.kernel.org/r/87fs6wuszr.wl-kuninori.morimoto.gx@renesas.com > Link: https://lore.kernel.org/r/878r7yqeo4.wl-kuninori.morimoto.gx@renesas.com > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> The Intel CI did not detect any issues with this patch, see https://github.com/thesofproject/linux/pull/4632, so Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Note however the -W1 error below > +static int snd_soc_compensate_channel_connection_map(struct snd_soc_card *card, > + struct snd_soc_dai_link *dai_link) > +{ > + struct snd_soc_dai_link_ch_map *ch_maps; > + int i, max; sound/soc/soc-core.c: In function ‘snd_soc_compensate_channel_connection_map’: sound/soc/soc-core.c:1050:16: error: variable ‘max’ set but not used [-Werror=unused-but-set-variable] 1050 | int i, max; | ^~~ > + /* > + * dai_link->ch_maps indicates how CPU/Codec are connected. > + * It will be a map seen from a larger number of DAI. > + * see > + * soc.h :: [dai_link->ch_maps Image sample] > + */ > + > + /* it should have ch_maps if connection was N:M */ > + if (dai_link->num_cpus > 1 && dai_link->num_codecs > 1 && > + dai_link->num_cpus != dai_link->num_codecs && !dai_link->ch_maps) { > + dev_err(card->dev, "need to have ch_maps when N:M connction (%s)", > + dai_link->name); > + return -EINVAL; > + } > + > + /* do nothing if it has own maps */ > + if (dai_link->ch_maps) > + goto sanity_check; > + > + /* check default map size */ > + if (dai_link->num_cpus > MAX_DEFAULT_CH_MAP_SIZE || > + dai_link->num_codecs > MAX_DEFAULT_CH_MAP_SIZE) { > + dev_err(card->dev, "soc-core.c needs update default_connection_maps"); > + return -EINVAL; > + } > + > + /* Compensate missing map for ... */ > + if (dai_link->num_cpus == dai_link->num_codecs) > + dai_link->ch_maps = default_ch_map_sync; /* for 1:1 or N:N */ > + else if (dai_link->num_cpus < dai_link->num_codecs) > + dai_link->ch_maps = default_ch_map_1cpu; /* for 1:N */ > + else > + dai_link->ch_maps = default_ch_map_1codec; /* for N:1 */ > + > +sanity_check: > + max = min(dai_link->num_cpus, dai_link->num_codecs); sound/soc/soc-core.c: In function ‘snd_soc_compensate_channel_connection_map’: sound/soc/soc-core.c:1050:16: error: variable ‘max’ set but not used [-Werror=unused-but-set-variable] 1050 | int i, max; | ^~~ > + > + dev_dbg(card->dev, "dai_link %s\n", dai_link->stream_name); > + for_each_link_ch_maps(dai_link, i, ch_maps) { > + if ((ch_maps->cpu >= dai_link->num_cpus) || > + (ch_maps->codec >= dai_link->num_codecs)) { > + dev_err(card->dev, > + "unexpected dai_link->ch_maps[%d] index (cpu(%d/%d) codec(%d/%d))", > + i, > + ch_maps->cpu, dai_link->num_cpus, > + ch_maps->codec, dai_link->num_codecs); > + return -EINVAL; > + } > + > + dev_dbg(card->dev, " [%d] cpu%d <-> codec%d\n", > + i, ch_maps->cpu, ch_maps->codec); > + } > + > + return 0; > +}
On Mon, Oct 23, 2023 at 07:47:09PM +0100, Mark Brown wrote: > On Mon, Oct 23, 2023 at 05:50:42PM +0100, Conor Dooley wrote: > > On Mon, Oct 23, 2023 at 05:36:09AM +0000, Kuninori Morimoto wrote: > > > > + ch-map-idx: > > > I would rather this be spelt out as "channel-map-index" - although I > > don't know if that is the best name for the property, as it seems very > > tied to a single operating systems variable names. > > I'll leave it to Mark as to whether there is a less linux implementation > > coupled name for this property. > > It's not particularly Linux coupled, this is a fairly general concept. You'd know better than I, it just seemed like a rip from the variable name :)
On Mon, Oct 23, 2023 at 10:58:28PM +0000, Kuninori Morimoto wrote: > > Hi Conor > > > > > + CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the > > > + numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and > > > > Again, relying on header files in an operating system to explain the > > property is not a runner. You need to explain how to populate this > > property in an operating system independent manner. > > Sample is not mandatory here, I will remove Linux header pointer from here in v6. Please don't just remove the reference to the header file, and actually explain the property instead.
On Mon, Oct 23, 2023 at 05:36:09AM +0000, Kuninori Morimoto wrote: > This patch adds ch-maps property to enable handling CPU:Codec = N:M > connection. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > .../devicetree/bindings/sound/audio-graph-port.yaml | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml > index 60b5e3fd1115..47f04cdd6670 100644 > --- a/Documentation/devicetree/bindings/sound/audio-graph-port.yaml > +++ b/Documentation/devicetree/bindings/sound/audio-graph-port.yaml > @@ -19,7 +19,12 @@ definitions: > properties: > mclk-fs: > $ref: simple-card.yaml#/definitions/mclk-fs > - > + ch-map-idx: > + description: It indicates index of ch_maps array for CPU / Codec if number of > + CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the > + numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and > + ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample. Why do we have a dtsi file hidden away here? We have examples and actual users. Do we really need a 3rd way? Rob
Hi Conor > > > Again, relying on header files in an operating system to explain the > > > property is not a runner. You need to explain how to populate this > > > property in an operating system independent manner. > > > > Sample is not mandatory here, I will remove Linux header pointer from here in v6. > > Please don't just remove the reference to the header file, and actually > explain the property instead. Yes, I will try my best. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Rob > > + ch-map-idx: > > + description: It indicates index of ch_maps array for CPU / Codec if number of > > + CPU(N) / Codec(M) DAIs were not same in one dai-link. ch-map-idx is not needed if the > > + numbers were 1:M or N:1 or N=M. see soc.h::[dai_link->ch_maps Image sample] and > > + ${LINUX}/sound/soc/generic/audio-graph-card2-custom-sample.dtsi. It is good sample. > > Why do we have a dtsi file hidden away here? > > We have examples and actual users. Do we really need a 3rd way? ASoC is supporting many type of (complex) connections, and Audio Graph Card2 is supporting all of them. There is no actual user who is using all type of connections. Thus there is no good sample for it. Above is using all type of connections. And I'm using it for Audio Graph Card2 test purpose. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Pierre-Louis > The Intel CI did not detect any issues with this patch, see (snip) > Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Thank you for your test, again ! > Note however the -W1 error below Thanks. Will fix in v6 Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Mark, Conor Thank you for your feedbacks. > > > > + ch-map-idx: > > > > > I would rather this be spelt out as "channel-map-index" - although I > > > don't know if that is the best name for the property, as it seems very > > > tied to a single operating systems variable names. > > > I'll leave it to Mark as to whether there is a less linux implementation > > > coupled name for this property. > > > > It's not particularly Linux coupled, this is a fairly general concept. > > You'd know better than I, it just seemed like a rip from the variable > name :) I have no special opinion about this, but let's use more generic naming. v6 will use "channel-map-index" for it. Thank you for your help !! Best regards --- Kuninori Morimoto