Re: [alsa-devel] [PATCH v3 1/5] ALSA: hda - move bus_parse_capabilities to core

On Wed, 03 Aug 2016 18:06:10 +0200, Vinod Koul wrote:

HDA capability introduced recently are moved to hdac core so that it can be used by legacy driver as well. Also move the capability pointers up to hdac_bus object.

Signed-off-by: Vinod Koul vinod.koul@intel.com

The patch looks almost good, but just minor nitpicking:

+int snd_hdac_bus_parse_capabilities(struct hdac_bus *bus) +{

• unsigned int cur_cap;
• unsigned int offset;
• unsigned int counter = 0;
• offset = snd_hdac_chip_readl(bus, LLCH);
• /* Lets walk the linked capabilities list */
• do {

• cur_cap = _snd_hdac_chip_read(l, bus, offset);
  

• dev_dbg(bus->dev, "Capability version: 0x%x\n",
  

• 		((cur_cap & AZX_CAP_HDR_VER_MASK) >> AZX_CAP_HDR_VER_OFF));
  

Too much indentation. Also the parentheses are superfluous (unlike below).

• dev_dbg(bus->dev, "HDA capability ID: 0x%x\n",
  

• 		(cur_cap & AZX_CAP_HDR_ID_MASK) >> AZX_CAP_HDR_ID_OFF);
  

Too much indentation.

• 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");
  

• 	bus->mlcap = bus->remap_addr + offset;
  

• 	break;
  

• case AZX_GTS_CAP_ID:
  

• 	dev_dbg(bus->dev, "Found GTS capability offset=%x\n", offset);
  

• 	bus->gtscap = 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);
  

• 	bus->ppcap = bus->remap_addr + offset;
  

• 	break;
  

• case AZX_SPB_CAP_ID:
  

• 	/* SPIB capability found, handler function */
  

• 	dev_dbg(bus->dev, "Found SPB capability\n");
  

• 	bus->spbcap = bus->remap_addr + offset;
  

• 	break;
  

• case AZX_DRSM_CAP_ID:
  

• 	/* DMA resume  capability found, handler function */
  

• 	dev_dbg(bus->dev, "Found DRSM capability\n");
  

• 	bus->drsmcap = bus->remap_addr + offset;
  

• 	break;
  

• default:
  

• 	dev_dbg(bus->dev, "Unknown capability %d\n", cur_cap);
  

• 	break;
  

• }
  

• counter++;
  

• if (counter > HDAC_MAX_CAPS) {
  

• 	dev_err(bus->dev, "We exceeded HDAC capablities!!!\n");
  

• 	break;
  

• }
  

• /* read the offset of next capabiity */
  

A typo.

thanks,

Takashi