At Mon, 22 Jun 2009 12:46:48 -0700, Igor Chernyshev wrote:
Hi Takashi,
here is the patch for:
Supported suspend/resume in Audiotrak Prodigy HD2
Please, let me know if you want some special testing, patch formatting, etc on my part. So far I did suspend-resume with MythTV playing AVI and DVD through RCA. In original ALSA 1.0.20, MythTV playback upon resume would start ~20% faster than before suspend, volume would go to max (or above? given how awfully loud the noises were) and there would be ton of noises. After the change, resume gives clean playback in MythTV. I suspect that I may not have covered different scenarios (e.g. driver implies there may be 3 pcm's, ac97, and mic) in my testing.
Thanks, Igor
Here is the part I know about : : )))
Signed-off-by: Igor Chernyshev <igor.ch75+alsa at gmail.com>
Thanks for the patch. Just looking through the changes, and it looks mostly OK.
Small comments.
diff -uN alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/ice1712.h alsa-driver-1.0.20/alsa-kernel/pci/ice1712/ice1712.h --- alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/ice1712.h 2009-05-06 00:06:04.000000000 -0700 +++ alsa-driver-1.0.20/alsa-kernel/pci/ice1712/ice1712.h 2009-06-19 22:27:12.000000000 -0700 @@ -378,6 +378,15 @@ unsigned char (*set_mclk)(struct snd_ice1712 *ice, unsigned int rate); void (*set_spdif_clock)(struct snd_ice1712 *ice);
+#ifdef CONFIG_PM
- int (*pm_suspend)(struct snd_ice1712 *);
- int (*pm_resume)(struct snd_ice1712 *);
- int pm_suspend_enabled:1;
- int pm_saved_is_spdif_master:1;
- unsigned int pm_saved_spdif_ctrl;
- unsigned char pm_saved_spdif_cfg;
- unsigned long pm_saved_route;
This shouldn't be long but int. Long can be 64bit unnecessarily.
+#endif };
diff -uN alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/ice1724.c alsa-driver-1.0.20/alsa-kernel/pci/ice1712/ice1724.c --- alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/ice1724.c 2009-05-06 00:06:04.000000000 -0700 +++ alsa-driver-1.0.20/alsa-kernel/pci/ice1712/ice1724.c 2009-06-22 12:13:26.000000000 -0700 @@ -568,6 +568,10 @@ spin_unlock(&ice->reg_lock); break;
- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_RESUME:
break;
At least, TRIGGER_SUSPEND neees to stop the PCM stream.
default: return -EINVAL; } @@ -2266,6 +2270,14 @@
outb(0, ICEREG1724(ice, POWERDOWN));
- /* MPU_RX and TX irq masks are cleared later dynamically */
- outb(VT1724_IRQ_MPU_RX | VT1724_IRQ_MPU_TX , ICEREG1724(ice, IRQMASK));
- /* don't handle FIFO overrun/underruns (just yet),
* since they cause machine lockups*/- outb(VT1724_MULTI_FIFO_ERR, ICEMT1724(ice, DMA_INT_MASK));
This is fine, but you need to remove __devinit from this function.
+#ifdef CONFIG_PM +static int snd_vt1724_suspend(struct pci_dev *pci, pm_message_t state) +{
- struct snd_card *card = pci_get_drvdata(pci);
- struct snd_ice1712 *ice = card->private_data;
- if (!ice->pm_suspend_enabled)
return 0;- snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- if (ice->pcm)
snd_pcm_suspend_all(ice->pcm);- if (ice->pcm_pro)
snd_pcm_suspend_all(ice->pcm_pro);- if (ice->pcm_ds)
snd_pcm_suspend_all(ice->pcm_ds);- if (ice->ac97)
snd_ac97_suspend(ice->ac97);
No need for NULL check here. All these check NULL by themselves.
- spin_lock_irq(&ice->reg_lock);
- ice->pm_saved_is_spdif_master = ice->is_spdif_master(ice);
- ice->pm_saved_spdif_ctrl = inw(ICEMT1724(ice, SPDIF_CTRL));
- ice->pm_saved_spdif_cfg = inb(ICEREG1724(ice, SPDIF_CFG));
- ice->pm_saved_route = inl(ICEMT1724(ice, ROUTE_PLAYBACK));
- spin_unlock_irq(&ice->reg_lock);
- if (ice->pm_suspend)
ice->pm_suspend(ice);- pci_disable_device(pci);
- pci_save_state(pci);
- pci_set_power_state(pci, pci_choose_state(pci, state));
- return 0;
+}
+static int snd_vt1724_resume(struct pci_dev *pci) +{
- struct snd_card *card = pci_get_drvdata(pci);
- struct snd_ice1712 *ice = card->private_data;
- if (!ice->pm_suspend_enabled)
return 0;- pci_set_power_state(pci, PCI_D0);
- pci_restore_state(pci);
- if (pci_enable_device(pci) < 0) {
snd_card_disconnect(card);return -EIO;- }
- pci_set_master(pci);
- snd_vt1724_chip_reset(ice);
- if (snd_vt1724_chip_init(ice) < 0) {
snd_card_disconnect(card);return -EIO;- }
- if (ice->pm_resume)
ice->pm_resume(ice);- spin_lock_irq(&ice->reg_lock);
- if (ice->pm_saved_is_spdif_master) {
/* switching to external clock via SPDIF */ice->set_spdif_clock(ice);- } else {
/* internal on-card clock */spin_unlock_irq(&ice->reg_lock);snd_vt1724_set_pro_rate(ice, ice->pro_rate_default, 1);spin_lock_irq(&ice->reg_lock);- }
These are basically no need to protect with spinlock, at least, in this function...
- update_spdif_bits(ice, ice->pm_saved_spdif_ctrl);
- outb(ice->pm_saved_spdif_cfg, ICEREG1724(ice, SPDIF_CFG));
- outl(ice->pm_saved_route, ICEMT1724(ice, ROUTE_PLAYBACK));
- spin_unlock_irq(&ice->reg_lock);
- if (ice->ac97)
snd_ac97_resume(ice->ac97);- snd_power_change_state(card, SNDRV_CTL_POWER_D0);
- return 0;
+} +#endif
static struct pci_driver driver = { .name = "ICE1724", .id_table = snd_vt1724_ids, .probe = snd_vt1724_probe, .remove = __devexit_p(snd_vt1724_remove), +#ifdef CONFIG_PM
- .suspend = snd_vt1724_suspend,
- .resume = snd_vt1724_resume,
+#endif };
static int __init alsa_card_ice1724_init(void) diff -uN alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/prodigy_hifi.c alsa-driver-1.0.20/alsa-kernel/pci/ice1712/prodigy_hifi.c --- alsa-driver-1.0.20.orig/alsa-kernel/pci/ice1712/prodigy_hifi.c 2009-05-06 00:06:04.000000000 -0700 +++ alsa-driver-1.0.20/alsa-kernel/pci/ice1712/prodigy_hifi.c 2009-06-22 12:06:28.000000000 -0700 @@ -1077,8 +1077,7 @@ /*
- initialize the chip
*/ -static int __devinit prodigy_hd2_init(struct snd_ice1712 *ice) -{ +static void ak4396_init(struct snd_ice1712 *ice) { static unsigned short ak4396_inits[] = { AK4396_CTRL1, 0x87, /* I2S Normal Mode, 24 bit */ AK4396_CTRL2, 0x02, @@ -1087,9 +1086,38 @@ AK4396_RCH_ATT, 0x00, };
- struct prodigy_hifi_spec *spec; unsigned int i;
- /* initialize ak4396 codec */
- /* reset codec */
- ak4396_write(ice, AK4396_CTRL1, 0x86);
- msleep(100);
- ak4396_write(ice, AK4396_CTRL1, 0x87);
- for (i = 0; i < ARRAY_SIZE(ak4396_inits); i += 2)
ak4396_write(ice, ak4396_inits[i], ak4396_inits[i+1]);+}
+#ifdef CONFIG_PM +static int __devinit prodigy_hd2_resume(struct snd_ice1712 *ice) +{
- /* initialize ak4396 codec and restore previous mixer volumes */
- struct prodigy_hifi_spec *spec = ice->spec;
- int i;
- mutex_lock(&ice->gpio_mutex);
- ak4396_init(ice);
- for (i = 0; i < 2; i++) {
ak4396_write(ice, AK4396_LCH_ATT + i, spec->vol[i] & 0xff);- }
- mutex_unlock(&ice->gpio_mutex);
- return 0;
+} +#endif
+static int __devinit prodigy_hd2_init(struct snd_ice1712 *ice) +{
- struct prodigy_hifi_spec *spec;
- ice->vt1720 = 0; ice->vt1724 = 1;
@@ -1112,14 +1140,12 @@ return -ENOMEM; ice->spec = spec;
- /* initialize ak4396 codec */
- /* reset codec */
- ak4396_write(ice, AK4396_CTRL1, 0x86);
- msleep(100);
- ak4396_write(ice, AK4396_CTRL1, 0x87);
- for (i = 0; i < ARRAY_SIZE(ak4396_inits); i += 2)
ak4396_write(ice, ak4396_inits[i], ak4396_inits[i+1]);+#ifdef CONFIG_PM
- ice->pm_resume = &prodigy_hd2_resume;
- ice->pm_suspend_enabled = 1;
+#endif
ak4396_init(ice);
return 0;
}
Last but not least, try to run $LINUX/scripts/checkpatch.pl to your patch once and fix the issues suggested there.
thanks,
Takashi