diff mbox

[7/9] ASoC: Intel: move PCI probe to a seprate file

Message ID 1414666312-4657-8-git-send-email-vinod.koul@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vinod Koul Oct. 30, 2014, 10:51 a.m. UTC
From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>

This allow the sst.c to be common across PCI and APCI usages

Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>

Author:    Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/soc/intel/Kconfig       |    4 +
 sound/soc/intel/sst/Makefile  |    4 +
 sound/soc/intel/sst/sst.c     |  200 +------------------------------------
 sound/soc/intel/sst/sst.h     |    6 +
 sound/soc/intel/sst/sst_pci.c |  225 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 240 insertions(+), 199 deletions(-)
 create mode 100644 sound/soc/intel/sst/sst_pci.c

Comments

Vinod Koul Oct. 30, 2014, 2:34 p.m. UTC | #1
On Thu, Oct 30, 2014 at 04:03:59PM +0100, Takashi Iwai wrote:
> At Thu, 30 Oct 2014 16:21:50 +0530,
> Vinod Koul wrote:
> > 
> > From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > 
> > This allow the sst.c to be common across PCI and APCI usages
> > 
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > 
> > Author:    Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > ---
> >  sound/soc/intel/Kconfig       |    4 +
> >  sound/soc/intel/sst/Makefile  |    4 +
> >  sound/soc/intel/sst/sst.c     |  200 +------------------------------------
> >  sound/soc/intel/sst/sst.h     |    6 +
> >  sound/soc/intel/sst/sst_pci.c |  225 +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 240 insertions(+), 199 deletions(-)
> >  create mode 100644 sound/soc/intel/sst/sst_pci.c
> > 
> > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> > index 2a3af88..cbc987e 100644
> > --- a/sound/soc/intel/Kconfig
> > +++ b/sound/soc/intel/Kconfig
> > @@ -4,6 +4,7 @@ config SND_MFLD_MACHINE
> >  	select SND_SOC_SN95031
> >  	select SND_SST_MFLD_PLATFORM
> >  	select SND_SST_IPC
> > +	select SND_SST_IPC_PCI
> >  	help
> >            This adds support for ASoC machine driver for Intel(R) MID Medfield platform
> >            used as alsa device in audio substem in Intel(R) MID devices
> > @@ -16,6 +17,9 @@ config SND_SST_MFLD_PLATFORM
> >  config SND_SST_IPC
> >  	tristate
> >  
> > +config SND_SST_IPC_PCI
> > +	bool
> > +
> >  config SND_SOC_INTEL_SST
> >  	tristate "ASoC support for Intel(R) Smart Sound Technology"
> >  	select SND_SOC_INTEL_SST_ACPI if ACPI
> > diff --git a/sound/soc/intel/sst/Makefile b/sound/soc/intel/sst/Makefile
> > index 4d0e79b..b3fbccd 100644
> > --- a/sound/soc/intel/sst/Makefile
> > +++ b/sound/soc/intel/sst/Makefile
> > @@ -1,3 +1,7 @@
> >  snd-intel-sst-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> >  
> > +ifneq ($(CONFIG_SND_SST_IPC_PCI),)
> > +snd-intel-sst-objs += sst_pci.o
> > +endif
> 
> The standard way is something like
> 
> snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> 
> But, when looking at the later patch, you try to build ACPI stuff into
> snd-intel-sst, too, and both are implemented as exclusive.  This
> doesn't work well in general.
The hardware, firmware so the driver is pretty same. So either it gets probed
as PCI device is SFI platforms and as APCI device on ACPI ones.
Since the probe method is the only one differing, the machine will select
either PCI or ACPI. That one would get compiled in.

Am okay to change if we have better method which works for both
Takashi Iwai Oct. 30, 2014, 3:03 p.m. UTC | #2
At Thu, 30 Oct 2014 16:21:50 +0530,
Vinod Koul wrote:
> 
> From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> 
> This allow the sst.c to be common across PCI and APCI usages
> 
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> 
> Author:    Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/soc/intel/Kconfig       |    4 +
>  sound/soc/intel/sst/Makefile  |    4 +
>  sound/soc/intel/sst/sst.c     |  200 +------------------------------------
>  sound/soc/intel/sst/sst.h     |    6 +
>  sound/soc/intel/sst/sst_pci.c |  225 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 240 insertions(+), 199 deletions(-)
>  create mode 100644 sound/soc/intel/sst/sst_pci.c
> 
> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 2a3af88..cbc987e 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -4,6 +4,7 @@ config SND_MFLD_MACHINE
>  	select SND_SOC_SN95031
>  	select SND_SST_MFLD_PLATFORM
>  	select SND_SST_IPC
> +	select SND_SST_IPC_PCI
>  	help
>            This adds support for ASoC machine driver for Intel(R) MID Medfield platform
>            used as alsa device in audio substem in Intel(R) MID devices
> @@ -16,6 +17,9 @@ config SND_SST_MFLD_PLATFORM
>  config SND_SST_IPC
>  	tristate
>  
> +config SND_SST_IPC_PCI
> +	bool
> +
>  config SND_SOC_INTEL_SST
>  	tristate "ASoC support for Intel(R) Smart Sound Technology"
>  	select SND_SOC_INTEL_SST_ACPI if ACPI
> diff --git a/sound/soc/intel/sst/Makefile b/sound/soc/intel/sst/Makefile
> index 4d0e79b..b3fbccd 100644
> --- a/sound/soc/intel/sst/Makefile
> +++ b/sound/soc/intel/sst/Makefile
> @@ -1,3 +1,7 @@
>  snd-intel-sst-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
>  
> +ifneq ($(CONFIG_SND_SST_IPC_PCI),)
> +snd-intel-sst-objs += sst_pci.o
> +endif

