Re: [alsa-devel] [PATCH v2 5/7] ASoC: hda - add Skylake HD audio driver

On Fri, Apr 17, 2015 at 12:06:50PM +0200, Takashi Iwai wrote:

At Fri, 17 Apr 2015 14:43:18 +0530, Vinod Koul wrote:

...

From: Jeeja KP jeeja.kp@intel.com

This patch adds ASoC Skylake HD audio controller driver

Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com

---

include/sound/hdaudio.h | 1 + sound/soc/hda/hda_skl.c | 748 +++++++++++++++++++++++++++++++++++++++++++++++ sound/soc/hda/hda_skl.h | 44 +++ 3 files changed, 793 insertions(+) create mode 100644 sound/soc/hda/hda_skl.c create mode 100644 sound/soc/hda/hda_skl.h

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 227e71956c35..a8feb761e00d 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -393,6 +393,7 @@ struct hdac_stream { bool running:1; bool no_period_wakeup:1; bool locked:1;

• bool prepared:1;

  /* timestamp */ unsigned long start_wallclk; /* start + minimum wallclk */

OK, this chunk should be in the earlier patch. Or I'll apply it now independently.

...

diff --git a/sound/soc/hda/hda_skl.c b/sound/soc/hda/hda_skl.c new file mode 100644 index 000000000000..8a66ef5a4ed8 --- /dev/null +++ b/sound/soc/hda/hda_skl.c @@ -0,0 +1,748 @@ +/*

• • hda_skl.c - Implementation of primary ASoC Intel HD Audio driver
• • Copyright (C) 2015 Intel Corp
• • Author: Jeeja KP jeeja.kp@intel.com
• • Copyright (c) 2004 Takashi Iwai tiwai@suse.de

• •                 PeiSen Hou <pshou@realtek.com.tw>
    

• • 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/delay.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/pci.h> +#include <linux/mutex.h> +#include <linux/io.h> +#include <linux/pm_runtime.h>

+#include <linux/platform_device.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/hda_register.h> +#include <sound/hdaudio.h> +#include "hda_skl.h"

+MODULE_LICENSE("GPL v2"); +MODULE_SUPPORTED_DEVICE("{Intel, PCH},"); +MODULE_DESCRIPTION("Intel aDSP HDA driver");

+/*

• • initialize the PCI registers
• */

+/* update bits in a PCI register byte */ +static void update_pci_byte(struct pci_dev *pci, unsigned int reg,

• 	    unsigned char mask, unsigned char val)
  

+{

• unsigned char data;
• pci_read_config_byte(pci, reg, &data);
• data &= ~mask;
• data |= (val & mask);
• pci_write_config_byte(pci, reg, data);

+}

+static void azx_init_pci(struct hdac_bus *chip) +{

• struct hda_soc_bus *hda = container_of(chip, struct hda_soc_bus, chip);
• /* Clear bits 0-2 of PCI register TCSEL (at offset 0x44)

• * TCSEL == Traffic Class Select Register, which sets PCI express QOS
  

• * Ensuring these bits are 0 clears playback static on some HD Audio
  

• * codecs.
  

• * The PCI register TCSEL is defined in the Intel manuals.
  

• */
  

• dev_dbg(chip->dev, "Clearing TCSEL\n");
• update_pci_byte(hda->pci, AZX_PCIREG_TCSEL, 0x07, 0);

+}

+irqreturn_t azx_interrupt(int irq, void *dev_id) +{

• struct hdac_bus *chip = dev_id;
• u32 status;

+#ifdef CONFIG_PM

• if (!pm_runtime_active(chip->dev))

• return IRQ_NONE;
  

+#endif

• spin_lock(&chip->reg_lock);
• status = snd_hdac_chip_readl(chip, INTSTS);
• if (status == 0 || status == 0xffffffff) {

• spin_unlock(&chip->reg_lock);
  

• return IRQ_NONE;
  

• }
• spin_unlock(&chip->reg_lock);
• return IRQ_WAKE_THREAD;

+}

+irqreturn_t azx_threaded_handler(int irq, void *dev_id) +{

• struct hdac_bus *chip = dev_id;
• u32 status;
• unsigned long cookie;
• status = snd_hdac_chip_readl(chip, INTSTS);
• spin_lock_irqsave(&chip->reg_lock, cookie);
• snd_hdac_bus_handle_stream_irq(chip, status, &azx_position_check);
• /* clear rirb int */
• status = snd_hdac_chip_readb(chip, RIRBSTS);
• if (status & RIRB_INT_MASK) {

• if (status & RIRB_INT_RESPONSE)
  

• 	snd_hdac_bus_update_rirb(chip);
  

• snd_hdac_chip_writeb(chip, RIRBSTS, RIRB_INT_MASK);
  

• }
• spin_unlock_irqrestore(&chip->reg_lock, cookie);
• return IRQ_HANDLED;

+}

+/* initialize SD streams, use seprate streeam tag for PB and CP */ +int azx_init_stream(struct hdac_bus *chip, int num_stream, int direction)

Missing static.

...

+{

• int i, tag;
• int stream_tag = 0;
• /* initialize each stream (aka device)

• * assign the starting bdl address to each stream (device)
  

• * and initialize
  

• */
  

• for (i = 0; i < num_stream; i++) {

• struct hdac_stream *hdac_stream =
  

• 	devm_kzalloc(chip->dev, sizeof(*hdac_stream), GFP_KERNEL);
  

hdac_bus_exit() has a check of list_empty() for the stream list. It means that it expects that the driver cleans up the stream list beforehand properly. That is, devm doesn't work well because it'll be performed after that... We can get rid of the check, too, if preferred, of course.

Okay, i think would prefer later as devm_ eases drivers job :) Will send patch for that

...

•  if (!hdac_stream) {
  

• 	dev_err(chip->dev, "kzalloc block failed");
  

• 	return -ENOMEM;
  

• }
  

• tag = ++stream_tag;
  

• snd_hdac_stream_init(chip, hdac_stream, i, direction, tag);
  

• list_add_tail(&hdac_stream->list, &chip->stream_list);
  

• }
• return 0;

+}

