From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH v6 2/3] ALSA: hdac_ext: add hdac extended controller Date: Mon, 08 Jun 2015 11:12:48 +0200 Message-ID: References: <1433411602-5444-1-git-send-email-vinod.koul@intel.com> <1433411602-5444-3-git-send-email-vinod.koul@intel.com> 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 7E0CB2605B2 for ; Mon, 8 Jun 2015 11:12:49 +0200 (CEST) In-Reply-To: <1433411602-5444-3-git-send-email-vinod.koul@intel.com> 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: liam.r.girdwood@linux.intel.com, patches.audio@intel.com, alsa-devel@alsa-project.org, broonie@kernel.org, Jeeja KP List-Id: alsa-devel@alsa-project.org At Thu, 4 Jun 2015 15:23:21 +0530, Vinod Koul wrote: > > The controller needs to support the new capabilities and allow reading, > parsing and initializing of these capabilities, so this patch does it > > Signed-off-by: Jeeja KP > Signed-off-by: Vinod Koul > --- > include/sound/hdaudio_ext.h | 174 +++++++++++++++++++++ > sound/hda/ext/Makefile | 2 +- > sound/hda/ext/hdac_ext_controller.c | 295 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 470 insertions(+), 1 deletion(-) > create mode 100644 sound/hda/ext/hdac_ext_controller.c > > diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h > index df1f8ae64693..b17d7f91f6ac 100644 > --- a/include/sound/hdaudio_ext.h > +++ b/include/sound/hdaudio_ext.h > @@ -90,4 +90,178 @@ void snd_hdac_ext_bus_ppcap_int_enable(struct hdac_ext_bus *chip, bool enable); > (snd_hdac_ext_bus_ppcap_readb(dev, reg) & \ > ~(mask)) | (val)) > > +void snd_hdac_ext_stream_spbcap_enable(struct hdac_ext_bus *chip, > + bool enable, int index); > +/* > + * macros for spcap register read/write > + */ > +#define _snd_hdac_ext_bus_spbcap_write(type, dev, reg, value) \ > + ((dev)->bus.io_ops->reg_write ## type(value, \ > + (dev)->spbcap_addr + (reg))) > +#define _snd_hdac_ext_bus_spbcap_read(type, dev, reg) \ > + ((dev)->bus.io_ops->reg_read ## type((dev)->spbcap_addr + (reg))) > + > +/* read/write a register, pass without AZX_REG_ prefix */ > +#define snd_hdac_ext_bus_spbcap_writel(dev, reg, value) \ > + _snd_hdac_ext_bus_spbcap_write(l, dev, AZX_REG_ ## reg, value) > +#define snd_hdac_ext_bus_spbcap_writew(dev, reg, value) \ > + _snd_hdac_ext_bus_spbcap_write(w, dev, AZX_REG_ ## reg, value) > +#define snd_hdac_ext_bus_spbcap_writeb(dev, reg, value) \ > + _snd_hdac_ext_bus_spbcap_write(b, dev, AZX_REG_ ## reg, value) > +#define snd_hdac_ext_bus_spbcap_readl(dev, reg) \ > + _snd_hdac_ext_bus_spbcap_read(l, dev, AZX_REG_ ## reg) > +#define snd_hdac_ext_bus_spbcap_readw(dev, reg) \ > + _snd_hdac_ext_bus_spbcap_read(w, dev, AZX_REG_ ## reg) > +#define snd_hdac_ext_bus_spbcap_readb(dev, reg) \ > + _snd_hdac_ext_bus_spbcap_read(b, dev, AZX_REG_ ## reg) > + > +/* update a register, pass without AZX_REG_ prefix */ > +#define snd_hdac_ext_bus_spbcap_updatel(dev, reg, mask, val) \ > + snd_hdac_ext_bus_spbcap_writel(dev, reg, \ > + (snd_hdac_ext_bus_spbcap_readl(dev, reg) & \ > + ~(mask)) | (val)) > +#define snd_hdac_ext_bus_spbcap_updatew(dev, reg, mask, val) \ > + snd_hdac_ext_bus_spbcap_writew(dev, reg, \ > + (snd_hdac_ext_bus_spbcap_readw(dev, reg) & \ > + ~(mask)) | (val)) > +#define snd_hdac_ext_bus_spbcap_updateb(dev, reg, mask, val) \ > + snd_hdac_ext_bus_spbcap_writeb(dev, reg, \ > + (snd_hdac_ext_bus_spbcap_readb(dev, reg) & \ > + ~(mask)) | (val)) The same question for these macros: whether we really want to wrap all. (Ditto for mlcap, and others.) > +struct hdac_ext_link { > + struct hdac_bus *bus; > + int index; > + void __iomem *ml_addr; /* link output stream reg pointer */ > + u32 lcaps; /* link capablities */ > + u16 lsdiid; /* link sdi identifier */ > + char codec[HDA_MAX_CODECS][32]; /* codecs connectes to the link */ Do we need to keep the codec name string? Isn't it just codec address we'd like to check the match...? If so, we may use bitmasks instead, too. > +/** > + * snd_hdac_ext_bus_parse_capabilities - parse capablity structure > + * @sbus: the pointer to extended bus object > + * > + * Returns 0 if successful, or a negative error code. > + */ > +int snd_hdac_ext_bus_parse_capabilities(struct hdac_ext_bus *sbus) > +{ > + unsigned int cur_cap; > + unsigned int offset; > + struct hdac_bus *bus = &sbus->bus; > + > + offset = snd_hdac_chip_readl(bus, LLCH); > + > + if (offset < 0) > + return -EIO; > + > + sbus->ppcap = false; > + sbus->mlcap = false; > + sbus->spbcap = false; > + sbus->gtscap = false; > + > + /* Lets walk the linked capabilities list */ > + do { > + cur_cap = _snd_hdac_chip_read(l, bus, offset); > + > + if (cur_cap < 0) > + return -EIO; > + > + dev_dbg(bus->dev, "Capability version: 0x%x\n", > + ((cur_cap & AZX_CAP_HDR_VER_MASK) >> AZX_CAP_HDR_VER_OFF)); > + > + dev_dbg(bus->dev, "HDA capability ID: 0x%x\n", > + (cur_cap & AZX_CAP_HDR_ID_MASK) >> AZX_CAP_HDR_ID_OFF); > + > + switch ((cur_cap & AZX_CAP_HDR_ID_MASK) >> AZX_CAP_HDR_ID_OFF) { > + case AZX_ML_CAP_ID: > + dev_dbg(bus->dev, "Found ML capability\n"); > + sbus->mlcap = true; > + sbus->mlcap_addr = bus->remap_addr + offset; Can be flags (mlcap, gtscap, etc) removed by just NULL-checking the corresponding address? > + break; > + > + case AZX_GTS_CAP_ID: > + dev_dbg(bus->dev, "Found GTS capability offset=%x\n", offset); > + sbus->gtscap = true; > + sbus->gtscap_addr = bus->remap_addr + offset; > + break; > + > + case AZX_PP_CAP_ID: > + /* PP capability found, the Audio DSP is present */ > + dev_dbg(bus->dev, "Found PP capability offset=%x\n", offset); > + sbus->ppcap = true; > + sbus->ppcap_addr = bus->remap_addr + offset; > + break; > + > + case AZX_SPB_CAP_ID: > + /* SPIB capability found, handler function */ > + dev_dbg(bus->dev, "Found SPB capability\n"); > + sbus->spbcap = true; > + sbus->spbcap_addr = bus->remap_addr + offset; > + break; > + > + default: > + dev_dbg(bus->dev, "Unknown capability %d\n", cur_cap); > + break; > + } > + > + /* read the offset of next capabiity */ > + offset = cur_cap & AZX_CAP_HDR_NXT_PTR_MASK; > + } while (offset); Better to have a loop counter for avoiding the endless loop. You should never trust the hardware. > +MODULE_DESCRIPTION("HDA extended core"); > +MODULE_LICENSE("GPL v2"); These should have been added in the first patch. thanks, Takashi