The standard way is something like

snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o

But, when looking at the later patch, you try to build ACPI stuff into
snd-intel-sst, too, and both are implemented as exclusive.  This
doesn't work well in general.


Takashi
Takashi Iwai Oct. 30, 2014, 3:27 p.m. UTC | #3
At Thu, 30 Oct 2014 20:04:44 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 30, 2014 at 04:03:59PM +0100, Takashi Iwai wrote:
> > At Thu, 30 Oct 2014 16:21:50 +0530,
> > Vinod Koul wrote:
> > > 
> > > From: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > 
> > > This allow the sst.c to be common across PCI and APCI usages
> > > 
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > 
> > > Author:    Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > ---
> > >  sound/soc/intel/Kconfig       |    4 +
> > >  sound/soc/intel/sst/Makefile  |    4 +
> > >  sound/soc/intel/sst/sst.c     |  200 +------------------------------------
> > >  sound/soc/intel/sst/sst.h     |    6 +
> > >  sound/soc/intel/sst/sst_pci.c |  225 +++++++++++++++++++++++++++++++++++++++++
> > >  5 files changed, 240 insertions(+), 199 deletions(-)
> > >  create mode 100644 sound/soc/intel/sst/sst_pci.c
> > > 
> > > diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> > > index 2a3af88..cbc987e 100644
> > > --- a/sound/soc/intel/Kconfig
> > > +++ b/sound/soc/intel/Kconfig
> > > @@ -4,6 +4,7 @@ config SND_MFLD_MACHINE
> > >  	select SND_SOC_SN95031
> > >  	select SND_SST_MFLD_PLATFORM
> > >  	select SND_SST_IPC
> > > +	select SND_SST_IPC_PCI
> > >  	help
> > >            This adds support for ASoC machine driver for Intel(R) MID Medfield platform
> > >            used as alsa device in audio substem in Intel(R) MID devices
> > > @@ -16,6 +17,9 @@ config SND_SST_MFLD_PLATFORM
> > >  config SND_SST_IPC
> > >  	tristate
> > >  
> > > +config SND_SST_IPC_PCI
> > > +	bool
> > > +
> > >  config SND_SOC_INTEL_SST
> > >  	tristate "ASoC support for Intel(R) Smart Sound Technology"
> > >  	select SND_SOC_INTEL_SST_ACPI if ACPI
> > > diff --git a/sound/soc/intel/sst/Makefile b/sound/soc/intel/sst/Makefile
> > > index 4d0e79b..b3fbccd 100644
> > > --- a/sound/soc/intel/sst/Makefile
> > > +++ b/sound/soc/intel/sst/Makefile
> > > @@ -1,3 +1,7 @@
> > >  snd-intel-sst-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > >  
> > > +ifneq ($(CONFIG_SND_SST_IPC_PCI),)
> > > +snd-intel-sst-objs += sst_pci.o
> > > +endif
> > 
> > The standard way is something like
> > 
> > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > 
> > But, when looking at the later patch, you try to build ACPI stuff into
> > snd-intel-sst, too, and both are implemented as exclusive.  This
> > doesn't work well in general.
> The hardware, firmware so the driver is pretty same. So either it gets probed
> as PCI device is SFI platforms and as APCI device on ACPI ones.
> Since the probe method is the only one differing, the machine will select
> either PCI or ACPI. That one would get compiled in.

... and this exclusion mechanism is missing in your patches.
Your current code doesn't allow to mix them.

> Am okay to change if we have better method which works for both

In general, it's better to provide individual modules for different
interfaces and a common module that is referred by them.  The
selective build makes the build tests more difficult and is rather
error-prone.


Takashi
Vinod Koul Oct. 30, 2014, 3:37 p.m. UTC | #4
On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > The standard way is something like
> > > 
> > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > 
> > > But, when looking at the later patch, you try to build ACPI stuff into
> > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > doesn't work well in general.
> > The hardware, firmware so the driver is pretty same. So either it gets probed
> > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > Since the probe method is the only one differing, the machine will select
> > either PCI or ACPI. That one would get compiled in.
> 
> ... and this exclusion mechanism is missing in your patches.
> Your current code doesn't allow to mix them.
> 
> > Am okay to change if we have better method which works for both
> 
> In general, it's better to provide individual modules for different
> interfaces and a common module that is referred by them.  The
> selective build makes the build tests more difficult and is rather
> error-prone.
Hmmm, that makes me wonder why cant we do (based on your input above)

snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o

then either this:
snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o

Or:
snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o

And the machine will select required ones..
Vinod Koul Oct. 30, 2014, 3:59 p.m. UTC | #5
On Thu, Oct 30, 2014 at 05:29:00PM +0100, Takashi Iwai wrote:
> At Thu, 30 Oct 2014 21:07:19 +0530,
> Vinod Koul wrote:
> > 
> > On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > > > The standard way is something like
> > > > > 
> > > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > > 
> > > > > But, when looking at the later patch, you try to build ACPI stuff into
> > > > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > > > doesn't work well in general.
> > > > The hardware, firmware so the driver is pretty same. So either it gets probed
> > > > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > > > Since the probe method is the only one differing, the machine will select
> > > > either PCI or ACPI. That one would get compiled in.
> > > 
> > > ... and this exclusion mechanism is missing in your patches.
> > > Your current code doesn't allow to mix them.
> > > 
> > > > Am okay to change if we have better method which works for both
> > > 
> > > In general, it's better to provide individual modules for different
> > > interfaces and a common module that is referred by them.  The
> > > selective build makes the build tests more difficult and is rather
> > > error-prone.
> > Hmmm, that makes me wonder why cant we do (based on your input above)
> > 
> > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > 
> > then either this:
> > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o
> 
> This may compile both sst_pci.c and sst_acpi.c into snd-intel-sst
> module, so it doesn't work unless both Kconfigs are really exclusive.
> 
> > Or:
> > snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> > snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o
> 
> This is wrong in two ways.  First off, you can't link a module against
> another module.
> 
> Second, this causes the problem when user build one driver as a module
> and another as built-in.  Then you'll get the doubly symbols.
oh no, the core is not a module. the PCI and ACPI will be modules and
allowed to be selected.
Can we prevent the module/built-in by making depends. If one is built-in
other cant be module. That way it would be okay

