[alsa-devel] [PATCH v2] ALSA: hda - suspend codecs in parallel

[Takashi Iwai]

At Wed, 27 Nov 2013 04:14:58 +0000,
Lin, Mengdong wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, November 26, 2013 10:05 PM
> 
> > > +#ifdef CONFIG_PM
> > > +	bus->pm_wq = create_workqueue("hda-pm-workq");
> > 
> > Better to use a unique name, e.g. with a card number suffix.
> 
> Ok.
> 
> > > +	if (!bus->pm_wq) {
> > > +		snd_printk(KERN_ERR "cannot create PM workqueue\n");
> > > +		snd_hda_bus_free(bus);
> > > +		return -ENOMEM;
> > > +	}
> > > +	workqueue_set_max_active(bus->pm_wq, HDA_MAX_NUM_CODECS);
> > 
> > As mentioned in another post, this can be set dynamically, e.g. at
> > snd_hda_suspend() with bus->num_codecs.
> 
> Can I move this to snd_hda_codec_new() like this? 
> 	list_add_tail(&codec->list, &bus->codec_list);
>    +bus->num_codecs++;
>    +workqueue_set_max_active(bus->pm_wq, bus->num_codecs);
> 
> The codec number does not change after initialization, and usually 1 or 2 codecs are connected to the bus.

It's not perfect but it'd be good enough, yes.
(A codec might be dynamically removed via snd_hda_codec_free() in
 theory, but this wouldn't happen actually.)

> > >  	err = snd_device_new(card, SNDRV_DEV_BUS, bus, &dev_ops);
> > >  	if (err < 0) {
> > >  		snd_hda_bus_free(bus);
> > > @@ -1434,6 +1451,8 @@ int snd_hda_codec_new(struct hda_bus *bus,
> > >  	 */
> > >  	hda_keep_power_on(codec);
> > >  	hda_call_pm_notify(bus, true);
> > > +
> > > +	INIT_WORK(&codec->pm_work, hda_pm_work);
> > 
> > Better to put this before hda_keep_power_on() call.
> 
> Ok.
> 
> > >  	if (codec->bus->modelname) {
> > > @@ -1445,6 +1464,7 @@ int snd_hda_codec_new(struct hda_bus *bus,
> > >  	}
> > >
> > >  	list_add_tail(&codec->list, &bus->codec_list);
> > > +	bus->num_codecs++;
> > >  	bus->caddr_tbl[codec_addr] = codec;
> > >
> > >  	codec->vendor_id = snd_hda_param_read(codec, AC_NODE_ROOT,
> > 
> > Also decrease bus->num_codecs in snd_hda_codec_free(), just to be sure.
> 
> Can I decrease bus->num_codecs in snd_hda_bus_free(), after the codec is freed? 
> 	list_for_each_entry_safe(codec, n, &bus->codec_list, list) {
> 		snd_hda_codec_free(codec);
> +		bus->num_codecs--;
> 	}

It looks inconsistent.  The increment is done in snd_hda_codec_new(),
so the decrement should be in its counterpart.


thanks,

Takashi