December 2018 - Sound-open-firmware - mailman.alsa-project.org

[Sound-open-firmware] [PATCH v3 00/14] Sound Open Firmware (SOF) core
by Pierre-Louis Bossart 30 Jan '19

30 Jan '19

Sound Open Firmware (SOF) is a host and DSP architecture agnostic audio DSP firmware. SOF is not tied to any specific host architecture or any specific physical IO communication type (it will work with on SoC DSPs, or DSP connected via SPI/I2C). SOF is also not coupled to any particular DSP architecture and has abstraction similar to Linux to allow porting to other DSP architectures. https://www.sofproject.org/ This patch series introduces the SOF core and utilities. Support for Intel devices is provided as a follow-up series. The SOF core manages all the core DSP services and ALSA/ASoC IO. The core is responsible for loading firmware, parsing topology, exposing PCMs and kcontrols, providing debug and trace mechanisms and performing IPC between host and DSP. The SOF core also has logic to allow reuse of existing machine drivers for other platforms/machines without any code modification. i.e. DAI links can be modified at runtime to bind with SOF and SOF topologies instead of existing hard coded DAI links and topology. Main changes since v2 (end of August): There are honestly too many changes to be listed... The biggest changes are the introduction of an ABI check and runtime_pm, as well as countless improvements coming from product validation. We will provide a revision history starting with the next update. Precisions: Since I've been maintaining the SOF kernel parts for most of 2018, Liam and I mutually agreed that it would be more efficient for me to send the patches upstream to avoid any disconnects. The code in this patchset is directly squashed from the SOF development branch [1], which tracks Mark Brown's for-next branch on a weekly basis and the configurations used for testing are based on the defconfigs at [2] Full disclosure on known limitation and issues (full list at [3]) a) the interaction with ASoC is not always perfect, there are a couple of points where a better solution is desired (use of private data, support for link DMA, etc). The known points are explicitly documented in the code and will be updated. To be clearer, it's our belief that SOF does not cripple ASoC in any way, and that merging this SOF core does not harm others. It's just complicated to get things right, especially on a couple of cases where the topology and DPCM frameworks generate complex flows that very few people understand. b) The get/put methods for controls generate an IPC and possibly a wake-up. This is not optimal and is being fixed. c) The debugfs parts will be updated for runtime_pm parts d) The topology free, firmware release and Hdaudio exit sequences will be updated. It is our intention to fully support module load/unload and free all resources. e) runtime_pm is being hardened and the use of SMART_SUSPEND was suggested. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre [1] https://github.com/thesofproject/linux [2] https://github.com/thesofproject/kconfig [3] https://github.com/thesofproject/linux/issues Liam Girdwood (12): ASoC: SOF: Add Sound Open Firmware driver core ASoC: SOF: Add Sound Open Firmware KControl support ASoC: SOF: Add driver debug support. ASoC: SOF: Add support for IPC IO between DSP and Host ASoC: SOF: Add PCM operations support ASoC: SOF: Add support for loading topologies ASoC: SOF: Add DSP firmware logger support ASoC: SOF: Add DSP HW abstraction operations ASoC: SOF: Add firmware loader support ASoC: SOF: Add userspace ABI support ASoC: SOF: Add PM support ASoC: SOF: Add Nocodec machine driver support Pierre-Louis Bossart (2): ASoC: SOF: Add xtensa support ASoC: SOF: Add utils include/sound/soc.h | 3 + include/sound/sof.h | 91 + include/sound/sof/control.h | 125 ++ include/sound/sof/dai-intel.h | 175 ++ include/sound/sof/dai.h | 75 + include/sound/sof/header.h | 158 ++ include/sound/sof/info.h | 118 ++ include/sound/sof/pm.h | 48 + include/sound/sof/stream.h | 149 ++ include/sound/sof/topology.h | 256 +++ include/sound/sof/trace.h | 66 + include/sound/sof/xtensa.h | 44 + include/uapi/sound/sof/abi.h | 63 + include/uapi/sound/sof/eq.h | 164 ++ include/uapi/sound/sof/fw.h | 67 + include/uapi/sound/sof/header.h | 27 + include/uapi/sound/sof/manifest.h | 188 ++ include/uapi/sound/sof/tokens.h | 95 + include/uapi/sound/sof/tone.h | 21 + include/uapi/sound/sof/trace.h | 93 + sound/soc/sof/control.c | 421 +++++ sound/soc/sof/core.c | 398 +++++ sound/soc/sof/debug.c | 176 ++ sound/soc/sof/ipc.c | 813 +++++++++ sound/soc/sof/loader.c | 322 ++++ sound/soc/sof/nocodec.c | 81 + sound/soc/sof/ops.c | 200 +++ sound/soc/sof/ops.h | 379 ++++ sound/soc/sof/pcm.c | 755 ++++++++ sound/soc/sof/pm.c | 403 +++++ sound/soc/sof/sof-priv.h | 574 ++++++ sound/soc/sof/topology.c | 2775 +++++++++++++++++++++++++++++ sound/soc/sof/trace.c | 295 +++ sound/soc/sof/utils.c | 173 ++ sound/soc/sof/xtensa/Kconfig | 3 + sound/soc/sof/xtensa/Makefile | 5 + sound/soc/sof/xtensa/core.c | 152 ++ 37 files changed, 9951 insertions(+) create mode 100644 include/sound/sof.h create mode 100644 include/sound/sof/control.h create mode 100644 include/sound/sof/dai-intel.h create mode 100644 include/sound/sof/dai.h create mode 100644 include/sound/sof/header.h create mode 100644 include/sound/sof/info.h create mode 100644 include/sound/sof/pm.h create mode 100644 include/sound/sof/stream.h create mode 100644 include/sound/sof/topology.h create mode 100644 include/sound/sof/trace.h create mode 100644 include/sound/sof/xtensa.h create mode 100644 include/uapi/sound/sof/abi.h create mode 100644 include/uapi/sound/sof/eq.h create mode 100644 include/uapi/sound/sof/fw.h create mode 100644 include/uapi/sound/sof/header.h create mode 100644 include/uapi/sound/sof/manifest.h create mode 100644 include/uapi/sound/sof/tokens.h create mode 100644 include/uapi/sound/sof/tone.h create mode 100644 include/uapi/sound/sof/trace.h create mode 100644 sound/soc/sof/control.c create mode 100644 sound/soc/sof/core.c create mode 100644 sound/soc/sof/debug.c create mode 100644 sound/soc/sof/ipc.c create mode 100644 sound/soc/sof/loader.c create mode 100644 sound/soc/sof/nocodec.c create mode 100644 sound/soc/sof/ops.c create mode 100644 sound/soc/sof/ops.h create mode 100644 sound/soc/sof/pcm.c create mode 100644 sound/soc/sof/pm.c create mode 100644 sound/soc/sof/sof-priv.h create mode 100644 sound/soc/sof/topology.c create mode 100644 sound/soc/sof/trace.c create mode 100644 sound/soc/sof/utils.c create mode 100644 sound/soc/sof/xtensa/Kconfig create mode 100644 sound/soc/sof/xtensa/Makefile create mode 100644 sound/soc/sof/xtensa/core.c -- 2.17.1