> So, snd-intel-sst.o must be an individual module.
Since only probe and apci/pci resource setup code is different I would still
like to keep the core lib as same and PCI/ACPI as modules.
Vinod Koul Oct. 30, 2014, 4:14 p.m. UTC | #6
On Thu, Oct 30, 2014 at 05:49:39PM +0100, Takashi Iwai wrote:
> At Thu, 30 Oct 2014 21:29:47 +0530,
> Vinod Koul wrote:
> > 
> > On Thu, Oct 30, 2014 at 05:29:00PM +0100, Takashi Iwai wrote:
> > > At Thu, 30 Oct 2014 21:07:19 +0530,
> > > Vinod Koul wrote:
> > > > 
> > > > On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > > > > > The standard way is something like
> > > > > > > 
> > > > > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > > > > 
> > > > > > > But, when looking at the later patch, you try to build ACPI stuff into
> > > > > > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > > > > > doesn't work well in general.
> > > > > > The hardware, firmware so the driver is pretty same. So either it gets probed
> > > > > > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > > > > > Since the probe method is the only one differing, the machine will select
> > > > > > either PCI or ACPI. That one would get compiled in.
> > > > > 
> > > > > ... and this exclusion mechanism is missing in your patches.
> > > > > Your current code doesn't allow to mix them.
> > > > > 
> > > > > > Am okay to change if we have better method which works for both
> > > > > 
> > > > > In general, it's better to provide individual modules for different
> > > > > interfaces and a common module that is referred by them.  The
> > > > > selective build makes the build tests more difficult and is rather
> > > > > error-prone.
> > > > Hmmm, that makes me wonder why cant we do (based on your input above)
> > > > 
> > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > 
> > > > then either this:
> > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o
> > > 
> > > This may compile both sst_pci.c and sst_acpi.c into snd-intel-sst
> > > module, so it doesn't work unless both Kconfigs are really exclusive.
> > > 
> > > > Or:
> > > > snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> > > > snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o
> > > 
> > > This is wrong in two ways.  First off, you can't link a module against
> > > another module.
> > > 
> > > Second, this causes the problem when user build one driver as a module
> > > and another as built-in.  Then you'll get the doubly symbols.
> > oh no, the core is not a module. the PCI and ACPI will be modules and
> > allowed to be selected.
> 
> Then you should have stripped snd- prefix from snd-intel-sst.o.
> snd-prefix is supposed to be used for the modules.
Sure I can do that

so we can have

sst-intel-core-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o

and then

snd-intel-sst-pci-y += sst-intel-core.o sst_pci.o
snd-intel-sst-acpi-y += st-intel-core.o sst_acpi.o

> > Can we prevent the module/built-in by making depends. If one is built-in
> > other cant be module. That way it would be okay
> 
> As said, this isn't the best.  It disallows the compile testing and
> other tests in a single shot.  Testing with various Kconfigs is more
> difficult than catching with make allyesconfig (or such).
well both can be compile tested and we will put in symbols about limitation
of having bpth apci and pci as built-in or module.
Mark Brown Oct. 30, 2014, 4:28 p.m. UTC | #7
On Thu, Oct 30, 2014 at 09:07:19PM +0530, Vinod Koul wrote:

> Hmmm, that makes me wonder why cant we do (based on your input above)

> snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o

> then either this:
> snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o

> Or:
> snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o

> And the machine will select required ones..

That's what I would expect to see.
Takashi Iwai Oct. 30, 2014, 4:29 p.m. UTC | #8
At Thu, 30 Oct 2014 21:07:19 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > > The standard way is something like
> > > > 
> > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > 
> > > > But, when looking at the later patch, you try to build ACPI stuff into
> > > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > > doesn't work well in general.
> > > The hardware, firmware so the driver is pretty same. So either it gets probed
> > > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > > Since the probe method is the only one differing, the machine will select
> > > either PCI or ACPI. That one would get compiled in.
> > 
> > ... and this exclusion mechanism is missing in your patches.
> > Your current code doesn't allow to mix them.
> > 
> > > Am okay to change if we have better method which works for both
> > 
> > In general, it's better to provide individual modules for different
> > interfaces and a common module that is referred by them.  The
> > selective build makes the build tests more difficult and is rather
> > error-prone.
> Hmmm, that makes me wonder why cant we do (based on your input above)
> 
> snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> 
> then either this:
> snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o

This may compile both sst_pci.c and sst_acpi.c into snd-intel-sst
module, so it doesn't work unless both Kconfigs are really exclusive.

> Or:
> snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o

This is wrong in two ways.  First off, you can't link a module against
another module.

Second, this causes the problem when user build one driver as a module
and another as built-in.  Then you'll get the doubly symbols.

So, snd-intel-sst.o must be an individual module.


