Max98373 SDW review #2062

naveen-manohar · 2020-04-28T20:05:29Z

As discussed in #2039 submitting Max98373-SDW codec and helper driver for review.

plbossart

Looks quite good @naveen-manohar
The main issue is really the race conditions that can happen. The Slave CANNOT start any transactions before it is fully enumerated, so you have to rely on the complete/wait_for_completion mechanism that was added last year. If you don't things will break.
Also the pm_runtime part is largely incorrect or duplicated.

I would also try and avoid duplicating the core parts of the codec. See e.g. the Realtek examples, where they tried to reuse the same code. A lot of parts don't change when you only change the interface.

It's encouraging though, thanks for sharing. @bardliao and I can help for the pm_runtime stuff.

sound/soc/codecs/Kconfig

sound/soc/codecs/max98373-sdw.h

sound/soc/codecs/max98373-sdw.c

sound/soc/intel/boards/sof_sdw_max98373.c

sound/soc/intel/common/soc-acpi-intel-tgl-match.c

sound/soc/codecs/max98373-sdw.c

sound/soc/codecs/max98373-sdw.h

sound/soc/intel/boards/sof_sdw_max98373.c

bardliao

Overall it looks good to me. Some changes needed.

sound/soc/codecs/max98373-sdw.c

sound/soc/intel/boards/sof_sdw.c

sound/soc/intel/boards/sof_sdw_max98373.c

naveen-manohar · 2020-04-30T07:59:43Z

Thank you All for the review and feedback, wrt max98373-sdw codec-driver, i will let @ryans-lee to address the feedback/queries, all others i shall re-check and update.

sound/soc/codecs/max98373-sdw.c

lyakh · 2020-05-04T06:48:31Z

sound/soc/codecs/max98373-sdw.c

please, remove

sound/soc/codecs/max98373-sdw.c

lyakh

there are still unanswered questions and unaddressed comments

sound/soc/codecs/max98373-sdw.c

kv2019i · 2020-05-11T15:33:44Z

@naveen-manohar There is a very high level of checkpatch warnings still:
https://travis-ci.org/github/thesofproject/linux/jobs/685699044

sound/soc/intel/common/soc-acpi-intel-tgl-match.c

naveen-manohar · 2020-05-12T05:15:54Z

@naveen-manohar There is a very high level of checkpatch warnings still:
https://travis-ci.org/github/thesofproject/linux/jobs/685699044

Apologies shall correct it.. i was concentrating only on the supporting files, missed the main codec driver check

naveen-manohar · 2020-05-13T03:57:30Z

@naveen-manohar There is a very high level of checkpatch warnings still:
https://travis-ci.org/github/thesofproject/linux/jobs/685699044

Apologies shall correct it.. i was concentrating only on the supporting files, missed the main codec driver check

Done.. Please help review.

sound/soc/codecs/max98373-sdw.c

RanderWang

LGTM

naveen-manohar · 2020-05-21T09:13:29Z

LGTM

Thank you for the review & support @RanderWang

plbossart

I only found a minor issue with num_of_ports. this is likely a copy-paste issue since the problem exists for other codecs, but let's fix it here.
Looks good otherwise, nice work @ryans-lee @naveen-manohar

sound/soc/codecs/max98373-sdw.c

plbossart

I only have a couple of nit-picks, this should be submitted upstream now for additional reviews.
Thanks @naveen-manohar and @ryans-lee

sound/soc/codecs/max98373-sdw.c

naveen-manohar · 2020-05-22T17:25:36Z

I only have a couple of nit-picks, this should be submitted upstream now for additional reviews.
Thanks @naveen-manohar and @ryans-lee

re-submitted the changes post verifying on-board,
Many Thanks @plbossart for the review & support.
Please help us moving this to upstream.

sound/soc/codecs/max98373-sdw.c

lyakh

looks mostly good to me, just a couple of minor cosmetic clean-up comments

sound/soc/codecs/max98373-sdw.c

sound/soc/codecs/max98373.h

