> Hi Mac,
>
> > +#define SOF_RT1011_SPEAKER_WL BIT(0)
> > +#define SOF_RT1011_SPEAKER_WR BIT(1)
> > +#define SOF_RT1011_SPEAKER_TL BIT(2)
> > +#define SOF_RT1011_SPEAKER_TR BIT(3)
> > +#define SPK_CH 4
>
> use a prefix maybe for consistency?
Yes. Considering that we have Woofers and Tweeters speakers consistencies.
> It's also unclear why this is needed when you can have 2 or more channels,
> and looking below
>
> > +
> > +/* Default: Woofer+Tweeter speakers */
/* Default: Woofer speakers */
>
> It's more like ALL devices have Woofers.
> Some devices don't have tweeters.
>
> the WL and WR quirks are always on apparently.
Yes, you are right. The purpose to do is Woofers as default and Tweeters+Woofers as specific project. I revised them below. Or any advices?
>
> > +static unsigned long sof_rt1011_quirk = SOF_RT1011_SPEAKER_WL |
> SOF_RT1011_SPEAKER_WR |
> > + SOF_RT1011_SPEAKER_TL |
> SOF_RT1011_SPEAKER_TR;
static unsigned long sof_rt1011_quirk = SOF_RT1011_SPEAKER_WL | SOF_RT1011_SPEAKER_WR;
> > +
> > +static int sof_rt1011_quirk_cb(const struct dmi_system_id *id) {
> > + sof_rt1011_quirk = (unsigned long)id->driver_data;
> > + return 1;
> > +}
> > +
> > +static const struct dmi_system_id sof_rt1011_quirk_table[] = {
> > + {
> > + .callback = sof_rt1011_quirk_cb,
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Google"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Palkia"),
DMI_MATCH(DMI_PRODUCT_NAME, "Helios"),
> > + },
> > + .driver_data = (void *)(SOF_RT1011_SPEAKER_WL |
> > + SOF_RT1011_SPEAKER_WR),
.driver_data = (void *)(SOF_RT1011_SPEAKER_WL | SOF_RT1011_SPEAKER_WR |
SOF_RT1011_SPEAKER_TL | SOF_RT1011_SPEAKER_TR),
> > + },
> > + {
> > + }
> > +};
>
> > +static const struct snd_soc_dapm_widget cml_rt1011_tt_widgets[] = {
> > + SND_SOC_DAPM_SPK("TL Ext Spk", NULL),
> > + SND_SOC_DAPM_SPK("TR Ext Spk", NULL), };
> > +
> > static const struct snd_soc_dapm_route cml_rt1011_rt5682_map[] = {
> > /*speaker*/
> > {"TL Ext Spk", NULL, "TL SPO"},
>
> Something's not right, if I look at the code after applying this patch I
> get:
>
> static const struct snd_soc_dapm_route cml_rt1011_rt5682_map[] = {
> /*speaker*/
> {"TL Ext Spk", NULL, "TL SPO"},
> {"TR Ext Spk", NULL, "TR SPO"},
>
> That's duplicaged from [1]
True. My mistake, just remain one(apart from rt1011_rt5682_map).
>
> > @@ -82,6 +118,12 @@ static const struct snd_soc_dapm_route
> cml_rt1011_rt5682_map[] = {
> > {"DMic", NULL, "SoC DMIC"},
> > };
> >
> > +static const struct snd_soc_dapm_route cml_rt1011_tt_map[] = {
> > + /*TL/TR speaker*/
> > + {"TL Ext Spk", NULL, "TL SPO" },
> > + {"TR Ext Spk", NULL, "TR SPO" },
> > +};
>
> [1] we should remove the tweeeter maps in cml_rt1011_rt5682_map, no?
Yes. Remove tweeters from rt1011_rt5682_map.
>
> > static int cml_rt5682_hw_params(struct snd_pcm_substream *substream,
> > struct snd_pcm_hw_params *params)
> > {
> > @@ -192,31 +263,52 @@ static int cml_rt1011_hw_params(struct
> snd_pcm_substream *substream,
> > * The feedback is captured for each codec individually.
> > * Hence all 4 codecs use 1 Tx slot each for feedback.
> > */
> > - if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:00")) {
> > - ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > - 0x4, 0x1, 4, 24);
> > - if (ret < 0)
> > - break;
> > - }
> > - if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:02")) {
> > - ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > - 0x1, 0x1, 4, 24);
> > - if (ret < 0)
> > - break;
> > - }
> > - /* TDM Rx slot 2 is used for Right Woofer & Tweeters pair */
> > - if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:01")) {
> > - ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > - 0x8, 0x2, 4, 24);
> > - if (ret < 0)
> > - break;
> > - }
> > - if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:03")) {
> > - ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > - 0x2, 0x2, 4, 24);
> > - if (ret < 0)
> > - break;
> > + if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_TL |
> > + SOF_RT1011_SPEAKER_TR)) {
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:00")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x4, 0x1, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:02")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x1, 0x1, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + /* TDM Rx slot 2 is used for Right Woofer & Tweeters
> pair */
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:01")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x8, 0x2, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:03")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x2, 0x2, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > + } else {
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:00")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x4, 0x1, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + if (!strcmp(codec_dai->component->name, "i2c-
> 10EC1011:01")) {
> > + ret = snd_soc_dai_set_tdm_slot(codec_dai,
> > + 0x8, 0x2, 4, 24);
> > + if (ret < 0)
> > + break;
> > + }
> > }
>
> the if/else case can be simplified. The baseline is two woofers, so they can be
> added unconditionally, and then you can add what's missing for the tweeters.
> That way you have a consistent way of handling the TL/TR quirk.
Thanks for addressing. I revised the if/else logic below
if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_WL |
SOF_RT1011_SPEAKER_WR)) {
/* TDM slots for WL/WR */
....
}
if (sof_rt1011_quirk & (SOF_RT1011_SPEAKER_TL |
SOF_RT1011_SPEAKER_TR)) {
/* TDM slots for TL/TR */
...
}
> > static int snd_cml_rt1011_probe(struct platform_device *pdev)
> > {
> > + struct snd_soc_dai_link_component *rt1011_dais_components;
> > + struct snd_soc_codec_conf *rt1011_dais_confs;
> > struct card_private *ctx;
> > struct snd_soc_acpi_mach *mach;
> > const char *platform_name;
> > - int ret;
> > + int ret, i;
> >
> > ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC);
>
> D'oh! Did we again let this slip in?
>
> cml_rt1011_rt5682.c: ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx),
> GFP_ATOMIC);
> sof_da7219_max98373.c: ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx),
> GFP_ATOMIC);
>
> This should be fixed in a separate patch, we don't need th ATOMIC attribute in
> any machine drivers - copy-paste!
Copy that.
>
> > if (!ctx)
> > @@ -456,6 +541,59 @@ 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);
> > +
> > + rt1011_dais_components = devm_kzalloc(&pdev->dev,
> > + sizeof(struct
> snd_soc_dai_link_component) *
> > + SPK_CH, GFP_KERNEL);
> > +
> > + for (i = 0; i < SPK_CH; i++) {
> > + rt1011_dais_confs[i].dlc.name =
> devm_kasprintf(&pdev->dev,
> > + GFP_KERNEL,
> > + "i2c-
> 10EC1011:0%d",
> > + i);
>
> Check for NULL and return -ENOMEM for all 3 devm_ calls above?
Yes, I will add the check conditions separately. Afterwards I will revise and submit the entirely new one.
