On Wed, Jul 08, 2015 at 08:01:14PM +0100, Mark Brown wrote:
> On Mon, Jul 06, 2015 at 08:54:24AM +0530, Vinod Koul wrote:
> 
> > +static irqreturn_t skl_interrupt(int irq, void *dev_id)
> > +{
> > +	struct hdac_ext_bus *ebus = dev_id;
> > +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> > +	u32 status;
> > +
> > +#ifdef CONFIG_PM
> > +	if (!pm_runtime_active(bus->dev))
> > +		return IRQ_NONE;
> > +#endif
> 
> There's a stub for pm_runtime_active for !PM which does the right thing
> here, no need for ifdefs.

Ah yes, I will fix it here and as well as HDA driver :)
 
> > +	/*configure PM */
> > +	pm_runtime_set_autosuspend_delay(bus->dev, SKL_SUSPEND_DELAY);
> > +	pm_runtime_use_autosuspend(bus->dev);
> > +	pm_runtime_put_noidle(bus->dev);
> > +	pm_runtime_allow(bus->dev);
> > +
> > +	pci_set_drvdata(skl->pci, ebus);
> 
> Shouldn't you be setting the driver data before you register the device
> and enable PM?  Otherwise the PM callbacks might run without it being
> set.
Right, will update

> 
> > +out_free:
> > +	skl->init_failed = 1;
> > +	skl_free(ebus);
> > +	pci_set_drvdata(skl->pci, NULL);
> 
> No need to set the driver data to NULL, the driver core will do it.
Ok

-- 
~Vinod