[alsa-devel] [PATCH v3 5/7] ASoC: intel - add Skylake HDA audio driver
Takashi Iwai
tiwai at suse.de
Thu Apr 30 12:18:55 CEST 2015
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
More information about the Alsa-devel
mailing list