Hi Sakamoto, > >-------- Forwarded Message -------- >Subject: Re: [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@xxxxxxxxxxxxx> >To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>, >alsa-devel@xxxxxxxxxxxxxxxx >CC: Daniel Baluta <daniel.baluta@xxxxxxxxx>, andriy.shevchenko@xxxxxxxxx, >tiwai@xxxxxxx, Pan Xiuli <xiuli.pan@xxxxxxxxxxxxxxx>, >liam.r.girdwood@xxxxxxxxxxxxxxx, vkoul@xxxxxxxxxx, broonie@xxxxxxxxxx, >sound-open-firmware@xxxxxxxxxxxxxxxx, Rander Wang ><rander.wang@xxxxxxxxxxxxxxx>, Alan Cox <alan@xxxxxxxxxxxxxxx> > >Hi, > >I have small nitpicking. > >On 2018/12/12 6:30, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood@xxxxxxxxxxxxxxx> >> >> Add support for the audio DSP hardware found on Intel Baytrail, >> Cherrytrail and Braswell based devices. >> >> Signed-off-by: Rander Wang <rander.wang@xxxxxxxxxxxxxxx> >> Signed-off-by: Pan Xiuli <xiuli.pan@xxxxxxxxxxxxxxx> >> Signed-off-by: Liam Girdwood <liam.r.girdwood@xxxxxxxxxxxxxxx> >> Signed-off-by: Pierre-Louis Bossart >> <pierre-louis.bossart@xxxxxxxxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxx >http://mailman.alsa-project.org/mailman/listinfo/alsa-devel _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