5 47

[Sound-open-firmware] [PATCH 00/21] Sound Open Firmware (SOF) - Intel support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

This patch series introduces the support for Intel devices (Baytrail, CherryTrail, Broadwell/Haswell, ApolloLake, GeminiLake, CannonLake, IceLake). Support for Skylake and KabyLake is incomplete at the moment but the basic building blocks required are already present. The series starts with the 'legacy' devices, then all the more recent ones and concludes with ACPI/PCI hooks. SKL+ platforms can be compiled without HDaudio link/codec support, in which case the hdac library is not used. When support for HDMI or HDaudio codecs is selected, parts of this library are used. With Takashi's latest patches, it is understood some fixes will be needed before merge, they will be provided as an update when merged by Mark. Patch 19/21 ('ALSA-HDA-export-process_unsol_events') was submitted as a dependency last week but not yet merged. It is provided for completeness should anyone try to compile the code. The machine drivers supported by SOF are not provided as part of this series, mainly because of likely conflicts with other patches in flight to support HDaudio fallback and additional work required for upstream. The build will only work in 'nocodec' mode for now. Support for SPI-based devices, compressed streams is also not provided for now since it's not ready for upstream just yet. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre Keyon Jie (3): ASoC: SOF: Intel: Add hda-bus support and initialization ASoC: SOF: Intel: SKL, CNL, APL platform DAIs ALSA: HDA: export process_unsol_events() Liam Girdwood (15): ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support. ASoC: SOF: Intel: Add HSW HW DSP support ASoC: SOF: Intel: Add support for BDW HW DSP support ASoC: SOF: Intel: Add APL/CNL HW DSP support ASoC: SOF: Intel: Add HDA controller for Intel DSP ASoC: SOF: Intel: Add Intel specific HDA DSP HW operations ASoC: SOF: Intel: Add Intel specific HDA IPC mechanisms. ASoC: SOF: Intel: Add Intel specific HDA firmware loader ASoC: SOF: Intel: Add Intel specific HDA PCM operations ASoC: SOF: Intel: Add Intel specific HDA stream operations ASoC: SOF: Intel: Add Intel specific HDA trace operations ASoC: SOF: Intel: Add platform differentiation for SKL, APL and CNL ASoC: SOF: Add ACPI device support ASoC: SOF: Add PCI device support ASoC: SOF: Add Build support for SOF core and Intel drivers Pierre-Louis Bossart (3): ASoC: SOF: Intel: Add support for HDAudio codecs ASoC: SOF: Intel: Add SKL-specific code loader ASoC: Intel: Kconfig: expose common option between SST and SOF drivers include/sound/hdaudio.h | 1 + sound/hda/hdac_bus.c | 7 +- sound/soc/Kconfig | 1 + sound/soc/Makefile | 1 + sound/soc/intel/Kconfig | 7 +- sound/soc/sof/Kconfig | 82 +++ sound/soc/sof/Makefile | 17 + sound/soc/sof/intel/Kconfig | 136 +++++ sound/soc/sof/intel/Makefile | 17 + sound/soc/sof/intel/apl.c | 99 ++++ sound/soc/sof/intel/bdw.c | 700 +++++++++++++++++++++++ sound/soc/sof/intel/byt.c | 805 +++++++++++++++++++++++++++ sound/soc/sof/intel/cnl.c | 242 ++++++++ sound/soc/sof/intel/hda-bus.c | 107 ++++ sound/soc/sof/intel/hda-codec.c | 185 ++++++ sound/soc/sof/intel/hda-ctrl.c | 166 ++++++ sound/soc/sof/intel/hda-dai.c | 368 ++++++++++++ sound/soc/sof/intel/hda-dsp.c | 417 ++++++++++++++ sound/soc/sof/intel/hda-ipc.c | 387 +++++++++++++ sound/soc/sof/intel/hda-loader-skl.c | 531 ++++++++++++++++++ sound/soc/sof/intel/hda-loader.c | 369 ++++++++++++ sound/soc/sof/intel/hda-pcm.c | 232 ++++++++ sound/soc/sof/intel/hda-stream.c | 661 ++++++++++++++++++++++ sound/soc/sof/intel/hda-trace.c | 79 +++ sound/soc/sof/intel/hda.c | 704 +++++++++++++++++++++++ sound/soc/sof/intel/hda.h | 537 ++++++++++++++++++ sound/soc/sof/intel/hsw.c | 701 +++++++++++++++++++++++ sound/soc/sof/intel/shim.h | 159 ++++++ sound/soc/sof/intel/skl.c | 92 +++ sound/soc/sof/sof-acpi-dev.c | 269 +++++++++ sound/soc/sof/sof-pci-dev.c | 361 ++++++++++++ 31 files changed, 8435 insertions(+), 5 deletions(-) create mode 100644 sound/soc/sof/Kconfig create mode 100644 sound/soc/sof/Makefile create mode 100644 sound/soc/sof/intel/Kconfig create mode 100644 sound/soc/sof/intel/Makefile create mode 100644 sound/soc/sof/intel/apl.c create mode 100644 sound/soc/sof/intel/bdw.c create mode 100644 sound/soc/sof/intel/byt.c create mode 100644 sound/soc/sof/intel/cnl.c create mode 100644 sound/soc/sof/intel/hda-bus.c create mode 100644 sound/soc/sof/intel/hda-codec.c create mode 100644 sound/soc/sof/intel/hda-ctrl.c create mode 100644 sound/soc/sof/intel/hda-dai.c create mode 100644 sound/soc/sof/intel/hda-dsp.c create mode 100644 sound/soc/sof/intel/hda-ipc.c create mode 100644 sound/soc/sof/intel/hda-loader-skl.c create mode 100644 sound/soc/sof/intel/hda-loader.c create mode 100644 sound/soc/sof/intel/hda-pcm.c create mode 100644 sound/soc/sof/intel/hda-stream.c create mode 100644 sound/soc/sof/intel/hda-trace.c create mode 100644 sound/soc/sof/intel/hda.c create mode 100644 sound/soc/sof/intel/hda.h create mode 100644 sound/soc/sof/intel/hsw.c create mode 100644 sound/soc/sof/intel/shim.h create mode 100644 sound/soc/sof/intel/skl.c create mode 100644 sound/soc/sof/sof-acpi-dev.c create mode 100644 sound/soc/sof/sof-pci-dev.c -- 2.17.1

