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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 21 Jul 2016 06:24:26 +0200,
Vinod Koul wrote:
> 
> 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.

It sounds not bad, indeed.

> > > +	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 :)

Intel created non-x86 chips, too :)

> I did compile it for ARM before sending.

With 32bit ARM?  arm64 and s390 have this header, casually.

The driver is for all architectures with PCI.  Try with powerpc or
sparc as well.  Or better to look through the tree to confirm who has
asm/cpufeature.h.


Takashi