Re: [PATCH 3/3] ASoC: mediatek: mt8188-mt6359: add SOF support

[Trevor Wu (吳文良)]

On Tue, 2023-07-25 at 09:11 +0200, AngeloGioacchino Del Regno wrote:
> Il 25/07/23 05:53, Trevor Wu ha scritto:
> > SOF is enabled when adsp phandle is assigned to "mediatek,adsp".
> > The required callback will be assigned when SOF is enabled.
> > 
> > Additionally, "mediatek,dai-link" is introduced to decide the
> > supported
> > dai links for a project, so user can reuse the machine driver
> > regardless
> > of dai link combination.
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@xxxxxxxxxxxx>
> > ---
> >   sound/soc/mediatek/mt8188/mt8188-mt6359.c | 217
> > ++++++++++++++++++++--
> >   1 file changed, 205 insertions(+), 12 deletions(-)
> > 
> > diff --git a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > index 667d79f33bf2..e205de2899a9 100644
> > --- a/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> > +++ b/sound/soc/mediatek/mt8188/mt8188-mt6359.c
> 
> ..snip..
> 
> > @@ -1074,21 +1215,64 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> >   	if (!card->name)
> >   		card->name = card_data->name;
> >   
> > -	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > -	if (!priv)
> > -		return  -ENOMEM;
> > -
> >   	if (of_property_read_bool(pdev->dev.of_node, "audio-routing"))
> > {
> >   		ret = snd_soc_of_parse_audio_routing(card, "audio-
> > routing");
> >   		if (ret)
> >   			return ret;
> >   	}
> >   
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	soc_card_data = devm_kzalloc(&pdev->dev, sizeof(*card_data),
> > GFP_KERNEL);
> > +	if (!soc_card_data)
> > +		return -ENOMEM;
> > +
> > +	soc_card_data->mach_priv = priv;
> > +
> > +	adsp_node = of_parse_phandle(pdev->dev.of_node,
> > "mediatek,adsp", 0);
> > +	if (adsp_node) {
> > +		struct mtk_sof_priv *sof_priv;
> > +
> > +		sof_priv = devm_kzalloc(&pdev->dev, sizeof(*sof_priv),
> > GFP_KERNEL);
> > +		if (!sof_priv) {
> > +			ret = -ENOMEM;
> > +			goto err_adsp_node;
> > +		}
> > +		sof_priv->conn_streams = g_sof_conn_streams;
> > +		sof_priv->num_streams = ARRAY_SIZE(g_sof_conn_streams);
> > +		soc_card_data->sof_priv = sof_priv;
> > +		card->probe = mtk_sof_card_probe;
> > +		card->late_probe = mtk_sof_card_late_probe;
> > +		if (!card->topology_shortname_created) {
> > +			snprintf(card->topology_shortname, 32, "sof-
> > %s", card->name);
> > +			card->topology_shortname_created = true;
> > +		}
> > +		card->name = card->topology_shortname;
> > +	}
> > +
> > +	if (of_property_read_bool(pdev->dev.of_node, "mediatek,dai-
> > link")) {
> > +		ret = mtk_sof_dailink_parse_of(card, pdev->dev.of_node,
> > +					       "mediatek,dai-link",
> > +					       mt8188_mt6359_dai_links,
> > +					       ARRAY_SIZE(mt8188_mt6359
> > _dai_links));
> > +		if (ret) {
> > +			dev_err_probe(&pdev->dev, ret, "Parse dai-link
> > fail\n");
> > +			goto err_adsp_node;
> > +		}
> > +	} else {
> > +		if (!adsp_node)
> > +			card->num_links = DAI_LINK_REGULAR_NUM;
> > +	}
> > +
> >   	platform_node = of_parse_phandle(pdev->dev.of_node,
> >   					 "mediatek,platform", 0);
> >   	if (!platform_node) {
> >   		ret = -EINVAL;
> > -		return dev_err_probe(&pdev->dev, ret, "Property
> > 'platform' missing or invalid\n");
> > +		dev_err_probe(&pdev->dev, ret, "Property 'platform'
> > missing or invalid\n");
> 
> We could shorten this with
> 
> ret = dev_err_probe(&pdev->dev, -EINVAL, "Property ....");
> goto err_adsp_node;
> 
> ...but I don't have strong opinions about that, it's your choice.
> 

It is concise. I will apply it in v2.

> > +		goto err_platform_node;
> > +
> >   	}
> >   
> >   	ret = parse_dai_link_info(card);
> > @@ -1096,8 +1280,12 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> >   		goto err;
> >   
> >   	for_each_card_prelinks(card, i, dai_link) {
> > -		if (!dai_link->platforms->name)
> > -			dai_link->platforms->of_node = platform_node;
> > +		if (!dai_link->platforms->name) {
> > +			if (!strncmp(dai_link->name, "AFE_SOF",
> > strlen("AFE_SOF")) && adsp_node)
> > +				dai_link->platforms->of_node =
> > adsp_node;
> > +			else
> > +				dai_link->platforms->of_node =
> > platform_node;
> > +		}
> >   
> >   		if (strcmp(dai_link->name, "DPTX_BE") == 0) {
> >   			if (strcmp(dai_link->codecs->dai_name, "snd-
> > soc-dummy-dai"))
> > @@ -1140,7 +1328,7 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> >   	}
> >   
> >   	priv->private_data = card_data;
> > -	snd_soc_card_set_drvdata(card, priv);
> > +	snd_soc_card_set_drvdata(card, soc_card_data);
> >   
> >   	ret = devm_snd_soc_register_card(&pdev->dev, card);
> >   	if (ret)
> > @@ -1149,6 +1337,11 @@ static int mt8188_mt6359_dev_probe(struct
> > platform_device *pdev)
> >   err:
> >   	of_node_put(platform_node);
> >   	clean_card_reference(card);
> > +
> > +err_adsp_node:
> > +err_platform_node:
> 
> Just one label is enough. Keep err_adsp_node, remove
> err_platform_node.
> 
OK. I will remove one label in v2.

Thanks,
Trevor