Re: [PATCH v3 5/7] ASoC: intel - add Skylake HDA audio driver

[Takashi Iwai <tiwai@suse.de>]

At Thu, 30 Apr 2015 15:41:40 +0530,
Vinod Koul wrote:
> 
> On Wed, Apr 29, 2015 at 02:49:05PM +0200, Takashi Iwai wrote:
> > At Wed, 29 Apr 2015 01:24:28 +0530,
> > Vinod Koul wrote:
> > > 
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_SUPPORTED_DEVICE("{Intel, PCH},");
> > > +MODULE_DESCRIPTION("Intel aDSP HDA driver");
> > 
> > Are the descriptions correct?
> What am i missing?

Is it for PCH?

Well, we've had entries for MODULE_SUPPORTED_DEVICE() that have been
used for alsaconf script.  But nowadays it's really obsoleted, and I
think we should get rid of MODULE_SUPPORTED_DEVICE() line for avoiding
confusion.

> > > +static irqreturn_t azx_interrupt(int irq, void *dev_id)
> > > +{
> > > +	struct soc_hdac_bus *sbus = dev_id;
> > > +	struct hdac_bus *bus = hdac_bus(sbus);
> > > +	u32 status;
> > > +
> > > +#ifdef CONFIG_PM
> > > +	if (!pm_runtime_active(bus->dev))
> > > +		return IRQ_NONE;
> > > +#endif
> > > +
> > > +	spin_lock(&bus->reg_lock);
> > > +
> > > +	status = snd_hdac_chip_readl(bus, INTSTS);
> > > +	if (status == 0 || status == 0xffffffff) {
> > > +		spin_unlock(&bus->reg_lock);
> > > +		return IRQ_NONE;
> > > +	}
> > > +
> > > +	/* clear rirb int */
> > > +	status = snd_hdac_chip_readb(bus, RIRBSTS);
> > > +	if (status & RIRB_INT_MASK) {
> > > +		if (status & RIRB_INT_RESPONSE)
> > > +			snd_hdac_bus_update_rirb(bus);
> > > +		snd_hdac_chip_writeb(bus, RIRBSTS, RIRB_INT_MASK);
> > > +		return IRQ_HANDLED;
> > 
> > An irq may have both RIRB and stream irq bits set.  If we return
> > IRQ_HANDLED here, I suppose the stream irqs won't be handled.
> They will be in the thread handler.

... and it's woken up only when IRQ_WAKE_THREAD is returned.

> Also worth mentioning here is that the
> aDSP interrupts will be handled by IPC code (shared interrupt)
> 
> Now yes we are missing fallback to coupled mode, if aDSP fails then stream
> should be handled here, will add that
> 
> > 
> > > +	}
> > > +
> > > +	spin_unlock(&bus->reg_lock);
> > > +	return IRQ_WAKE_THREAD;
> > > +}
> > 
> > The threaded irq handler checks only the stream irqs, so better to
> > check INTSTS here.  That is, something like:
> >   return snd_hdac_chip_readl(bus, INTSTS) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
> Yes that makese sense
(snip)
> > > +static int azx_add_codec_device(int addr, struct hdac_bus *bus)
> > > +{
> > > +	struct hdac_device *hdev = NULL;
> > > +	char name[10];
> > > +	int ret;
> > > +
> > > +	hdev = devm_kzalloc(bus->dev, sizeof(*hdev), GFP_KERNEL);
> > > +	if (!hdev)
> > > +		return -ENOMEM;
> > > +
> > > +	snprintf(name, sizeof(name), "codec#%03x", addr);
> > 
> > The name should be unique, and this would conflict when there are
> > multiple sound cards with the same codec address.  The legacy driver,
> > for example, encodes the name with the card index and the codec
> > address.
> would we have multiple cards on a system with hda codecs? I am not too sure,
> but yes lets add this

Yes, typically with discrete graphics (AMD or Nvidia).


Takashi