+/* calculate runtime delay from LPIB */ +int azx_get_delay_from_lpib(struct hdac_bus *chip,

• 		struct hdac_stream *azx_dev,
  

• 		unsigned int pos)
  

+{

• struct snd_pcm_substream *substream = azx_dev->substream;
• int stream = substream->stream;
• unsigned int lpib_pos = snd_hdac_stream_get_pos_lpib(azx_dev);
• int delay;
• if (stream == SNDRV_PCM_STREAM_PLAYBACK)

• delay = pos - lpib_pos;
  

• else

• delay = lpib_pos - pos;
  

• if (delay < 0) {

• if (delay >= azx_dev->delay_negative_threshold)
  

• 	delay = 0;
  

• else
  

• 	delay += azx_dev->bufsize;
  

• }
• if (delay >= azx_dev->period_bytes) {

• dev_info(chip->dev,
  

• 	 "Unstable LPIB (%d >= %d); disabling LPIB delay counting\n",
  

• 	 delay, azx_dev->period_bytes);
  

• delay = 0;
  

• }
• return bytes_to_frames(substream->runtime, delay);

+}

+/*

• • Check whether the current DMA position is acceptable for updating
• • periods. Returns non-zero if it's OK.
• • Many HD-audio controllers appear pretty inaccurate about
• • the update-IRQ timing. The IRQ is issued before actually the
• • data is processed. So, we need to process it afterwords in a
• • workqueue.
• */

+static int azx_position_ok(struct hdac_bus *chip, struct hdac_stream *azx_dev) +{

• u32 wallclk;
• unsigned int pos = 0;
• wallclk = snd_hdac_chip_readl(chip, WALLCLK) - azx_dev->start_wallclk;
• if (wallclk < (azx_dev->period_wallclk * 2) / 3)

• return -1;	/* bogus (too early) interrupt */
  

• if (chip->use_posbuf) {

• /* use the position buffer as default */
  

• pos = snd_hdac_stream_get_pos_posbuf(azx_dev);
  

• if (!pos || pos == (u32)-1) {
  

• 	dev_info(chip->dev,
  

• 		 "Invalid pos buffer, using LPIB read method instead.\n");
  

• 	pos = snd_hdac_stream_get_pos_lpib(azx_dev);
  

• }
  

• }
• if (pos >= azx_dev->bufsize)

• pos = 0;
  

• if (WARN_ONCE(!azx_dev->period_bytes,

•       "hda-skl: zero azx_dev->period_bytes"))
  

• return -1; /* this shouldn't happen! */
  

• if (wallclk < (azx_dev->period_wallclk * 5) / 4 &&

•    pos % azx_dev->period_bytes > azx_dev->period_bytes / 2)
  

• /* NG - it's below the first next period boundary */
  

• return chip->bdl_pos_adj ? 0 : -1;
  

• azx_dev->start_wallclk += wallclk;
• return 1; /* OK, it's fine */

+}

+/* called from IRQ */ +void azx_position_check(struct hdac_bus *chip, struct hdac_stream *azx_dev) +{

• if (azx_position_ok(chip, azx_dev) > 0)

• snd_pcm_period_elapsed(azx_dev->substream);
  

+}

+static int azx_acquire_irq(struct hdac_bus *chip, int do_disconnect) +{

• struct hda_soc_bus *hda = container_of(chip, struct hda_soc_bus, chip);
• if (request_threaded_irq(hda->pci->irq, azx_interrupt,

• 	azx_threaded_handler,
  

• 	hda->msi ? 0 : IRQF_SHARED,
  

• 	KBUILD_MODNAME, chip)) {
  

• dev_err(chip->dev,
  

• 	"unable to grab IRQ %d, disabling device\n",
  

• 	hda->pci->irq);
  

• return -1;
  

• }
• pci_intx(hda->pci, !hda->msi);

You should keep the irq number tracking in bus->irq. Otherwise it may result in a wrong free_irq() in the error paths....

ah yes missed that, will update

...

• return 0;

+}

+#ifdef CONFIG_PM_SLEEP +/*

• • power management
• */

+static int azx_suspend(struct device *dev) +{

• struct pci_dev *pci = to_pci_dev(dev);
• struct hdac_bus *chip = pci_get_drvdata(pci);
• struct hda_soc_bus *hda = container_of(chip, struct hda_soc_bus, chip);
• snd_hdac_bus_stop_chip(chip);
• snd_hdac_bus_enter_link_reset(chip);
• if (pci->irq >= 0)

.... here check bus->irq.

Yup