refine sdw_rt711_rt1308_rt715 by RanderWang · Pull Request #1796 · thesofproject/linux

RanderWang · 2020-02-16T12:29:31Z

Refine HDMI support for recent changes
Fix DMIC id at 4

Tested on Comet Lake U & H. Tiger Lake with 1308 in SDW mode and I2S mode
fixes #1788

bardliao · 2020-02-17T08:51:51Z


-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC)
-static struct snd_soc_jack hdmi[3];
+#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI)


This is conflict with #1794. And we already depends on SND_HDA_CODEC_HDMI on Kconfig.

@bardliao Ack, let's go with the approach with #1794 .
@RanderWang Once 1794 is merged, I think this patch is still needed. You could make similar change as I did in #1777 and make HDMI optional at runtime.

kv2019i

Looks good. Maybe let's wait for #1794 to go in, and then add a bit more HDMI logic (like done in #1777) and then this is good to go.

kv2019i · 2020-02-17T10:25:32Z


-#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_COMMON_HDMI_CODEC)
-static struct snd_soc_jack hdmi[3];
+#if IS_ENABLED(CONFIG_SND_HDA_CODEC_HDMI)


@bardliao Ack, let's go with the approach with #1794 .
@RanderWang Once 1794 is merged, I think this patch is still needed. You could make similar change as I did in #1777 and make HDMI optional at runtime.

lyakh · 2020-02-17T11:01:43Z

