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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
