Re: Patch for suspend/resume in ICE1724 for Audiotrak Prodigy HD2

[Takashi Iwai <tiwai@suse.de>]

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