3 32

[Sound-open-firmware] sof_text_start address in baytrail.x.in
by Daniel Baluta 07 Jan '19

07 Jan '19

Hi, In src/platform/baytrail/baytrail.x.in we have: vector_double_text : org = XCHAL_DOUBLEEXC_VECTOR_PADDR, len = SOF_MEM_VECT_TEXT_SIZE sof_text_start : org = XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_SIZE, Is the address of sof_text_start correct here? vector_double_text ends at: XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_TEXT_SIZE but sof_text_start (which is the next segment in memory) doesn't start immediately after vector_double_text because SOF_MEM_VECT_SIZE = SOF_MEM_VECT_TEXT_SIZE + SOF_MEM_VECT_LIT_SIZE so there is a gap of SOF_MEM_VECT_LIT_SIZE (4 bytes) between vector_double_text and sof_text_start. Is this on purpose? thanks, Daniel.

2 1

[Sound-open-firmware] Linker scripts
by Daniel Baluta 21 Dec '18

21 Dec '18

Hi Liam, Do you have any guide on how to generate the linker scripts for sof/? For example sof/src/platform/suecreek/suecreek.x.in. Did you use Xtensa Xplorer? Any public docs or hints would be excellent. thanks, Daniel.

2 1

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware, loader support
by Bard liao 21 Dec '18

