Re: [alsa-devel] [PATCH v2 1/2] ALSA - hda: Add support for parsing new HDA capabilities

On Wed, Jul 20, 2016 at 07:31:17AM +0200, Takashi Iwai wrote:

...

• unsigned int offset; unsigned int counter = 0;

Need a line break.

arghh, will fix..

...

• offset = azx_readl(chip, LLCH);
• /* Lets walk the linked capabilities list */
• do {

• cur_cap = _snd_hdac_chip_read(l, azx_bus(chip), offset);
  

• switch ((cur_cap & CAP_HDR_ID_MASK) >> CAP_HDR_ID_OFF) {
  

• case GTS_CAP_ID:
  

• 	dev_dbg(chip->card->dev, "Found GTS capability");
  

• 	chip->gts_present = 1;
  

• 	break;
  

• default:
  

• 	break;
  

• }
  

• counter++;
  

• if (counter > AZX_MAX_CAPS) {
  

• 	dev_err(chip->card->dev, "We exceeded azx capabilities!!!\n");
  

• 	break;
  

• }
  

• /* read the offset of next capabiity */
  

• offset = cur_cap & CAP_HDR_NXT_PTR_MASK;
  

• } while (offset);

Wouldn't it be safer to use a normal while () {} loop? The first LLCH read might be zero, in theory.

Then in that case first read will give error. But yes I see the benifits.

Btw this is same as snd_hdac_ext_bus_parse_capabilities()

Should we move this to hdac and use for both. Ofcourse many new capablities do not make sense to legacy driver, so we would have to ignore them.

...

• if (!(chip->gts_present && boot_cpu_has(X86_FEATURE_ART)))

• chip->gts_present = false;
  

Need #ifdef CONFIG_X86 guard here, too. Also, the inclusion of <asm/cpufeature.h> isn't needed? (Again X86-only.)

This is intel.c :) I did compile it for ARM before sending.