+	for (i = 0; i < hdmi_num; i++) {
+		struct snd_soc_dai_link_component *idisp_components;
+
+		idisp_components = devm_kzalloc(dev, sizeof(*idisp_components),


Why do you want to allocate them one by one? What was wrong with allocating the whole array at once?

@lyakh First this array is defined out of #ifdef HDMI_CODE but used here. So if HDMI is not defined, I got a build warning message. I don't want to add anther #ifdef HDMI for definition of this array, so I moved the definition into this loop.

@RanderWang so you prefer to run 4 kmalloc calls with error handling in the likely case of HDMI being used (in about 100% of the cases) to having one more #ifdef? That doesn't seem like a good choice to me. #ifdefs are frowned upon in the Linux kernel in .c files, that's true, but we have a ton of them around... And you can certainly find many ways to avoid that additional #ifdef. In fact you can avoid them completely here. You can use the if (hdmi_num > 0) clause above, but that would add indentation. Or you can make a new function. Either way choosing code aesthetics and sacrificing run-time doesn't seem a good solution to me in this case.

lyakh · 2020-02-17T11:03:42Z

 			return -ENOMEM;
-	}

-	for (i = 0; i < hdmi_num; i++) {


Unless there's a real practical reason for this, please, revert. Please keep the array.

see above reason, thanks!

lyakh · 2020-02-17T11:13:40Z

+	/*
+	 * dai link id of SSP & DMIC & HDMI are based on last
+	 * link id used by sdw link. The dai_id is not always equal
+	 * to id for dai link id may be changed by some sdw codecs.


Let's capitalise: "DAI," "ID," not sure what the preferred way for "sdw" is - it isn't an acronym, so, maybe in small is ok.

In your comment you say, that you unified the acronym style by writing them all in lower-case, but here it's still mixed. And I don't think "hdmi" is a widely-accepted style, so I'd really use "HDMI," "DAI," "ID"... But let's see what others say, maybe I'm wrong on this.

yes, I grep git log and sound, HDMI DAI ID is more popular

...and still this comment hasn't been updated.

sorry, missed, done

lyakh · 2020-02-17T11:16:07Z

+	 * on generic TGL platform, I2S or SDW mode is supported
+	 * based on board rework. A acpi device is registered in
+	 * system only when I2S mode is supported, not SDW mode.
+	 * Here check acpi id to confirm I2S is supported.


"ACPI," "ID" - also in the commit message. It looks a bit weird to me to see "I2S," "TGL," "SDW capitalised and "ACPI" and "ID" - not. BTW, there's again the question for "SDW" vs. "sdw." Also "An ACPI" both here and in the commit message

thanks , I will unify the style

RanderWang · 2020-02-18T08:48:09Z

@lyakh @kv2019i I update my PR.
(1) revert my change about dai array
(2) unify my comment style, all name in non-cap

I also checked the 1777, Now I don't pick it to this PR because there is no such issue with use case.

Thanks!

lyakh · 2020-02-18T09:49:22Z

+	 * dai id is fixed at 4 for 715 to keep sdw
+	 * dmic and hdmi setting static in ucm
+	 */
+	dai_links->id = 4;


If this has to agree with UCM, this is a part of the user ABI, right? Then we might need some macros for all such values...

thanks, I will refine it

lyakh · 2020-02-18T09:50:17Z

-	ssp_num = hweight_long(ssp_mask);
+	/*
+	 * on generic tgl platform, i2s or sdw mode is supported
+	 * based on board rework. A acpi device is registered in


"acpi" and "i2s" isn't a commonly accepted style IMHO

I search acpi & ACPI, i2s & I2S in git log. yes, ACPI & I2S is more popular. Thanks! I will refine it

RanderWang · 2020-02-18T12:31:31Z

@lyakh thanks for your advice. I update my PR to adopt formal style.

Tested on Comet Lake-U, Comet Lake-H and Tiger Lake RVP.

Use CONFIG_SND_HDA_CODEC_HDMI instead of SND_SOC_SOF_HDA_COMMON_HDMI_CODEC which is deleted by commit 8c10db2 ("ASoC: SOF: Intel: hda: remove SND_SOC_SOF_HDA_COMMON_HDMI_CODEC"). Also fix build warning when CONFIG_SND_HDA_CODEC_HDMI is not defined. Now the maximum HDMI count on intel platform is 4, so increase HDMI jack count to 4. On Tiger Lake, 4 HDMI jacks are used, and only 3 on platforms before Tiger Lake. Tested on Comet Lake-U, Comet Lake-H and Tiger Lake RVP. Signed-off-by: randerwang <rander.wang@linux.intel.com>

bardliao

Thanks @RanderWang LGTM now

RanderWang · 2020-02-19T01:10:41Z

fix the conflict file

…itude To make sdw DMIC & HDMI setting static in UCM, DAI link ID of rt715 is fixed at 4 in topology file. Refine machine driver according to this change. Signed-off-by: randerwang <rander.wang@linux.intel.com>

… mode On generic Tiger Lake platform, I2S or sdw mode is supported based on board rework. A ACPI device is registered in system only when I2S mode is supported, not sdw mode. Here check ACPI ID to confirm whether I2S mode is supported. Signed-off-by: randerwang <rander.wang@linux.intel.com>

RanderWang · 2020-02-19T08:21:08Z

update PR, check platform quirk info then fix DAI ID of rt711 at 4

YvonneYang2 · 2020-02-19T10:59:10Z

Tested it with latest PR1796, it can work and no side effect.

plbossart · 2020-02-19T17:04:35Z

@xiulipan @fredoh9 CI farm is completely in the weeds I am afraid. Ignoring all unrelated HDAudio issues.

We do not use struct socket in unix_autobind() and pass struct sock to unix_bind_bsd() and unix_bind_abstract(). Let's pass it to unix_autobind() as well. Also, this patch fixes these errors by checkpatch.pl. ERROR: do not use assignment in if condition thesofproject#1795: FILE: net/unix/af_unix.c:1795: + if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr CHECK: Logical continuations should be on the previous line thesofproject#1796: FILE: net/unix/af_unix.c:1796: + if (test_bit(SOCK_PASSCRED, &sock->flags) && !u->addr + && (err = unix_autobind(sock)) != 0) Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

RanderWang requested review from lgirdwood and plbossart as code owners February 16, 2020 12:29

RanderWang requested review from bardliao, kv2019i, lyakh and ranj063 February 16, 2020 12:32

RanderWang mentioned this pull request Feb 17, 2020

[TEST] Soundwire/multi link support #1769

Merged

bardliao reviewed Feb 17, 2020

View reviewed changes

kv2019i requested changes Feb 17, 2020

View reviewed changes

kv2019i mentioned this pull request Feb 17, 2020

ASoC: Intel: sdw_rt711_rt1308_rt715: fix HDMI support #1797

Closed

lyakh requested changes Feb 17, 2020

View reviewed changes

RanderWang force-pushed the dmic_fix branch 2 times, most recently from b5dc85c to 646a527 Compare February 18, 2020 08:02

lyakh reviewed Feb 18, 2020

View reviewed changes

RanderWang force-pushed the dmic_fix branch 2 times, most recently from 6d8e8e2 to 4634dad Compare February 18, 2020 12:28

plbossart previously approved these changes Feb 18, 2020

View reviewed changes

RanderWang dismissed plbossart’s stale review via bd626c6 February 19, 2020 01:09

RanderWang force-pushed the dmic_fix branch from 4634dad to bd626c6 Compare February 19, 2020 01:09

bardliao previously approved these changes Feb 19, 2020

View reviewed changes

RanderWang dismissed bardliao’s stale review via 64b2a04 February 19, 2020 08:10

RanderWang force-pushed the dmic_fix branch 2 times, most recently from 64b2a04 to bdf5cc8 Compare February 19, 2020 08:12

RanderWang added 2 commits February 19, 2020 16:18

ASoC: Intel: sdw_rt711_rt1308_rt715: fix DAI link ID for rt715 on Lat…

6f627a7

…itude To make sdw DMIC & HDMI setting static in UCM, DAI link ID of rt715 is fixed at 4 in topology file. Refine machine driver according to this change. Signed-off-by: randerwang <rander.wang@linux.intel.com>

RanderWang force-pushed the dmic_fix branch from bdf5cc8 to 4629f5b Compare February 19, 2020 08:19

lyakh approved these changes Feb 19, 2020

View reviewed changes

plbossart approved these changes Feb 19, 2020

View reviewed changes

plbossart merged commit 737f886 into thesofproject:topic/sof-dev Feb 19, 2020

Uh oh!

Conversation

RanderWang commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Feb 19, 2020

Uh oh!

RanderWang commented Feb 19, 2020

Uh oh!

YvonneYang2 commented Feb 19, 2020

Uh oh!

plbossart commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

RanderWang commented Feb 16, 2020 •

edited

Loading

RanderWang commented Feb 18, 2020 •

edited

Loading

lyakh Feb 18, 2020 •

edited

Loading

lyakh Feb 18, 2020 •

edited

Loading

RanderWang commented Feb 18, 2020 •

edited

Loading