| Message ID | 20220216171408.265605-1-rf@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | ASoC: audio_graph_card2: Support variable slot widths | expand |
Hi Richard Thank you for your patch. One comment from me. > struct asoc_simple_dai { > const char *name; > unsigned int sysclk; > @@ -26,6 +31,9 @@ struct asoc_simple_dai { > unsigned int rx_slot_mask; > struct clk *clk; > bool clk_fixed; > + struct asoc_simple_tdm_width_map *tdm_width_map; > + int n_tdm_widths; > + struct snd_soc_dai *dai; > }; (snip) > + ret = snd_soc_dai_set_tdm_slot(simple_dai->dai, > + simple_dai->tx_slot_mask, > + simple_dai->rx_slot_mask, > + simple_dai->slots, > + slot_width); (snip) > + for_each_prop_dai_codec(props, i, pdai) { > + ret = asoc_simple_set_tdm(pdai, params); > + if (ret < 0) > + return ret; > + } > + > + for_each_prop_dai_cpu(props, i, pdai) { > + ret = asoc_simple_set_tdm(pdai, params); > + if (ret < 0) > + return ret; > + } (snip) > @@ -386,6 +479,8 @@ static int asoc_simple_init_dai(struct snd_soc_dai *dai, > if (!simple_dai) > return 0; > > + simple_dai->dai = dai; Indeed the relationship between asoc_simple_dai and snd_soc_dai are very mystery, and current utils is using confusable naming. We want to have some better solution about there. Having snd_soc_dai pointer inside asoc_simple_dai itself is not bad idea. But we can get snd_soc_dai pointer without it. Please check asoc_simple_dai_init(). Not tested, but we can replace the code like this ? => struct snd_soc_dai *dai; for_each_prop_dai_codec(props, i, pdai) { => dai = asoc_rtd_to_codec(rtd, i); ret = asoc_simple_set_tdm(dai, pdai, params); if (ret < 0) return ret; } Thank you for your help !! Best regards --- Kuninori Morimoto
On 17/02/2022 00:23, Kuninori Morimoto wrote: > > Hi Richard > > Thank you for your patch. > One comment from me. > >> struct asoc_simple_dai { >> const char *name; >> unsigned int sysclk; >> @@ -26,6 +31,9 @@ struct asoc_simple_dai { >> unsigned int rx_slot_mask; >> struct clk *clk; >> bool clk_fixed; >> + struct asoc_simple_tdm_width_map *tdm_width_map; >> + int n_tdm_widths; >> + struct snd_soc_dai *dai; >> }; > (snip) (snip) > (snip) >> @@ -386,6 +479,8 @@ static int asoc_simple_init_dai(struct snd_soc_dai *dai, >> if (!simple_dai) >> return 0; >> >> + simple_dai->dai = dai; > > Indeed the relationship between asoc_simple_dai and snd_soc_dai are > very mystery, and current utils is using confusable naming. > We want to have some better solution about there. > > Having snd_soc_dai pointer inside asoc_simple_dai itself is not bad idea. > But we can get snd_soc_dai pointer without it. > > Please check asoc_simple_dai_init(). > Not tested, but we can replace the code like this ? > > => struct snd_soc_dai *dai; > > for_each_prop_dai_codec(props, i, pdai) { > => dai = asoc_rtd_to_codec(rtd, i); > ret = asoc_simple_set_tdm(dai, pdai, params); > if (ret < 0) > return ret; > } > > I first thought about doing it like that. But I was not sure whether it is safe to assume [i] is the same entry for both arrays. If it is ok, then I can use that and do not need to add the snd_soc_dai * to struct asoc_simple_dai. I will look at this and send a V2 set if it is ok.