[Sound-open-firmware] [alsa-devel] [PATCH 01/21] ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support.

[Yang, Libin]

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 at sakamocchi.jp>
>To: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>,
>alsa-devel at alsa-project.org
>CC: Daniel Baluta <daniel.baluta at gmail.com>, andriy.shevchenko at intel.com,
>tiwai at suse.de, Pan Xiuli <xiuli.pan at linux.intel.com>,
>liam.r.girdwood at linux.intel.com, vkoul at kernel.org, broonie at kernel.org,
>sound-open-firmware at alsa-project.org, Rander Wang
><rander.wang at linux.intel.com>, Alan Cox <alan at linux.intel.com>
>
>Hi,
>
>I have small nitpicking.
>
>On 2018/12/12 6:30, Pierre-Louis Bossart wrote:
>> From: Liam Girdwood <liam.r.girdwood at 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 at linux.intel.com>
>> Signed-off-by: Pan Xiuli <xiuli.pan at linux.intel.com>
>> Signed-off-by: Liam Girdwood <liam.r.girdwood at linux.intel.com>
>> Signed-off-by: Pierre-Louis Bossart
>> <pierre-louis.bossart at 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 at alsa-project.org
>http://mailman.alsa-project.org/mailman/listinfo/alsa-devel