Hi Kuninori, On Wed, 24 May 2023 00:08:50 +0000 Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx> wrote: > Hi > > > An additional-devs subnode can be present in the simple-card top node. > > This subnode is used to declared some "virtual" additional devices. > > > > Create related devices from this subnode and avoid this subnode presence > > to interfere with the already supported subnodes analysis. > > > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx> > > --- > > simple-card is used in many boards, but is old. > Adding new feature on audio-graph-card/audio-graph-card2 instead of simple-card > is my ideal, but it is OK. > > simple-card is possible to handle multiple DAI links by using > "dai-link" node on 1 Sound Card. see > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/sound/simple-card.yaml?h=v6.4-rc3#n294 > > Is this "additional-devs" available only one per 1 Card ? > If it is possible to use 1 additional-devs per 1 DAI link, I think this patch want to > care "dai-link". > Or adding temporally NOTE or FIXME message like /* NOTE: it doesn't support dai-link so far */ > is good idea. As you said on your other mail 1 "additional-devs" per 1 Card. And further more, I think that "additional-devs" has nothing to do with DAI link. These additional devices are really related to the the Card itself and not DAI links. simple_populate_aux() needs to be called once per Card. > > > static int asoc_simple_probe(struct platform_device *pdev) > > { > > struct asoc_simple_priv *priv; > > @@ -688,6 +731,11 @@ static int asoc_simple_probe(struct platform_device *pdev) > > return ret; > > > > if (np && of_device_is_available(np)) { > > + ret = simple_populate_aux(priv); > > + if (ret < 0) { > > + dev_err_probe(dev, ret, "populate aux error\n"); > > + goto err; > > + } > > > > ret = simple_parse_of(priv, li); > > if (ret < 0) { > > -- > > 2.40.1 > > > > Calling simple_populate_aux() before calling simple_parse_of() is possible, > but looks strange for me. > see below > > > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c > > index 5a5e4ecd0f61..4992ab433d6a 100644 > > --- a/sound/soc/generic/simple-card.c > > +++ b/sound/soc/generic/simple-card.c > (snip) > > @@ -359,6 +360,8 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv, > > is_top = 1; > > } > > > > + add_devs = of_get_child_by_name(top, PREFIX "additional-devs"); > > I think better position to call simple_populate_aux() is here. > But __simple_for_each_link() will be called multiple times for CPU and for Codec. > So maybe you want to calling it under CPU turn. > > /* NOTE: it doesn't support dai-link so far */ > add_devs = of_get_child_by_name(top, PREFIX "additional-devs"); > if (add_devs && li->cpu) { > ret = simple_populate_aux(priv); > ... > } So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is not correct as it has nothing to do with DAI links and must be call once per Card. simple_populate_aux() could be called by simple_parse_of() itself or after simple_parse_of() call. I would prefer calling it before snd_soc_of_parse_aux_devs() because this function parses aux-devs phandles and these phandles can refer an auxiliary device present in the additional-devs node. The current code has no issue with this but some evolution can lead to EPROBE_DEFER if the device related to the phandle was not probed. If simple_populate_aux() is called after snd_soc_of_parse_aux_devs(), an EPROBE_DEFER related to the missing probe() call has no chance to be resolved. Tell be what you prefer: a) Call before simple_parse_of() (no changes) or b) Call at the very beginning of simple_parse_of() or c) Other suggestion ... Thanks a lot for your review. Best regards, Hervé > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
