RE: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

RE: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Andrew Chew]

I sincerely apologize about the mangled patch.  I'll be more careful
next time (and check my mailer settings).

The #ifdef was for consistency (I noticed that there were other IDs
similarly defined in intel8x0.c).  I don't see why we'd need it, either.
We should probably remove PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO and
PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO #defines from intel8x0.c as well, as
they're similarly redundant.  For that matter, why not remove all of the
PCI_DEVICE_ID_* #defines from the intel8x0.c driver, and make sure the
device IDs are defined in pci_ids.h.

Want me to submit a patch for that?

> -----Original Message-----
> From: Andrew Morton [mailto:akpm@osdl.org] 
> Sent: Monday, July 26, 2004 5:38 PM
> To: Andrew Chew
> Cc: linux-kernel@vger.kernel.org; jgarzik@pobox.com
> Subject: Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 
> audio support
> 
> 
> "Andrew Chew" <achew@nvidia.com> wrote:
> >
> > This patch updates include/linux/pci_ids.h with the CK804 audio  
> > controller ID, and adds the CK804 audio controller to the  
> > sound/pci/intel8x0.c audio driver.
> 
> I'm getting many workwrapped and tab-replaced patches 
> nowadays.  Could people pleeeeze ensure that their email 
> clients are sending unmangled patches?
> 
> I fixed this one up.  I usually do :(
> 
> 
> >  --- linux-2.6.8-rc2/include/linux/pci_ids.h	2004-07-21
> >  15:22:57.000000000 -0700
> >  +++ linux/include/linux/pci_ids.h	2004-07-20 18:49:22.000000000
> >  -0700
> >  @@ -1071,6 +1071,7 @@
> >   #define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_SATA2	0x0055
> >   #define PCI_DEVICE_ID_NVIDIA_NVENET_8		0x0056
> >   #define PCI_DEVICE_ID_NVIDIA_NVENET_9		0x0057
> >  +#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO	0x0059
> >   #define PCI_DEVICE_ID_NVIDIA_NFORCE2_IDE	0x0065
> >   #define PCI_DEVICE_ID_NVIDIA_NVENET_2		0x0066
> >   #define PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO		0x006a
> >  --- linux-2.6.8-rc2/sound/pci/intel8x0.c	2004-07-21
> >  15:22:59.000000000 -0700
> >  +++ linux/sound/pci/intel8x0.c	2004-07-20 
> 18:52:07.000000000 -0700
> >  @@ -155,6 +155,9 @@
> >   #ifndef PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO
> >   #define PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO	0x00ea
> >   #endif
> >  +#ifndef PCI_DEVICE_ID_NVIDIA_CK804_AUDIO
> >  +#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO 0x0059
> >  +#endif
> >   
> 
> Why does the driver need this ifdef?  We just added the ID to 
> pci_ids.h?
> 

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Andrew Morton]

"Andrew Chew" <achew@nvidia.com> wrote:
>
> I sincerely apologize about the mangled patch.  I'll be more careful
> next time (and check my mailer settings).

Is OK - you're in good company ;)

> The #ifdef was for consistency (I noticed that there were other IDs
> similarly defined in intel8x0.c).  I don't see why we'd need it, either.
> We should probably remove PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO and
> PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO #defines from intel8x0.c as well, as
> they're similarly redundant.  For that matter, why not remove all of the
> PCI_DEVICE_ID_* #defines from the intel8x0.c driver, and make sure the
> device IDs are defined in pci_ids.h.
> 
> Want me to submit a patch for that?

Let's leave that up to Jeff.

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Jeff Garzik]

Andrew Morton wrote:
> "Andrew Chew" <achew@nvidia.com> wrote:
> 
>>I sincerely apologize about the mangled patch.  I'll be more careful
>>next time (and check my mailer settings).
> 
> 
> Is OK - you're in good company ;)
> 
> 
>>The #ifdef was for consistency (I noticed that there were other IDs
>>similarly defined in intel8x0.c).  I don't see why we'd need it, either.
>>We should probably remove PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO and
>>PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO #defines from intel8x0.c as well, as
>>they're similarly redundant.  For that matter, why not remove all of the
>>PCI_DEVICE_ID_* #defines from the intel8x0.c driver, and make sure the
>>device IDs are defined in pci_ids.h.
>>
>>Want me to submit a patch for that?
> 
> 
> Let's leave that up to Jeff.