Takashi
Takashi Iwai Oct. 30, 2014, 4:49 p.m. UTC | #9
At Thu, 30 Oct 2014 21:29:47 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 30, 2014 at 05:29:00PM +0100, Takashi Iwai wrote:
> > At Thu, 30 Oct 2014 21:07:19 +0530,
> > Vinod Koul wrote:
> > > 
> > > On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > > > > The standard way is something like
> > > > > > 
> > > > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > > > 
> > > > > > But, when looking at the later patch, you try to build ACPI stuff into
> > > > > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > > > > doesn't work well in general.
> > > > > The hardware, firmware so the driver is pretty same. So either it gets probed
> > > > > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > > > > Since the probe method is the only one differing, the machine will select
> > > > > either PCI or ACPI. That one would get compiled in.
> > > > 
> > > > ... and this exclusion mechanism is missing in your patches.
> > > > Your current code doesn't allow to mix them.
> > > > 
> > > > > Am okay to change if we have better method which works for both
> > > > 
> > > > In general, it's better to provide individual modules for different
> > > > interfaces and a common module that is referred by them.  The
> > > > selective build makes the build tests more difficult and is rather
> > > > error-prone.
> > > Hmmm, that makes me wonder why cant we do (based on your input above)
> > > 
> > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > 
> > > then either this:
> > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o
> > 
> > This may compile both sst_pci.c and sst_acpi.c into snd-intel-sst
> > module, so it doesn't work unless both Kconfigs are really exclusive.
> > 
> > > Or:
> > > snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> > > snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o
> > 
> > This is wrong in two ways.  First off, you can't link a module against
> > another module.
> > 
> > Second, this causes the problem when user build one driver as a module
> > and another as built-in.  Then you'll get the doubly symbols.
> oh no, the core is not a module. the PCI and ACPI will be modules and
> allowed to be selected.

Then you should have stripped snd- prefix from snd-intel-sst.o.
snd-prefix is supposed to be used for the modules.

> Can we prevent the module/built-in by making depends. If one is built-in
> other cant be module. That way it would be okay

As said, this isn't the best.  It disallows the compile testing and
other tests in a single shot.  Testing with various Kconfigs is more
difficult than catching with make allyesconfig (or such).


Takashi
Takashi Iwai Oct. 30, 2014, 5:07 p.m. UTC | #10
At Thu, 30 Oct 2014 21:44:28 +0530,
Vinod Koul wrote:
> 
> On Thu, Oct 30, 2014 at 05:49:39PM +0100, Takashi Iwai wrote:
> > At Thu, 30 Oct 2014 21:29:47 +0530,
> > Vinod Koul wrote:
> > > 
> > > On Thu, Oct 30, 2014 at 05:29:00PM +0100, Takashi Iwai wrote:
> > > > At Thu, 30 Oct 2014 21:07:19 +0530,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > On Thu, Oct 30, 2014 at 04:27:09PM +0100, Takashi Iwai wrote:
> > > > > > > > The standard way is something like
> > > > > > > > 
> > > > > > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > > > > > 
> > > > > > > > But, when looking at the later patch, you try to build ACPI stuff into
> > > > > > > > snd-intel-sst, too, and both are implemented as exclusive.  This
> > > > > > > > doesn't work well in general.
> > > > > > > The hardware, firmware so the driver is pretty same. So either it gets probed
> > > > > > > as PCI device is SFI platforms and as APCI device on ACPI ones.
> > > > > > > Since the probe method is the only one differing, the machine will select
> > > > > > > either PCI or ACPI. That one would get compiled in.
> > > > > > 
> > > > > > ... and this exclusion mechanism is missing in your patches.
> > > > > > Your current code doesn't allow to mix them.
> > > > > > 
> > > > > > > Am okay to change if we have better method which works for both
> > > > > > 
> > > > > > In general, it's better to provide individual modules for different
> > > > > > interfaces and a common module that is referred by them.  The
> > > > > > selective build makes the build tests more difficult and is rather
> > > > > > error-prone.
> > > > > Hmmm, that makes me wonder why cant we do (based on your input above)
> > > > > 
> > > > > snd-intel-sst-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> > > > > 
> > > > > then either this:
> > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_PCI) += sst_pci.o
> > > > > snd-intel-sst-$(CONFIG_SND_SST_IPC_ACPI) += sst_acpi.o
> > > > 
> > > > This may compile both sst_pci.c and sst_acpi.c into snd-intel-sst
> > > > module, so it doesn't work unless both Kconfigs are really exclusive.
> > > > 
> > > > > Or:
> > > > > snd-intel-sst-pci-y += snd-intel-sst.o sst_pci.o
> > > > > snd-intel-sst-acpi-y += snd-intel-sst.o sst_acpi.o
> > > > 
> > > > This is wrong in two ways.  First off, you can't link a module against
> > > > another module.
> > > > 
> > > > Second, this causes the problem when user build one driver as a module
> > > > and another as built-in.  Then you'll get the doubly symbols.
> > > oh no, the core is not a module. the PCI and ACPI will be modules and
> > > allowed to be selected.
> > 
> > Then you should have stripped snd- prefix from snd-intel-sst.o.
> > snd-prefix is supposed to be used for the modules.
> Sure I can do that
> 
> so we can have
> 
> sst-intel-core-y := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
> 
> and then
> 
> snd-intel-sst-pci-y += sst-intel-core.o sst_pci.o
> snd-intel-sst-acpi-y += st-intel-core.o sst_acpi.o

Kbuild doesn't allow this syntax, AFAIK.

> > > Can we prevent the module/built-in by making depends. If one is built-in
> > > other cant be module. That way it would be okay
> > 
> > As said, this isn't the best.  It disallows the compile testing and
> > other tests in a single shot.  Testing with various Kconfigs is more
> > difficult than catching with make allyesconfig (or such).
> well both can be compile tested and we will put in symbols about limitation
> of having bpth apci and pci as built-in or module.

