At Thu, 30 Aug 2012 14:17:19 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, August 29, 2012 10:59 PM
The patch below is the revised version. I think it's a good cleanup at the same time. Basically it makes pm_notify callback simply turning up/down the controller power without checking extra flags there. The conditions are checked in the caller side instead.
The pm_notify is called at codec creation and deletion times (but without checking extra flags), thus the refcounts can be managed more easily.
Hi Takashi,
Usage of the new pm_notify() looks nice. The v3 patch works well on my platform.
However, maybe we can add some check before notifying the codec power-down in snd_hda_codec_free().
static void snd_hda_codec_free(struct hda_codec *codec) { ... restore_init_pincfgs(codec); #ifdef CONFIG_SND_HDA_POWER_SAVE cancel_delayed_work(&codec->power_work); flush_workqueue(codec->bus->workq); #endif ... if (codec->patch_ops.free) codec->patch_ops.free(codec); module_put(codec->owner); hda_call_pm_notify(codec->bus, false); ... }
restore_init_pincfgs() will resume the codec to D0, and since the power_save timeout is at least 1second, so it's almost always safe to call hda_call_pm_notify(codec->bus, false) here. But an extremely low possibility is: the codec will be suspended again and call hda_call_pm_notify(,false) in hda_power_work() before the work is cancelled and queue is flushed.
Yeah, the check seems slipped from my previous post by some reason. Maybe I cherry-picked a wrong branch. It should be if (codec->power_on) hda_call_pm_notify(codec->bus, false); and it should be called before module_put().
And the flag 'codec->d3_stop_clk_ok' also cannot tell us whether a pm_notify(, false) has already been called or not, because codec->patch_ops.free() may call hda_set_power_state (D3) unexpectedly. So, I think that each codec still needs a flag 'power_count' and let the pm_notify modify the count:
static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec, bool power_up) { struct azx *chip = bus->private_data;
if (power_up) { pm_runtime_get_sync(&chip->pci->dev); codec->power_count ++; } else { pm_runtime_put_sync(&chip->pci->dev); codec->power_count --; } }
And static void snd_hda_codec_free(struct hda_codec *codec) { ... +#ifdef CONFIG_SND_HDA_POWER_SAVE
- If(codec-> power_count) hda_call_pm_notify(codec->bus, false);
+#endif ... }
It's not needed. In the case of snd_hda_codec_free(), the only interest is whether pm_notify down was already called for this this particular codec. And this can be checked by codec->power_on flag.
Takashi