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

[Takashi Iwai]

At Tue, 7 Jul 2009 11:30:00 +0530,
Harsha, Priya wrote:
> 
> >(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.

Hm, OK, but then don't forget to remove ifdef after that part is
merged.  An ifdef around the include file isn't recommended at all.

> >> +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.

I meant the latter, sst_ops.  intel_sst_ops would be OK.


> >> +	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.

I understand the underlying platform isn't merged so we need another
ifdef.  But, once after it got merged, no reason to keep another
special config, right?

Keeping own kconfigs look easy but this will increase maintenance
labor a lot later.  If it's a power-management, using only CONFIG_PM
would make the life happier.


thanks,

Takashi