Jaroslav and the ALSA guys...  no idea who maintains the ALSA i810 driver.

In any case, I think the device id constants are a waste, and have 
stopped using the *_DEVICE_ID_* constants in my drivers.  The vendor id 
is generally common across many drivers, but rarely does one care about 
sharing arbitrary name<->arbitrary id mappings these days, when one 
driver supports so many chips.

	Jeff

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Takashi Iwai]

At Mon, 26 Jul 2004 22:09:11 -0400,
Jeff Garzik wrote:
> 
> Andrew Morton wrote:
> > "Andrew Chew" <achew@nvidia.com> wrote:
> > 
> >>I sincerely apologize about the mangled patch.  I'll be more careful
> >>next time (and check my mailer settings).
> > 
> > 
> > Is OK - you're in good company ;)
> > 
> > 
> >>The #ifdef was for consistency (I noticed that there were other IDs
> >>similarly defined in intel8x0.c).  I don't see why we'd need it, either.
> >>We should probably remove PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO and
> >>PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO #defines from intel8x0.c as well, as
> >>they're similarly redundant.  For that matter, why not remove all of the
> >>PCI_DEVICE_ID_* #defines from the intel8x0.c driver, and make sure the
> >>device IDs are defined in pci_ids.h.
> >>
> >>Want me to submit a patch for that?
> > 
> > 
> > Let's leave that up to Jeff.
> 
> Jaroslav and the ALSA guys...  no idea who maintains the ALSA i810 driver.
 
ALSA team ;)
I included the patch to ALSA tree.  Thanks.
Jaroslav will submit the patchset later.

> In any case, I think the device id constants are a waste, and have 
> stopped using the *_DEVICE_ID_* constants in my drivers.  The vendor id 
> is generally common across many drivers, but rarely does one care about 
> sharing arbitrary name<->arbitrary id mappings these days, when one 
> driver supports so many chips.

Well, I agree in general.  It would be still reasonable to define a
const when it appears more than once, though.


Takashi

[PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Andrew Chew]

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

I left out support for two other NVIDIA audio controllers in my previous
patch.

Attached is a patch which adds audio support for MCP04 and MCP2S audio
controllers for the intel8x0 audio driver.  This patch is to be applied
on top of the one I had previously submitted (so in total, there should
be CK804, MCP04, and MCP2S added to the intel8x0 driver as of
2.6.8-rc2).

