Re: [PATCH 06/18] ASoC: SOF: Intel: hda-mlink: add structures to parse ALT links

[Takashi Iwai <tiwai@suse.de>]

On Thu, 30 Mar 2023 18:09:36 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 3/30/23 10:51, Takashi Iwai wrote:
> > On Mon, 27 Mar 2023 13:29:19 +0200,
> > Peter Ujfalusi wrote:
> >>
> >>  int hda_bus_ml_get_capabilities(struct hdac_bus *bus)
> >>  {
> >> -	if (bus->mlcap)
> >> -		return snd_hdac_ext_bus_get_ml_capabilities(bus);
> >> +	u32 link_count;
> >> +	int ret;
> >> +	int i;
> >> +
> >> +	if (!bus->mlcap)
> >> +		return 0;
> >> +
> >> +	link_count = readl(bus->mlcap + AZX_REG_ML_MLCD) + 1;
> >> +
> >> +	dev_dbg(bus->dev, "HDAudio Multi-Link count: %d\n", link_count);
> >> +
> >> +	for (i = 0; i < link_count; i++) {
> >> +		ret = hda_ml_alloc_h2link(bus, i);
> >> +		if (ret < 0) {
> >> +			hda_bus_ml_free(bus);
> >> +			return ret;
> >> +		}
> >> +	}
> >>  	return 0;
> > 
> > This makes that each call of hda_bus_ml_get_capabilities() adds the
> > h2link entries blindly.  If the driver calls it multiple times
> > mistakenly (the function name sounds as if it's just a helper to query
> > the capability bits), it'll lead to doubly entries.  Maybe adding some
> > check would be safer, IMO.
> 
> Interesting comment, I didn't think of that one.
> 
> This function is currently called in the probe and indirectly via
> hda_init_caps(). I think the driver framework guarantees the probe is
> only called once, doesn't it?
> 
> we can also rename this function to make it clearer if there are any
> suggestions, but the name is aligned with previous implementations of
> the snd_hdac_ext stuff.

Yeah, naming it as an init function would avoid the misuse.


Takashi