Changes since v13: * cosmetic clean-up Changes since v12: * Removed obsolete code in set_tdm_slot function. Changes since v11: * Removed initial value of num_of_ports Changes since v10: * Removed redundant interrupt_callback in slave_ops * Removed assigning slave_ops in probe function Changes since v9: * Added rx_mask, slot to configure TDM slot properly Changes since v8: * Removed duplicated parts with I2S driver. * Support TDM * Removed functions not needed. * Minor cosmetic changes. Changes since v7: * addred return if sdw_stream is null in set_sdw_stream. Changes since v6: * Changed dpn type from SDW_DPN_SIMPLE to SDW_DPN_FULL to configure DPN_HCtrl Changes since v5: * BDE and limiter default on * Added missing pm_init_once flag set Changes since v4: * Added pm_init_once to do pm stuff once. * Added return if SW RESET command is failed. * ADded SET_RUNTIME_PM_OPS * Removed all warning in checkpatch * Modifed pm_runtime stuff * Added GENMASK * Removed interrupt callback function Changes since v3: * Added return after software reset failure * Removed empty line in max98373_snd_probe * Removed superfluous braces in max98373_clock_calculate Changes since v2: * Removed unused variables : tdm_mode, first_hw_init * Replaced while loop by for loop to replace open-coded code * Removed unnecessary condition check in max98373_io_init * Removed unnecessary runtime pm control in max98373_io_init * Added more amp initializtion code to max98373_io_init * Removed unnecessary global shutdown on/off to avoid pop noise * Added max98373_clock_calculate to calculate SWCLK * snd_soc_register_component -> devm_snd_soc_register_component Changes since v1: * Modified GPL-v2.0 to GPL-v2.0-or-later * Added soundwire registers to the header. * Modified comments. Control registers->Control Port Registers * removed duplication code for the pm control * Added module_sdw_driver() instread of using module_init/exit * Removed ret variable. * Removed empty lines. Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> Signed-off-by: Naveen Manohar <naveen.m@intel.com>

…oard Add max98373-sdw helper function, which configures 2x MAX98373 codecs to Link1. Changes since v2: * SPDX license with GPL-2.0-only as per PR#2028 Changes since v1: * Separated Makefile entry to avoid mixing of realtek and maxim * Corrected sof_sdw_max98373.c driver info comment style * Added quirk for volteer to indicate speaker number Signed-off-by: Naveen Manohar <naveen.m@intel.com>

…ire driver RT5682 is in Soundwire mode on Link0 & 2x MAX98373 on link1. Signed-off-by: Naveen Manohar <naveen.m@intel.com>

RanderWang

Tested on TGL

plbossart · 2020-06-08T18:50:05Z

@naveen-manohar @ryans-lee do you want us to send this to alsa-devel, along with the Reviewed-by tags we already have? Just checking, it's straightforward to do for me/Kai/Ranjani. Thanks!

ryans-lee · 2020-06-08T23:44:58Z

yes, please. I appreciate for your help.

RanderWang · 2020-06-09T07:36:35Z

@naveen-manohar @ryans-lee do you want us to send this to alsa-devel, along with the Reviewed-by tags we already have? Just checking, it's straightforward to do for me/Kai/Ranjani. Thanks!

@plbossart please merge #2178. Thanks!

naveen-manohar requested review from dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners April 28, 2020 20:05

naveen-manohar mentioned this pull request Apr 28, 2020

TGL: max98373-sdw codec observes SDW_DP0_INTMASK read failed:-61 #2039

Closed

plbossart requested changes Apr 28, 2020

View reviewed changes

lyakh requested changes Apr 29, 2020

View reviewed changes

bardliao requested changes Apr 29, 2020

View reviewed changes

naveen-manohar force-pushed the max98373-sdw_review branch from ed70c4c to e7b00e6 Compare May 2, 2020 03:49

naveen-manohar requested review from bardliao, lyakh and plbossart May 2, 2020 03:52

naveen-manohar force-pushed the max98373-sdw_review branch from e7b00e6 to 54f461d Compare May 2, 2020 04:31