[-- Attachment #2: intel8x0.diff2 --]
[-- Type: application/octet-stream, Size: 2631 bytes --]

diff -ru linux-2.6.7-rc2/include/linux/pci_ids.h linux/include/linux/pci_ids.h
--- linux-2.6.7-rc2/include/linux/pci_ids.h	2004-07-28 16:56:36.000000000 -0700
+++ linux/include/linux/pci_ids.h	2004-07-28 16:33:50.000000000 -0700
@@ -1065,6 +1065,7 @@
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP04_SATA	0x0036
 #define PCI_DEVICE_ID_NVIDIA_NVENET_10		0x0037
 #define PCI_DEVICE_ID_NVIDIA_NVENET_11		0x0038
+#define PCI_DEVICE_ID_NVIDIA_MCP04_AUDIO	0x003a
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_MCP04_SATA2	0x003e
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_IDE	0x0053
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_SATA	0x0054
@@ -1077,6 +1078,7 @@
 #define PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO		0x006a
 #define PCI_DEVICE_ID_NVIDIA_NFORCE2S_IDE	0x0085
 #define PCI_DEVICE_ID_NVIDIA_NVENET_4		0x0086
+#define PCI_DEVICE_ID_NVIDIA_MCP2S_AUDIO	0x008a
 #define PCI_DEVICE_ID_NVIDIA_NVENET_5		0x008c
 #define PCI_DEVICE_ID_NVIDIA_NFORCE2S_SATA	0x008e
 #define PCI_DEVICE_ID_NVIDIA_ITNT2		0x00A0
diff -ru linux-2.6.7-rc2/sound/pci/intel8x0.c linux/sound/pci/intel8x0.c
--- linux-2.6.7-rc2/sound/pci/intel8x0.c	2004-07-28 16:31:28.000000000 -0700
+++ linux/sound/pci/intel8x0.c	2004-07-28 16:34:25.000000000 -0700
@@ -466,9 +466,11 @@
 	{ 0x1039, 0x7012, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_SIS },	/* SI7012 */
 	{ 0x10de, 0x01b1, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* NFORCE */
 	{ 0x10de, 0x006a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* NFORCE2 */
+	{ 0x10de, 0x008a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* NFORCE2 */
 	{ 0x10de, 0x00da, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* NFORCE3 */
 	{ 0x10de, 0x00ea, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* CK8S */
 	{ 0x10de, 0x0059, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* CK804 */
+	{ 0x10de, 0x003a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },	/* MCP04 */
 	{ 0x1022, 0x746d, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_INTEL },	/* AMD8111 */
 	{ 0x1022, 0x7445, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_INTEL },	/* AMD768 */
 	{ 0x10b9, 0x5455, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_ALI },   /* Ali5455 */
@@ -2616,9 +2618,11 @@
 	{ PCI_DEVICE_ID_SI_7012, "SiS SI7012" },
 	{ PCI_DEVICE_ID_NVIDIA_MCP_AUDIO, "NVidia nForce" },
 	{ PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO, "NVidia nForce2" },
+	{ PCI_DEVICE_ID_NVIDIA_MCP2S_AUDIO, "NVidia nForce2" },
 	{ PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO, "NVidia nForce3" },
 	{ PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO, "NVidia CK8S" },
 	{ PCI_DEVICE_ID_NVIDIA_CK804_AUDIO, "NVidia CK804" },
+	{ PCI_DEVICE_ID_NVIDIA_MCP04_AUDIO, "NVidia MCP04" },
 	{ 0x746d, "AMD AMD8111" },
 	{ 0x7445, "AMD AMD768" },
 	{ 0x5455, "ALi M5455" },

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Takashi Iwai]

At Wed, 28 Jul 2004 17:13:04 -0700,
Andrew Chew wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> I left out support for two other NVIDIA audio controllers in my previous
> patch.
> 
> Attached is a patch which adds audio support for MCP04 and MCP2S audio
> controllers for the intel8x0 audio driver.  This patch is to be applied
> on top of the one I had previously submitted (so in total, there should
> be CK804, MCP04, and MCP2S added to the intel8x0 driver as of
> 2.6.8-rc2).

MCP2S was already added in the recent version.
I added MCP04 entries.


Thanks.

Takashi

[PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Andrew Chew]

This patch updates include/linux/pci_ids.h with the CK804 audio
controller ID, and adds the CK804 audio controller to the
sound/pci/intel8x0.c audio driver.

--- linux-2.6.8-rc2/include/linux/pci_ids.h	2004-07-21
15:22:57.000000000 -0700
+++ linux/include/linux/pci_ids.h	2004-07-20 18:49:22.000000000
-0700
@@ -1071,6 +1071,7 @@
 #define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_SATA2	0x0055
 #define PCI_DEVICE_ID_NVIDIA_NVENET_8		0x0056
 #define PCI_DEVICE_ID_NVIDIA_NVENET_9		0x0057
+#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO	0x0059
 #define PCI_DEVICE_ID_NVIDIA_NFORCE2_IDE	0x0065
 #define PCI_DEVICE_ID_NVIDIA_NVENET_2		0x0066
 #define PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO		0x006a
--- linux-2.6.8-rc2/sound/pci/intel8x0.c	2004-07-21
15:22:59.000000000 -0700
+++ linux/sound/pci/intel8x0.c	2004-07-20 18:52:07.000000000 -0700
@@ -155,6 +155,9 @@
 #ifndef PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO
 #define PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO	0x00ea
 #endif
+#ifndef PCI_DEVICE_ID_NVIDIA_CK804_AUDIO
+#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO 0x0059
+#endif
 
 enum { DEVICE_INTEL, DEVICE_INTEL_ICH4, DEVICE_SIS, DEVICE_ALI,
DEVICE_NFORCE };
 
@@ -465,6 +468,7 @@
 	{ 0x10de, 0x006a, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },
/* NFORCE2 */
 	{ 0x10de, 0x00da, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },
/* NFORCE3 */
 	{ 0x10de, 0x00ea, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },
/* CK8S */
+	{ 0x10de, 0x0059, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_NFORCE },
/* CK804 */
 	{ 0x1022, 0x746d, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_INTEL },
/* AMD8111 */
 	{ 0x1022, 0x7445, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_INTEL },
/* AMD768 */
 	{ 0x10b9, 0x5455, PCI_ANY_ID, PCI_ANY_ID, 0, 0, DEVICE_ALI },
/* Ali5455 */
@@ -2614,6 +2618,7 @@
 	{ PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO, "NVidia nForce2" },
 	{ PCI_DEVICE_ID_NVIDIA_MCP3_AUDIO, "NVidia nForce3" },
 	{ PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO, "NVidia CK8S" },
+	{ PCI_DEVICE_ID_NVIDIA_CK804_AUDIO, "NVidia CK804" },
 	{ 0x746d, "AMD AMD8111" },
 	{ 0x7445, "AMD AMD768" },
 	{ 0x5455, "ALi M5455" },

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Andrew Morton]

"Andrew Chew" <achew@nvidia.com> wrote:
>
> This patch updates include/linux/pci_ids.h with the CK804 audio
>  controller ID, and adds the CK804 audio controller to the
>  sound/pci/intel8x0.c audio driver.

I'm getting many workwrapped and tab-replaced patches nowadays.  Could
people pleeeeze ensure that their email clients are sending unmangled
patches?

I fixed this one up.  I usually do :(


>  --- linux-2.6.8-rc2/include/linux/pci_ids.h	2004-07-21
>  15:22:57.000000000 -0700
>  +++ linux/include/linux/pci_ids.h	2004-07-20 18:49:22.000000000
>  -0700
>  @@ -1071,6 +1071,7 @@
>   #define PCI_DEVICE_ID_NVIDIA_NFORCE_CK804_SATA2	0x0055
>   #define PCI_DEVICE_ID_NVIDIA_NVENET_8		0x0056
>   #define PCI_DEVICE_ID_NVIDIA_NVENET_9		0x0057
>  +#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO	0x0059
>   #define PCI_DEVICE_ID_NVIDIA_NFORCE2_IDE	0x0065
>   #define PCI_DEVICE_ID_NVIDIA_NVENET_2		0x0066
>   #define PCI_DEVICE_ID_NVIDIA_MCP2_AUDIO		0x006a
>  --- linux-2.6.8-rc2/sound/pci/intel8x0.c	2004-07-21
>  15:22:59.000000000 -0700
>  +++ linux/sound/pci/intel8x0.c	2004-07-20 18:52:07.000000000 -0700
>  @@ -155,6 +155,9 @@
>   #ifndef PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO
>   #define PCI_DEVICE_ID_NVIDIA_CK8S_AUDIO	0x00ea
>   #endif
>  +#ifndef PCI_DEVICE_ID_NVIDIA_CK804_AUDIO
>  +#define PCI_DEVICE_ID_NVIDIA_CK804_AUDIO 0x0059
>  +#endif
>   

Why does the driver need this ifdef?  We just added the ID to pci_ids.h?

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Lee Revell]

On Mon, 2004-07-26 at 20:38, Andrew Morton wrote:
> "Andrew Chew" <achew@nvidia.com> wrote:
> >
> > This patch updates include/linux/pci_ids.h with the CK804 audio
> >  controller ID, and adds the CK804 audio controller to the
> >  sound/pci/intel8x0.c audio driver.
> 
> I'm getting many workwrapped and tab-replaced patches nowadays.  Could
> people pleeeeze ensure that their email clients are sending unmangled
> patches?
> 
> I fixed this one up.  I usually do :(
> 

The problem is not people's email clients, the problem is that xterm,
gnome-terminal, and konsole all mangle tabs to spaces when copying
text.  There is nothing the mailer can do about it.  For now the only
fix for people using an X environment is to pipe to a text file and then
use their email client's 'insert file' feature.  There is an open Debian
bug report for xterm regarding this issue, I encourage everyone bothered
by this to add comments.

I realize it has always been this way, but people coming from any other
OS expect to be able to copy and paste a tab and have it stay a tab.

Many other devel lists encourage patches be sent as MIME attachents due
to this issue.  I would prefer that the console apps be fixed.

Lee

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Paul Jackson]

> For now the only fix for people using an X environment ... insert file ...

Another fix is to copy from a non-brain damaged window, such as a gui
text editor window (nedit, for example).

I'm guessing that the tabs are lost in the cut or copy operation, not in
the paste operation.

But file insertion is, in general, a sufficiently winning choice that I
think it's better just to get in the habit of always inserting patches
that way, at least on email clients that support it.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Denis Vlasenko]

On Tuesday 27 July 2004 07:57, Paul Jackson wrote:
> > For now the only fix for people using an X environment ... insert file
> > ...
>
> Another fix is to copy from a non-brain damaged window, such as a gui
> text editor window (nedit, for example).
>
> I'm guessing that the tabs are lost in the cut or copy operation, not in
> the paste operation.

because 'cut' in a xterm cannot know that those eight spaces
once were a tab. xterm is probably storing screen as a char+attr
two-dimensional array. There are no tabs, only spaces.

> But file insertion is, in general, a sufficiently winning choice that I
> think it's better just to get in the habit of always inserting patches
> that way, at least on email clients that support it.

I do it all the time.
-- 
vda

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Lee Revell]

On Tue, 2004-07-27 at 02:43, Denis Vlasenko wrote:
> On Tuesday 27 July 2004 07:57, Paul Jackson wrote:
> > > For now the only fix for people using an X environment ... insert file
> > > ...
> >
> > I'm guessing that the tabs are lost in the cut or copy operation, not in
> > the paste operation.
> 
> because 'cut' in a xterm cannot know that those eight spaces
> once were a tab. xterm is probably storing screen as a char+attr
> two-dimensional array. There are no tabs, only spaces.

Yes, but unless you are copying from vim or emacs, where the editor is
converting tabs to N spaces based on your settings, this is fixable.   
For example it should be possible to run diff from the command line and
copy the output, preserving tabs correctly.  There is an alpha quality
patch floating around to add this functionality to xterm.

But, this is horribly OT.  My main point was that it's almost always not
the mailer's fault if your patches are getting mangled in this way.

Lee

Re: [PATCH 2.6.8-rc2] intel8x0.c to include CK804 audio support

[Lee Revell]

On Tue, 2004-07-27 at 02:43, Denis Vlasenko wrote:
> On Tuesday 27 July 2004 07:57, Paul Jackson wrote:
> > > For now the only fix for people using an X environment ... insert file
> > > ...
> >
> > Another fix is to copy from a non-brain damaged window, such as a gui
> > text editor window (nedit, for example).
> >
> > I'm guessing that the tabs are lost in the cut or copy operation, not in
> > the paste operation.
> 
> because 'cut' in a xterm cannot know that those eight spaces
> once were a tab. xterm is probably storing screen as a char+attr
> two-dimensional array. There are no tabs, only spaces.
> 
> > But file insertion is, in general, a sufficiently winning choice that I
> > think it's better just to get in the habit of always inserting patches
> > that way, at least on email clients that support it.
> 
> I do it all the time.

The easiest suggestion I have seen so far is:

diff foo bar | xclip

Much easier than 'insert file', works perfectly.  This is not installed
on Debian by default for some reason.

Might be a useful FAQ addition.

Lee