No, the point is that the exclusiveness in Kconfig level gives more
demerits.  This makes impossible to build the both codes in a single
shot, which makes also impossible to cover wider build tests, etc.
My concern isn't about the actual operation but about testing.

That is: don't try to side step such a build issue.  I bet it'll
strike back later.  Better to keep rather the simple and common
approach other drivers take.


Takashi
Mark Brown Oct. 30, 2014, 5:31 p.m. UTC | #11
On Thu, Oct 30, 2014 at 06:07:00PM +0100, Takashi Iwai wrote:
> Vinod Koul wrote:

> > snd-intel-sst-pci-y += sst-intel-core.o sst_pci.o
> > snd-intel-sst-acpi-y += st-intel-core.o sst_acpi.o

> Kbuild doesn't allow this syntax, AFAIK.

I've not followed the whole discussion but why not just have the core
code as a separate module, this seems like an awful lot of trouble for
something that's normally straightforward...

> > well both can be compile tested and we will put in symbols about limitation
> > of having bpth apci and pci as built-in or module.

> No, the point is that the exclusiveness in Kconfig level gives more
> demerits.  This makes impossible to build the both codes in a single
> shot, which makes also impossible to cover wider build tests, etc.
> My concern isn't about the actual operation but about testing.

> That is: don't try to side step such a build issue.  I bet it'll
> strike back later.  Better to keep rather the simple and common
> approach other drivers take.

Right.
Vinod Koul Oct. 31, 2014, 5:55 a.m. UTC | #12
On Thu, Oct 30, 2014 at 05:31:12PM +0000, Mark Brown wrote:
> On Thu, Oct 30, 2014 at 06:07:00PM +0100, Takashi Iwai wrote:
> > Vinod Koul wrote:
> 
> > > snd-intel-sst-pci-y += sst-intel-core.o sst_pci.o
> > > snd-intel-sst-acpi-y += st-intel-core.o sst_acpi.o
> 
> > Kbuild doesn't allow this syntax, AFAIK.
> 
> I've not followed the whole discussion but why not just have the core
> code as a separate module, this seems like an awful lot of trouble for
> something that's normally straightforward...
Hmm seems like my attempt to have just two module is fraught with risks. I
will convert core to module which can be linked either by PCI or ACPI.

Thats should clean this out :)
Thanks for all the points

Mark, please drop this patch, the ones before will still be required for
module so pls do review them

Thanks
Takashi Iwai Oct. 31, 2014, 5:32 p.m. UTC | #13
At Thu, 30 Oct 2014 17:31:12 +0000,
Mark Brown wrote:
> 
> On Thu, Oct 30, 2014 at 06:07:00PM +0100, Takashi Iwai wrote:
> > Vinod Koul wrote:
> 
> > > snd-intel-sst-pci-y += sst-intel-core.o sst_pci.o
> > > snd-intel-sst-acpi-y += st-intel-core.o sst_acpi.o
> 
> > Kbuild doesn't allow this syntax, AFAIK.
> 
> I've not followed the whole discussion but why not just have the core
> code as a separate module, this seems like an awful lot of trouble for
> something that's normally straightforward...

Yep, that's what I've suggested, too.

(Became so late response as we had (and maybe well have again) a
 network problem...)


Takashi
diff mbox

Patch

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 2a3af88..cbc987e 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -4,6 +4,7 @@  config SND_MFLD_MACHINE
 	select SND_SOC_SN95031
 	select SND_SST_MFLD_PLATFORM
 	select SND_SST_IPC
+	select SND_SST_IPC_PCI
 	help
           This adds support for ASoC machine driver for Intel(R) MID Medfield platform
           used as alsa device in audio substem in Intel(R) MID devices
@@ -16,6 +17,9 @@  config SND_SST_MFLD_PLATFORM
 config SND_SST_IPC
 	tristate
 
+config SND_SST_IPC_PCI
+	bool
+
 config SND_SOC_INTEL_SST
 	tristate "ASoC support for Intel(R) Smart Sound Technology"
 	select SND_SOC_INTEL_SST_ACPI if ACPI
diff --git a/sound/soc/intel/sst/Makefile b/sound/soc/intel/sst/Makefile
index 4d0e79b..b3fbccd 100644
--- a/sound/soc/intel/sst/Makefile
+++ b/sound/soc/intel/sst/Makefile
@@ -1,3 +1,7 @@ 
 snd-intel-sst-objs := sst.o sst_ipc.o sst_stream.o sst_drv_interface.o sst_loader.o sst_pvt.o
 
+ifneq ($(CONFIG_SND_SST_IPC_PCI),)
+snd-intel-sst-objs += sst_pci.o
+endif
+
 obj-$(CONFIG_SND_SST_IPC) += snd-intel-sst.o
diff --git a/sound/soc/intel/sst/sst.c b/sound/soc/intel/sst/sst.c
index ad16101..97c737a 100644
--- a/sound/soc/intel/sst/sst.c
+++ b/sound/soc/intel/sst/sst.c
@@ -20,30 +20,19 @@ 
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 #include <linux/module.h>
-#include <linux/pci.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/firmware.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_qos.h>
 #include <linux/async.h>
-#include <linux/delay.h>
-#include <linux/acpi.h>
 #include <sound/core.h>
-#include <sound/pcm.h>
 #include <sound/soc.h>
-#include <sound/compress_driver.h>
-#include <asm/intel-mid.h>
 #include <asm/platform_sst_audio.h>
 #include "../sst-mfld-platform.h"
 #include "sst.h"
 #include "../sst-dsp.h"
 
