public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
@ 2009-01-28 10:02 Thadeu Lima de Souza Cascardo
  2009-01-28 12:06 ` Takashi Iwai
  0 siblings, 1 reply; 8+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2009-01-28 10:02 UTC (permalink / raw)
  To: tiwai; +Cc: linux-kernel, Thadeu Lima de Souza Cascardo

Check in a quirk list if it should do cold reset when AC97 power saving
is enabled. Some devices do not resume properly when cold reset,
although power saving works OK.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 sound/pci/intel8x0.c |   60 ++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 19d3391..69b3ed8 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2287,23 +2287,23 @@ static void do_ali_reset(struct intel8x0 *chip)
 	iputdword(chip, ICHREG(ALI_INTERRUPTSR), 0x00000000);
 }
 
-static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
-{
-	unsigned long end_time;
-	unsigned int cnt, status, nstatus;
-	
-	/* put logic to right state */
-	/* first clear status bits */
-	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
-	if (chip->device_type == DEVICE_NFORCE)
-		status |= ICH_NVSPINT;
-	cnt = igetdword(chip, ICHREG(GLOB_STA));
-	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
+#ifdef CONFIG_SND_AC97_POWER_SAVE
+static struct snd_pci_quirk ich_chip_reset_mode[] = {
+	SND_PCI_QUIRK(0x1014, 0x051f, "Thinkpad R32", 1),
+	{ } /* end */
+};
 
+static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
+{
+	unsigned int cnt;
 	/* ACLink on, 2 channels */
+
+	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
+		return -EIO;
+
 	cnt = igetdword(chip, ICHREG(GLOB_CNT));
 	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
-#ifdef CONFIG_SND_AC97_POWER_SAVE
+
 	/* do cold reset - the full ac97 powerdown may leave the controller
 	 * in a warm state but actually it cannot communicate with the codec.
 	 */
@@ -2312,22 +2312,50 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 	udelay(10);
 	iputdword(chip, ICHREG(GLOB_CNT), cnt | ICH_AC97COLD);
 	msleep(1);
+	return 0;
+}
 #else
+#define snd_intel8x0_ich_chip_cold_reset(x) 0
+#endif
+
+static int snd_intel8x0_ich_chip_reset(struct intel8x0 *chip)
+{
+	unsigned int cnt;
+	/* ACLink on, 2 channels */
+	cnt = igetdword(chip, ICHREG(GLOB_CNT));
+	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
 	/* finish cold or do warm reset */
 	cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
 	iputdword(chip, ICHREG(GLOB_CNT), cnt);
 	end_time = (jiffies + (HZ / 4)) + 1;
 	do {
 		if ((igetdword(chip, ICHREG(GLOB_CNT)) & ICH_AC97WARM) == 0)
-			goto __ok;
+			return 0;
 		schedule_timeout_uninterruptible(1);
 	} while (time_after_eq(end_time, jiffies));
 	snd_printk(KERN_ERR "AC'97 warm reset still in progress? [0x%x]\n",
 		   igetdword(chip, ICHREG(GLOB_CNT)));
 	return -EIO;
+}
+
+static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
+{
+	unsigned long end_time;
+	unsigned int status, nstatus;
+	int err;
+
+	/* put logic to right state */
+	/* first clear status bits */
+	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
+	if (chip->device_type == DEVICE_NFORCE)
+		status |= ICH_NVSPINT;
+	cnt = igetdword(chip, ICHREG(GLOB_STA));
+	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
+
+	if (snd_intel8x0_ich_chip_cold_reset(chip) < 0)
+		if ((err = snd_intel8x0_ich_chip_reset(chip)) < 0)
+			return err;
 
-      __ok:
-#endif
 	if (probing) {
 		/* wait for any codec ready status.
 		 * Once it becomes ready it should remain ready
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 10:02 [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets Thadeu Lima de Souza Cascardo
@ 2009-01-28 12:06 ` Takashi Iwai
  2009-01-28 12:27   ` Thadeu Lima de Souza Cascardo
  0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2009-01-28 12:06 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel

At Wed, 28 Jan 2009 08:02:36 -0200,
Thadeu Lima de Souza Cascardo wrote:
> 
> Check in a quirk list if it should do cold reset when AC97 power saving
> is enabled. Some devices do not resume properly when cold reset,
> although power saving works OK.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Thanks for the patch.
We can go better further with this patch now :)

The logic is basically fine, but just a few comments...

> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 19d3391..69b3ed8 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
...
> +static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
> +{
> +	unsigned int cnt;
>  	/* ACLink on, 2 channels */
> +
> +	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> +		return -EIO;

I feel it isn't intuitive to return -EIO here.
It'd be more straightforward to call snd_intel8x0_ich_chip_reset()
from here?

	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
		return snd_intl8x0_ich_chip_reset(chip);


> +	if (snd_intel8x0_ich_chip_cold_reset(chip) < 0)
> +		if ((err = snd_intel8x0_ich_chip_reset(chip)) < 0)
> +			return err;

Please make the change checkpatch-clean.

Could you fix and repost?


thanks,

Takashi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 12:06 ` Takashi Iwai
@ 2009-01-28 12:27   ` Thadeu Lima de Souza Cascardo
  2009-01-28 13:02     ` Takashi Iwai
  0 siblings, 1 reply; 8+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2009-01-28 12:27 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On Wed, Jan 28, 2009 at 01:06:20PM +0100, Takashi Iwai wrote:
> At Wed, 28 Jan 2009 08:02:36 -0200,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > Check in a quirk list if it should do cold reset when AC97 power saving
> > is enabled. Some devices do not resume properly when cold reset,
> > although power saving works OK.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> 
> Thanks for the patch.
> We can go better further with this patch now :)
> 
> The logic is basically fine, but just a few comments...
> 
> > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > index 19d3391..69b3ed8 100644
> > --- a/sound/pci/intel8x0.c
> > +++ b/sound/pci/intel8x0.c
> ...
> > +static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
> > +{
> > +	unsigned int cnt;
> >  	/* ACLink on, 2 channels */
> > +
> > +	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> > +		return -EIO;
> 
> I feel it isn't intuitive to return -EIO here.
> It'd be more straightforward to call snd_intel8x0_ich_chip_reset()
> from here?
> 
> 	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> 		return snd_intl8x0_ich_chip_reset(chip);
> 
> 
> > +	if (snd_intel8x0_ich_chip_cold_reset(chip) < 0)
> > +		if ((err = snd_intel8x0_ich_chip_reset(chip)) < 0)
> > +			return err;
> 
> Please make the change checkpatch-clean.
> 
> Could you fix and repost?

I did checkpatch it, but since this style is used in many places in the
intel8x0.c code, I thought I would leave it this way. Should I send a
cleanup patch too?

By the way, I have some very trivial comment typo patches lying around.
I will post them too.

> 
> 
> thanks,
> 
> Takashi

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 12:27   ` Thadeu Lima de Souza Cascardo
@ 2009-01-28 13:02     ` Takashi Iwai
  0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2009-01-28 13:02 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel

At Wed, 28 Jan 2009 10:27:30 -0200,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Wed, Jan 28, 2009 at 01:06:20PM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2009 08:02:36 -0200,
> > Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > Check in a quirk list if it should do cold reset when AC97 power saving
> > > is enabled. Some devices do not resume properly when cold reset,
> > > although power saving works OK.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > 
> > Thanks for the patch.
> > We can go better further with this patch now :)
> > 
> > The logic is basically fine, but just a few comments...
> > 
> > > diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> > > index 19d3391..69b3ed8 100644
> > > --- a/sound/pci/intel8x0.c
> > > +++ b/sound/pci/intel8x0.c
> > ...
> > > +static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
> > > +{
> > > +	unsigned int cnt;
> > >  	/* ACLink on, 2 channels */
> > > +
> > > +	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> > > +		return -EIO;
> > 
> > I feel it isn't intuitive to return -EIO here.
> > It'd be more straightforward to call snd_intel8x0_ich_chip_reset()
> > from here?
> > 
> > 	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> > 		return snd_intl8x0_ich_chip_reset(chip);
> > 
> > 
> > > +	if (snd_intel8x0_ich_chip_cold_reset(chip) < 0)
> > > +		if ((err = snd_intel8x0_ich_chip_reset(chip)) < 0)
> > > +			return err;
> > 
> > Please make the change checkpatch-clean.
> > 
> > Could you fix and repost?
> 
> I did checkpatch it, but since this style is used in many places in the
> intel8x0.c code, I thought I would leave it this way. Should I send a
> cleanup patch too?

Just make your patch checkpatch-clean.  The other parts don't have to
be changed in this case.

> By the way, I have some very trivial comment typo patches lying around.
> I will post them too.

That'll be fine.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
@ 2009-01-28 14:40 Thadeu Lima de Souza Cascardo
  2009-01-28 15:20 ` Takashi Iwai
  0 siblings, 1 reply; 8+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2009-01-28 14:40 UTC (permalink / raw)
  To: tiwai; +Cc: linux-kernel, Thadeu Lima de Souza Cascardo

Check in a quirk list if it should do cold reset when AC97 power saving
is enabled. Some devices do not resume properly when cold reset,
although power saving works OK.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 sound/pci/intel8x0.c |   68 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 19d3391..b37bd26 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -2287,23 +2287,23 @@ static void do_ali_reset(struct intel8x0 *chip)
 	iputdword(chip, ICHREG(ALI_INTERRUPTSR), 0x00000000);
 }
 
-static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
-{
-	unsigned long end_time;
-	unsigned int cnt, status, nstatus;
-	
-	/* put logic to right state */
-	/* first clear status bits */
-	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
-	if (chip->device_type == DEVICE_NFORCE)
-		status |= ICH_NVSPINT;
-	cnt = igetdword(chip, ICHREG(GLOB_STA));
-	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
+#ifdef CONFIG_SND_AC97_POWER_SAVE
+static struct snd_pci_quirk ich_chip_reset_mode[] = {
+	SND_PCI_QUIRK(0x1014, 0x051f, "Thinkpad R32", 1),
+	{ } /* end */
+};
 
+static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
+{
+	unsigned int cnt;
 	/* ACLink on, 2 channels */
+
+	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
+		return -EIO;
+
 	cnt = igetdword(chip, ICHREG(GLOB_CNT));
 	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
-#ifdef CONFIG_SND_AC97_POWER_SAVE
+
 	/* do cold reset - the full ac97 powerdown may leave the controller
 	 * in a warm state but actually it cannot communicate with the codec.
 	 */
@@ -2312,22 +2312,58 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
 	udelay(10);
 	iputdword(chip, ICHREG(GLOB_CNT), cnt | ICH_AC97COLD);
 	msleep(1);
+	return 0;
+}
+#define snd_intel8x0_ich_chip_can_cold_reset(chip) \
+	(!snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
 #else
+#define snd_intel8x0_ich_chip_cold_reset(x) do { } while (0)
+#define snd_intel8x0_ich_chip_can_cold_reset(chip) (0)
+#endif
+
+static int snd_intel8x0_ich_chip_reset(struct intel8x0 *chip)
+{
+	unsigned long end_time;
+	unsigned int cnt;
+	/* ACLink on, 2 channels */
+	cnt = igetdword(chip, ICHREG(GLOB_CNT));
+	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
 	/* finish cold or do warm reset */
 	cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
 	iputdword(chip, ICHREG(GLOB_CNT), cnt);
 	end_time = (jiffies + (HZ / 4)) + 1;
 	do {
 		if ((igetdword(chip, ICHREG(GLOB_CNT)) & ICH_AC97WARM) == 0)
-			goto __ok;
+			return 0;
 		schedule_timeout_uninterruptible(1);
 	} while (time_after_eq(end_time, jiffies));
 	snd_printk(KERN_ERR "AC'97 warm reset still in progress? [0x%x]\n",
 		   igetdword(chip, ICHREG(GLOB_CNT)));
 	return -EIO;
+}
+
+static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
+{
+	unsigned long end_time;
+	unsigned int status, nstatus;
+	unsigned int cnt;
+	int err;
+
+	/* put logic to right state */
+	/* first clear status bits */
+	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
+	if (chip->device_type == DEVICE_NFORCE)
+		status |= ICH_NVSPINT;
+	cnt = igetdword(chip, ICHREG(GLOB_STA));
+	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
+
+	if (snd_intel8x0_ich_chip_can_cold_reset(chip))
+		err = snd_intel8x0_ich_chip_cold_reset(chip);
+	else
+		err = snd_intel8x0_ich_chip_reset(chip);
+	if (err < 0)
+		return err;
 
-      __ok:
-#endif
 	if (probing) {
 		/* wait for any codec ready status.
 		 * Once it becomes ready it should remain ready
-- 
1.6.0.6


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 14:40 Thadeu Lima de Souza Cascardo
@ 2009-01-28 15:20 ` Takashi Iwai
  2009-01-28 15:52   ` Thadeu Lima de Souza Cascardo
  0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2009-01-28 15:20 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel

At Wed, 28 Jan 2009 12:40:42 -0200,
Thadeu Lima de Souza Cascardo wrote:
> 
> Check in a quirk list if it should do cold reset when AC97 power saving
> is enabled. Some devices do not resume properly when cold reset,
> although power saving works OK.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Thanks, applied now.

... and soon I found a build error when CONFIG_SND_AC97_POWERSAVE=n.
Fixed, too :)


Takashi

> ---
>  sound/pci/intel8x0.c |   68 ++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
> index 19d3391..b37bd26 100644
> --- a/sound/pci/intel8x0.c
> +++ b/sound/pci/intel8x0.c
> @@ -2287,23 +2287,23 @@ static void do_ali_reset(struct intel8x0 *chip)
>  	iputdword(chip, ICHREG(ALI_INTERRUPTSR), 0x00000000);
>  }
>  
> -static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> -{
> -	unsigned long end_time;
> -	unsigned int cnt, status, nstatus;
> -	
> -	/* put logic to right state */
> -	/* first clear status bits */
> -	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
> -	if (chip->device_type == DEVICE_NFORCE)
> -		status |= ICH_NVSPINT;
> -	cnt = igetdword(chip, ICHREG(GLOB_STA));
> -	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
> +#ifdef CONFIG_SND_AC97_POWER_SAVE
> +static struct snd_pci_quirk ich_chip_reset_mode[] = {
> +	SND_PCI_QUIRK(0x1014, 0x051f, "Thinkpad R32", 1),
> +	{ } /* end */
> +};
>  
> +static int snd_intel8x0_ich_chip_cold_reset(struct intel8x0 *chip)
> +{
> +	unsigned int cnt;
>  	/* ACLink on, 2 channels */
> +
> +	if (snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
> +		return -EIO;
> +
>  	cnt = igetdword(chip, ICHREG(GLOB_CNT));
>  	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
> -#ifdef CONFIG_SND_AC97_POWER_SAVE
> +
>  	/* do cold reset - the full ac97 powerdown may leave the controller
>  	 * in a warm state but actually it cannot communicate with the codec.
>  	 */
> @@ -2312,22 +2312,58 @@ static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
>  	udelay(10);
>  	iputdword(chip, ICHREG(GLOB_CNT), cnt | ICH_AC97COLD);
>  	msleep(1);
> +	return 0;
> +}
> +#define snd_intel8x0_ich_chip_can_cold_reset(chip) \
> +	(!snd_pci_quirk_lookup(chip->pci, ich_chip_reset_mode))
>  #else
> +#define snd_intel8x0_ich_chip_cold_reset(x) do { } while (0)
> +#define snd_intel8x0_ich_chip_can_cold_reset(chip) (0)
> +#endif
> +
> +static int snd_intel8x0_ich_chip_reset(struct intel8x0 *chip)
> +{
> +	unsigned long end_time;
> +	unsigned int cnt;
> +	/* ACLink on, 2 channels */
> +	cnt = igetdword(chip, ICHREG(GLOB_CNT));
> +	cnt &= ~(ICH_ACLINK | ICH_PCM_246_MASK);
>  	/* finish cold or do warm reset */
>  	cnt |= (cnt & ICH_AC97COLD) == 0 ? ICH_AC97COLD : ICH_AC97WARM;
>  	iputdword(chip, ICHREG(GLOB_CNT), cnt);
>  	end_time = (jiffies + (HZ / 4)) + 1;
>  	do {
>  		if ((igetdword(chip, ICHREG(GLOB_CNT)) & ICH_AC97WARM) == 0)
> -			goto __ok;
> +			return 0;
>  		schedule_timeout_uninterruptible(1);
>  	} while (time_after_eq(end_time, jiffies));
>  	snd_printk(KERN_ERR "AC'97 warm reset still in progress? [0x%x]\n",
>  		   igetdword(chip, ICHREG(GLOB_CNT)));
>  	return -EIO;
> +}
> +
> +static int snd_intel8x0_ich_chip_init(struct intel8x0 *chip, int probing)
> +{
> +	unsigned long end_time;
> +	unsigned int status, nstatus;
> +	unsigned int cnt;
> +	int err;
> +
> +	/* put logic to right state */
> +	/* first clear status bits */
> +	status = ICH_RCS | ICH_MCINT | ICH_POINT | ICH_PIINT;
> +	if (chip->device_type == DEVICE_NFORCE)
> +		status |= ICH_NVSPINT;
> +	cnt = igetdword(chip, ICHREG(GLOB_STA));
> +	iputdword(chip, ICHREG(GLOB_STA), cnt & status);
> +
> +	if (snd_intel8x0_ich_chip_can_cold_reset(chip))
> +		err = snd_intel8x0_ich_chip_cold_reset(chip);
> +	else
> +		err = snd_intel8x0_ich_chip_reset(chip);
> +	if (err < 0)
> +		return err;
>  
> -      __ok:
> -#endif
>  	if (probing) {
>  		/* wait for any codec ready status.
>  		 * Once it becomes ready it should remain ready
> -- 
> 1.6.0.6
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 15:20 ` Takashi Iwai
@ 2009-01-28 15:52   ` Thadeu Lima de Souza Cascardo
  2009-01-29  7:37     ` Takashi Iwai
  0 siblings, 1 reply; 8+ messages in thread
From: Thadeu Lima de Souza Cascardo @ 2009-01-28 15:52 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

On Wed, Jan 28, 2009 at 04:20:12PM +0100, Takashi Iwai wrote:
> At Wed, 28 Jan 2009 12:40:42 -0200,
> Thadeu Lima de Souza Cascardo wrote:
> > 
> > Check in a quirk list if it should do cold reset when AC97 power saving
> > is enabled. Some devices do not resume properly when cold reset,
> > although power saving works OK.
> > 
> > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> 
> Thanks, applied now.
> 
> ... and soon I found a build error when CONFIG_SND_AC97_POWERSAVE=n.
> Fixed, too :)


