From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v3 5/7] ASoC: intel - add Skylake HDA audio driver Date: Thu, 30 Apr 2015 12:18:55 +0200 Message-ID: References: <1430250870-3169-1-git-send-email-vinod.koul@intel.com> <1430250870-3169-6-git-send-email-vinod.koul@intel.com> <20150430101140.GN3521@localhost> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id 90B522614E3 for ; Thu, 30 Apr 2015 12:18:56 +0200 (CEST) In-Reply-To: <20150430101140.GN3521@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Vinod Koul Cc: alsa-devel@alsa-project.org, patches.audio@intel.com, liam.r.girdwood@linux.intel.com, broonie@kernel.org, Jeeja KP , "Subhransu S. Prusty" List-Id: alsa-devel@alsa-project.org 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