At Wed, 24 Jun 2009 23:15:50 -0700, Igor Chernyshev wrote:
Takashi,
below is the same patch based on the latest snapshot.
Your MUA (gmail?) broke the embedded patch by line-breaking, so it can't be applied.
Either fix your MUA setup not to do that or use an attachment if it's difficult.
thanks,
Takashi
Thanks, Igor
On Wed, Jun 24, 2009 at 3:13 AM, Takashi Iwaitiwai@suse.de wrote:
At Tue, 23 Jun 2009 15:50:44 -0700, Igor Chernyshev wrote:
- unsigned long pm_saved_route;
This shouldn't be long but int. Long can be 64bit unnecessarily.
Done. However, note that existing code uses results of "inl(ICEMT1724(ice, ROUTE_PLAYBACK))" as both int and long.
- case SNDRV_PCM_TRIGGER_SUSPEND:
- case SNDRV_PCM_TRIGGER_RESUME:
- break;
At least, TRIGGER_SUSPEND neees to stop the PCM stream.
Consolidated with SNDRV_PCM_TRIGGER_STOP processing.
- outb(VT1724_MULTI_FIFO_ERR, ICEMT1724(ice, DMA_INT_MASK));
This is fine, but you need to remove __devinit from this function.
Done.
- if (ice->ac97)
- snd_ac97_suspend(ice->ac97);
No need for NULL check here. All these check NULL by themselves.
Removed all 4 checks (pcm's and ac97).
- 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...
Removed locking in "resume" only.
Last but not least, try to run $LINUX/scripts/checkpatch.pl to your patch once and fix the issues suggested there.
Thanks for the pointer. Fixed 3 code style problems. It also complains about missing "signed off" line, but I hope that line here will suffice:
Signed-off-by: Igor Chernyshev <igor.ch75+alsa at gmail.com>
Thanks, this looks better. But, I got some rejects when applying to the latest tree, and I guess your patch is based on 1.0.20 release.
Please try to rebase to the latest alsa-driver snapshot and repost the patch, either against sound git tree or alsa-driver-snapshot tarball below: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git ftp://ftp.kernel.org/pub/linux/kernel/people/tiwai/snapshot/alsa-driver-snapshot.tar.gz
thanks,
Takashi
diff -uN alsa-driver.orig/alsa-kernel/pci/ice1712/ice1712.h alsa-driver/alsa-kernel/pci/ice1712/ice1712.h --- alsa-driver.orig/alsa-kernel/pci/ice1712/ice1712.h 2009-06-24 15:05:06.000000000 -0700 +++ alsa-driver/alsa-kernel/pci/ice1712/ice1712.h 2009-06-24 21:51:53.000000000 -0700 @@ -379,6 +379,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 int pm_saved_route;
+#endif };
diff -uN alsa-driver.orig/alsa-kernel/pci/ice1712/ice1724.c alsa-driver/alsa-kernel/pci/ice1712/ice1724.c --- alsa-driver.orig/alsa-kernel/pci/ice1712/ice1724.c 2009-06-24 15:05:06.000000000 -0700 +++ alsa-driver/alsa-kernel/pci/ice1712/ice1724.c 2009-06-24 22:13:25.000000000 -0700 @@ -560,6 +560,7 @@
case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_STOP:
- case SNDRV_PCM_TRIGGER_SUSPEND: spin_lock(&ice->reg_lock); old = inb(ICEMT1724(ice, DMA_CONTROL)); if (cmd == SNDRV_PCM_TRIGGER_START)
@@ -570,6 +571,10 @@ spin_unlock(&ice->reg_lock); break;
- case SNDRV_PCM_TRIGGER_RESUME:
/* apps will have to restart stream */break;- default: return -EINVAL; }
@@ -2272,7 +2277,7 @@ msleep(10); }
-static int __devinit snd_vt1724_chip_init(struct snd_ice1712 *ice) +static int snd_vt1724_chip_init(struct snd_ice1712 *ice) { outb(ice->eeprom.data[ICE_EEP2_SYSCONF], ICEREG1724(ice, SYS_CFG)); outb(ice->eeprom.data[ICE_EEP2_ACLINK], ICEREG1724(ice, AC97_CFG)); @@ -2287,6 +2292,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));
- return 0;
}
@@ -2431,6 +2444,8 @@ snd_vt1724_proc_init(ice); synchronize_irq(pci->irq);
- card->private_data = ice;
- err = pci_request_regions(pci, "ICE1724"); if (err < 0) { kfree(ice);
@@ -2459,14 +2474,6 @@ return -EIO; }
- /* 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));
- err = snd_device_new(card, SNDRV_DEV_LOWLEVEL, ice, &ops); if (err < 0) { snd_vt1724_free(ice);
@@ -2650,11 +2657,96 @@ pci_set_drvdata(pci, NULL); }
+#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);
- snd_pcm_suspend_all(ice->pcm);
- snd_pcm_suspend_all(ice->pcm_pro);
- snd_pcm_suspend_all(ice->pcm_ds);
- snd_ac97_suspend(ice->ac97);
- 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);- if (ice->pm_saved_is_spdif_master) {
/* switching to external clock via SPDIF */ice->set_spdif_clock(ice);- } else {
/* internal on-card clock */snd_vt1724_set_pro_rate(ice, ice->pro_rate_default, 1);- }
- 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));
- 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.orig/alsa-kernel/pci/ice1712/prodigy_hifi.c alsa-driver/alsa-kernel/pci/ice1712/prodigy_hifi.c --- alsa-driver.orig/alsa-kernel/pci/ice1712/prodigy_hifi.c 2009-06-24 15:05:06.000000000 -0700 +++ alsa-driver/alsa-kernel/pci/ice1712/prodigy_hifi.c 2009-06-24 22:00:04.000000000 -0700 @@ -1077,7 +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 */ @@ -1087,9 +1087,37 @@ 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;
}