[alsa-devel] [PATCH] [RFC 2/13] Intel SST driver

[Harsha, Priya]

>(snip)
>> +#ifndef CONFIG_SST_IPC_NOT_INCLUDED
>> +#include <asm/ipc_defs.h>
>> +#endif
>
>What is this file?
This file has the function definitions to communicate to the sound card. The sound card is a part of PMIC chip. The sound card registers are not accessible by CPU directly. There is a platform driver that enables to communicate to the sound card. Audio driver uses this header file of that diver to communicate to the sound card. This is one of the dependant drivers for audio driver. This is being submitted upstream by Intel as well.

>> +struct intel_sst_drv *sst_ops;
>
>I'm afraid it's a bit too generic name.
INTEL_SST (Intel Smart Sound Technology) is the marketing name for the audio stack. Is sst_ops looking like a generic name or intel_sst_drv a generic name? Any alternative name you could suggest would help.

>
>> +	if (retval != 0)
>> +		sst_err("scu ipc write1 failed %d", retval);
>> +	/*Reset the Audio subsystem*/
>> +	retval = sst_scu_ipc_write(0xff11d118, 0x7ffffcff);
>> +	if (retval != 0)
>> +		sst_err("scu ipc write2 failed %d", retval);
>> +	/*Bring it out of Audio subsystem reset*/
>> +	retval = sst_scu_ipc_write(0xff11d118, 0x7fffffff);
>> +	if (retval != 0)
>> +		sst_err("scu ipc write3 failed %d", retval);
>> +	/*Enable fabric clock at 50MHz*/
>> +	retval = sst_scu_ipc_write(0xff11d83c, 0x80008301);
>> +	if (retval != 0)
>> +		sst_err("scu ipc write4 failed %d", retval);
>> +	/*Write to the Shim register for ratio 1:1*/
>> +	retval = sst_scu_ipc_write(0xffae8000, 0x382);
>> +	if (retval != 0)
>> +		sst_err("scu ipc write5 failed %d", retval);
>> +	/*Enable the core clock at 1:2*/
>> +	retval = sst_scu_ipc_write(0xff11d83c, 0x80000301);
>> +	if (retval != 0)
>> +		sst_err("scu ipc write6 failed %d", retval);
>> +	return;
>
>So... no these errors are no critical errors?
These are critical errors. The recovery mechanism is not in place yet. I should have marked it as TBD. We will resubmit with the recovery mechanism next time.

>
>> +int sst_parse_module(struct fw_module_header *module)
>
>Hmm... are all these global?
No, it's local. I'll make it static.

>
>> +/*fops Routines*/
>> +const struct file_operations intel_sst_fops = {
>
>Shouldn't be statc?
Yes. Will change it.

>
>> +#ifdef CONFIG_SST_OSPM_SUPPORT
>
>Can't be simple CONFIG_PM?
No. OSPM is new power management module being developed for the platform. The code defined under this #def is to provide support from audio stack to this new power management module. This is also being up streamed by Intel.

-Harsha