21 Dec '18

> Date: Wed, 12 Dec 2018 12:23:33 +0100 > From: Takashi Iwai <tiwai(a)suse.de> > To: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> > Cc: alsa-devel(a)alsa-project.org, andriy.shevchenko(a)intel.com, Daniel > Baluta <daniel.baluta(a)gmail.com>, liam.r.girdwood(a)linux.intel.com, > vkoul(a)kernel.org, broonie(a)kernel.org, Alan Cox <alan(a)linux.intel.com>, > sound-open-firmware(a)alsa-project.org > Subject: Re: [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware > loader support > Message-ID: <s5htvjjj8ei.wl-tiwai(a)suse.de> > Content-Type: text/plain; charset=US-ASCII > > On Tue, 11 Dec 2018 22:23:13 +0100, > Pierre-Louis Bossart wrote: > >> + >> + snd_sof_dsp_block_write(sdev, offset, >> + (void *)block + sizeof(*block), >> + block->size); >> + >> + /* next block */ >> + block = (void *)block + sizeof(*block) + block->size; > This may lead to an unaligned access. Did you mean we should double check the block->size to prevent access to an invalid address? > Also how is the endianess guaranteed? Did you mean we should guarantee the driver can work no matter what kernel's endianess is? ie. Use le32_to_cpu() to handle it? > > > thanks, > > Takashi > >

4 5

[Sound-open-firmware] Loading coefficients for Audio IP
by Daniel Baluta 19 Dec '18

19 Dec '18

Hello Pierre, Liam, Mark, What is the standard ALSA way of loading tables of coefficients for various audio IPs? We have IPs like PDM or ASRC which need large table of coefficients depending on configuration. This tables can reach up to 120K in size. Creating this kind of tables as arrays of integers in kernel doesn't seem to be a common used solution and we will end up with large files that contains only numbers. Another approach that we are thinking of is to create and compile this tables in userspace as raw binary files and then loading them in kernel using the firmware interface. Would this be an acceptable approach? thanks, Daniel.

3 2

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 13/14] ASoC: SOF: Add xtensa support
by Pierre-Louis Bossart 17 Dec '18