lyakh requested changes May 4, 2020

View reviewed changes

naveen-manohar force-pushed the max98373-sdw_review branch 2 times, most recently from 224bfb6 to a92cf97 Compare May 8, 2020 12:13

naveen-manohar requested a review from lyakh May 8, 2020 12:15

lyakh requested changes May 11, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

naveen-manohar force-pushed the max98373-sdw_review branch from a92cf97 to eb0ca0c Compare May 11, 2020 14:49

sathya-nujella reviewed May 12, 2020

View reviewed changes

sound/soc/intel/common/soc-acpi-intel-tgl-match.c Outdated Show resolved Hide resolved

naveen-manohar force-pushed the max98373-sdw_review branch from eb0ca0c to d339891 Compare May 13, 2020 03:56

naveen-manohar requested a review from lyakh May 13, 2020 03:57

lyakh reviewed May 13, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

naveen-manohar force-pushed the max98373-sdw_review branch 2 times, most recently from 8ec9091 to 6e7a83f Compare May 17, 2020 16:56

naveen-manohar force-pushed the max98373-sdw_review branch from b64c5ed to 43c9b16 Compare May 21, 2020 08:24

RanderWang previously approved these changes May 21, 2020

View reviewed changes

plbossart requested changes May 21, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

naveen-manohar dismissed RanderWang’s stale review via af8efbb May 22, 2020 02:33

naveen-manohar force-pushed the max98373-sdw_review branch from 43c9b16 to af8efbb Compare May 22, 2020 02:33

naveen-manohar requested a review from plbossart May 22, 2020 02:50

plbossart previously approved these changes May 22, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

naveen-manohar dismissed plbossart’s stale review via f2014e3 May 22, 2020 17:21

naveen-manohar force-pushed the max98373-sdw_review branch from af8efbb to f2014e3 Compare May 22, 2020 17:21

naveen-manohar requested a review from plbossart May 23, 2020 01:57

lyakh reviewed May 25, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

lyakh requested changes May 25, 2020

View reviewed changes

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373-sdw.c Outdated Show resolved Hide resolved

sound/soc/codecs/max98373.h Outdated Show resolved Hide resolved

Ryan Lee and others added 3 commits May 26, 2020 06:40

ASoC: Intel: common: add match table for TGL MAX98373 + RT5682 SoundW…

732b5cc

…ire driver RT5682 is in Soundwire mode on Link0 & 2x MAX98373 on link1. Signed-off-by: Naveen Manohar <naveen.m@intel.com>

naveen-manohar force-pushed the max98373-sdw_review branch from f2014e3 to 732b5cc Compare May 26, 2020 07:22

naveen-manohar requested a review from lyakh May 26, 2020 07:34

plbossart approved these changes May 26, 2020

View reviewed changes

lyakh approved these changes May 27, 2020

View reviewed changes

RanderWang approved these changes May 27, 2020

View reviewed changes

ranj063 merged commit 0d22f66 into thesofproject:topic/sof-dev May 27, 2020

ClarexZhou mentioned this pull request Jun 2, 2020

[BUG] [TGL] ipc error when tirgger wov pipline with s24_le and s32_le thesofproject/sof#3009

Closed

This was referenced Jun 2, 2020

[TGL][SDW] runtime PM always in active on TGL chromebook/SDW mode #2162

Closed

[BUG][TGL]two sides amplifiers both output “front left” and no amplifier output "front Right" when do test speaker test on speaker #2169

Closed

YvonneYang2 mentioned this pull request Jun 9, 2020

[BUG][TGL][DSM] one additional sample will be captured after playback ends thesofproject/sof#2880

Closed

Max98373 SDW review #2062

Max98373 SDW review #2062

Uh oh!

Conversation

naveen-manohar commented Apr 28, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naveen-manohar commented Apr 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyakh May 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kv2019i commented May 11, 2020

Uh oh!

Uh oh!

naveen-manohar commented May 12, 2020

Uh oh!

naveen-manohar commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

naveen-manohar commented May 21, 2020

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!