-MODULE_AUTHOR("Vinod Koul <vinod.koul@intel.com>");
-MODULE_AUTHOR("Harsha Priya <priya.harsha@intel.com>");
-MODULE_DESCRIPTION("Intel (R) SST(R) Audio Engine Driver");
-MODULE_LICENSE("GPL v2");
-
 static inline bool sst_is_process_reply(u32 msg_id)
 {
 	return ((msg_id & PROCESS_MSG) ? true : false);
@@ -340,174 +329,7 @@  void sst_configure_runtime_pm(struct intel_sst_drv *ctx)
 	pm_runtime_put_noidle(ctx->dev);
 }
 
-static int sst_platform_get_resources(struct intel_sst_drv *ctx)
-{
-	int ddr_base, ret = 0;
-	struct pci_dev *pci = ctx->pci;
-	ret = pci_request_regions(pci, SST_DRV_NAME);
-	if (ret)
-		return ret;
-
-	/* map registers */
-	/* DDR base */
-	if (ctx->dev_id == SST_MRFLD_PCI_ID) {
-		ctx->ddr_base = pci_resource_start(pci, 0);
-		/* check that the relocated IMR base matches with FW Binary */
-		ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base);
-		if (!ctx->pdata->lib_info) {
-			dev_err(ctx->dev, "lib_info pointer NULL\n");
-			ret = -EINVAL;
-			goto do_release_regions;
-		}
-		if (ddr_base != ctx->pdata->lib_info->mod_base) {
-			dev_err(ctx->dev,
-					"FW LSP DDR BASE does not match with IFWI\n");
-			ret = -EINVAL;
-			goto do_release_regions;
-		}
-		ctx->ddr_end = pci_resource_end(pci, 0);
-
-		ctx->ddr = pcim_iomap(pci, 0,
-					pci_resource_len(pci, 0));
-		if (!ctx->ddr) {
-			ret = -EINVAL;
-			goto do_release_regions;
-		}
-		dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
-	} else {
-		ctx->ddr = NULL;
-	}
-	/* SHIM */
-	ctx->shim_phy_add = pci_resource_start(pci, 1);
-	ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1));
-	if (!ctx->shim) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
-	dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
-
-	/* Shared SRAM */
-	ctx->mailbox_add = pci_resource_start(pci, 2);
-	ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2));
-	if (!ctx->mailbox) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
-	dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
-
-	/* IRAM */
-	ctx->iram_end = pci_resource_end(pci, 3);
-	ctx->iram_base = pci_resource_start(pci, 3);
-	ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3));
-	if (!ctx->iram) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
-	dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
-
-	/* DRAM */
-	ctx->dram_end = pci_resource_end(pci, 4);
-	ctx->dram_base = pci_resource_start(pci, 4);
-	ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4));
-	if (!ctx->dram) {
-		ret = -EINVAL;
-		goto do_release_regions;
-	}
-	dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
-do_release_regions:
-	pci_release_regions(pci);
-	return 0;
-}
-/*
-* intel_sst_probe - PCI probe function
-*
-* @pci:	PCI device structure
-* @pci_id: PCI device ID structure
-*
-*/
-static int intel_sst_probe(struct pci_dev *pci,
-			const struct pci_device_id *pci_id)
-{
-	int ret = 0;
-	struct intel_sst_drv *sst_drv_ctx;
-	struct sst_platform_info *sst_pdata = pci->dev.platform_data;
-
-	dev_dbg(&pci->dev, "Probe for DID %x\n", pci->device);
-
-	ret = sst_alloc_drv_context(&sst_drv_ctx, &pci->dev, pci->device);
-	if (ret < 0)
-		return ret;
-
-	sst_drv_ctx->pdata = sst_pdata;
-	sst_drv_ctx->irq_num = pci->irq;
-
-	ret = sst_context_init(sst_drv_ctx);
-	if (ret < 0)
-		goto do_free_drv_ctx;
-
-
-	/* Init the device */
-	ret = pcim_enable_device(pci);
-	if (ret) {
-		dev_err(sst_drv_ctx->dev,
-			"device can't be enabled. Returned err: %d\n", ret);
-		goto do_destroy_wq;
-	}
-	sst_drv_ctx->pci = pci_dev_get(pci);
-
-	ret = sst_platform_get_resources(sst_drv_ctx);
-	if (ret < 0)
-		goto do_destroy_wq;
-
-	sst_set_fw_state_locked(sst_drv_ctx, SST_RESET);
-	snprintf(sst_drv_ctx->firmware_name, sizeof(sst_drv_ctx->firmware_name),
-			"%s%04x%s", "fw_sst_",
-			sst_drv_ctx->dev_id, ".bin");
-	dev_dbg(sst_drv_ctx->dev,
-		"Requesting FW %s now...\n", sst_drv_ctx->firmware_name);
-	ret = request_firmware_nowait(THIS_MODULE, 1,
-			sst_drv_ctx->firmware_name, sst_drv_ctx->dev,
-			GFP_KERNEL, sst_drv_ctx, sst_firmware_load_cb);
-
-	if (ret) {
-		dev_err(sst_drv_ctx->dev,
-			"Firmware load failed with error: %d\n", ret);
-		goto do_release_regions;
-	}
-
-
-	pci_set_drvdata(pci, sst_drv_ctx);
-	sst_configure_runtime_pm(sst_drv_ctx);
-	sst_register(sst_drv_ctx->dev);
-
-	return ret;
-
-do_release_regions:
-	pci_release_regions(pci);
-do_destroy_wq:
-	destroy_workqueue(sst_drv_ctx->post_msg_wq);
-do_free_drv_ctx:
-	dev_err(sst_drv_ctx->dev, "Probe failed with %d\n", ret);
-	return ret;
-}
-
-/**
-* intel_sst_remove - PCI remove function
-*
-* @pci:	PCI device structure
-*
-* This function is called by OS when a device is unloaded
-* This frees the interrupt etc
-*/
-static void intel_sst_remove(struct pci_dev *pci)
-{
-	struct intel_sst_drv *sst_drv_ctx = pci_get_drvdata(pci);
 
-	sst_context_cleanup(sst_drv_ctx);
-	pci_dev_put(sst_drv_ctx->pci);
-	pci_release_regions(pci);
-	pci_set_drvdata(pci, NULL);
-}
 
 static int intel_sst_runtime_suspend(struct device *dev)
 {
@@ -550,27 +372,7 @@  static int intel_sst_runtime_resume(struct device *dev)
 	return ret;
 }
 