Sorry for this one. I've cleaned the modules and built them again with a
changed config. Must have done something wrong in the build. Thanks for
the fix.

Regarding the warning fix in 92aab0a0, it is related to this commit:
248c982a, which was proposed by you during our previous discussion, and
didn't really fix my problem.

I've been watching your tree ever since and this patch has been in your
master branch since then, perhaps awaiting some conclusion about this
issue.

Do you think it solves any problems for other device models? In my case,
if the cold reset was done, this would simply not work.

Looking closer to the patch now, I see a point in keeping it, which may
be the reason you did so. It will make the resume faster in those cases
the init_bits are different from what the driver though would be there.

In this case, I think the log message should be more clear about that,
since it seems to indicate that the commit makes the driver wait for
more codecs (all codec slots) than before the commit.

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets
  2009-01-28 15:52   ` Thadeu Lima de Souza Cascardo
@ 2009-01-29  7:37     ` Takashi Iwai
  0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2009-01-29  7:37 UTC (permalink / raw)
  To: Thadeu Lima de Souza Cascardo; +Cc: linux-kernel

At Wed, 28 Jan 2009 13:52:59 -0200,
Thadeu Lima de Souza Cascardo wrote:
> 
> On Wed, Jan 28, 2009 at 04:20:12PM +0100, Takashi Iwai wrote:
> > At Wed, 28 Jan 2009 12:40:42 -0200,
> > Thadeu Lima de Souza Cascardo wrote:
> > > 
> > > Check in a quirk list if it should do cold reset when AC97 power saving
> > > is enabled. Some devices do not resume properly when cold reset,
> > > although power saving works OK.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > 
> > Thanks, applied now.
> > 
> > ... and soon I found a build error when CONFIG_SND_AC97_POWERSAVE=n.
> > Fixed, too :)
> 
> 
> Sorry for this one. I've cleaned the modules and built them again with a
> changed config. Must have done something wrong in the build. Thanks for
> the fix.
> 
> Regarding the warning fix in 92aab0a0, it is related to this commit:
> 248c982a, which was proposed by you during our previous discussion, and
> didn't really fix my problem.