17 Dec '18

On 12/11/18 5:08 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:17PM -0600, Pierre-Louis Bossart wrote: >> Add common directory for xtensa architecture >> + for (i = 0; i <= stack_words - 4; i += 4) { >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x 0x%8.8x 0x%8.8x\n", >> + stack_ptr + i, stack[i], stack[i + 1], stack[i + 2], >> + stack[i + 3]); >> + } >> + >> + /* deal with any remaining words */ >> + switch (stack_words - i) { >> + case 0: >> + break; >> + case 1: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x\n", >> + stack_ptr + stack_words - 1, stack[stack_words - 1]); >> + break; >> + case 2: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x\n", >> + stack_ptr + stack_words - 2, stack[stack_words - 2], >> + stack[stack_words - 1]); >> + break; >> + case 3: >> + dev_err(sdev->dev, "0x%8.8x: 0x%8.8x 0x%8.8x 0x%8.8x\n", >> + stack_ptr + stack_words - 3, stack[stack_words - 3], >> + stack[stack_words - 2], stack[stack_words - 1]); >> + break; >> + default: >> + break; >> + } > hex_dump_to_buffer(). I vaguely recall there was a reason not to do it, we'll relook at this.

3 3

Re: [Sound-open-firmware] [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.
by Yang, Libin 17 Dec '18

17 Dec '18

Hi Sakamoto, > >-------- Forwarded Message -------- >Subject: Re: [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and >BSW DSP HW support. >Date: Wed, 12 Dec 2018 13:08:48 +0900 >From: Takashi Sakamoto <o-takashi(a)sakamocchi.jp> >To: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com>, >alsa-devel(a)alsa-project.org >CC: Daniel Baluta <daniel.baluta(a)gmail.com>, andriy.shevchenko(a)intel.com, >tiwai(a)suse.de, Pan Xiuli <xiuli.pan(a)linux.intel.com>, >liam.r.girdwood(a)linux.intel.com, vkoul(a)kernel.org, broonie(a)kernel.org, >sound-open-firmware(a)alsa-project.org, Rander Wang ><rander.wang(a)linux.intel.com>, Alan Cox <alan(a)linux.intel.com> > >Hi, > >I have small nitpicking. > >On 2018/12/12 6:30, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Add support for the audio DSP hardware found on Intel Baytrail, >> Cherrytrail and Braswell based devices. >> >> Signed-off-by: Rander Wang <rander.wang(a)linux.intel.com> >> Signed-off-by: Pan Xiuli <xiuli.pan(a)linux.intel.com> >> Signed-off-by: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> Signed-off-by: Pierre-Louis Bossart >> <pierre-louis.bossart(a)linux.intel.com> >> --- >> sound/soc/sof/intel/byt.c | 805 >+++++++++++++++++++++++++++++++++++++ >> sound/soc/sof/intel/shim.h | 159 ++++++++ >> 2 files changed, 964 insertions(+) >> create mode 100644 sound/soc/sof/intel/byt.c >> create mode 100644 sound/soc/sof/intel/shim.h .. >> diff --git a/sound/soc/sof/intel/shim.h b/sound/soc/sof/intel/shim.h >> new file mode 100644 index 000000000000..2fdcb2d262f4 >> --- /dev/null >> +++ b/sound/soc/sof/intel/shim.h >> @@ -0,0 +1,159 @@ >> ... >> +extern struct snd_sof_dsp_ops sof_byt_ops; extern struct >> +snd_sof_dsp_ops sof_cht_ops; > >These two symbols are found in this patch. > >> +extern struct snd_sof_dsp_ops sof_hsw_ops; > >This symbol is added in second patch and this first patch has no reference to it. > >> +extern struct snd_sof_dsp_ops sof_bdw_ops; > >This symbol is added in third patch and this first patch has no reference to it. > >> + >> +#endif > >I think it better to add these extern declarations when adding corresponding >symbols (Of course no issues as is). > >Furthermore, if you have no plan to change contents of these symbols in >kernel run time, it's better to have 'const' qualifier to locate the symbol to >readonly section. However, user of these symbols is machine driver and you >have a plan to implement it later. They can get 'const' >when you work for arrangement of existent/new codes. > >In this meaning, it might be good to add a new member to 'struct >snd_soc_acpi_mach' for new drivers to refer to the const ops, then 'pdata' >member is used from the existent drivers. I made a patch based on your suggestion. Could you please help review to see if my understanding is right? https://github.com/thesofproject/linux/pull/457 Regards, Libin > > >Regards > >Takashi Sakamoto >_______________________________________________ >Alsa-devel mailing list >Alsa-devel(a)alsa-project.org >http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

1 0

Re: [Sound-open-firmware] [PATCH v3 02/14] ASoC: SOF: Add Sound Open Firmware KControl support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

>> + max_size = (be->max < max_size) ? be->max : max_size; > min() / max() ? > The use of min() and min3() seems to generate tons of sparse warnings (see below), so I'll leave the code with an explicit test and a comment stating why. sparse helped us find quite a few issues with endianess, const, iomem qualifiers, I'd like to keep it quiet on non-issues. CHECK sound/soc/sof/control.c sound/soc/sof/control.c:300:20: warning: expression using sizeof(void) sound/soc/sof/control.c:300:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void) sound/soc/sof/control.c:396:20: warning: expression using sizeof(void)

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

Thanks for the review. >> +++ b/sound/soc/sof/intel/shim.h >> @@ -0,0 +1,159 @@ >> ... >> +extern struct snd_sof_dsp_ops sof_byt_ops; >> +extern struct snd_sof_dsp_ops sof_cht_ops; > > These two symbols are found in this patch. > >> +extern struct snd_sof_dsp_ops sof_hsw_ops; > > This symbol is added in second patch and this first patch has > no reference to it. > >> +extern struct snd_sof_dsp_ops sof_bdw_ops; > > This symbol is added in third patch and this first patch has > no reference to it. > >> + >> +#endif > > I think it better to add these extern declarations when adding > corresponding symbols (Of course no issues as is). Yes, that's no big deal. It won't break as is though since the build is added in the last patch. > > Furthermore, if you have no plan to change contents of these symbols in > kernel run time, it's better to have 'const' qualifier to locate the > symbol to readonly section. However, user of these symbols is machine > driver and you have a plan to implement it later. They can get 'const' > when you work for arrangement of existent/new codes. Good point, I don't think the ops are changed, ever. I'll double-check and change if indeed this is the case, thanks for the suggestion. > > In this meaning, it might be good to add a new member to 'struct > snd_soc_acpi_mach' for new drivers to refer to the const ops, then > 'pdata' member is used from the existent drivers. Sorry, I don't get your point. the ops structure is included in the sof_ops_table, not the snd_soc_acpi_mach.

1 1