Re: [PATCH 2/2] ASoC: mediatek: mt8192: support rt1015p_rt5682s

[Jiaxin Yu]

On Tue, 2022-03-01 at 17:03 +0800, Tzung-Bi Shih wrote:
> On Tue, Mar 01, 2022 at 03:29:24PM +0800, Jiaxin Yu wrote:
> > From: Jiaxin Yu <jiaxin.yu@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> 
> The environment didn't configure properly so that the header showed
> up.
> See [1].
> 
> [1]: 
> https://git-scm.com/docs/git-send-email#Documentation/git-send-email.txt---fromltaddressgt
> 
Ok, I will take a look at the "from: " header.

> > diff --git a/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-
> > rt5682.c b/sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
> 
> [...]
> > +static struct snd_soc_card mt8192_mt6359_rt1015p_rt5682s_card = {
> > +	.name = "mt8192_mt6359_rt1015p_rt5682s",
> > +	.owner = THIS_MODULE,
> > +	.dai_link = mt8192_mt6359_dai_links,
> > +	.num_links = ARRAY_SIZE(mt8192_mt6359_dai_links),
> > +	.controls = mt8192_mt6359_rt1015p_rt5682_controls,
> > +	.num_controls =
> > ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_controls),
> > +	.dapm_widgets = mt8192_mt6359_rt1015p_rt5682_widgets,
> > +	.num_dapm_widgets =
> > ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_widgets),
> > +	.dapm_routes = mt8192_mt6359_rt1015p_rt5682_routes,
> > +	.num_dapm_routes =
> > ARRAY_SIZE(mt8192_mt6359_rt1015p_rt5682_routes),
> > +};
> 
> Are the two cards only different from names
> (mt8192_mt6359_rt1015p_rt5682_card vs.
> mt8192_mt6359_rt1015p_rt5682s_card)?
> 
Yes, they are only different form names.

> > @@ -1150,6 +1177,52 @@ static int mt8192_mt6359_dev_probe(struct
> > platform_device *pdev)
> >  				dai_link->num_platforms =
> >  					ARRAY_SIZE(i2s3_rt1015p_platfor
> > ms);
> >  			}
> > +		} else if (strcmp(dai_link->name, "I2S8") == 0) {
> > +			if (card == &mt8192_mt6359_rt1015_rt5682_card
> > ||
> > +			    card == &mt8192_mt6359_rt1015p_rt5682_card)
> > {
> > +				dai_link->cpus = i2s8_rt5682_cpus;
> > +				dai_link->num_cpus =
> > +					ARRAY_SIZE(i2s8_rt5682_cpus);
> > +				dai_link->codecs = i2s8_rt5682_codecs;
> > +				dai_link->num_codecs =
> > +					ARRAY_SIZE(i2s8_rt5682_codecs);
> > +				dai_link->platforms =
> > i2s8_rt5682_platforms;
> > +				dai_link->num_platforms =
> > +					ARRAY_SIZE(i2s8_rt5682_platform
> > s);
> > +			} else if (card ==
> > &mt8192_mt6359_rt1015p_rt5682s_card) {
> > +				dai_link->cpus = i2s8_rt5682s_cpus;
> > +				dai_link->num_cpus =
> > +					ARRAY_SIZE(i2s8_rt5682s_cpus);
> > +				dai_link->codecs = i2s8_rt5682s_codecs;
> > +				dai_link->num_codecs =
> > +					ARRAY_SIZE(i2s8_rt5682s_codecs)
> > ;
> > +				dai_link->platforms =
> > i2s8_rt5682s_platforms;
> > +				dai_link->num_platforms =
> > +					ARRAY_SIZE(i2s8_rt5682s_platfor
> > ms);
> > +			}
> > +		} else if (strcmp(dai_link->name, "I2S9") == 0) {
> > +			if (card == &mt8192_mt6359_rt1015_rt5682_card
> > ||
> > +			    card == &mt8192_mt6359_rt1015p_rt5682_card)
> > {
> > +				dai_link->cpus = i2s9_rt5682_cpus;
> > +				dai_link->num_cpus =
> > +					ARRAY_SIZE(i2s9_rt5682_cpus);
> > +				dai_link->codecs = i2s9_rt5682_codecs;
> > +				dai_link->num_codecs =
> > +					ARRAY_SIZE(i2s9_rt5682_codecs);
> > +				dai_link->platforms =
> > i2s9_rt5682_platforms;
> > +				dai_link->num_platforms =
> > +					ARRAY_SIZE(i2s9_rt5682_platform
> > s);
> > +			} else if (card ==
> > &mt8192_mt6359_rt1015p_rt5682s_card) {
> > +				dai_link->cpus = i2s9_rt5682s_cpus;
> > +				dai_link->num_cpus =
> > +					ARRAY_SIZE(i2s9_rt5682s_cpus);
> > +				dai_link->codecs = i2s9_rt5682s_codecs;
> > +				dai_link->num_codecs =
> > +					ARRAY_SIZE(i2s9_rt5682s_codecs)
> > ;
> > +				dai_link->platforms =
> > i2s9_rt5682s_platforms;
> > +				dai_link->num_platforms =
> > +					ARRAY_SIZE(i2s9_rt5682s_platfor
> > ms);
> > +			}
> 
> After seeing the code, I am starting to wonder if the reuse is
> overkill.  If
> they (RT5682 vs. RT5682S) only have some minor differences, probably
> it could
> reuse more by:
> 
> SND_SOC_DAILINK_DEFS(i2s8, ...
> SND_SOC_DAILINK_DEFS(i2s9, ...
> 
> ...
> 
> if (card == &mt8192_mt6359_rt1015p_rt5682s_card) {
>         i2s8_codecs.name = RT5682S_DEV0_NAME;
>         i2s8_codecs.dai_name = RT5682S_CODEC_DAI;
>         ...
> }
> 
> Or even uses of_device_is_compatible() if it would like to reuse the
> struct
> snd_soc_card.
If we reuse the struct snd_soc_card, the card .name will be same.
Should I change the card .name through of_device_is_compatible()?

Maybe like below:

Remove rt5682x related words for snd_soc_card.
static struct snd_soc_card mt8192_mt6359_rt1015p_card = {
	.owner = THIS_MODULE,
	.dai_link = mt8192_mt6359_dai_links,
	.num_links = ARRAY_SIZE(mt8192_mt6359_dai_links),
	.controls = mt8192_mt6359_rt1015p_controls,
	.num_controls = ARRAY_SIZE(mt8192_mt6359_rt1015p_controls),
	.dapm_widgets = mt8192_mt6359_rt1015p_widgets,
	.num_dapm_widgets = ARRAY_SIZE(mt8192_mt6359_rt1015p_widgets),
	.dapm_routes = mt8192_mt6359_rt1015p_routes,
	.num_dapm_routes = ARRAY_SIZE(mt8192_mt6359_rt1015p_routes),
};

static const struct of_device_id mt8192_mt6359_dt_match[] = {
	{
		.compatible = "mediatek,mt8192_mt6359_rt1015_rt5682",
		.data = &mt8192_mt6359_rt1015_rt5682_card,
	},
	{
		.compatible = "mediatek,mt8192_mt6359_rt1015p_rt5682",
		.data = &mt8192_mt6359_rt1015p_card,
	},
	{
		.compatible = "mediatek,mt8192_mt6359_rt1015p_rt5682s",
		.data = &mt8192_mt6359_rt1015p_card,
	},
	{}
};

if (of_device_is_compatible(np, "mediatek,
mt8192_mt6359_rt1015p_rt5682")) {
	card.name = "mt8192_mt6359_rt1015p_rt5682";
	...
} else if (of_device_is_compatible(np, "mediatek,
mt8192_mt6359_rt1015p_rt5682s")) {
	card.name = "mt8192_mt6359_rt1015p_rt5682s";
	...
}