[PATCH v2] ASoC: Intel: boards: add stereo playback by woofer speaker

[Chiang, Mac]

Thanks Pierre.
I will modify these changes. And as previously as you mentioned, I reply below to be confirmed. If they are all good as well, I will send the v3 patch.

> > @@ -413,14 +488,6 @@ static struct snd_soc_codec_conf rt1011_conf[] = {
> >   		.dlc = COMP_CODEC_CONF("i2c-10EC1011:01"),
> >   		.name_prefix = "WR",
> >   	},
> > -	{
> > -		.dlc = COMP_CODEC_CONF("i2c-10EC1011:02"),
> > -		.name_prefix = "TL",
> > -	},
> > -	{
> > -		.dlc = COMP_CODEC_CONF("i2c-10EC1011:03"),
> > -		.name_prefix = "TR",
> > -	},
> 
> is there a reason to remove the prefixes for Tweeters? Or is this handled
> somewhere else?
Yes, the "TL", "TR" name_prefix handling is moving to snd_cml_rt1011_probe() allocated by rt1011_dais_confs[]

> > +		for (i = 0; i < SPK_CH; i++) {
> > +			rt1011_dais_confs[i].dlc.name =
> devm_kasprintf(&pdev->dev,
> > +								GFP_KERNEL,
> > +								"i2c-
> 10EC1011:0%d",
> > +								i);
> 
> if (!rt1011_dais_confs[i].dlc.name)
>      return -ENOMEM;
> 
> > +			switch (i) {
> > +			case 0:
> > +				rt1011_dais_confs[i].name_prefix = "WL";
> > +				break;
> > +
> > +			case 1:
> > +				rt1011_dais_confs[i].name_prefix = "WR";
> > +				break;
> > +			case 2:
> > +				rt1011_dais_confs[i].name_prefix = "TL";
> > +				break;
> > +			case 3:
> > +				rt1011_dais_confs[i].name_prefix = "TR";
> > +				break;
			default:
				return -EINVAL;
> > +			}
> 
> How does this work? the rt1011_dais_confs only has 2 components now?
> This would probably generate a NULL access or generate an out-of-bounds
> access in the array.
By default there were 2 components allocated by rt1011_conf[]. However, rt1011_dais_confs has 4 components when woofers+tweeters support. BTW, I should add the default case to handle the invalid values as well.

>> Subject: [PATCH v2] ASoC: Intel: boards: add stereo playback by woofer 
>> speaker
>> support woofer stereo speakers individually
>Both the commit title and message are a bit misleading. should be something like
>"
>ASoC: Intel: boards: cml_rt1011: split woofer and tweeter support
>Support Woofer stereo speakers by default and optionally Tweeter stereo speakers with a DMI quirk "
Modified the title and messages.

> > @@ -302,10 +378,8 @@ SND_SOC_DAILINK_DEF(ssp1_pin,
> >   	DAILINK_COMP_ARRAY(COMP_CPU("SSP1 Pin")));
> >   SND_SOC_DAILINK_DEF(ssp1_codec,
> >   	DAILINK_COMP_ARRAY(
> > -	/* WL */ COMP_CODEC("i2c-10EC1011:00",
> CML_RT1011_CODEC_DAI),
> > -	/* WR */ COMP_CODEC("i2c-10EC1011:01",
> CML_RT1011_CODEC_DAI),
> > -	/* TL */ COMP_CODEC("i2c-10EC1011:02",
> CML_RT1011_CODEC_DAI),
> > -	/* TR */ COMP_CODEC("i2c-10EC1011:03",
> CML_RT1011_CODEC_DAI)));
> > +       /* WL */ COMP_CODEC("i2c-10EC1011:00",
> CML_RT1011_CODEC_DAI),
> > +       /* WR */ COMP_CODEC("i2c-10EC1011:01",
> > + CML_RT1011_CODEC_DAI)));
> 
> is the alignment change needed?
No. Fixed.
> 
> > @@ -456,6 +525,65 @@ static int snd_cml_rt1011_probe(struct
> platform_device *pdev)
> >   	snd_soc_card_cml.dev = &pdev->dev;
> >   	platform_name = mach->mach_params.platform;
> >
> > +	dmi_check_system(sof_rt1011_quirk_table);
> > +
> > +	dev_info(&pdev->dev, "sof_rt1011_quirk = %lx\n", sof_rt1011_quirk);
> > +
> > +	if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_TL |
> > +				SOF_RT1011_SPEAKER_TR)) {
> > +		rt1011_dais_confs = devm_kzalloc(&pdev->dev,
> > +					sizeof(struct snd_soc_codec_conf) *
> > +					SPK_CH, GFP_KERNEL);
> > +
> > +		if (!rt1011_dais_confs)
> > +			return -ENOMEM;
> > +
> > +		rt1011_dais_components = devm_kzalloc(&pdev->dev,
> > +					sizeof(struct
> snd_soc_dai_link_component) *
> > +					SPK_CH, GFP_KERNEL);
> > +
> > +		if (!rt1011_dais_components)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < SPK_CH; i++) {
> > +			rt1011_dais_confs[i].dlc.name =
> devm_kasprintf(&pdev->dev,
> > +								GFP_KERNEL,
> > +								"i2c-
> 10EC1011:0%d",
> > +								i);
> 
> if (!rt1011_dais_confs[i].dlc.name)
>      return -ENOMEM;
added the checks.
> 
> > +			switch (i) {
> > +			case 0:
> > +				rt1011_dais_confs[i].name_prefix = "WL";
> > +				break;
> > +
> 
> spurious newline?
Removed the newline.
> 
> > +			case 1: