Patchwork [Maverick,SRU] : Acer aspire 7736 - Playback not working

login
register
mail settings
Submitter David Henningsson
Date Nov. 11, 2010, 10:16 a.m.
Message ID <4CDBC26E.7010103@canonical.com>
Download mbox | patch
Permalink /patch/70789/
State Rejected
Delegated to: Brad Figg
Headers show

Comments

David Henningsson - Nov. 11, 2010, 10:16 a.m.
BugLink: https://launchpad.net/bugs/617647

SRU Justification:

Impact: A regression in Maverick caused audio playback to stop working 
on Acer aspire 7736.

Fix: Require some new low-impact quirk infrastructure plus the actual quirk.

Testcase: Try playing back through internal speakers and headphones on 
the machine.

Both patches has been accepted upstream (as commits 90622917 and 
c3d226ab) but it does not make sense to send to stable@kernel.org since 
2.6.35 isn't maintained anymore.
Leann Ogasawara - Nov. 11, 2010, 6:19 p.m.
On Thu, 2010-11-11 at 11:16 +0100, David Henningsson wrote:
> BugLink: https://launchpad.net/bugs/617647
> 
> SRU Justification:
> 
> Impact: A regression in Maverick caused audio playback to stop working 
> on Acer aspire 7736.
> 
> Fix: Require some new low-impact quirk infrastructure plus the actual quirk.
> 
> Testcase: Try playing back through internal speakers and headphones on 
> the machine.
> 
> Both patches has been accepted upstream (as commits 90622917 and 
> c3d226ab) but it does not make sense to send to stable@kernel.org since 
> 2.6.35 isn't maintained anymore.

Looks reasonable and there is positive test confirmation in the bug
report.  Only minor nit picks would be that I'd also add the BugLink to
Patch 1/2 and I'd also add references to the upstream commits in both
patches.  But I'll leave it to Brad/Steve to decide if they want to fix
that up by hand when applying.

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>
Stefan Bader - Nov. 12, 2010, 8:33 a.m.
On 11/11/2010 07:19 PM, Leann Ogasawara wrote:
> On Thu, 2010-11-11 at 11:16 +0100, David Henningsson wrote:
>> BugLink: https://launchpad.net/bugs/617647
>>
>> SRU Justification:
>>
>> Impact: A regression in Maverick caused audio playback to stop working 
>> on Acer aspire 7736.
>>
>> Fix: Require some new low-impact quirk infrastructure plus the actual quirk.
>>
>> Testcase: Try playing back through internal speakers and headphones on 
>> the machine.
>>
>> Both patches has been accepted upstream (as commits 90622917 and 
>> c3d226ab) but it does not make sense to send to stable@kernel.org since 
>> 2.6.35 isn't maintained anymore.
> 
> Looks reasonable and there is positive test confirmation in the bug
> report.  Only minor nit picks would be that I'd also add the BugLink to
> Patch 1/2 and I'd also add references to the upstream commits in both
> patches.  But I'll leave it to Brad/Steve to decide if they want to fix
> that up by hand when applying.
> 
> Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>
> 
> 
As Leann said, the BugLink and upstream references. Also another nitpick, just
because .35 is your target and is not supported anymore, it may be nice as a
courtesy to send them there for .36. ;-)

Acked-by: Stefan Bader <stefan.bader@canonical.com>
David Henningsson - Nov. 12, 2010, 9:12 a.m.
On 2010-11-11 19:19, Leann Ogasawara wrote:
> On Thu, 2010-11-11 at 11:16 +0100, David Henningsson wrote:
>> BugLink: https://launchpad.net/bugs/617647
>>
>> SRU Justification:
>>
>> Impact: A regression in Maverick caused audio playback to stop working
>> on Acer aspire 7736.
>>
>> Fix: Require some new low-impact quirk infrastructure plus the actual quirk.
>>
>> Testcase: Try playing back through internal speakers and headphones on
>> the machine.
>>
>> Both patches has been accepted upstream (as commits 90622917 and
>> c3d226ab) but it does not make sense to send to stable@kernel.org since
>> 2.6.35 isn't maintained anymore.
>
> Looks reasonable and there is positive test confirmation in the bug
> report.  Only minor nit picks would be that I'd also add the BugLink to
> Patch 1/2 and I'd also add references to the upstream commits in both
> patches.  But I'll leave it to Brad/Steve to decide if they want to fix
> that up by hand when applying.
>
> Acked-by: Leann Ogasawara<leann.ogasawara@canonical.com>
>

How about then I'll instead ask you to cherrypick commits 90622917 and
c3d226ab from linux-2.6? That way you'll get upstream references 
automatically, I assume.

As for the buglink, I don't agree - you have the buglink in the second 
one only (both in c3d226ab and in the patch posted), which should be 
correct. That way you don't risk the bug appear to be fixed by only 
applying the first one by mistake. (And for the sake of argument, the 
second one won't even apply, or at least not build, without the first 
one, so if you do the mistake the other way, you'll find it.)
Stefan Bader - Nov. 12, 2010, 11:09 a.m.
On 11/12/2010 10:12 AM, David Henningsson wrote:
> On 2010-11-11 19:19, Leann Ogasawara wrote:
>> On Thu, 2010-11-11 at 11:16 +0100, David Henningsson wrote:
>>> BugLink: https://launchpad.net/bugs/617647
>>>
>>> SRU Justification:
>>>
>>> Impact: A regression in Maverick caused audio playback to stop working
>>> on Acer aspire 7736.
>>>
>>> Fix: Require some new low-impact quirk infrastructure plus the actual quirk.
>>>
>>> Testcase: Try playing back through internal speakers and headphones on
>>> the machine.
>>>
>>> Both patches has been accepted upstream (as commits 90622917 and
>>> c3d226ab) but it does not make sense to send to stable@kernel.org since
>>> 2.6.35 isn't maintained anymore.
>>
>> Looks reasonable and there is positive test confirmation in the bug
>> report.  Only minor nit picks would be that I'd also add the BugLink to
>> Patch 1/2 and I'd also add references to the upstream commits in both
>> patches.  But I'll leave it to Brad/Steve to decide if they want to fix
>> that up by hand when applying.
>>
>> Acked-by: Leann Ogasawara<leann.ogasawara@canonical.com>
>>
> 
> How about then I'll instead ask you to cherrypick commits 90622917 and
> c3d226ab from linux-2.6? That way you'll get upstream references 
> automatically, I assume.
> 
But still would then miss your s-o-b for the sru (to document who submitted it
for that). It is quite similar to what you would do for submitting stable
patches to Greg. He also wants a "commit <sha1> upstream" added to the patch.
For our srus you alternatively could extend the s-o-b area by

(cherry-picked from <sha1> upstream)
Signed-of-by: <you>

> As for the buglink, I don't agree - you have the buglink in the second 
> one only (both in c3d226ab and in the patch posted), which should be 
> correct. That way you don't risk the bug appear to be fixed by only 
> applying the first one by mistake. (And for the sake of argument, the 
> second one won't even apply, or at least not build, without the first 
> one, so if you do the mistake the other way, you'll find it.)
> 
No it isn't. All patches added for SRU need to reference the bug for which they
are needed. Duplicates are allowed and ok. But the SRU team will usually reject
updates when they contain patches without reference. And they are not keen on
reading patches to find out about any dependencies.

-Stefan
Tim Gardner - Nov. 19, 2010, 2:37 p.m.
On 11/11/2010 03:16 AM, David Henningsson wrote:
> BugLink: https://launchpad.net/bugs/617647
>
> SRU Justification:
>
> Impact: A regression in Maverick caused audio playback to stop working
> on Acer aspire 7736.
>
> Fix: Require some new low-impact quirk infrastructure plus the actual
> quirk.
>
> Testcase: Try playing back through internal speakers and headphones on
> the machine.
>
> Both patches has been accepted upstream (as commits 90622917 and
> c3d226ab) but it does not make sense to send to stable@kernel.org since
> 2.6.35 isn't maintained anymore.
>

pulled and pushed to master-next with minor commit log edits.

Patch

From cebe004745a54b5dec1cdfa2f8ce9708c301941a Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 14 Oct 2010 15:42:08 +0200
Subject: [PATCH 2/2] ALSA: HDA: Apply SKU override for Acer aspire 7736z

BugLink: http://launchpad.net/bugs/617647

The current SKU value disables playback, so ignore the SKU value.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_realtek.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index f826bad..1c82c59 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -10627,6 +10627,7 @@  static struct alc_config_preset alc882_presets[] = {
 enum {
 	PINFIX_ABIT_AW9D_MAX,
 	PINFIX_PB_M5210,
+	PINFIX_ACER_ASPIRE_7736,
 };
 
 static const struct alc_fixup alc882_fixups[] = {
@@ -10644,11 +10645,15 @@  static const struct alc_fixup alc882_fixups[] = {
 			{}
 		}
 	},
+	[PINFIX_ACER_ASPIRE_7736] = {
+		.sku = ALC_FIXUP_SKU_IGNORE,
+	},
 };
 
 static struct snd_pci_quirk alc882_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x1025, 0x0155, "Packard-Bell M5120", PINFIX_PB_M5210),
 	SND_PCI_QUIRK(0x147b, 0x107a, "Abit AW9D-MAX", PINFIX_ABIT_AW9D_MAX),
+	SND_PCI_QUIRK(0x1025, 0x0296, "Acer Aspire 7736z", PINFIX_ACER_ASPIRE_7736),
 	{}
 };
 
-- 
1.7.1