I can't find these commit ids.  Which tree are you referring to?


thanks,

Takashi

> I've been watching your tree ever since and this patch has been in your
> master branch since then, perhaps awaiting some conclusion about this
> issue.
> 
> Do you think it solves any problems for other device models? In my case,
> if the cold reset was done, this would simply not work.
> 
> Looking closer to the patch now, I see a point in keeping it, which may
> be the reason you did so. It will make the resume faster in those cases
> the init_bits are different from what the driver though would be there.
> 
> In this case, I think the log message should be more clear about that,
> since it seems to indicate that the commit makes the driver wait for
> more codecs (all codec slots) than before the commit.
> 
> Regards,
> Cascardo.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2009-01-29  7:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-01-28 10:02 [PATCH] ALSA: Don't cold reset AC97 codecs in some ICH chipsets Thadeu Lima de Souza Cascardo
2009-01-28 12:06 ` Takashi Iwai
2009-01-28 12:27   ` Thadeu Lima de Souza Cascardo
2009-01-28 13:02     ` Takashi Iwai
  -- strict thread matches above, loose matches on Subject: below --
2009-01-28 14:40 Thadeu Lima de Souza Cascardo
2009-01-28 15:20 ` Takashi Iwai
2009-01-28 15:52   ` Thadeu Lima de Souza Cascardo
2009-01-29  7:37     ` Takashi Iwai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox