On Thu, Sep 05, 2024 at 03:02:55PM +0800, Binbin Zhou wrote:
> When the Codec is compiled into a module, we can't use
> snd_soc_of_get_dlc() to get the codec dai_name, use
> snd_soc_get_dai_name() instead.
>
> Also, for the cpu dailink, its dai_name is already defined as
> "loongson-i2s", so just get the corresponding of_node attribute here.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> ---
> sound/soc/loongson/loongson_card.c | 89 +++++++++++++++++++++---------
> 1 file changed, 63 insertions(+), 26 deletions(-)
>
> diff --git a/sound/soc/loongson/loongson_card.c b/sound/soc/loongson/loongson_card.c
> index a25287efdd5c..d45a3e77cb90 100644
> --- a/sound/soc/loongson/loongson_card.c
> +++ b/sound/soc/loongson/loongson_card.c
> @@ -119,44 +119,81 @@ static int loongson_card_parse_acpi(struct loongson_card_data *data)
> return 0;
> }
>
> -static int loongson_card_parse_of(struct loongson_card_data *data)
> +static int loongson_parse_cpu(struct device *dev, struct device_node **dai_node)
> {
> - struct snd_soc_card *card = &data->snd_card;
> - struct device_node *cpu, *codec;
> - struct device *dev = card->dev;
> - int ret, i;
> + struct device_node *cpu;
> + int ret = 0;
>
> cpu = of_get_child_by_name(dev->of_node, "cpu");
> - if (!cpu) {
> - dev_err(dev, "platform property missing or invalid\n");
> + if (!cpu)
> return -EINVAL;
> - }
> +
> + *dai_node = of_parse_phandle(cpu, "sound-dai", 0);
> + if (!*dai_node)
> + ret = -EINVAL;
> +
> + of_node_put(cpu);
This goes just after of_parse_phandle, which simplifies your code.
> + return ret;
> +}
> +
> +static int loongson_parse_codec(struct device *dev, struct device_node **dai_node,
> + const char **dai_name)
> +{
> + struct of_phandle_args args;
> + struct device_node *codec;
> + int ret = 0;
>
> codec = of_get_child_by_name(dev->of_node, "codec");
> - if (!codec) {
> - dev_err(dev, "audio-codec property missing or invalid\n");
> + if (!codec)
Hm? So you exit here and then caller does of_node_put on stack value.
This is buggy.
> + return -EINVAL;
> +
> + ret = of_parse_phandle_with_args(codec, "sound-dai", "#sound-dai-cells", 0, &args);
> + if (ret)
> + goto free_codec;
> +
> + ret = snd_soc_get_dai_name(&args, dai_name);
> + if (ret)
Your error paths should clean here. Each function is responsible to
clean up after itself in case of errors, not rely on caller.
> + goto free_codec;
> +
> + *dai_node = of_parse_phandle(codec, "sound-dai", 0);
> + if (!*dai_node)
> ret = -EINVAL;
> - goto free_cpu;
> +
> +free_codec:
You are not freeing any resources here (at least not directly). You are
dropping reference. Use matching label name. free is associated with
kalloc()..
> + of_node_put(codec);
> + return ret;
> +}
> +
> +static int loongson_card_parse_of(struct loongson_card_data *data)
> +{
> + struct device_node *codec_dai_node, *cpu_dai_node;
> + struct snd_soc_card *card = &data->snd_card;
> + struct device *dev = card->dev;
> + const char *codec_dai_name;
> + int ret = 0, i;
> +
> + ret = loongson_parse_cpu(dev, &cpu_dai_node);
> + if (ret) {
> + dev_err(dev, "cpu property missing or invalid.\n");
> + goto out;
> + }
> +
> + ret = loongson_parse_codec(dev, &codec_dai_node, &codec_dai_name);
> + if (ret) {
> + dev_err(dev, "audio-codec property missing or invalid.\n");
> + goto out;
> }
>
> for (i = 0; i < card->num_links; i++) {
> - ret = snd_soc_of_get_dlc(cpu, NULL, loongson_dai_links[i].cpus, 0);
> - if (ret) {
> - dev_err(dev, "getting cpu dlc error (%d)\n", ret);
> - goto free_codec;
> - }
> -
> - ret = snd_soc_of_get_dlc(codec, NULL, loongson_dai_links[i].codecs, 0);
> - if (ret) {
> - dev_err(dev, "getting codec dlc error (%d)\n", ret);
> - goto free_codec;
> - }
> + loongson_dai_links[i].platforms->of_node = cpu_dai_node;
> + loongson_dai_links[i].cpus->of_node = cpu_dai_node;
> + loongson_dai_links[i].codecs->of_node = codec_dai_node;
> + loongson_dai_links[i].codecs->dai_name = codec_dai_name;
> }
>
> -free_codec:
> - of_node_put(codec);
> -free_cpu:
> - of_node_put(cpu);
> +out:
> + of_node_put(codec_dai_node);
Yeah, so here we see drop of reference from stack-based pointer...
Best regards,
Krzysztof
