| Message ID | 20241010141432.716868-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ASoC: rsnd: Fix probe failure on HiHope boards due to endpoint parsing | expand |
Hi > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > On the HiHope boards, we have a single port with a single endpoint defined > as below: > .... > rsnd_port: port { > rsnd_endpoint: endpoint { > remote-endpoint = <&dw_hdmi0_snd_in>; > > dai-format = "i2s"; > bitclock-master = <&rsnd_endpoint>; > frame-master = <&rsnd_endpoint>; > > playback = <&ssi2>; > }; > }; > .... > > With commit 547b02f74e4a ("ASoC: rsnd: enable multi Component support for > Audio Graph Card/Card2"), support for multiple ports was added. This caused > probe failures on HiHope boards, as the endpoint could not be retrieved due > to incorrect device node pointers being used. > > This patch fixes the issue by updating the `rsnd_dai_of_node()` and > `rsnd_dai_probe()` functions to use the correct device node pointers based > on the port names ('port' or 'ports'). It ensures that the endpoint is > properly parsed for both single and multi-port configurations, restoring > compatibility with HiHope boards. > > Fixes: 547b02f74e4a ("ASoC: rsnd: enable multi Component support for Audio Graph Card/Card2") > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > Hi Geert, > I was intending to rename the sh folder to renesas, do you think that should be OK? > Cheers, Prabhakar > --- > sound/soc/sh/rcar/core.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c > index 9784718a2b6f..eca5ce096e54 100644 > --- a/sound/soc/sh/rcar/core.c > +++ b/sound/soc/sh/rcar/core.c > @@ -1281,7 +1281,9 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph) > if (!of_node_name_eq(ports, "ports") && > !of_node_name_eq(ports, "port")) > continue; > - priv->component_dais[i] = of_graph_get_endpoint_count(ports); > + priv->component_dais[i] = > + of_graph_get_endpoint_count(of_node_name_eq(ports, "ports") ? > + ports : np); > nr += priv->component_dais[i]; > i++; > if (i >= RSND_MAX_COMPONENT) { > @@ -1493,7 +1495,8 @@ static int rsnd_dai_probe(struct rsnd_priv *priv) > if (!of_node_name_eq(ports, "ports") && > !of_node_name_eq(ports, "port")) > continue; > - for_each_endpoint_of_node(ports, dai_np) { > + for_each_endpoint_of_node(of_node_name_eq(ports, "ports") ? > + ports : np, dai_np) { > __rsnd_dai_probe(priv, dai_np, dai_np, 0, dai_i); > if (!rsnd_is_gen1(priv) && !rsnd_is_gen2(priv)) { > rdai = rsnd_rdai_get(priv, dai_i); Ah, I had a plan to clean-up around here, but Of-Graph rejected the idea. Indeed it will be issue if DT doesn't have "ports". I would like to re-cleanup around here in the future. But anyway, for this patch Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Prabhakar, On Thu, Oct 10, 2024 at 4:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > I was intending to rename the sh folder to renesas, do you think that should be OK? That's up to Morimoto-san and the other sound maintainers. > sound/soc/sh/rcar/core.c | 7 +++++-- Gr{oetje,eeting}s, Geert
Hi Prabhakar, On Thu, Oct 10, 2024 at 4:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > On the HiHope boards, we have a single port with a single endpoint defined > as below: > .... > rsnd_port: port { > rsnd_endpoint: endpoint { > remote-endpoint = <&dw_hdmi0_snd_in>; > > dai-format = "i2s"; > bitclock-master = <&rsnd_endpoint>; > frame-master = <&rsnd_endpoint>; > > playback = <&ssi2>; > }; > }; > .... > > With commit 547b02f74e4a ("ASoC: rsnd: enable multi Component support for > Audio Graph Card/Card2"), support for multiple ports was added. This caused > probe failures on HiHope boards, as the endpoint could not be retrieved due > to incorrect device node pointers being used. > > This patch fixes the issue by updating the `rsnd_dai_of_node()` and > `rsnd_dai_probe()` functions to use the correct device node pointers based > on the port names ('port' or 'ports'). It ensures that the endpoint is > properly parsed for both single and multi-port configurations, restoring > compatibility with HiHope boards. > > Fixes: 547b02f74e4a ("ASoC: rsnd: enable multi Component support for Audio Graph Card/Card2") > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/sound/soc/sh/rcar/core.c > +++ b/sound/soc/sh/rcar/core.c > @@ -1281,7 +1281,9 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph) > if (!of_node_name_eq(ports, "ports") && > !of_node_name_eq(ports, "port")) > continue; > - priv->component_dais[i] = of_graph_get_endpoint_count(ports); > + priv->component_dais[i] = > + of_graph_get_endpoint_count(of_node_name_eq(ports, "ports") ? > + ports : np); As of_node_name_eq() is not inline or __pure, of_node_name_eq(ports, "ports") will be called twice. So it may make sense to add a helper, combining the selection with the validation above: const struct device_node *pick_endpoint_node_for_ports(const struct device_node *np, const struct device_node *e_ports, const struct device_node *e_port) { if (of_node_name_eq(ports, "ports")) return e_ports; if (of_node_name_eq(ports, "port")) return e_port; return NULL; } > nr += priv->component_dais[i]; > i++; > if (i >= RSND_MAX_COMPONENT) { > @@ -1493,7 +1495,8 @@ static int rsnd_dai_probe(struct rsnd_priv *priv) > if (!of_node_name_eq(ports, "ports") && > !of_node_name_eq(ports, "port")) > continue; > - for_each_endpoint_of_node(ports, dai_np) { > + for_each_endpoint_of_node(of_node_name_eq(ports, "ports") ? > + ports : np, dai_np) { Likewise. > __rsnd_dai_probe(priv, dai_np, dai_np, 0, dai_i); > if (!rsnd_is_gen1(priv) && !rsnd_is_gen2(priv)) { > rdai = rsnd_rdai_get(priv, dai_i); Gr{oetje,eeting}s, Geert
Hi Geert and Kuninori, On Fri, Oct 11, 2024 at 7:27 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Oct 10, 2024 at 4:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > I was intending to rename the sh folder to renesas, do you think that should be OK? > > That's up to Morimoto-san and the other sound maintainers. > Thanks Geert. Morimoto-san, what do you think of renaming the sh folder to renesas? Cheers, Prabhakar
Hi Geert, Thank you for the review. On Fri, Oct 11, 2024 at 7:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Oct 10, 2024 at 4:15 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > On the HiHope boards, we have a single port with a single endpoint defined > > as below: > > .... > > rsnd_port: port { > > rsnd_endpoint: endpoint { > > remote-endpoint = <&dw_hdmi0_snd_in>; > > > > dai-format = "i2s"; > > bitclock-master = <&rsnd_endpoint>; > > frame-master = <&rsnd_endpoint>; > > > > playback = <&ssi2>; > > }; > > }; > > .... > > > > With commit 547b02f74e4a ("ASoC: rsnd: enable multi Component support for > > Audio Graph Card/Card2"), support for multiple ports was added. This caused > > probe failures on HiHope boards, as the endpoint could not be retrieved due > > to incorrect device node pointers being used. > > > > This patch fixes the issue by updating the `rsnd_dai_of_node()` and > > `rsnd_dai_probe()` functions to use the correct device node pointers based > > on the port names ('port' or 'ports'). It ensures that the endpoint is > > properly parsed for both single and multi-port configurations, restoring > > compatibility with HiHope boards. > > > > Fixes: 547b02f74e4a ("ASoC: rsnd: enable multi Component support for Audio Graph Card/Card2") > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/sound/soc/sh/rcar/core.c > > +++ b/sound/soc/sh/rcar/core.c > > @@ -1281,7 +1281,9 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph) > > if (!of_node_name_eq(ports, "ports") && > > !of_node_name_eq(ports, "port")) > > continue; > > - priv->component_dais[i] = of_graph_get_endpoint_count(ports); > > + priv->component_dais[i] = > > + of_graph_get_endpoint_count(of_node_name_eq(ports, "ports") ? > > + ports : np); > > As of_node_name_eq() is not inline or __pure, of_node_name_eq(ports, > "ports") will be called twice. So it may make sense to add a helper, > combining the selection with the validation above: > > const struct device_node *pick_endpoint_node_for_ports(const > struct device_node *np, > const struct device_node *e_ports, const struct > device_node *e_port) > { > if (of_node_name_eq(ports, "ports")) > return e_ports; > if (of_node_name_eq(ports, "port")) > return e_port; > return NULL; > } > I'll drop the const maybe to reduce the diff and have something like below: +static struct device_node* + pick_endpoint_node_for_ports(struct device_node *e_ports, + struct device_node *e_port) +{ + if (of_node_name_eq(e_ports, "ports")) + return e_ports; + + if (of_node_name_eq(e_ports, "port")) + return e_port; + + return NULL; +} and use it like below in the rsnd_dai_of_node() function: @@ -1278,10 +1291,10 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph) * Audio-Graph-Card */ for_each_child_of_node(np, ports) { - if (!of_node_name_eq(ports, "ports") && - !of_node_name_eq(ports, "port")) + node = pick_endpoint_node_for_ports(ports, np); + if (!node) continue; - priv->component_dais[i] = of_graph_get_endpoint_count(ports); + priv->component_dais[i] = of_graph_get_endpoint_count(node); > > nr += priv->component_dais[i]; > > i++; > > if (i >= RSND_MAX_COMPONENT) { > > @@ -1493,7 +1495,8 @@ static int rsnd_dai_probe(struct rsnd_priv *priv) > > if (!of_node_name_eq(ports, "ports") && > > !of_node_name_eq(ports, "port")) > > continue; > > - for_each_endpoint_of_node(ports, dai_np) { > > + for_each_endpoint_of_node(of_node_name_eq(ports, "ports") ? > > + ports : np, dai_np) { > > Likewise. > Ok, now I will have to create a local variable for this loop, which will look like something below: @@ -1486,14 +1499,15 @@ static int rsnd_dai_probe(struct rsnd_priv *priv) */ dai_i = 0; if (is_graph) { + struct device_node *dai_np_port; struct device_node *ports; struct device_node *dai_np; for_each_child_of_node(np, ports) { - if (!of_node_name_eq(ports, "ports") && - !of_node_name_eq(ports, "port")) + dai_np_port = pick_endpoint_node_for_ports(ports, np); + if (!dai_np_port) continue; - for_each_endpoint_of_node(ports, dai_np) { + for_each_endpoint_of_node(dai_np_port, dai_np) { __rsnd_dai_probe(priv, dai_np, dai_np, 0, dai_i); if (!rsnd_is_gen1(priv) && !rsnd_is_gen2(priv)) { rdai = rsnd_rdai_get(priv, dai_i); Does that sound OK? Cheers, Prabhakar
On Thu, 10 Oct 2024 15:14:32 +0100, Prabhakar wrote: > On the HiHope boards, we have a single port with a single endpoint defined > as below: > .... > rsnd_port: port { > rsnd_endpoint: endpoint { > remote-endpoint = <&dw_hdmi0_snd_in>; > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: rsnd: Fix probe failure on HiHope boards due to endpoint parsing commit: 9b064d200aa8fee9d1d7ced05d8a617e45966715 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi Lad > > > I was intending to rename the sh folder to renesas, do you think that should be OK? (snip) > Morimoto-san, what do you think of renaming the sh folder to renesas? Yeah, it sounds good idea ! Best regards --- Kuninori Morimoto
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index 9784718a2b6f..eca5ce096e54 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1281,7 +1281,9 @@ static int rsnd_dai_of_node(struct rsnd_priv *priv, int *is_graph) if (!of_node_name_eq(ports, "ports") && !of_node_name_eq(ports, "port")) continue; - priv->component_dais[i] = of_graph_get_endpoint_count(ports); + priv->component_dais[i] = + of_graph_get_endpoint_count(of_node_name_eq(ports, "ports") ? + ports : np); nr += priv->component_dais[i]; i++; if (i >= RSND_MAX_COMPONENT) { @@ -1493,7 +1495,8 @@ static int rsnd_dai_probe(struct rsnd_priv *priv) if (!of_node_name_eq(ports, "ports") && !of_node_name_eq(ports, "port")) continue; - for_each_endpoint_of_node(ports, dai_np) { + for_each_endpoint_of_node(of_node_name_eq(ports, "ports") ? + ports : np, dai_np) { __rsnd_dai_probe(priv, dai_np, dai_np, 0, dai_i); if (!rsnd_is_gen1(priv) && !rsnd_is_gen2(priv)) { rdai = rsnd_rdai_get(priv, dai_i);