From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Chernyshev Subject: Re: Patch for suspend/resume in ICE1724 for Audiotrak Prodigy HD2 Date: Tue, 23 Jun 2009 15:50:44 -0700 Message-ID: <75570e410906231550g47bffd92gfaa24d988bbcdca2@mail.gmail.com> References: <75570e410906192337p1e9aea9cn9edbea987f6ffbb4@mail.gmail.com> <75570e410906221246l3125dega728835cb407ba4@mail.gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from an-out-0708.google.com (an-out-0708.google.com [209.85.132.247]) by alsa0.perex.cz (Postfix) with ESMTP id 1518724563 for ; Wed, 24 Jun 2009 00:50:45 +0200 (CEST) Received: by an-out-0708.google.com with SMTP id c38so188397ana.2 for ; Tue, 23 Jun 2009 15:50:45 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org >> + =A0 =A0 unsigned long pm_saved_route; > > This shouldn't be long but int. =A0Long can be 64bit unnecessarily. Done. However, note that existing code uses results of "inl(ICEMT1724(ice, ROUTE_PLAYBACK))" as both int and long. >> + =A0 =A0 case SNDRV_PCM_TRIGGER_SUSPEND: >> + =A0 =A0 case SNDRV_PCM_TRIGGER_RESUME: >> + =A0 =A0 =A0 =A0 =A0 =A0 break; > > At least, TRIGGER_SUSPEND neees to stop the PCM stream. Consolidated with SNDRV_PCM_TRIGGER_STOP processing. >> + =A0 =A0 outb(VT1724_MULTI_FIFO_ERR, ICEMT1724(ice, DMA_INT_MASK)); >> + > > This is fine, but you need to remove __devinit from this function. Done. >> + =A0 =A0 if (ice->ac97) >> + =A0 =A0 =A0 =A0 =A0 =A0 snd_ac97_suspend(ice->ac97); > > No need for NULL check here. =A0All these check NULL by themselves. Removed all 4 checks (pcm's and ac97). >> + =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock_irq(&ice->reg_lock); >> + =A0 =A0 =A0 =A0 =A0 =A0 snd_vt1724_set_pro_rate(ice, ice->pro_rate_def= ault, 1); >> + =A0 =A0 =A0 =A0 =A0 =A0 spin_lock_irq(&ice->reg_lock); >> + =A0 =A0 } > > 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 Here is the updated patch file: --------------------------------------------- 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-23 14:20:56.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 int pm_saved_route; +#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-23 14:47:23.000000000 -0700 @@ -558,6 +558,7 @@ case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_STOP: + case SNDRV_PCM_TRIGGER_SUSPEND: spin_lock(&ice->reg_lock); old =3D inb(ICEMT1724(ice, DMA_CONTROL)); if (cmd =3D=3D SNDRV_PCM_TRIGGER_START) @@ -568,6 +569,10 @@ spin_unlock(&ice->reg_lock); break; + case SNDRV_PCM_TRIGGER_RESUME: + /* apps will have to restart stream */ + break; + default: return -EINVAL; } @@ -2251,7 +2256,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)); @@ -2266,6 +2271,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; } @@ -2407,6 +2420,8 @@ snd_vt1724_proc_init(ice); synchronize_irq(pci->irq); + card->private_data =3D ice; + err =3D pci_request_regions(pci, "ICE1724"); if (err < 0) { kfree(ice); @@ -2435,14 +2450,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 =3D snd_device_new(card, SNDRV_DEV_LOWLEVEL, ice, &ops); if (err < 0) { snd_vt1724_free(ice); @@ -2626,11 +2633,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 =3D pci_get_drvdata(pci); + struct snd_ice1712 *ice =3D 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 =3D ice->is_spdif_master(ice); + ice->pm_saved_spdif_ctrl =3D inw(ICEMT1724(ice, SPDIF_CTRL)); + ice->pm_saved_spdif_cfg =3D inb(ICEREG1724(ice, SPDIF_CFG)); + ice->pm_saved_route =3D 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 =3D pci_get_drvdata(pci); + struct snd_ice1712 *ice =3D 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 =3D { .name =3D "ICE1724", .id_table =3D snd_vt1724_ids, .probe =3D snd_vt1724_probe, .remove =3D __devexit_p(snd_vt1724_remove), +#ifdef CONFIG_PM + .suspend =3D snd_vt1724_suspend, + .resume =3D 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-23 14:58:35.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[] =3D { 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 =3D 0; i < ARRAY_SIZE(ak4396_inits); i +=3D 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 =3D ice->spec; + int i; + mutex_lock(&ice->gpio_mutex); + ak4396_init(ice); + for (i =3D 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 =3D 0; ice->vt1724 =3D 1; @@ -1112,14 +1140,12 @@ return -ENOMEM; ice->spec =3D spec; - /* initialize ak4396 codec */ - /* reset codec */ - ak4396_write(ice, AK4396_CTRL1, 0x86); - msleep(100); - ak4396_write(ice, AK4396_CTRL1, 0x87); - = - for (i =3D 0; i < ARRAY_SIZE(ak4396_inits); i +=3D 2) - ak4396_write(ice, ak4396_inits[i], ak4396_inits[i+1]); +#ifdef CONFIG_PM + ice->pm_resume =3D &prodigy_hd2_resume; + ice->pm_suspend_enabled =3D 1; +#endif + + ak4396_init(ice); return 0; }