-static const struct dev_pm_ops intel_sst_pm = {
+const struct dev_pm_ops intel_sst_pm = {
 	.runtime_suspend = intel_sst_runtime_suspend,
 	.runtime_resume = intel_sst_runtime_resume,
 };
-
-/*PCI Routines*/
-static struct pci_device_id intel_sst_ids[] = {
-	{ PCI_VDEVICE(INTEL, SST_MRFLD_PCI_ID), 0},
-	{ 0, }
-};
-
-static struct pci_driver sst_driver = {
-	.name = SST_DRV_NAME,
-	.id_table = intel_sst_ids,
-	.probe = intel_sst_probe,
-	.remove = intel_sst_remove,
-#ifdef CONFIG_PM
-	.driver = {
-		.pm = &intel_sst_pm,
-	},
-#endif
-};
-
-module_pci_driver(sst_driver);
diff --git a/sound/soc/intel/sst/sst.h b/sound/soc/intel/sst/sst.h
index b65b9c0..3ee555e 100644
--- a/sound/soc/intel/sst/sst.h
+++ b/sound/soc/intel/sst/sst.h
@@ -40,6 +40,7 @@ 
 #define MRFLD_FW_FEATURE_BASE_OFFSET 0x4
 #define MRFLD_FW_BSS_RESET_BIT 0
 
+extern const struct dev_pm_ops intel_sst_pm;
 enum sst_states {
 	SST_FW_LOADING = 1,
 	SST_FW_RUNNING,
@@ -537,4 +538,9 @@  void sst_fill_header_dsp(struct ipc_dsp_hdr *dsp, int msg,
 int sst_register(struct device *);
 int sst_unregister(struct device *);
 
+int sst_alloc_drv_context(struct intel_sst_drv **ctx,
+		struct device *dev, unsigned int dev_id);
+int sst_context_init(struct intel_sst_drv *ctx);
+void sst_context_cleanup(struct intel_sst_drv *ctx);
+void sst_configure_runtime_pm(struct intel_sst_drv *ctx);
 #endif
diff --git a/sound/soc/intel/sst/sst_pci.c b/sound/soc/intel/sst/sst_pci.c
new file mode 100644
index 0000000..c7eb28b
--- /dev/null
+++ b/sound/soc/intel/sst/sst_pci.c
@@ -0,0 +1,225 @@ 
+/*
+ *  sst_pci.c - SST (LPE) driver init file for pci enumeration.
+ *
+ *  Copyright (C) 2008-14	Intel Corp
+ *  Authors:	Vinod Koul <vinod.koul@intel.com>
+ *		Harsha Priya <priya.harsha@intel.com>
+ *		Dharageswari R <dharageswari.r@intel.com>
+ *		KP Jeeja <jeeja.kp@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/fs.h>
+#include <linux/firmware.h>
+#include <linux/pm_runtime.h>
+#include <sound/core.h>
+#include <sound/soc.h>
+#include <asm/platform_sst_audio.h>
+#include "../sst-mfld-platform.h"
+#include "sst.h"
+
+static int sst_platform_get_resources(struct intel_sst_drv *ctx)
+{
+	int ddr_base, ret = 0;
+	struct pci_dev *pci = ctx->pci;
+
+	ret = pci_request_regions(pci, SST_DRV_NAME);
+	if (ret)
+		return ret;
+
+	/* map registers */
+	/* DDR base */
+	if (ctx->dev_id == SST_MRFLD_PCI_ID) {
+		ctx->ddr_base = pci_resource_start(pci, 0);
+		/* check that the relocated IMR base matches with FW Binary */
+		ddr_base = relocate_imr_addr_mrfld(ctx->ddr_base);
+		if (!ctx->pdata->lib_info) {
+			dev_err(ctx->dev, "lib_info pointer NULL\n");
+			ret = -EINVAL;
+			goto do_release_regions;
+		}
+		if (ddr_base != ctx->pdata->lib_info->mod_base) {
+			dev_err(ctx->dev,
+					"FW LSP DDR BASE does not match with IFWI\n");
+			ret = -EINVAL;
+			goto do_release_regions;
+		}
+		ctx->ddr_end = pci_resource_end(pci, 0);
+
+		ctx->ddr = pcim_iomap(pci, 0,
+					pci_resource_len(pci, 0));
+		if (!ctx->ddr) {
+			ret = -EINVAL;
+			goto do_release_regions;
+		}
+		dev_dbg(ctx->dev, "sst: DDR Ptr %p\n", ctx->ddr);
+	} else {
+		ctx->ddr = NULL;
+	}
+	/* SHIM */
+	ctx->shim_phy_add = pci_resource_start(pci, 1);
+	ctx->shim = pcim_iomap(pci, 1, pci_resource_len(pci, 1));
+	if (!ctx->shim) {
+		ret = -EINVAL;
+		goto do_release_regions;
+	}
+	dev_dbg(ctx->dev, "SST Shim Ptr %p\n", ctx->shim);
+
+	/* Shared SRAM */
+	ctx->mailbox_add = pci_resource_start(pci, 2);
+	ctx->mailbox = pcim_iomap(pci, 2, pci_resource_len(pci, 2));
+	if (!ctx->mailbox) {
+		ret = -EINVAL;
+		goto do_release_regions;
+	}
+	dev_dbg(ctx->dev, "SRAM Ptr %p\n", ctx->mailbox);
+
+	/* IRAM */
+	ctx->iram_end = pci_resource_end(pci, 3);
+	ctx->iram_base = pci_resource_start(pci, 3);
+	ctx->iram = pcim_iomap(pci, 3, pci_resource_len(pci, 3));
+	if (!ctx->iram) {
+		ret = -EINVAL;
+		goto do_release_regions;
+	}
+	dev_dbg(ctx->dev, "IRAM Ptr %p\n", ctx->iram);
+
+	/* DRAM */
+	ctx->dram_end = pci_resource_end(pci, 4);
+	ctx->dram_base = pci_resource_start(pci, 4);
+	ctx->dram = pcim_iomap(pci, 4, pci_resource_len(pci, 4));
+	if (!ctx->dram) {
+		ret = -EINVAL;
+		goto do_release_regions;
+	}
+	dev_dbg(ctx->dev, "DRAM Ptr %p\n", ctx->dram);
+do_release_regions:
+	pci_release_regions(pci);
+	return 0;
+}
+
+/*
+ * intel_sst_probe - PCI probe function
+ *
+ * @pci:	PCI device structure
+ * @pci_id: PCI device ID structure
+ *
+ */
+static int intel_sst_probe(struct pci_dev *pci,
+			const struct pci_device_id *pci_id)
+{
+	int ret = 0;
+	struct intel_sst_drv *sst_drv_ctx;
+	struct sst_platform_info *sst_pdata = pci->dev.platform_data;
+
+	dev_dbg(&pci->dev, "Probe for DID %x\n", pci->device);
+	ret = sst_alloc_drv_context(&sst_drv_ctx, &pci->dev, pci->device);
+	if (ret < 0)
+		return ret;
+
+	sst_drv_ctx->pdata = sst_pdata;
+	sst_drv_ctx->irq_num = pci->irq;
+	ret = sst_context_init(sst_drv_ctx);
+	if (ret < 0)
+		goto do_free_drv_ctx;
+
+	/* Init the device */
+	ret = pcim_enable_device(pci);
+	if (ret) {
+		dev_err(sst_drv_ctx->dev,
+			"device can't be enabled. Returned err: %d\n", ret);
+		goto do_destroy_wq;
+	}
+	sst_drv_ctx->pci = pci_dev_get(pci);
+	ret = sst_platform_get_resources(sst_drv_ctx);
+	if (ret < 0)
+		goto do_destroy_wq;
+
+	sst_set_fw_state_locked(sst_drv_ctx, SST_RESET);
+	snprintf(sst_drv_ctx->firmware_name, sizeof(sst_drv_ctx->firmware_name),
+			"%s%04x%s", "fw_sst_",
+			sst_drv_ctx->dev_id, ".bin");
+	dev_dbg(sst_drv_ctx->dev,
+		"Requesting FW %s now...\n", sst_drv_ctx->firmware_name);
+	ret = request_firmware_nowait(THIS_MODULE, 1,
+			sst_drv_ctx->firmware_name, sst_drv_ctx->dev,
+			GFP_KERNEL, sst_drv_ctx, sst_firmware_load_cb);
+
+	if (ret) {
+		dev_err(sst_drv_ctx->dev,
+			"Firmware load failed with error: %d\n", ret);
+		goto do_release_regions;
+	}
+
+	pci_set_drvdata(pci, sst_drv_ctx);
+	sst_configure_runtime_pm(sst_drv_ctx);
+	sst_register(sst_drv_ctx->dev);
+
+	return ret;
+
+do_release_regions:
+	pci_release_regions(pci);
+do_destroy_wq:
+	destroy_workqueue(sst_drv_ctx->post_msg_wq);
+do_free_drv_ctx:
+	dev_err(sst_drv_ctx->dev, "Probe failed with %d\n", ret);
+	return ret;
+}
+
+/**
+ * intel_sst_remove - PCI remove function
+ *
+ * @pci:	PCI device structure
+ *
+ * This function is called by OS when a device is unloaded
+ * This frees the interrupt etc
+ */
+static void intel_sst_remove(struct pci_dev *pci)
+{
+	struct intel_sst_drv *sst_drv_ctx = pci_get_drvdata(pci);
+
+	sst_context_cleanup(sst_drv_ctx);
+	pci_dev_put(sst_drv_ctx->pci);
+	pci_release_regions(pci);
+	pci_set_drvdata(pci, NULL);
+}
+
+/* PCI Routines */
+static struct pci_device_id intel_sst_ids[] = {
+	{ PCI_VDEVICE(INTEL, SST_MRFLD_PCI_ID), 0},
+	{ 0, }
+};
+
+static struct pci_driver sst_driver = {
+	.name = SST_DRV_NAME,
+	.id_table = intel_sst_ids,
+	.probe = intel_sst_probe,
+	.remove = intel_sst_remove,
+#ifdef CONFIG_PM
+	.driver = {
+		.pm = &intel_sst_pm,
+	},
+#endif
+};
+
+module_pci_driver(sst_driver);
+
+MODULE_DESCRIPTION("Intel (R) SST(R) Audio Engine PCI Driver");
+MODULE_AUTHOR("Vinod Koul <vinod.koul@intel.com>");
+MODULE_AUTHOR("Harsha Priya <priya.harsha@intel.com>");
+MODULE_AUTHOR("Dharageswari R <dharageswari.r@intel.com>");
+MODULE_AUTHOR("KP Jeeja <jeeja.kp@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("sst");