[alsa-devel] [PATCH v6 2/3] ALSA: hdac_ext: add hdac extended controller

[Takashi Iwai]

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 <jeeja.kp at intel.com>
> Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> ---
>  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