| Message ID | 20230523151223.109551-10-herve.codina@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for IIO devices in ASoC | expand |
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@bootlin.com> > --- 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. > 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); ... } Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Kuninori, On Wed, 24 May 2023 00:08:50 +0000 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> 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@bootlin.com> > > --- > > 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
Hi Herve Thank you for your reply. > 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. My biggest concern is that this code is calling same code multiple times. It is easy to forget such thing when updating in this kind of code. We don't forget / take mistake if these are merged. But we have such code everywhere ;) this is just my concern, not a big deal. static int __simple_for_each_link (...) { ... => add_devs = of_get_child_by_name(top, PREFIX "additional-devs"); ... } static int simple_populate_aux(...) { ... => node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs"); ... } Thank you for your help !! Best regards --- Kuninori Morimoto
On Thu, 25 May 2023 00:01:14 +0000 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Hi Herve > > Thank you for your reply. > > > 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. > > My biggest concern is that this code is calling same code multiple times. > It is easy to forget such thing when updating in this kind of code. > We don't forget / take mistake if these are merged. > But we have such code everywhere ;) this is just my concern, not a big deal. > > static int __simple_for_each_link (...) > { > ... > => add_devs = of_get_child_by_name(top, PREFIX "additional-devs"); > ... > } > > static int simple_populate_aux(...) > { > ... > => node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs"); > ... > } > Well, of_get_child_by_name() is called twice to retrieve the additional-devs node but for very different reason. In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI. In simple_populate_aux() to take care of the devices declared in the node. I am not sure that we should avoid that. It will lead to a more complex code and flags just to avoid this call. Not sure that it will be better. __simple_for_each_link() is called multiple times and is supposed to look at links. To avoid the of_get_child_by_name() filter-out call, __simple_for_each_link() will look at link *and* populate devices calling simple_populate_aux(). And to do that correctly it will use a flag to be sure that simple_populate_aux() was called only once. In order to avoid some kind of duplication (at least the node name): static struct device_node *simple_of_get_add_devs(struct device_node *node) { return of_get_child_by_name(node, PREFIX "additional-devs"); } static int __simple_for_each_link (...) { ... => add_devs = simple_of_get_add_devs(top); ... } static int simple_populate_aux(...) { ... => node = simple_of_get_add_devs(dev->of_node); ... } Does it look better ? Best regards, Hervé > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
Tue, May 23, 2023 at 05:12:23PM +0200, Herve Codina kirjoitti: > 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. ... > +static int simple_populate_aux(struct asoc_simple_priv *priv) > +{ > + struct device *dev = simple_priv_to_dev(priv); > + struct device_node *node; > + struct device **ptr; > + int ret; > + > + node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs"); > + if (!node) > + return 0; > + > + ptr = devres_alloc(simple_populate_aux_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return -ENOMEM; > + > + ret = of_platform_populate(node, NULL, NULL, dev); > + if (ret) { > + devres_free(ptr); > + } else { > + *ptr = dev; > + devres_add(dev, ptr); > + } > + return ret; This can be well simplified by using devm_add_action_or_reset(). > +}
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 @@ -348,6 +348,7 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv, struct device *dev = simple_priv_to_dev(priv); struct device_node *top = dev->of_node; struct device_node *node; + struct device_node *add_devs; uintptr_t dpcm_selectable = (uintptr_t)of_device_get_match_data(dev); bool is_top = 0; int ret = 0; @@ -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"); + /* loop for all dai-link */ do { struct asoc_simple_data adata; @@ -367,6 +370,12 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv, struct device_node *np; int num = of_get_child_count(node); + /* Skip additional-devs node */ + if (node == add_devs) { + node = of_get_next_child(top, node); + continue; + } + /* get codec */ codec = of_get_child_by_name(node, is_top ? PREFIX "codec" : "codec"); @@ -380,12 +389,15 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv, /* get convert-xxx property */ memset(&adata, 0, sizeof(adata)); - for_each_child_of_node(node, np) + for_each_child_of_node(node, np) { + if (np == add_devs) + continue; simple_parse_convert(dev, np, &adata); + } /* loop for all CPU/Codec node */ for_each_child_of_node(node, np) { - if (plat == np) + if (plat == np || add_devs == np) continue; /* * It is DPCM @@ -428,6 +440,7 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv, } while (!is_top && node); error: + of_node_put(add_devs); of_node_put(node); return ret; } @@ -652,6 +665,36 @@ static int simple_soc_probe(struct snd_soc_card *card) return 0; } +static void simple_populate_aux_release(struct device *dev, void *res) +{ + of_platform_depopulate(*(struct device **)res); +} + +static int simple_populate_aux(struct asoc_simple_priv *priv) +{ + struct device *dev = simple_priv_to_dev(priv); + struct device_node *node; + struct device **ptr; + int ret; + + node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs"); + if (!node) + return 0; + + ptr = devres_alloc(simple_populate_aux_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = of_platform_populate(node, NULL, NULL, dev); + if (ret) { + devres_free(ptr); + } else { + *ptr = dev; + devres_add(dev, ptr); + } + return ret; +} + 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) {
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@bootlin.com> --- sound/soc/generic/simple-card.c | 52 +++++++++++++++++++++++++++++++-- 1 file changed, 50 insertions(+), 2 deletions(-)