| Message ID | 1593233625-14961-13-git-send-email-spujar@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Tegra210 Audio | expand |
Hi Sameer > The simple-card driver supports multiple CPU and single Codec entries > for DPCM DAI links. In some cases it is required to have multiple > CPU/Codecs. Currently parsing logic for DPCM link loops over all > children of DAI link but assumes that there is a single Codec entry. > When DAI link has multiple Codecs it considers only the first Codec > entry and remaining Codecs are wrongly treated as CPU. This happens > because first Codec is used as reference for parsing all other child > nodes. (snip) > @@ -137,8 +136,13 @@ static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv, > * Codec |return|Pass > * np > */ > - if (li->cpu == (np == codec)) > - return 0; > + if (li->cpu) { > + if (!strcmp(np->name, "codec")) > + return 0; > + } else { > + if (!strcmp(np->name, "cpu")) > + return 0; > + } Checking node name is maybe nice idea, but please consider "prefix" here. Maybe base issue for multiple codec support is that simple_for_each_link() is caring first codec only ? simple_for_each_link(...) { ... do { => /* get codec */ => codec = of_get_child_by_name(...); ... } } Remove above and having simple_node_is_codec(np, xxx) function or something can help it ? Thank you for your help !! Best regards --- Kuninori Morimoto
On 6/29/2020 6:54 AM, Kuninori Morimoto wrote: > External email: Use caution opening links or attachments > > > Hi Sameer > >> The simple-card driver supports multiple CPU and single Codec entries >> for DPCM DAI links. In some cases it is required to have multiple >> CPU/Codecs. Currently parsing logic for DPCM link loops over all >> children of DAI link but assumes that there is a single Codec entry. >> When DAI link has multiple Codecs it considers only the first Codec >> entry and remaining Codecs are wrongly treated as CPU. This happens >> because first Codec is used as reference for parsing all other child >> nodes. > (snip) >> @@ -137,8 +136,13 @@ static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv, >> * Codec |return|Pass >> * np >> */ >> - if (li->cpu == (np == codec)) >> - return 0; >> + if (li->cpu) { >> + if (!strcmp(np->name, "codec")) >> + return 0; >> + } else { >> + if (!strcmp(np->name, "cpu")) >> + return 0; >> + } > Checking node name is maybe nice idea, > but please consider "prefix" here. Sorry I missed that example where DAI is defined at sound level. I will update. > > Maybe base issue for multiple codec support > is that simple_for_each_link() is caring first codec only ? Yes that is true. > > simple_for_each_link(...) > { > ... > do { > => /* get codec */ > => codec = of_get_child_by_name(...); > ... > } > } > > Remove above and having simple_node_is_codec(np, xxx) function > or something can help it ? Ideally I wanted to remove above two lines and allow empty codec list. But some users may expect the parsing to fail if no 'Codec' is found in the DAI link, hence did not remove above. If it is fine to remove above two lines it would be simpler. The loop inside simple_for_each_link() would anyway loop for each child node of DAI link and simple_dai_link_of_dpcm() can parse each 'np'. > > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
Hi Sameer > > Maybe base issue for multiple codec support > > is that simple_for_each_link() is caring first codec only ? (snip) > Ideally I wanted to remove above two lines and allow empty codec > list. But some users may expect the parsing to fail if no 'Codec' is > found in the DAI link, hence did not remove above. If it is fine to > remove above two lines it would be simpler. The loop inside > simple_for_each_link() would anyway loop for each child node of DAI > link and simple_dai_link_of_dpcm() can parse each 'np'. Current simple-card is not assuming multi Codec, thus, we need to update it correctly, not quick-hack. I'm not sure how to do it, but it seems we need to update many functions to support it, not only simple-card driver. For example, simple-card-utils, soc-core, etc, etc... I'm not sure, this is just my guess. I'm happy if we can support it more easily :) But, if it was difficult to keep compatibility on simple-card, we/you need to have new one. Actually, I had a plan to create more flexible sound card driver, but it is not hi priority for me in these days. Thank you for your help !! Best regards --- Kuninori Morimoto
On 6/30/2020 6:54 AM, Kuninori Morimoto wrote: > External email: Use caution opening links or attachments > > > Hi Sameer > >>> Maybe base issue for multiple codec support >>> is that simple_for_each_link() is caring first codec only ? > (snip) >> Ideally I wanted to remove above two lines and allow empty codec >> list. But some users may expect the parsing to fail if no 'Codec' is >> found in the DAI link, hence did not remove above. If it is fine to >> remove above two lines it would be simpler. The loop inside >> simple_for_each_link() would anyway loop for each child node of DAI >> link and simple_dai_link_of_dpcm() can parse each 'np'. > Current simple-card is not assuming multi Codec, > thus, we need to update it correctly, not quick-hack. > > I'm not sure how to do it, but it seems we need to update > many functions to support it, not only simple-card driver. > For example, simple-card-utils, soc-core, etc, etc... > > I'm not sure, this is just my guess. > I'm happy if we can support it more easily :) Right now I am trying re-use simple-card driver as much as possible and still make it work for flexible sound cards. I will be happy to discuss and improve the patch wherever necessary. Please help me understand which part you think looks to be hacky. > But, if it was difficult to keep compatibility on simple-card, > we/you need to have new one. Patch 17/23 and 18/23 introduce new compatible 'simple-cc-audio-card'. Idea was to use component chaining which allows connection of FE<->BE and multiple BE<->BE components along the DAPM path (patch 16/23). Do you think it would be fine? > Actually, I had a plan to create more flexible sound card > driver, but it is not hi priority for me in these days. > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
Hi Sameer Thank you for explaining detail at off-list mail. Your issue happen on (C) case, and you are tring to solve it. It is easy to understand if it was indicated at log area. I have imagined other system from "multiple CPU/Codec support". (A) (B) FE <-> BE (C) (D) BE <-> BE > > I'm not sure, this is just my guess. > > I'm happy if we can support it more easily :) > Right now I am trying re-use simple-card driver as much as possible > and still make it work for flexible sound cards. I will be happy to > discuss and improve the patch wherever necessary. Please help me > understand which part you think looks to be hacky. > > But, if it was difficult to keep compatibility on simple-card, > > we/you need to have new one. > Patch 17/23 and 18/23 introduce new compatible > 'simple-cc-audio-card'. Idea was to use component chaining which > allows connection of FE<->BE and multiple BE<->BE components along the > DAPM path (patch 16/23). Do you think it would be fine? This seems very complex system for current simple-card. "concept" of simple-card is simple (but "code" is not so simple...) Because of it, it doesn't assume flexible connection. Maybe your patch works for you, but might breaks other systems. Or, makes code or DT settings more complex/ununderstandable. Not sure, but my guess. Using cpu@0 node for BE is the 1st confusable point for me. Using fe/be instead of cpu/codec is easy to understand. Thank you for your help !! Best regards --- Kuninori Morimoto
On 6/30/2020 12:25 PM, Kuninori Morimoto wrote: > External email: Use caution opening links or attachments > > > Hi Sameer > > Thank you for explaining detail at off-list mail. > > Your issue happen on (C) case, and you are tring to solve it. > It is easy to understand if it was indicated at log area. > I have imagined other system from "multiple CPU/Codec support". > > (A) (B) > FE <-> BE > > (C) (D) > BE <-> BE > >>> I'm not sure, this is just my guess. >>> I'm happy if we can support it more easily :) >> Right now I am trying re-use simple-card driver as much as possible >> and still make it work for flexible sound cards. I will be happy to >> discuss and improve the patch wherever necessary. Please help me >> understand which part you think looks to be hacky. >>> But, if it was difficult to keep compatibility on simple-card, >>> we/you need to have new one. >> Patch 17/23 and 18/23 introduce new compatible >> 'simple-cc-audio-card'. Idea was to use component chaining which >> allows connection of FE<->BE and multiple BE<->BE components along the >> DAPM path (patch 16/23). Do you think it would be fine? > This seems very complex system for current simple-card. > "concept" of simple-card is simple (but "code" is not so simple...) > Because of it, it doesn't assume flexible connection. > > Maybe your patch works for you, but might breaks other systems. > Or, makes code or DT settings more complex/ununderstandable. > Not sure, but my guess. Yes there are complex use cases, but if we look at the amount of changes required in simple-card driver that is not too much. Existing binding for simple-card driver would still work fine for our cases. Yes there are some deviations and we don't want to break existing users, that is why a *new* compatible was introduced and specific items can be pushed under it. Majority of the simple-card driver is getting re-used here. We just need to make sure it does not affect anyone else. > > Using cpu@0 node for BE is the 1st confusable point for me. Don't we use the same methodology for CODEC<->CODEC link and still describe the DAI link with 'cpu@0' and 'codec@0'? > Using fe/be instead of cpu/codec is easy to understand. I guess you are referring to DT binding part. The parsing code specifically looks for "codec" sub node and thus present conventions had to be used. > > Thank you for your help !! > > Best regards > --- > Kuninori Morimoto
On Tue, Jun 30, 2020 at 01:22:29PM +0530, Sameer Pujar wrote: > Yes there are complex use cases, but if we look at the amount of changes > required in simple-card driver that is not too much. Existing binding for > simple-card driver would still work fine for our cases. Yes there are some > deviations and we don't want to break existing users, that is why a *new* > compatible was introduced and specific items can be pushed under it. > Majority of the simple-card driver is getting re-used here. We just need to > make sure it does not affect anyone else. Why simple-card and not audio-graph-card? > > Using fe/be instead of cpu/codec is easy to understand. > I guess you are referring to DT binding part. The parsing code specifically > looks for "codec" sub node and thus present conventions had to be used. Remember that this stuff gets fixed into the ABI so we'd have to live with this for ever.
On 6/30/2020 4:31 PM, Mark Brown wrote: > On Tue, Jun 30, 2020 at 01:22:29PM +0530, Sameer Pujar wrote: > >> Yes there are complex use cases, but if we look at the amount of changes >> required in simple-card driver that is not too much. Existing binding for >> simple-card driver would still work fine for our cases. Yes there are some >> deviations and we don't want to break existing users, that is why a *new* >> compatible was introduced and specific items can be pushed under it. >> Majority of the simple-card driver is getting re-used here. We just need to >> make sure it does not affect anyone else. > Why simple-card and not audio-graph-card? Frankly speaking I have not used audio-graph-card before. I had a brief look at the related binding. It seems it can use similar DT properties that simple-card uses, although the binding style appears to be different. However I am not sure if it offers better solutions to the problems I am facing. For example, the ability to connect or form a chain of components to realize more complicated use cases with DPCM, some of which were discussed in [0]. Can you please help me understand why it could be preferred? [0] https://lkml.org/lkml/2020/4/30/519 >>> Using fe/be instead of cpu/codec is easy to understand. >> I guess you are referring to DT binding part. The parsing code specifically >> looks for "codec" sub node and thus present conventions had to be used. > Remember that this stuff gets fixed into the ABI so we'd have to live > with this for ever.
On Tue, Jun 30, 2020 at 06:23:49PM +0530, Sameer Pujar wrote: > On 6/30/2020 4:31 PM, Mark Brown wrote: > > Why simple-card and not audio-graph-card? > Frankly speaking I have not used audio-graph-card before. I had a brief look > at the related binding. It seems it can use similar DT properties that > simple-card uses, although the binding style appears to be different. > However I am not sure if it offers better solutions to the problems I am > facing. For example, the ability to connect or form a chain of components to > realize more complicated use cases with DPCM, some of which were discussed > in [0]. Can you please help me understand why it could be preferred? > [0] https://lkml.org/lkml/2020/4/30/519 It's the more modern thing which covers everything simple-card does and more, I'd expect new development to go into that rather than simple-card.
On 6/30/2020 9:02 PM, Mark Brown wrote: > On Tue, Jun 30, 2020 at 06:23:49PM +0530, Sameer Pujar wrote: >> On 6/30/2020 4:31 PM, Mark Brown wrote: >>> Why simple-card and not audio-graph-card? >> Frankly speaking I have not used audio-graph-card before. I had a brief look >> at the related binding. It seems it can use similar DT properties that >> simple-card uses, although the binding style appears to be different. >> However I am not sure if it offers better solutions to the problems I am >> facing. For example, the ability to connect or form a chain of components to >> realize more complicated use cases with DPCM, some of which were discussed >> in [0]. Can you please help me understand why it could be preferred? >> [0] https://lkml.org/lkml/2020/4/30/519 > It's the more modern thing which covers everything simple-card does and > more, I'd expect new development to go into that rather than > simple-card. Hi Mark & Kuninori, For the HW I am using, there are no fixed endpoints and I am not sure if it is allowed to have empty endpoints in audio-graph-card. Crossbar/router provides the flexibility to connect the components in any required order. Patch [05/23] exposes required graph and MUX controls for the flexible configurations. Mostly, in DT, I am trying to model the component itself and finally router can help me specify the audio path to interconnect various components. Hence I was trying to understand if it is really necessary to represent the links using audio-graph-card. Kindly help me understand what more it offers. If simple-card works fine, are we allowed to use it? Thank you, Sameer.
On Thu, Jul 02, 2020 at 04:06:14PM +0530, Sameer Pujar wrote: > For the HW I am using, there are no fixed endpoints and I am not sure if it > is allowed to have empty endpoints in audio-graph-card. Crossbar/router > provides the flexibility to connect the components in any required order. > Patch [05/23] exposes required graph and MUX controls for the flexible > configurations. Mostly, in DT, I am trying to model the component itself and > finally router can help me specify the audio path to interconnect various > components. Hence I was trying to understand if it is really necessary to > represent the links using audio-graph-card. Kindly help me understand what > more it offers. If simple-card works fine, are we allowed to use it? The links in the graph card should be the physical links at the edge of the devices, those must be fixed no matter what.
Hi Mark, On 7/2/2020 5:56 PM, Mark Brown wrote: > On Thu, Jul 02, 2020 at 04:06:14PM +0530, Sameer Pujar wrote: > >> For the HW I am using, there are no fixed endpoints and I am not sure if it >> is allowed to have empty endpoints in audio-graph-card. Crossbar/router >> provides the flexibility to connect the components in any required order. >> Patch [05/23] exposes required graph and MUX controls for the flexible >> configurations. Mostly, in DT, I am trying to model the component itself and >> finally router can help me specify the audio path to interconnect various >> components. Hence I was trying to understand if it is really necessary to >> represent the links using audio-graph-card. Kindly help me understand what >> more it offers. If simple-card works fine, are we allowed to use it? > The links in the graph card should be the physical links at the edge of > the devices, those must be fixed no matter what. I used graph-card and could get few things working with it. Based on the feedback so far, I am planning to split the series as suggested and send two series as below. [1] Tegra AHUB series and related DT bindings as V5. [2] Audio graph card enhancements and related DT bindings for Tegra platforms as V1. Thanks, Sameer.
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 02d6295..15c4388 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -116,7 +116,6 @@ static void simple_parse_mclk_fs(struct device_node *top, static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv, struct device_node *np, - struct device_node *codec, struct link_info *li, bool is_top) { @@ -137,8 +136,13 @@ static int simple_dai_link_of_dpcm(struct asoc_simple_priv *priv, * Codec |return|Pass * np */ - if (li->cpu == (np == codec)) - return 0; + if (li->cpu) { + if (!strcmp(np->name, "codec")) + return 0; + } else { + if (!strcmp(np->name, "cpu")) + return 0; + } dev_dbg(dev, "link_of DPCM (%pOF)\n", np); @@ -354,7 +358,6 @@ static int simple_for_each_link(struct asoc_simple_priv *priv, struct link_info *li, bool is_top), int (*func_dpcm)(struct asoc_simple_priv *priv, struct device_node *np, - struct device_node *codec, struct link_info *li, bool is_top)) { struct device *dev = simple_priv_to_dev(priv); @@ -407,7 +410,7 @@ static int simple_for_each_link(struct asoc_simple_priv *priv, if (dpcm_selectable && (num > 2 || adata.convert_rate || adata.convert_channels)) - ret = func_dpcm(priv, np, codec, li, is_top); + ret = func_dpcm(priv, np, li, is_top); /* else normal sound */ else ret = func_noml(priv, np, codec, li, is_top); @@ -527,12 +530,11 @@ static int simple_count_noml(struct asoc_simple_priv *priv, static int simple_count_dpcm(struct asoc_simple_priv *priv, struct device_node *np, - struct device_node *codec, struct link_info *li, bool is_top) { li->dais++; /* CPU or Codec */ li->link++; /* CPU-dummy or dummy-Codec */ - if (np == codec) + if (!strcmp(np->name, "codec")) li->conf++; return 0;
The simple-card driver supports multiple CPU and single Codec entries for DPCM DAI links. In some cases it is required to have multiple CPU/Codecs. Currently parsing logic for DPCM link loops over all children of DAI link but assumes that there is a single Codec entry. When DAI link has multiple Codecs it considers only the first Codec entry and remaining Codecs are wrongly treated as CPU. This happens because first Codec is used as reference for parsing all other child nodes. This is fixed by using string comparisons of child node names instead of node pointer reference in simple_dai_link_of_dpcm(). So All CPU get parsed in first run and all Codecs in subsequent run. After this simple_dai_link_of_dpcm() does not required 'codec' argument and hence is removed. Signed-off-by: Sameer Pujar <spujar@nvidia.com> --- sound/soc/generic/simple-card.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)