[alsa-devel] [PATCH v2 1/3] ALSA: hdac: add link pm and ref counting

[Takashi Iwai]

On Mon, 09 May 2016 09:58:23 +0200,
Vinod Koul wrote:
> 
> On Mon, May 09, 2016 at 08:40:56AM +0200, Takashi Iwai wrote:
> > > >   * @gtscap: gts capabilities pointer
> > > >   * @drsmcap: dma resume capabilities pointer
> > > >   * @hlink_list: link list of HDA links
> > > > + * @lock: lock for link mgmt
> > > > + * @cmd_io: state of cmd_io
> > > >   */
> > > >  struct hdac_ext_bus {
> > > >  	struct hdac_bus bus;
> > > > @@ -27,6 +29,9 @@ struct hdac_ext_bus {
> > > >  	void __iomem *drsmcap;
> > > >  
> > > >  	struct list_head hlink_list;
> > > > +
> > > > +	struct mutex lock;
> > > > +	int cmd_io;
> > 
> > It'd be better to put some comments what the flag means.
> > Also, a bool would be clearer.
> 
> Well we do have comment that it means state of cmd_io but yes we can make it
> clearer by explicitly saying the state of cmd DMAs CORB and RIRB

Or, name it more explicitly.

> 
> I will make it a bool
> 
> > > > +int snd_hdac_ext_bus_link_put(struct hdac_ext_bus *ebus,
> > > > +				struct hdac_ext_link *link)
> > > > +{
> > > > +	int ret = 0, state = 0;
> > > > +	struct hdac_ext_link *hlink = NULL;
> > 
> > Why initializing hlink?
> 
> Not required :)
> 
> > > > +	/*
> > > > +	 * if we move from 1 to 0, count will be 0
> > > > +	 * so power down this link as well
> > > > +	 */
> > > > +	if (--link->ref_count == 0) {
> > > > +		ret = snd_hdac_ext_bus_link_power_down(link);
> > > > +
> > > > +		/*
> > > > +		 * now check if all links are off, if so turn off
> > > > +		 * cmd dma as well
> > > > +		 */
> > > > +		list_for_each_entry(hlink, &ebus->hlink_list, list) {
> > > > +			if (hlink->ref_count)
> > > > +				state++;
> > > > +		}
> > 
> > Basically you can break at the first match.  But it's supposed to be a
> > relatively short list, so no performance impact should be seen.
> > So, take micro-optimization only if it's simple enough.
> 
> No not in this case, we are scanning the state of all links to see if they
> are on or not and turning off DMAs when all the links are off, so we can't
> break at first match in this case.

You need to check whether there is any link, not the number of links,
right?  Then it'd be like:

	link_up = false;
	list_for_each_entry(hlink, &ebus->hlink_list, list) {
		if (hlink->ref_count) {
			link_up = true;
			break;
		}
	}

	if (!link_up) {
		cmd_io_off();
	}


Takashi

> Yes for other places where we find a link, first match break will help, I
> will check that
> 
> Thanks
> -- 
> ~Vinod
>