C99 Initialisers

C99 Initialisers

[CaT]

Is there any interest ins omeone 'fixing up' as many structs in the
kernel from the form:

struct foobar = {
	cow:	moo,
	cat:	meow,
}

to:

struct foobar = {
	.cow	= moo,
	.cat	= neow,
}

And if so, what form should I feed it back in? Big patches? 1 patch
per file? 1 per dir?

Main reaosn I'm asking is that it's slowly being done but isn't in
the janitor list of things to do yet. If it were I'd just do it. ;)

(not on kj-discuss btw so please CC me or lk (or both))

-- 
"How can I not love the Americans? They helped me with a flat tire the
other day," he said.
	- http://tinyurl.com/h6fo

Re: C99 Initialisers

[Robert Love]

On Mon, 2003-08-11 at 19:02, CaT wrote:
> Is there any interest ins omeone 'fixing up' as many structs in the
> kernel from the form:

Yes, indeed, especially for 2.6. There has been a lot of work already in
this direction -- not too much should be left.

> And if so, what form should I feed it back in? Big patches? 1 patch
> per file? 1 per dir?

Whatever makes most sense. One per directory is probably OK for most
things.

> Main reaosn I'm asking is that it's slowly being done but isn't in
> the janitor list of things to do yet. If it were I'd just do it. ;)

It should be in the list.

Convert GNU-style to C99-style.  I think converting unnamed initializers
to named initializers is a Good Thing, too.

	Robert Love

Re: C99 Initialisers

[Matthew Wilcox]

On Mon, Aug 11, 2003 at 07:18:53PM -0700, Robert Love wrote:
> Convert GNU-style to C99-style.  I think converting unnamed initializers
> to named initializers is a Good Thing, too.

By and large ... here's a counterexample:

static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
        { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700,
          PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
        { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701,
          PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
...

I don't think anyone would appreciate you converting that to:

static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
	{
		.vendor		= PCI_VENDOR_ID_BROADCOM,
		.device		= PCI_DEVICE_ID_TIGON3_5700,
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID,
		.class		= 0,
		.class_mask	= 0,
		.driver_data	= 0,
	},
	{
		.vendor		= PCI_VENDOR_ID_BROADCOM,
		.device		= PCI_DEVICE_ID_TIGON3_5701,
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID,
		.class		= 0,
		.class_mask	= 0,
		.driver_data	= 0,
	},
...

Not unless they're paid by the line ;-)

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Robert Love]

On Mon, 2003-08-11 at 19:39, Matthew Wilcox wrote:

> Not unless they're paid by the line ;-)

Agreed.

So, Kernel Janitor folks, here is a good line for the TODO:

"Convert GNU C-style named initializers to C99 named initializers"

And:

"Where the end result is cleaner and more maintainable, convert unnamed
initializers to C99-style named initializers. In almost all cases, named
initializers are safer and less ambiguous, but in some cases the
resulting change is large and unwieldy. Exercise judgment."

	Robert Love

Re: C99 Initialisers

[Dagfinn Ilmari Mannsåker]

Matthew Wilcox <willy@debian.org> writes:

> On Mon, Aug 11, 2003 at 07:18:53PM -0700, Robert Love wrote:
>> Convert GNU-style to C99-style.  I think converting unnamed initializers
>> to named initializers is a Good Thing, too.
>
> By and large ... here's a counterexample:

[snip unnamed initialisation]

> I don't think anyone would appreciate you converting that to:

[snip C99 named initialisation]

To the contrary (and I agree with Jeff):

From: Jeff Garzik <jgarzik@pobox.com>
To: davej@redhat.com
Cc: torvalds@transmeta.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] c99 initialisers for random.c
Message-ID: <20030811144709.GA32180@gtf.org>
References: <E19mCuO-0003da-00@tetrachloride>

On Mon, Aug 11, 2003 at 02:40:24PM +0100, davej@redhat.com wrote:
> diff -urpN --exclude-from=/home/davej/.exclude bk-linus/drivers/char/random.c linux-2.5/drivers/char/random.c
> --- bk-linus/drivers/char/random.c	2003-08-04 01:00:22.000000000 +0100
> +++ linux-2.5/drivers/char/random.c	2003-08-06 18:59:31.000000000 +0100
>@@ -1850,27 +1850,62 @@ static int uuid_strategy(ctl_table *tabl
> }
> 
> ctl_table random_table[] = {
>-	{RANDOM_POOLSIZE, "poolsize",
>-	 &sysctl_poolsize, sizeof(int), 0644, NULL,
>-	 &proc_do_poolsize, &poolsize_strategy},
[...]
>-       {0}
>+	{
>+		.ctl_name	= RANDOM_POOLSIZE,
>+		.procname	= "poolsize",
>+		.data		= &sysctl_poolsize,
>+		.maxlen		= sizeof(int),
>+		.mode		= 0644,
>+		.proc_handler	= &proc_do_poolsize,
>+		.strategy	= &poolsize_strategy,
>+	},
[...]
>+	{ .ctl_name = 0 }
> };
> 
> static void sysctl_init_random(struct entropy_store *random_state)

Wow.  That is so much more clean (to my eyes).

	Jeff

-- 
ilmari

Re: C99 Initialisers

[Greg KH]

On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:
> On Mon, Aug 11, 2003 at 07:18:53PM -0700, Robert Love wrote:
> > Convert GNU-style to C99-style.  I think converting unnamed initializers
> > to named initializers is a Good Thing, too.
> 
> By and large ... here's a counterexample:
> 
> static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
>         { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700,
>           PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
>         { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701,
>           PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
> ...
> 
> I don't think anyone would appreciate you converting that to:
> 
> static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> 	{
> 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> 		.subvendor	= PCI_ANY_ID,
> 		.subdevice	= PCI_ANY_ID,
> 		.class		= 0,
> 		.class_mask	= 0,
> 		.driver_data	= 0,
> 	},

I sure would.  Oh, you can drop the .class, .class_mask, and
.driver_data lines, and then it even looks cleaner.

I would love to see that kind of change made for pci drivers.

thanks,

greg k-h

Re: C99 Initialisers

[Maciej Soltysiak]

> > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > 	{
> > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > 		.subvendor	= PCI_ANY_ID,
> > 		.subdevice	= PCI_ANY_ID,
> > 		.class		= 0,
> > 		.class_mask	= 0,
> > 		.driver_data	= 0,
> > 	},
>
> I sure would.  Oh, you can drop the .class, .class_mask, and
> .driver_data lines, and then it even looks cleaner.
Just a quick question. if we drop these, will they _always_
be initialised 0 ? I have made a test to see, and it seemed as though,
but I would like to be 100% sure.

Regards,
Maciej

Re: C99 Initialisers

[Geert Uytterhoeven]

On Tue, 12 Aug 2003, Maciej Soltysiak wrote:
> > > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > > 	{
> > > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > > 		.subvendor	= PCI_ANY_ID,
> > > 		.subdevice	= PCI_ANY_ID,
> > > 		.class		= 0,
> > > 		.class_mask	= 0,
> > > 		.driver_data	= 0,
> > > 	},
> >
> > I sure would.  Oh, you can drop the .class, .class_mask, and
> > .driver_data lines, and then it even looks cleaner.
> Just a quick question. if we drop these, will they _always_
> be initialised 0 ? I have made a test to see, and it seemed as though,
> but I would like to be 100% sure.

For globals and static locals: yes.
For non-static locals: no.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: C99 Initialisers

[Jakub Jelinek]

On Tue, Aug 12, 2003 at 12:03:21PM +0200, Geert Uytterhoeven wrote:
> > > I sure would.  Oh, you can drop the .class, .class_mask, and
> > > .driver_data lines, and then it even looks cleaner.
> > Just a quick question. if we drop these, will they _always_
> > be initialised 0 ? I have made a test to see, and it seemed as though,
> > but I would like to be 100% sure.
> 
> For globals and static locals: yes.
> For non-static locals: no.

No, for all initializers members which are not explicitely initialized are
zero initialized.
Only if you provide no initializer at all, globals/static locals will
still be zero initializers while non-static locals may contain anything.
So, if you write:

struct A { int a; int b; int c; int d; };
void bar (void *);
void foo (void)
{
  struct A a = { .a = 21 };
  bar (&a);
}

then it is the same as:

struct A { int a; int b; int c; int d; };
void bar (void *);
void foo (void)
{
  struct A a = { .a = 21, .b = 0, .c = 0, .d = 0 };
  bar (&a);
}

	Jakub

Re: C99 Initialisers

[Matthew Wilcox]

On Mon, Aug 11, 2003 at 10:38:27PM -0700, Greg KH wrote:
> On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:
> > I don't think anyone would appreciate you converting that to:
> > 
> > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > 	{
> > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > 		.subvendor	= PCI_ANY_ID,
> > 		.subdevice	= PCI_ANY_ID,
> > 		.class		= 0,
> > 		.class_mask	= 0,
> > 		.driver_data	= 0,
> > 	},
> 
> I sure would.  Oh, you can drop the .class, .class_mask, and
> .driver_data lines, and then it even looks cleaner.
> 
> I would love to see that kind of change made for pci drivers.

I really strongly disagree.  For a struct that's as well-known as
pci_device_id, the argument of making it clearer doesn't make sense.
It's also not subject to change, unlike say file_operations, so the
argument of adding new elements without breaking anything is also not
relevant.

In tg3, the table definition is already 32 lines long with 2 lines per
device_id.  In the scheme you favour so much, that becomes 92 lines, for
no real gain that I can see.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Ian Hastie]

On Tuesday 12 Aug 2003 12:27, Matthew Wilcox wrote:
> On Mon, Aug 11, 2003 at 10:38:27PM -0700, Greg KH wrote:
> > On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:
> > > I don't think anyone would appreciate you converting that to:
> > >
> > > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > > 	{
> > > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > > 		.subvendor	= PCI_ANY_ID,
> > > 		.subdevice	= PCI_ANY_ID,
> > > 		.class		= 0,
> > > 		.class_mask	= 0,
> > > 		.driver_data	= 0,
> > > 	},
> >
> > I sure would.  Oh, you can drop the .class, .class_mask, and
> > .driver_data lines, and then it even looks cleaner.
> >
> > I would love to see that kind of change made for pci drivers.
>
> I really strongly disagree.  For a struct that's as well-known as
> pci_device_id, the argument of making it clearer doesn't make sense.

Which is OK for those very familiar with the code, but not so good for anyone 
else.

> It's also not subject to change, unlike say file_operations, so the
> argument of adding new elements without breaking anything is also not
> relevant.

Even if it is not subject to change, which can never be a total certainty, it 
will need to be applied in new code.  The greater clarity provided by the 
C-99 format will make it easier to ensure the appropriate values are put into 
the right fields.

> In tg3, the table definition is already 32 lines long with 2 lines per
> device_id.  In the scheme you favour so much, that becomes 92 lines, for
> no real gain that I can see.

Smaller source code is not everything.  A consistant style is also very 
important.

-- 
Ian.

Re: C99 Initialisers

[Dave Jones]

On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:

 > By and large ... here's a counterexample:
 > 
 > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
 >         { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700,
 >           PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
 >         { PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701,
 >           PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
 > ...
 > 
 > I don't think anyone would appreciate you converting that to:
 > <snip C99>

Depends. If it's a huuuge struct (see the device ID struct in 2.4's
agpgart for eg) it becomes much more readable. Whitespace good, clutter bad.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: C99 Initialisers

[Matthew Wilcox]

On Tue, Aug 12, 2003 at 06:37:07PM +0100, Dave Jones wrote:
> Depends. If it's a huuuge struct (see the device ID struct in 2.4's
> agpgart for eg) it becomes much more readable. Whitespace good, clutter bad.

Yup, absolutely.  My point is that struct pci_device_id is really really
common.  If you've ever looked at a Linux PCI driver, you've seen it.
The agp_bridge_info example is specific to this one driver, so it's new
every time you look at it.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Greg KH]

On Tue, Aug 12, 2003 at 12:27:29PM +0100, Matthew Wilcox wrote:
> On Mon, Aug 11, 2003 at 10:38:27PM -0700, Greg KH wrote:
> > On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:
> > > I don't think anyone would appreciate you converting that to:
> > > 
> > > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > > 	{
> > > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > > 		.subvendor	= PCI_ANY_ID,
> > > 		.subdevice	= PCI_ANY_ID,
> > > 		.class		= 0,
> > > 		.class_mask	= 0,
> > > 		.driver_data	= 0,
> > > 	},
> > 
> > I sure would.  Oh, you can drop the .class, .class_mask, and
> > .driver_data lines, and then it even looks cleaner.
> > 
> > I would love to see that kind of change made for pci drivers.
> 
> I really strongly disagree.  For a struct that's as well-known as
> pci_device_id, the argument of making it clearer doesn't make sense.

It's not _that_ well known.  I often have to go look it up to mentally
match up the fields when looking at pci drivers again after a few weeks
away.  With the above change, I can instantly know what is going on.

> It's also not subject to change, unlike say file_operations, so the
> argument of adding new elements without breaking anything is also not
> relevant.

Who knows, it might change in the future, never say never :)

> In tg3, the table definition is already 32 lines long with 2 lines per
> device_id.  In the scheme you favour so much, that becomes 92 lines, for
> no real gain that I can see.

In the end, it's up to the maintainer of the driver what they want to
do.  So, Jeff and David, here's a patch against the latest 2.6.0-test3
tg3.c that converts the pci_device_id table to C99 initializers.  If you
want to, please apply it.

thanks,

greg k-h


# Convert tg3 pci_device_id table to C99 initializers

--- a/drivers/net/tg3.c	Mon Aug 11 09:21:41 2003
+++ b/drivers/net/tg3.c	Tue Aug 12 10:58:30 2003
@@ -128,36 +128,96 @@
 static int tg3_debug = -1;	/* -1 == use TG3_DEF_MSG_ENABLE as value */
 
 static struct pci_device_id tg3_pci_tbl[] = {
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702FE,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702X,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703X,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704S,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702A3,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703A3,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_SYSKONNECT, 0x4400,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1000,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1001,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC9100,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5700,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5701,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5702,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5703,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5704,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5702FE,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5702X,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5703X,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5704S,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5702A3,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_BROADCOM,
+		.device		= PCI_DEVICE_ID_TIGON3_5703A3,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_SYSKONNECT,
+		.device		= 0x4400,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_ALTIMA,
+		.device		= PCI_DEVICE_ID_ALTIMA_AC1000,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_ALTIMA,
+		.device		= PCI_DEVICE_ID_ALTIMA_AC1001,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
+	{
+		.vendor		= PCI_VENDOR_ID_ALTIMA,
+		.device		= PCI_DEVICE_ID_ALTIMA_AC9100,
+		.subvendor	= PCI_ANY_ID,
+		.subdevice	= PCI_ANY_ID
+	},
 	{ 0, }
 };
 

Re: C99 Initialisers

[Ian Hastie]

On Tuesday 12 Aug 2003 18:48, Matthew Wilcox wrote:
> On Tue, Aug 12, 2003 at 06:37:07PM +0100, Dave Jones wrote:
> > Depends. If it's a huuuge struct (see the device ID struct in 2.4's
> > agpgart for eg) it becomes much more readable. Whitespace good, clutter
> > bad.
>
> Yup, absolutely.  My point is that struct pci_device_id is really really
> common.  If you've ever looked at a Linux PCI driver, you've seen it.

That could be used as an argument for just as much as against.  The very fact 
of it's ubiquity makes it all the more important to minimise the possibility 
of confusion.  C99 initialisers will help a lot with this.

> The agp_bridge_info example is specific to this one driver, so it's new
> every time you look at it.

That really doesn't make sense to me.

-- 
Ian.

Re: C99 Initialisers

[Dave Jones]

On Tue, Aug 12, 2003 at 11:01:58AM -0700, Greg KH wrote:

What would be *really* nice, would be the ability to do something
to the effect of..

	{
		.vendor		= PCI_VENDOR_ID_BROADCOM,
		.devices	= {
			PCI_DEVICE_ID_TIGON3_5700,
			PCI_DEVICE_ID_TIGON3_5701,
			PCI_DEVICE_ID_TIGON3_5702,
			PCI_DEVICE_ID_TIGON3_5703,
			PCI_DEVICE_ID_TIGON3_5704,
			PCI_DEVICE_ID_TIGON3_5702FE,
			PCI_DEVICE_ID_TIGON3_5702X,
			PCI_DEVICE_ID_TIGON3_5703X,
			PCI_DEVICE_ID_TIGON3_5704S,
			PCI_DEVICE_ID_TIGON3_5702A3,
			PCI_DEVICE_ID_TIGON3_5703A3,
		},
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID
	},
	{
		.vendor		= PCI_VENDOR_ID_SYSKONNECT,
		.device		= 0x4400,
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID
	},
	{
		.vendor		= PCI_VENDOR_ID_ALTIMA,
		.devices	= {
			PCI_DEVICE_ID_ALTIMA_AC1000,
			PCI_DEVICE_ID_ALTIMA_AC1001,
			PCI_DEVICE_ID_ALTIMA_AC9100,
		}
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID
	},
	{ 0, }
   };


-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: C99 Initialisers

[Jeff Garzik]

Greg KH wrote:
> In the end, it's up to the maintainer of the driver what they want to
> do.  So, Jeff and David, here's a patch against the latest 2.6.0-test3
> tg3.c that converts the pci_device_id table to C99 initializers.  If you
> want to, please apply it.


it expands a few lines to a bazillion :(   I would rather leave it as 
is...  you'll find several PCI ethernet drivers with pci_device_id 
entries that fit entirely on one line, and I think that compactness has 
value at least to me.

	Jeff

Re: C99 Initialisers

[Matthew Wilcox]

On Wed, Aug 13, 2003 at 12:53:24AM +0100, Dave Jones wrote:
> What would be *really* nice, would be the ability to do something
> to the effect of..

While we're off in never-never land, it'd be nice to specify default
values for struct initialisers.  eg, something like:

struct pci_device_id {
        __u32 vendor = PCI_ANY_ID;
        __u32 device = PCI_ANY_ID;
        __u32 subvendor = PCI_ANY_ID;
	__u32 subdevice = PCI_ANY_ID;
        __u32 class = 0;
	__u32 class_mask = 0;
        kernel_ulong_t driver_data = 0;
};

Erm, hang on a second ...  Since when are PCI IDs 32-bit?  What is this
ridiculous bloat?  You can't even argue that this makes things pack
better since this packs equally well:

struct pci_device_id {
        __u16 vendor;
        __u16 device;
        __u16 subvendor;
        __u16 subdevice;     
        __u32 class;
        __u32 class_mask;
        kernel_ulong_t driver_data;
};

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Randy.Dunlap]

On Tue, 12 Aug 2003 20:02:03 -0400 Jeff Garzik <jgarzik@pobox.com> wrote:

| Greg KH wrote:
| > In the end, it's up to the maintainer of the driver what they want to
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| > do.  So, Jeff and David, here's a patch against the latest 2.6.0-test3
| > tg3.c that converts the pci_device_id table to C99 initializers.  If you
| > want to, please apply it.

I strongly agree with Greg's comment above.
| 
| it expands a few lines to a bazillion :(   I would rather leave it as 
| is...  you'll find several PCI ethernet drivers with pci_device_id 
| entries that fit entirely on one line, and I think that compactness has 
| value at least to me.

However, I would change for readability.  Maybe not my readability,
but for all others who read and try to help maintain all of Linux
source code.

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 01:08:41AM +0100, Matthew Wilcox wrote:
> On Wed, Aug 13, 2003 at 12:53:24AM +0100, Dave Jones wrote:
> > What would be *really* nice, would be the ability to do something
> > to the effect of..

Yeah, that would be cool to do.  2.7 :)

> While we're off in never-never land, it'd be nice to specify default
> values for struct initialisers.  eg, something like:

Yeah, I've wanted that for a while too.  Don't really know how to get
the compiler to do that though :(

> Erm, hang on a second ...  Since when are PCI IDs 32-bit?  What is this
> ridiculous bloat?  You can't even argue that this makes things pack
> better since this packs equally well:

Yeah, it was just a port from 2.4 which says:

	struct pci_device_id {
		unsigned int vendor, device;
		unsigned int subvendor, subdevice;
		unsigned int class, class_mask;
		unsigned long driver_data;
	};

We could probably change it now if you really want to.  Don't know if it
will save much space though.

thanks,

greg k-h

Re: C99 Initialisers

[Matthew Wilcox]

On Tue, Aug 12, 2003 at 05:23:44PM -0700, Greg KH wrote:
> On Wed, Aug 13, 2003 at 01:08:41AM +0100, Matthew Wilcox wrote:
> > On Wed, Aug 13, 2003 at 12:53:24AM +0100, Dave Jones wrote:
> > > What would be *really* nice, would be the ability to do something
> > > to the effect of..
> 
> Yeah, that would be cool to do.  2.7 :)

Well, we'd need some cooperation from the GCC folks for it ... I can't
see them being too interested in this extension.

> Yeah, it was just a port from 2.4 which says:
> 
> 	struct pci_device_id {
> 		unsigned int vendor, device;
> 		unsigned int subvendor, subdevice;
> 		unsigned int class, class_mask;
> 		unsigned long driver_data;
> 	};
> 
> We could probably change it now if you really want to.  Don't know if it
> will save much space though.

Think about it, 8 bytes per pci_device_id; that's 120 bytes in tg3 alone.
Admittedly tg3 has more than its fair share of IDs, but distro kernels
will love it.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Jeff Garzik]

Randy.Dunlap wrote:
> On Tue, 12 Aug 2003 20:02:03 -0400 Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> | Greg KH wrote:
> | > In the end, it's up to the maintainer of the driver what they want to
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> | > do.  So, Jeff and David, here's a patch against the latest 2.6.0-test3
> | > tg3.c that converts the pci_device_id table to C99 initializers.  If you
> | > want to, please apply it.
> 
> I strongly agree with Greg's comment above.
> | 
> | it expands a few lines to a bazillion :(   I would rather leave it as 
> | is...  you'll find several PCI ethernet drivers with pci_device_id 
> | entries that fit entirely on one line, and I think that compactness has 
> | value at least to me.
> 
> However, I would change for readability.  Maybe not my readability,
> but for all others who read and try to help maintain all of Linux
> source code.


I find the compact form quite readable, and comfortable on the eyes. 
Users don't seem to complain, either.  I get compact-form pci_device_id 
patches from Joe Sixpack quite often :)

Expanding this device id struct to use C99 initializers isn't terribly 
scalable:  once you get past just a few ids, you bloat up the source 
code considerably.  I would much rather move the PCI ids out of the 
drivers altogether, into some metadata file(s) in the kernel source 
tree, than bloat up tg3, tulip, e100, and the other PCI id-heavy 
drivers' source code.

	Jeff

Re: C99 Initialisers

[Randy.Dunlap]

On Tue, 12 Aug 2003 20:31:41 -0400 Jeff Garzik <jgarzik@pobox.com> wrote:

| Randy.Dunlap wrote:
| > On Tue, 12 Aug 2003 20:02:03 -0400 Jeff Garzik <jgarzik@pobox.com> wrote:
| > 
| > | Greg KH wrote:
| > | > In the end, it's up to the maintainer of the driver what they want to
| >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| > | > do.  So, Jeff and David, here's a patch against the latest 2.6.0-test3
| > | > tg3.c that converts the pci_device_id table to C99 initializers.  If you
| > | > want to, please apply it.
| > 
| > I strongly agree with Greg's comment above.
| > | 
| > | it expands a few lines to a bazillion :(   I would rather leave it as 
| > | is...  you'll find several PCI ethernet drivers with pci_device_id 
| > | entries that fit entirely on one line, and I think that compactness has 
| > | value at least to me.
| > 
| > However, I would change for readability.  Maybe not my readability,
| > but for all others who read and try to help maintain all of Linux
| > source code.
| 
| 
| I find the compact form quite readable, and comfortable on the eyes. 

and since you are the drivers/net/ maintainer, you can make the decision.
However, in the end, it's not just about you.  You are the primary
maintainer but not the only user or maintainer of those drivers.

| Users don't seem to complain, either.  I get compact-form pci_device_id 
| patches from Joe Sixpack quite often :)
| 
| Expanding this device id struct to use C99 initializers isn't terribly 
| scalable:  once you get past just a few ids, you bloat up the source 
| code considerably.  I would much rather move the PCI ids out of the 
| drivers altogether, into some metadata file(s) in the kernel source 
| tree, than bloat up tg3, tulip, e100, and the other PCI id-heavy 
| drivers' source code.

That last few lines certainly sounds desirable.

--
~Randy				For Linux-2.6, see:
http://www.kernel.org/pub/linux/kernel/people/davej/misc/post-halloween-2.5.txt

Re: C99 Initialisers

[Dave Jones]

On Tue, Aug 12, 2003 at 05:37:42PM -0700, Randy.Dunlap wrote:
 > | I would much rather move the PCI ids out of the 
 > | drivers altogether, into some metadata file(s) in the kernel source 
 > | tree, than bloat up tg3, tulip, e100, and the other PCI id-heavy 
 > | drivers' source code.
 > 
 > That last few lines certainly sounds desirable.

What exactly would be the benefit of this ?
The only thing I could think of was out-of-kernel tools to do
things like matching modules to pci IDs, but that seems to be
done mechanically by various distros already reading the pci_driver
structs.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: C99 Initialisers

[Jeff Garzik]

Dave Jones wrote:
> On Tue, Aug 12, 2003 at 05:37:42PM -0700, Randy.Dunlap wrote:
>  > | I would much rather move the PCI ids out of the 
>  > | drivers altogether, into some metadata file(s) in the kernel source 
>  > | tree, than bloat up tg3, tulip, e100, and the other PCI id-heavy 
>  > | drivers' source code.
>  > 
>  > That last few lines certainly sounds desirable.
> 
> What exactly would be the benefit of this ?
> The only thing I could think of was out-of-kernel tools to do
> things like matching modules to pci IDs, but that seems to be
> done mechanically by various distros already reading the pci_driver
> structs.


Fundamentally, the PCI ID list is not C code.  And if anyone ever wants 
to get to the PCI ID lists at the _source code_ level, they have to 
parse C or assembler :)  It's data, so I say, put it in a data file. 
Stuffing the PCI ID list in C code is a sometimes convenient, sometimes 
inconvenient form of packaging, nothing more :)

I would rather store the PCI ID list in a more natural form, and then 
use small tool to generate the pci_device_id tables that are linked into 
the kernel.

	Jeff

Re: C99 Initialisers

[Randy.Dunlap]

> On Tue, Aug 12, 2003 at 05:37:42PM -0700, Randy.Dunlap wrote:
>  > | I would much rather move the PCI ids out of the
>  > | drivers altogether, into some metadata file(s) in the kernel source  |
> tree, than bloat up tg3, tulip, e100, and the other PCI id-heavy  |
> drivers' source code.
>  >
>  > That last few lines certainly sounds desirable.
>
> What exactly would be the benefit of this ?
> The only thing I could think of was out-of-kernel tools to do
> things like matching modules to pci IDs, but that seems to be
> done mechanically by various distros already reading the pci_driver structs.

Maybe I read too much into it.  It made me think of Jeff's previous
remarks about driver config and help being close to but split from
driver source code.  I saw (read) it as an extension of that:
a way to package all driver information neatly close together,
but in separate files.  Someone could modify the config, help, or IDs
file(s) without mucking with the driver source file(s).

~Randy

Re: C99 Initialisers

[Jeff Garzik]

Randy.Dunlap wrote:
> Maybe I read too much into it.  It made me think of Jeff's previous
> remarks about driver config and help being close to but split from
> driver source code.  I saw (read) it as an extension of that:
> a way to package all driver information neatly close together,
> but in separate files.  Someone could modify the config, help, or IDs
> file(s) without mucking with the driver source file(s).


You got it.  I've even queried Roman Zippel about moving some of the 
more simple per-driver fragments in drivers/*/Makefile into 
drivers/*/Kconfig, so that people adding drivers only have to add one 
file, and patch one file.  All of the driver-specific metadata stays in 
one place.

It would be straightforward to put the PCI IDs into Kconfig, even.  We 
already have the parser for it, so writing the tool to generate 
pci_device_id C code initializers already has the backend written.

	Jeff, the radical

Re: C99 Initialisers

[David S. Miller]

Hey guys, define a set of macros to make it more readable.
This would keep the number of lines down and also make
the C99 folks happy.

I think there is real value in moving over the C99 completely.
And we can do this without the source bloat effect.

Re: C99 Initialisers

[Timothy Miller]

Dave Jones wrote:
> On Tue, Aug 12, 2003 at 11:01:58AM -0700, Greg KH wrote:
> 
> What would be *really* nice, would be the ability to do something
> to the effect of..
> 
> 	{
> 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> 		.devices	= {
> 			PCI_DEVICE_ID_TIGON3_5700,
> 			PCI_DEVICE_ID_TIGON3_5701,
> 			PCI_DEVICE_ID_TIGON3_5702,
> 			PCI_DEVICE_ID_TIGON3_5703,
> 			PCI_DEVICE_ID_TIGON3_5704,
> 			PCI_DEVICE_ID_TIGON3_5702FE,
> 			PCI_DEVICE_ID_TIGON3_5702X,
> 			PCI_DEVICE_ID_TIGON3_5703X,
> 			PCI_DEVICE_ID_TIGON3_5704S,
> 			PCI_DEVICE_ID_TIGON3_5702A3,
> 			PCI_DEVICE_ID_TIGON3_5703A3,
> 		},
> 		.subvendor	= PCI_ANY_ID,
> 		.subdevice	= PCI_ANY_ID

[snip]

That's not a bad idea.  Is there a way the structures could be filled so 
that code comes in at boot time and fills structures out?

Or even better, could there be an array of devices for each vendor, and 
the single vendor structure points to that list?


struct devicelist BROADCOM_devs[] {
	PCI_DEVICE_ID_TIGON3_5700,
	PCI_DEVICE_ID_TIGON3_5701,
	PCI_DEVICE_ID_TIGON3_5702,
	PCI_DEVICE_ID_TIGON3_5703,
	PCI_DEVICE_ID_TIGON3_5704,
	PCI_DEVICE_ID_TIGON3_5702FE,
	PCI_DEVICE_ID_TIGON3_5702X,
	PCI_DEVICE_ID_TIGON3_5703X,
	PCI_DEVICE_ID_TIGON3_5704S,
	PCI_DEVICE_ID_TIGON3_5702A3,
	PCI_DEVICE_ID_TIGON3_5703A3,
	LIST_TERMINATOR};

struct pci_table VENCOR_list[] {
	.vendor		= PCI_VENDOR_ID_BROADCOM,
	.devices	= &BROADCOM_devs,
	.subvendor 	= PCI_ANY_ID
	..........

};

Re: C99 Initialisers

[CaT]

On Mon, Aug 11, 2003 at 07:18:53PM -0700, Robert Love wrote:
> On Mon, 2003-08-11 at 19:02, CaT wrote:
> > Is there any interest ins omeone 'fixing up' as many structs in the
> > kernel from the form:
> 
> Yes, indeed, especially for 2.6. There has been a lot of work already in
> this direction -- not too much should be left.

Cool. Since noone screamed 'OH FOR THE LOVE OF GOD, NO!' I'll do it (or
at least try to :) Should hopefully have something by the weekend. :)

> > And if so, what form should I feed it back in? Big patches? 1 patch
> > per file? 1 per dir?
> 
> Whatever makes most sense. One per directory is probably OK for most
> things.

Cool.

-- 
"How can I not love the Americans? They helped me with a flat tire the
other day," he said.
	- http://tinyurl.com/h6fo

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 03:14:32AM -0700, David S. Miller wrote:
> 
> Hey guys, define a set of macros to make it more readable.
> This would keep the number of lines down and also make
> the C99 folks happy.

Heh, much like the USB people have had for years :)

How about this patch?  If you like it I'll add the pci.h change to the
tree and let you take the tg3.c part.

thanks,

greg k-h

# add PCI_DEVICE() macro to make pci_device_id tables easier to read.

diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
--- a/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
+++ b/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
@@ -128,36 +128,21 @@
 static int tg3_debug = -1;	/* -1 == use TG3_DEF_MSG_ENABLE as value */
 
 static struct pci_device_id tg3_pci_tbl[] = {
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702FE,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702X,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703X,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704S,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702A3,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703A3,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_SYSKONNECT, 0x4400,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1000,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1001,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
-	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC9100,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702FE) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702X) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703X) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704S) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702A3) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703A3) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_SYSKONNECT, 0x4400) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1000) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC1001) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC9100) },
 	{ 0, }
 };
 
diff -Nru a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h	Wed Aug 13 10:29:08 2003
+++ b/include/linux/pci.h	Wed Aug 13 10:29:08 2003
@@ -524,6 +524,18 @@
 
 #define	to_pci_driver(drv) container_of(drv,struct pci_driver, driver)
 
+/**
+ * PCI_DEVICE - macro used to describe a specific pci device
+ * @vend: the 16 bit PCI Vendor ID
+ * @dev: the 16 bit PCI Device ID
+ *
+ * This macro is used to create a struct pci_device_id that matches a
+ * specific device.  The subvendor and subdevice fields will be set to
+ * PCI_ANY_ID.
+ */
+#define PCI_DEVICE(vend,dev) \
+	.vendor = (vend), .device = (dev), \
+	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
 
 /* these external functions are only available when PCI support is enabled */
 #ifdef CONFIG_PCI

Re: C99 Initialisers

[David S. Miller]

On Wed, 13 Aug 2003 10:31:51 -0700
Greg KH <greg@kroah.com> wrote:

> How about this patch?
...
> +#define PCI_DEVICE(vend,dev) \
> +	.vendor = (vend), .device = (dev), \
> +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID

Looks fine to me.

Re: C99 Initialisers

[Jeff Garzik]

Greg KH wrote:
> # add PCI_DEVICE() macro to make pci_device_id tables easier to read.
> 
> diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
> --- a/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
> +++ b/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003


This patch is ok with me.

And I agree with David that, in generic, C99 initializers is the way to 
go.  However, the higher level point remains:

PCI IDs, and data like them, are fundamentally not C code.

I'm a strong believer in putting data in its most natural form, and then 
transforming it via automated tools into the desired form.  C code is a 
natural form of data that describes "process and procedure", and the 
compiler is the automated tool that transforms it.  PCI ID tables are 
data that is not process/procedure, but instead much more of a 
traditional data table.  So it should be a form more suitable for its 
multiple uses.  Distro installers and other utilities already pay 
attention to the PCI ID tables in drivers.  Why are we compiling 
non-code into ELF .o objects, and then forcing people to extract that 
non-code from .o files?  In the South we call it "going around your 
elbow to get to your thumb" :)

	Jeff

Re: C99 Initialisers

[Jeff Garzik]

Timothy Miller wrote:
> Dave Jones wrote:
>>     {
>>         .vendor        = PCI_VENDOR_ID_BROADCOM,
>>         .devices    = {
>>             PCI_DEVICE_ID_TIGON3_5700,
>>             PCI_DEVICE_ID_TIGON3_5701,
>>             PCI_DEVICE_ID_TIGON3_5702,
>>             PCI_DEVICE_ID_TIGON3_5703,
>>             PCI_DEVICE_ID_TIGON3_5704,
>>             PCI_DEVICE_ID_TIGON3_5702FE,
>>             PCI_DEVICE_ID_TIGON3_5702X,
>>             PCI_DEVICE_ID_TIGON3_5703X,
>>             PCI_DEVICE_ID_TIGON3_5704S,
>>             PCI_DEVICE_ID_TIGON3_5702A3,
>>             PCI_DEVICE_ID_TIGON3_5703A3,
>>         },
>>         .subvendor    = PCI_ANY_ID,
>>         .subdevice    = PCI_ANY_ID

> struct devicelist BROADCOM_devs[] {
>     PCI_DEVICE_ID_TIGON3_5700,
>     PCI_DEVICE_ID_TIGON3_5701,
>     PCI_DEVICE_ID_TIGON3_5702,
>     PCI_DEVICE_ID_TIGON3_5703,
>     PCI_DEVICE_ID_TIGON3_5704,
>     PCI_DEVICE_ID_TIGON3_5702FE,
>     PCI_DEVICE_ID_TIGON3_5702X,
>     PCI_DEVICE_ID_TIGON3_5703X,
>     PCI_DEVICE_ID_TIGON3_5704S,
>     PCI_DEVICE_ID_TIGON3_5702A3,
>     PCI_DEVICE_ID_TIGON3_5703A3,
>     LIST_TERMINATOR};


This is proving my point ;-)

You guys are stretching the bounds of C with syntactic sugar, to make it 
do something it doesn't do well:  store data.

Better to store the data outside the C code, where you don't have to do 
all this C mangling, and then use an automated tool to generate the C 
code representing pci_device_id tables.

	Jeff

Re: C99 Initialisers

[Sam Ravnborg]

On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> 
> How about this patch?  If you like it I'll add the pci.h change to the
> tree and let you take the tg3.c part.
> 
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
Why not without the extra {}'s so something like this:

> +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701),
> +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702),
>  	{ 0, }
>  };
>  
> +#define PCI_DEVICE(vend,dev) { \
> +	.vendor = (vend), .device = (dev), \
> +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID }

	Sam

Re: C99 Initialisers

[Jeff Garzik]

Sam Ravnborg wrote:
> On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> 
>>How about this patch?  If you like it I'll add the pci.h change to the
>>tree and let you take the tg3.c part.
>>
>>+	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> 
> Why not without the extra {}'s so something like this:
> 
> 
>>+	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701),
>>+	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702),


Either way is fine with me; they're both shorter.

	Jeff

Re: C99 Initialisers

[Matthew Wilcox]

On Wed, Aug 13, 2003 at 07:50:09PM +0200, Sam Ravnborg wrote:
> On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> > 
> > How about this patch?  If you like it I'll add the pci.h change to the
> > tree and let you take the tg3.c part.
> > 
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> Why not without the extra {}'s so something like this:
> 
> > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701),
> > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702),
> >  	{ 0, }
> >  };
> >  
> > +#define PCI_DEVICE(vend,dev) { \
> > +	.vendor = (vend), .device = (dev), \
> > +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID }

... and while we're at it:

+#define END_OF_LIST { 0, }

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 07:50:09PM +0200, Sam Ravnborg wrote:
> On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> > 
> > How about this patch?  If you like it I'll add the pci.h change to the
> > tree and let you take the tg3.c part.
> > 
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> Why not without the extra {}'s so something like this:
> 
> > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701),
> > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702),
> >  	{ 0, }
> >  };
> >  
> > +#define PCI_DEVICE(vend,dev) { \
> > +	.vendor = (vend), .device = (dev), \
> > +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID }

No, you want to be able to add a ".driver_data = foo" after a
PCI_DEVICE() macro.  If you have the {} there, that prevents that from
happening.

thanks,

greg k-h

Re: C99 Initialisers

[Jeff Garzik]

Matthew Wilcox wrote:
> +#define END_OF_LIST { 0, }


I prefer the shorter LIST_END or END_LIST.

	Jeff, always picking the tiniest of nits

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 01:47:54PM -0400, Jeff Garzik wrote:
> Greg KH wrote:
> ># add PCI_DEVICE() macro to make pci_device_id tables easier to read.
> >
> >diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
> >--- a/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
> >+++ b/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
> 
> 
> This patch is ok with me.
> 
> And I agree with David that, in generic, C99 initializers is the way to 
> go.  However, the higher level point remains:
> 
> PCI IDs, and data like them, are fundamentally not C code.

But the kernel, using C code, uses those ids to match drivers to
devices.  So we have to create C structures out of those ids some how.

The idea was that since the kernel already keeps track of these ids, we
might as well export them to userspace, so that it too can see what the
kernel support.  That brought forth the modules.*map files and enabled
the hotplug scripts to automatically load a module based on a device id
(this is much nicer than other os schemes which force a text file to be
created for every driver listing these ids.  They are usually created by
hand, and can get out of sync.)

I agree that now that more and more tools are using this data, we should
put it into a form that everyone can easily get at, without having to
parse module attributes or even the modules.*map files.

Any suggestions that do not involve XML?  :)

thanks,

greg k-h

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 06:54:22PM +0100, Matthew Wilcox wrote:
> On Wed, Aug 13, 2003 at 07:50:09PM +0200, Sam Ravnborg wrote:
> > On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> > > 
> > > How about this patch?  If you like it I'll add the pci.h change to the
> > > tree and let you take the tg3.c part.
> > > 
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> > Why not without the extra {}'s so something like this:
> > 
> > > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701),
> > > +	PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702),
> > >  	{ 0, }
> > >  };
> > >  
> > > +#define PCI_DEVICE(vend,dev) { \
> > > +	.vendor = (vend), .device = (dev), \
> > > +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID }
> 
> ... and while we're at it:
> 
> +#define END_OF_LIST { 0, }

Now you're just getting silly.

greg k-h

Re: C99 Initialisers

[Russell King]

On Wed, Aug 13, 2003 at 07:50:09PM +0200, Sam Ravnborg wrote:
> On Wed, Aug 13, 2003 at 10:31:51AM -0700, Greg KH wrote:
> > 
> > How about this patch?  If you like it I'll add the pci.h change to the
> > tree and let you take the tg3.c part.
> > 
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> Why not without the extra {}'s so something like this:

Take a moment to think about this (as it is currently):

#define CB_ID(vend,dev,type)                            \
        {                                               \
                .vendor         = vend,                 \
                .device         = dev,                  \
                .subvendor      = PCI_ANY_ID,           \
                .subdevice      = PCI_ANY_ID,           \
                .class          = PCI_CLASS_BRIDGE_CARDBUS << 8, \
                .class_mask     = ~0,                   \
                .driver_data    = CARDBUS_TYPE_##type,  \
        }

vs:

#define CB_ID(vend,dev,type)                            \
        {                                               \
                PCI_DEVICE(vend,dev),			\
                .class          = PCI_CLASS_BRIDGE_CARDBUS << 8, \
                .class_mask     = ~0,                   \
                .driver_data    = CARDBUS_TYPE_##type,  \
        }

and realise that you can't do the second if you include {} into
PCI_DEVICE.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: C99 Initialisers

[Sam Ravnborg]

On Wed, Aug 13, 2003 at 10:58:10AM -0700, Greg KH wrote:
> 
> No, you want to be able to add a ".driver_data = foo" after a
> PCI_DEVICE() macro.  If you have the {} there, that prevents that from
> happening.

Obvious, thanks for the explanation.

	Sam

Re: C99 Initialisers

[Jeff Garzik]

Greg KH wrote:
> On Wed, Aug 13, 2003 at 01:47:54PM -0400, Jeff Garzik wrote:
> 
>>Greg KH wrote:
>>
>>># add PCI_DEVICE() macro to make pci_device_id tables easier to read.
>>>
>>>diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
>>>--- a/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
>>>+++ b/drivers/net/tg3.c	Wed Aug 13 10:29:08 2003
>>
>>
>>This patch is ok with me.
>>
>>And I agree with David that, in generic, C99 initializers is the way to 
>>go.  However, the higher level point remains:
>>
>>PCI IDs, and data like them, are fundamentally not C code.
> 
> 
> But the kernel, using C code, uses those ids to match drivers to
> devices.  So we have to create C structures out of those ids some how.
> 
> The idea was that since the kernel already keeps track of these ids, we
> might as well export them to userspace, so that it too can see what the
> kernel support.  That brought forth the modules.*map files and enabled
> the hotplug scripts to automatically load a module based on a device id
> (this is much nicer than other os schemes which force a text file to be
> created for every driver listing these ids.  They are usually created by
> hand, and can get out of sync.)

Oh, no argument about how we got here.  The ids started out in C code 
for good reasons.  Linus always says "do what you need to do, and no 
more" and IMO he's right.  And we did exactly that :)


> I agree that now that more and more tools are using this data, we should
> put it into a form that everyone can easily get at, without having to
> parse module attributes or even the modules.*map files.
> 
> Any suggestions that do not involve XML?  :)

Again, my philosophy:  put the data in its most natural form.  In 
CS-speak, domain-specific languages.  So, just figure out what you want 
the data files to look like, and I'll volunteer to write the parser for it.

An overall goal for metadata is to collect it in one place.  I mentioned 
earlier about moving the simple "obj-$(foo) += foo.o" out of Makefiles 
and into Kconfig.  So putting PCI IDs in Kconfig files is one idea. 
Note that Kconfigs can be split up and #include'd, so it can be 
partitioned neatly in a single directory as the maintainer desires.

Another option is a few collections of files:  drivers/net/pci.ids, 
drivers/sound/pci.ids, and these would hold pci ids and driver assocations.

I'm sure the people CC'd here have even better suggestions.

One the PCI ID data format is chosen, automated tools generate the 
required C code so that the kernel source code (and behavior) is 
essentially unchanged.

	Jeff

Re: C99 Initialisers

[Russell King]

On Wed, Aug 13, 2003 at 02:26:11PM -0400, Jeff Garzik wrote:
> Again, my philosophy:  put the data in its most natural form.  In 
> CS-speak, domain-specific languages.  So, just figure out what you want 
> the data files to look like, and I'll volunteer to write the parser for it.

But what's the point of the extra complexity?  People already put
references to other structures in the driver_data element, and
enums, so completely splitting the device IDs from the module
source is not going to be practical.

Are you thinking of a parser which outputs C code for the module to
include?  That might be made to work, but it doesn't sound as elegant
as the solution being described previously in this thread.

"Make the easy things easy (PCI_DEVICE(vendor,device)) and make the
not so easy things possible (the long form)"

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: C99 Initialisers

[Jeff Garzik]

Russell King wrote:
> On Wed, Aug 13, 2003 at 02:26:11PM -0400, Jeff Garzik wrote:
> 
>>Again, my philosophy:  put the data in its most natural form.  In 
>>CS-speak, domain-specific languages.  So, just figure out what you want 
>>the data files to look like, and I'll volunteer to write the parser for it.
> 
> 
> But what's the point of the extra complexity?  People already put
> references to other structures in the driver_data element, and
> enums, so completely splitting the device IDs from the module
> source is not going to be practical.

enums are easy  putting direct references would be annoying, but I also 
argue it's potentially broken and wrong to store and export that 
information publicly anyway.  The use of enums instead of pointers is 
practically required because there is a many-to-one relationship of ids 
to board information structs.


> Are you thinking of a parser which outputs C code for the module to
> include?  That might be made to work, but it doesn't sound as elegant
> as the solution being described previously in this thread.
> 
> "Make the easy things easy (PCI_DEVICE(vendor,device)) and make the
> not so easy things possible (the long form)"

That ignores the people who also need to get at the data, which must 
first be compiled from C into object code, then extracted via modutils, 
then turned into a computer readable form again, then used.

	Jeff

Re: C99 Initialisers

[Matthew Wilcox]

On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> enums are easy  putting direct references would be annoying, but I also 
> argue it's potentially broken and wrong to store and export that 
> information publicly anyway.  The use of enums instead of pointers is 
> practically required because there is a many-to-one relationship of ids 
> to board information structs.

The hard part is that it's actually many-to-many.  The same card can have
multiple drivers.  one driver can support many cards.

Let me give you a true story that your solution needs to address.
I recently got myself a Compaq Evo with an eepro100 onboard.  So I took
my Debian 3.0 CD and tried to install on it.  Failed because the eepro
on the board had PCI IDs that were more recent than the driver.

So I took the driver module, put it on a floppy, hand-edited the binary
to replace one of the PCI IDs with the ones that came back from lspci.
Stuck the floppy back in the Evo, loaded the hacked module and finished
the install.  Then compiled a new kernel ;-)

I haven't seen anything to address this in a nicer way yet.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 08:54:12PM +0100, Matthew Wilcox wrote:
> On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> > enums are easy  putting direct references would be annoying, but I also 
> > argue it's potentially broken and wrong to store and export that 
> > information publicly anyway.  The use of enums instead of pointers is 
> > practically required because there is a many-to-one relationship of ids 
> > to board information structs.
> 
> The hard part is that it's actually many-to-many.  The same card can have
> multiple drivers.  one driver can support many cards.
> 
> Let me give you a true story that your solution needs to address.
> I recently got myself a Compaq Evo with an eepro100 onboard.  So I took
> my Debian 3.0 CD and tried to install on it.  Failed because the eepro
> on the board had PCI IDs that were more recent than the driver.
> 
> So I took the driver module, put it on a floppy, hand-edited the binary
> to replace one of the PCI IDs with the ones that came back from lspci.
> Stuck the floppy back in the Evo, loaded the hacked module and finished
> the install.  Then compiled a new kernel ;-)
> 
> I haven't seen anything to address this in a nicer way yet.

Then you haven't seen the "new_id" file in sysfs for all pci drivers,
have you?  :)

greg k-h

Re: C99 Initialisers

[Dave Jones]

On Wed, Aug 13, 2003 at 08:54:12PM +0100, Matthew Wilcox wrote:
 > 
 > So I took the driver module, put it on a floppy, hand-edited the binary
 > to replace one of the PCI IDs with the ones that came back from lspci.
 > Stuck the floppy back in the Evo, loaded the hacked module and finished
 > the install.  Then compiled a new kernel ;-)
 > 
 > I haven't seen anything to address this in a nicer way yet.

This situation is exactly what the new_id stuff in sysfs is for AIUI.

		Dave

-- 
 Dave Jones     http://www.codemonkey.org.uk

Re: C99 Initialisers

[Krzysztof Halasa]

Greg KH <greg@kroah.com> writes:

> -	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC9100,
> -	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701) },

We might even consider

+	{ PCI_DEVICE(BROADCOM, TIGON3_5700) },
+	{ PCI_DEVICE(BROADCOM, TIGON3_5701) },

As probably using anything but PCI_DEVICE_ID_* and PCI_VENDOR_ID_*
would be a bug.

+#define PCI_DEVICE(vend,dev) \
+	.vendor = (PCI_VENDOR_ID_##vend), .device = (PCI_DEVICE_ID_##dev), \
+	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID

-- 
Krzysztof Halasa
Network Administrator

Re: C99 Initialisers

[Jeff Garzik]

Matthew Wilcox wrote:
> On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> 
>>enums are easy  putting direct references would be annoying, but I also 
>>argue it's potentially broken and wrong to store and export that 
>>information publicly anyway.  The use of enums instead of pointers is 
>>practically required because there is a many-to-one relationship of ids 
>>to board information structs.
> 
> 
> The hard part is that it's actually many-to-many.  The same card can have
> multiple drivers.  one driver can support many cards.

pci_device_tables are (and must be) at per-driver granularity.  Sure the 
same card can have multiple drivers, but that doesn't really matter in 
this context, simply because I/we cannot break that per-driver 
granularity.  Any solution must maintain per-driver granularity.


> Let me give you a true story that your solution needs to address.
> I recently got myself a Compaq Evo with an eepro100 onboard.  So I took
> my Debian 3.0 CD and tried to install on it.  Failed because the eepro
> on the board had PCI IDs that were more recent than the driver.
> 
> So I took the driver module, put it on a floppy, hand-edited the binary
> to replace one of the PCI IDs with the ones that came back from lspci.
> Stuck the floppy back in the Evo, loaded the hacked module and finished
> the install.  Then compiled a new kernel ;-)
> 
> I haven't seen anything to address this in a nicer way yet.

Well, the step I wish to take - moving the ids from location X to 
location Y, would have no effect on that scenario, positive or negative.

	Jeff

Re: C99 Initialisers

[Matt Domsch]

> This situation is exactly what the new_id stuff in sysfs is for AIUI.

And coincidentally, the e100 driver is what I hacked to test new_id with, 
so it's even pretty likely to work.  :-)

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: C99 Initialisers

[Sam Ravnborg]

On Wed, Aug 13, 2003 at 04:29:21PM -0400, Jeff Garzik wrote:
> pci_device_tables are (and must be) at per-driver granularity.  Sure the 
> same card can have multiple drivers, but that doesn't really matter in 
> this context, simply because I/we cannot break that per-driver 
> granularity.  Any solution must maintain per-driver granularity.

So you are still advocating for a KConfig enhancement?

Could you try to describe the layout of a KConfig file that you
have in mind.

I gave it a shot:

It must specify - 
a) Objects file used for the driver
b) module name of the driver
c) optional object files used by that driver
d) data used by the driver, for example PCI Data?
e) other stuff?

driver MAXTOR_SATA "SATA for Maxtor IDE"
	depends on LIB_SATA
	kbuild
	  obj-$(MAXTOR_SATA)  := maxtorsata.o
	  maxtorsata-y := libsata.o smaxtor.o
	  maxtorsata-$(VERBOSE_LOGGING) += maxtorlog.o
	data
	  PCIDEVICE(X,Y)

Not complete - and no dought with some missing pieces.
Primarly to try to find out what you have in mind.

	Sam

Re: C99 Initialisers

[Russell King]

On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> Russell King wrote:
> > But what's the point of the extra complexity?  People already put
> > references to other structures in the driver_data element, and
> > enums, so completely splitting the device IDs from the module
> > source is not going to be practical.
> 
> enums are easy  putting direct references would be annoying, but I also 
> argue it's potentially broken and wrong to store and export that 
> information publicly anyway.

Until new_id occurred, driver_data wasn't public, so this is a new
problem.

> The use of enums instead of pointers is practically required because
> there is a many-to-one relationship of ids to board information structs.

Hmm.  Now that driver_data is public, people will bitch when the enums
change.  This is _NOT_ something what I want to deal with - if I want
to add a new TI bridge type, I want to add the new TI enumeration
identifier along side the other TI enumeration identifiers, not at the
end.  I don't want to worry about whether the user is using the enum
values or not.  (eg, so I can use expressions like: 
 if (id->driver_data >= first_ti_id && id->driver_data <= last_ti_id))

By separating this, it effectively taking away some of the driver authors
freedoms to make choices like this, and this /will/ make driver code more
ugly.

> > Are you thinking of a parser which outputs C code for the module to
> > include?  That might be made to work, but it doesn't sound as elegant
> > as the solution being described previously in this thread.
> > 
> > "Make the easy things easy (PCI_DEVICE(vendor,device)) and make the
> > not so easy things possible (the long form)"
> 
> That ignores the people who also need to get at the data, which must 
> first be compiled from C into object code, then extracted via modutils, 
> then turned into a computer readable form again, then used.

Could you describe the "user" side of your idea more?  ie, what would
get installed onto the filesystem, and how would the tables be used?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: C99 Initialisers

[Eduardo Pereira Habkost]

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

On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
<snip>
> 
> That ignores the people who also need to get at the data, which must 
> first be compiled from C into object code, then extracted via modutils, 
> then turned into a computer readable form again, then used.

Agreed. Someone could want to look at the data about the pci devices
_before_ the module is compiled, without needing to compile the module
or parsing C code.

I'm not sure if it will be worth, but it would be possible, for example,
to have a tool that says what modules you'll need to compile, just
looking at your hardware, at config time.

Just my 2 cents,

-- 
Eduardo

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

Re: C99 Initialisers

[David S. Miller]

On 13 Aug 2003 22:21:48 +0200
Krzysztof Halasa <khc@pm.waw.pl> wrote:

> We might even consider
> 
> +	{ PCI_DEVICE(BROADCOM, TIGON3_5700) },
> +	{ PCI_DEVICE(BROADCOM, TIGON3_5701) },
> 

I personally don't like that, that breaks grepping.

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 10:21:48PM +0200, Krzysztof Halasa wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > -	{ PCI_VENDOR_ID_ALTIMA, PCI_DEVICE_ID_ALTIMA_AC9100,
> > -	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5700) },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5701) },
> 
> We might even consider
> 
> +	{ PCI_DEVICE(BROADCOM, TIGON3_5700) },
> +	{ PCI_DEVICE(BROADCOM, TIGON3_5701) },
> 
> As probably using anything but PCI_DEVICE_ID_* and PCI_VENDOR_ID_*
> would be a bug.

Someone else mentioned that to me, but that's just mean as this file
shows that not all device ids are in the pci id table.

Maybe that's a 2.7 thing :)

thanks,

greg k-h

Re: C99 Initialisers

[Roman Zippel]

Hi,

On Wed, 13 Aug 2003, Sam Ravnborg wrote:

> driver MAXTOR_SATA "SATA for Maxtor IDE"
> 	depends on LIB_SATA
> 	kbuild
> 	  obj-$(MAXTOR_SATA)  := maxtorsata.o
> 	  maxtorsata-y := libsata.o smaxtor.o
> 	  maxtorsata-$(VERBOSE_LOGGING) += maxtorlog.o
> 	data
> 	  PCIDEVICE(X,Y)

Something I really want to avoid is Makefile specific syntax in Kconfig.
IMO it should somehow like this:

module maxtorsata MAXTOR_SATA
	{tristate|prompt} "SATA for Maxtor IDE"
	depends on LIB_SATA
	source smaxtor.c
	source maxtorlog.c if MAXTOR_VERBOSE
	...

bye, Roman

Re: C99 Initialisers

[H. Peter Anvin]

Followup to:  <20030813000841.GP10015@parcelfarce.linux.theplanet.co.uk>
By author:    Matthew Wilcox <willy@debian.org>
In newsgroup: linux.dev.kernel
>
> On Wed, Aug 13, 2003 at 12:53:24AM +0100, Dave Jones wrote:
> > What would be *really* nice, would be the ability to do something
> > to the effect of..
> 
> While we're off in never-never land, it'd be nice to specify default
> values for struct initialisers.  eg, something like:
> 
> struct pci_device_id {
>         __u32 vendor = PCI_ANY_ID;
>         __u32 device = PCI_ANY_ID;
>         __u32 subvendor = PCI_ANY_ID;
> 	__u32 subdevice = PCI_ANY_ID;
>         __u32 class = 0;
> 	__u32 class_mask = 0;
>         kernel_ulong_t driver_data = 0;
> };
> 
> Erm, hang on a second ...  Since when are PCI IDs 32-bit?  What is this
> ridiculous bloat?  You can't even argue that this makes things pack
> better since this packs equally well:
> 

I usually find that treating VID:DID and SVID:SDID as two 32-bit
numbers makes a lot more sense than four 16-bit fields.

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
If you send me mail in HTML format I will assume it's spam.
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64

Re: C99 Initialisers

[Maciej Soltysiak]

> Cool. Since noone screamed 'OH FOR THE LOVE OF GOD, NO!' I'll do it (or
> at least try to :) Should hopefully have something by the weekend. :)
OH FOR THE LOVE OF GOD, PLEASE Include these i made some time ago, and
tried sending them twice now :)))

Apply to current 2.6 (vanilla,bk,mm, all's fine)

diff -Nru linux-2.6.0-test2/sound/oss/ac97_plugin_ad1980.c linux-2.6.0-test2-bk1/sound/oss/ac97_plugin_ad1980.c
--- linux-2.6.0-test2/sound/oss/ac97_plugin_ad1980.c	2003-07-27 18:55:53.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/ac97_plugin_ad1980.c	2003-08-03 09:00:34.000000000 +0200
@@ -83,11 +83,11 @@


 static struct ac97_driver ad1980_driver = {
-	codec_id: 0x41445370,
-	codec_mask: 0xFFFFFFFF,
-	name: "AD1980 example",
-	probe:	ad1980_probe,
-	remove: __devexit_p(ad1980_remove),
+	.codec_id	= 0x41445370,
+	.codec_mask	= 0xFFFFFFFF,
+	.name		= "AD1980 example",
+	.probe		= ad1980_probe,
+	.remove		= __devexit_p(ad1980_remove),
 };

 /**
diff -Nru linux-2.6.0-test2/sound/oss/ad1889.c linux-2.6.0-test2-bk1/sound/oss/ad1889.c
--- linux-2.6.0-test2/sound/oss/ad1889.c	2003-07-27 19:00:25.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/ad1889.c	2003-08-03 08:59:09.000000000 +0200
@@ -775,14 +775,14 @@
 }

 static struct file_operations ad1889_fops = {
-	llseek:         no_llseek,
-	read:           ad1889_read,
-	write:          ad1889_write,
-	poll:           ad1889_poll,
-	ioctl:          ad1889_ioctl,
-	mmap:           ad1889_mmap,
-	open:           ad1889_open,
-	release:        ad1889_release,
+	.llseek		= no_llseek,
+	.read		= ad1889_read,
+	.write		= ad1889_write,
+	.poll		= ad1889_poll,
+	.ioctl		= ad1889_ioctl,
+	.mmap		= ad1889_mmap,
+	.open		= ad1889_open,
+	.release	= ad1889_release,
 };

 /************************* /dev/mixer interfaces ************************ */
@@ -810,10 +810,10 @@
 }

 static struct file_operations ad1889_mixer_fops = {
-	llseek:         no_llseek,
-	ioctl:		ad1889_mixer_ioctl,
-	open:		ad1889_mixer_open,
-	release:	ad1889_mixer_release,
+	.llseek		= no_llseek,
+	.ioctl		= ad1889_mixer_ioctl,
+	.open		= ad1889_mixer_open,
+	.release	= ad1889_mixer_release,
 };

 /************************* AC97 interfaces ****************************** */
@@ -1064,10 +1064,10 @@
 MODULE_LICENSE("GPL");

 static struct pci_driver ad1889_driver = {
-	name:		DEVNAME,
-	id_table:	ad1889_id_tbl,
-	probe:		ad1889_probe,
-	remove:		__devexit_p(ad1889_remove),
+	.name		= DEVNAME,
+	.id_table	= ad1889_id_tbl,
+	.probe		= ad1889_probe,
+	.remove		= __devexit_p(ad1889_remove),
 };

 static int __init ad1889_init_module(void)
diff -Nru linux-2.6.0-test2/sound/oss/ali5455.c linux-2.6.0-test2-bk1/sound/oss/ali5455.c
--- linux-2.6.0-test2/sound/oss/ali5455.c	2003-07-27 19:07:14.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/ali5455.c	2003-08-03 08:59:09.000000000 +0200
@@ -2933,15 +2933,15 @@
 }

 static /*const */ struct file_operations ali_audio_fops = {
-	owner:THIS_MODULE,
-	llseek:no_llseek,
-	read:ali_read,
-	write:ali_write,
-	poll:ali_poll,
-	ioctl:ali_ioctl,
-	mmap:ali_mmap,
-	open:ali_open,
-	release:ali_release,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.read		= ali_read,
+	.write		= ali_write,
+	.poll		= ali_poll,
+	.ioctl		= ali_ioctl,
+	.mmap		= ali_mmap,
+	.open		= ali_open,
+	.release	= ali_release,
 };

 /* Read AC97 codec registers */
@@ -3060,10 +3060,10 @@
 }

 static /*const */ struct file_operations ali_mixer_fops = {
-	owner:THIS_MODULE,
-	llseek:no_llseek,
-	ioctl:ali_ioctl_mixdev,
-	open:ali_open_mixdev,
+	.owner	= THIS_MODULE,
+	.llseek	= no_llseek,
+	.ioctl	= ali_ioctl_mixdev,
+	.open	= ali_open_mixdev,
 };

 /* AC97 codec initialisation.  These small functions exist so we don't
@@ -3661,10 +3661,13 @@
 MODULE_PARM(controller_independent_spdif_locked, "i");
 #define ALI5455_MODULE_NAME "ali5455"
 static struct pci_driver ali_pci_driver = {
-	name:ALI5455_MODULE_NAME, id_table:ali_pci_tbl, probe:ali_probe,
-	    remove:__devexit_p(ali_remove),
+	.name		= ALI5455_MODULE_NAME,
+	.id_table	= ali_pci_tbl,
+	.probe		= ali_probe,
+	.remove		= __devexit_p(ali_remove),
 #ifdef CONFIG_PM
-	suspend:ali_pm_suspend, resume:ali_pm_resume,
+	.suspend	= ali_pm_suspend,
+	.resume		= ali_pm_resume,
 #endif				/* CONFIG_PM */
 };

diff -Nru linux-2.6.0-test2/sound/oss/au1000.c linux-2.6.0-test2-bk1/sound/oss/au1000.c
--- linux-2.6.0-test2/sound/oss/au1000.c	2003-07-27 19:04:19.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/au1000.c	2003-08-03 08:59:09.000000000 +0200
@@ -911,11 +911,11 @@
 }

 static /*const */ struct file_operations au1000_mixer_fops = {
-	owner:THIS_MODULE,
-	llseek:au1000_llseek,
-	ioctl:au1000_ioctl_mixdev,
-	open:au1000_open_mixdev,
-	release:au1000_release_mixdev,
+	.owner		= THIS_MODULE,
+	.llseek		= au1000_llseek,
+	.ioctl		= au1000_ioctl_mixdev,
+	.open		= au1000_open_mixdev,
+	.release	= au1000_release_mixdev,
 };

 /* --------------------------------------------------------------------- */
@@ -1940,15 +1940,15 @@
 }

 static /*const */ struct file_operations au1000_audio_fops = {
-	owner:		THIS_MODULE,
-	llseek:		au1000_llseek,
-	read:		au1000_read,
-	write:		au1000_write,
-	poll:		au1000_poll,
-	ioctl:		au1000_ioctl,
-	mmap:		au1000_mmap,
-	open:		au1000_open,
-	release:	au1000_release,
+	.owner		= THIS_MODULE,
+	.llseek		= au1000_llseek,
+	.read		= au1000_read,
+	.write		= au1000_write,
+	.poll		= au1000_poll,
+	.ioctl		= au1000_ioctl,
+	.mmap		= au1000_mmap,
+	.open		= au1000_open,
+	.release	= au1000_release,
 };


diff -Nru linux-2.6.0-test2/sound/oss/forte.c linux-2.6.0-test2-bk1/sound/oss/forte.c
--- linux-2.6.0-test2/sound/oss/forte.c	2003-07-27 19:00:29.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/forte.c	2003-08-03 08:59:09.000000000 +0200
@@ -361,11 +361,11 @@


 static struct file_operations forte_mixer_fops = {
-	owner:	    		THIS_MODULE,
-	llseek:         	no_llseek,
-	ioctl:          	forte_mixer_ioctl,
-	open:           	forte_mixer_open,
-	release:        	forte_mixer_release,
+	.owner			= THIS_MODULE,
+	.llseek         	= no_llseek,
+	.ioctl          	= forte_mixer_ioctl,
+	.open           	= forte_mixer_open,
+	.release        	= forte_mixer_release,
 };


@@ -1632,15 +1632,15 @@


 static struct file_operations forte_dsp_fops = {
-	owner:			THIS_MODULE,
-	llseek:     		&no_llseek,
-	read:       		&forte_dsp_read,
-	write:      		&forte_dsp_write,
-	poll:       		&forte_dsp_poll,
-	ioctl:      		&forte_dsp_ioctl,
-	open:       		&forte_dsp_open,
-	release:    		&forte_dsp_release,
-	mmap:			&forte_dsp_mmap,
+	.owner			= THIS_MODULE,
+	.llseek     		= &no_llseek,
+	.read       		= &forte_dsp_read,
+	.write      		= &forte_dsp_write,
+	.poll       		= &forte_dsp_poll,
+	.ioctl      		= &forte_dsp_ioctl,
+	.open       		= &forte_dsp_open,
+	.release    		= &forte_dsp_release,
+	.mmap			= &forte_dsp_mmap,
 };


@@ -2099,10 +2099,10 @@


 static struct pci_driver forte_pci_driver = {
-	name:       		DRIVER_NAME,
-	id_table:   		forte_pci_ids,
-	probe:      		forte_probe,
-	remove:     		forte_remove,
+	.name			= DRIVER_NAME,
+	.id_table		= forte_pci_ids,
+	.probe	 		= forte_probe,
+	.remove			= forte_remove,

 };

diff -Nru linux-2.6.0-test2/sound/oss/hal2.c linux-2.6.0-test2-bk1/sound/oss/hal2.c
--- linux-2.6.0-test2/sound/oss/hal2.c	2003-08-03 09:15:09.651638104 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/hal2.c	2003-08-03 08:59:09.000000000 +0200
@@ -1313,22 +1313,22 @@
 }

 static struct file_operations hal2_audio_fops = {
-	owner:		THIS_MODULE,
-	llseek:		no_llseek,
-	read:		hal2_read,
-	write:		hal2_write,
-	poll:		hal2_poll,
-	ioctl:		hal2_ioctl,
-	open:		hal2_open,
-	release:	hal2_release,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.read		= hal2_read,
+	.write		= hal2_write,
+	.poll		= hal2_poll,
+	.ioctl		= hal2_ioctl,
+	.open		= hal2_open,
+	.release	= hal2_release,
 };

 static struct file_operations hal2_mixer_fops = {
-	owner:		THIS_MODULE,
-	llseek:		no_llseek,
-	ioctl:		hal2_ioctl_mixdev,
-	open:		hal2_open_mixdev,
-	release:	hal2_release_mixdev,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.ioctl		= hal2_ioctl_mixdev,
+	.open		= hal2_open_mixdev,
+	.release	= hal2_release_mixdev,
 };

 static int hal2_request_irq(hal2_card_t *hal2, int irq)
diff -Nru linux-2.6.0-test2/sound/oss/harmony.c linux-2.6.0-test2-bk1/sound/oss/harmony.c
--- linux-2.6.0-test2/sound/oss/harmony.c	2003-08-03 09:15:09.653637800 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/harmony.c	2003-08-03 08:59:09.000000000 +0200
@@ -822,14 +822,14 @@
  */

 static struct file_operations harmony_audio_fops = {
-	owner:	THIS_MODULE,
-	llseek:	no_llseek,
-	read: 	harmony_audio_read,
-	write:	harmony_audio_write,
-	poll: 	harmony_audio_poll,
-	ioctl: 	harmony_audio_ioctl,
-	open: 	harmony_audio_open,
-	release:harmony_audio_release,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.read		= harmony_audio_read,
+	.write		= harmony_audio_write,
+	.poll		= harmony_audio_poll,
+	.ioctl		= harmony_audio_ioctl,
+	.open		= harmony_audio_open,
+	.release	= harmony_audio_release,
 };

 static int harmony_audio_init(void)
@@ -1144,11 +1144,11 @@
 }

 static struct file_operations harmony_mixer_fops = {
-	owner:		THIS_MODULE,
-	llseek:		no_llseek,
-	open:		harmony_mixer_open,
-	release:	harmony_mixer_release,
-	ioctl:		harmony_mixer_ioctl,
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.open		= harmony_mixer_open,
+	.release	= harmony_mixer_release,
+	.ioctl		= harmony_mixer_ioctl,
 };


@@ -1274,9 +1274,9 @@
 MODULE_DEVICE_TABLE(parisc, harmony_tbl);

 static struct parisc_driver harmony_driver = {
-	name:		"Lasi Harmony",
-	id_table:	harmony_tbl,
-	probe:		harmony_driver_callback,
+	.name		= "Lasi Harmony",
+	.id_table	= harmony_tbl,
+	.probe		= harmony_driver_callback,
 };

 static int __init init_harmony(void)
diff -Nru linux-2.6.0-test2/sound/oss/ite8172.c linux-2.6.0-test2-bk1/sound/oss/ite8172.c
--- linux-2.6.0-test2/sound/oss/ite8172.c	2003-07-27 18:57:51.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/ite8172.c	2003-08-03 09:02:01.000000000 +0200
@@ -1003,11 +1003,11 @@
 }

 static /*const*/ struct file_operations it8172_mixer_fops = {
-	owner:	THIS_MODULE,
-	llseek:	it8172_llseek,
-	ioctl:	it8172_ioctl_mixdev,
-	open:	it8172_open_mixdev,
-	release:	it8172_release_mixdev,
+	.owner		= THIS_MODULE,
+	.llseek		= it8172_llseek,
+	.ioctl		= it8172_ioctl_mixdev,
+	.open		= it8172_open_mixdev,
+	.release	= it8172_release_mixdev,
 };

 /* --------------------------------------------------------------------- */
@@ -1872,15 +1872,15 @@
 }

 static /*const*/ struct file_operations it8172_audio_fops = {
-	owner:	THIS_MODULE,
-	llseek:	it8172_llseek,
-	read:	it8172_read,
-	write:	it8172_write,
-	poll:	it8172_poll,
-	ioctl:	it8172_ioctl,
-	mmap:	it8172_mmap,
-	open:	it8172_open,
-	release:	it8172_release,
+	.owner		= THIS_MODULE,
+	.llseek		= it8172_llseek,
+	.read		= it8172_read,
+	.write		= it8172_write,
+	.poll		= it8172_poll,
+	.ioctl		= it8172_ioctl,
+	.mmap		= it8172_mmap,
+	.open		= it8172_open,
+	.release	= it8172_release,
 };


diff -Nru linux-2.6.0-test2/sound/oss/kahlua.c linux-2.6.0-test2-bk1/sound/oss/kahlua.c
--- linux-2.6.0-test2/sound/oss/kahlua.c	2003-07-27 19:09:14.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/kahlua.c	2003-08-03 09:03:22.000000000 +0200
@@ -203,10 +203,10 @@
 MODULE_DEVICE_TABLE(pci, id_tbl);

 static struct pci_driver kahlua_driver = {
-	name:		"kahlua",
-	id_table:	id_tbl,
-	probe:		probe_one,
-	remove:         __devexit_p(remove_one),
+	.name		= "kahlua",
+	.id_table	= id_tbl,
+	.probe		= probe_one,
+	.remove		= __devexit_p(remove_one),
 };


diff -Nru linux-2.6.0-test2/sound/oss/swarm_cs4297a.c linux-2.6.0-test2-bk1/sound/oss/swarm_cs4297a.c
--- linux-2.6.0-test2/sound/oss/swarm_cs4297a.c	2003-08-03 09:15:09.668635520 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/swarm_cs4297a.c	2003-08-03 09:04:24.000000000 +0200
@@ -1590,10 +1590,10 @@
 //   Mixer file operations struct.
 // ******************************************************************************************
 static /*const */ struct file_operations cs4297a_mixer_fops = {
-	llseek:cs4297a_llseek,
-	ioctl:cs4297a_ioctl_mixdev,
-	open:cs4297a_open_mixdev,
-	release:cs4297a_release_mixdev,
+	.llseek		= cs4297a_llseek,
+	.ioctl		= cs4297a_ioctl_mixdev,
+	.open		= cs4297a_open_mixdev,
+	.release	= cs4297a_release_mixdev,
 };

 // ---------------------------------------------------------------------
@@ -2508,14 +2508,14 @@
 //   Wave (audio) file operations struct.
 // ******************************************************************************************
 static /*const */ struct file_operations cs4297a_audio_fops = {
-	llseek:cs4297a_llseek,
-	read:cs4297a_read,
-	write:cs4297a_write,
-	poll:cs4297a_poll,
-	ioctl:cs4297a_ioctl,
-	mmap:cs4297a_mmap,
-	open:cs4297a_open,
-	release:cs4297a_release,
+	.llseek		= cs4297a_llseek,
+	.read		= cs4297a_read,
+	.write		= cs4297a_write,
+	.poll		= cs4297a_poll,
+	.ioctl		= cs4297a_ioctl,
+	.mmap		= cs4297a_mmap,
+	.open		= cs4297a_open,
+	.release	= cs4297a_release,
 };

 static irqreturn_t cs4297a_interrupt(int irq, void *dev_id, struct pt_regs *regs)
diff -Nru linux-2.6.0-test2/sound/oss/via82cxxx_audio.c linux-2.6.0-test2-bk1/sound/oss/via82cxxx_audio.c
--- linux-2.6.0-test2/sound/oss/via82cxxx_audio.c	2003-07-27 18:59:39.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/oss/via82cxxx_audio.c	2003-08-03 09:05:26.000000000 +0200
@@ -398,10 +398,10 @@


 static struct pci_driver via_driver = {
-	name:		VIA_MODULE_NAME,
-	id_table:	via_pci_tbl,
-	probe:		via_init_one,
-	remove:		__devexit_p(via_remove_one),
+	.name		= VIA_MODULE_NAME,
+	.id_table	= via_pci_tbl,
+	.probe		= via_init_one,
+	.remove		= __devexit_p(via_remove_one),
 };


@@ -2057,15 +2057,15 @@
  */

 static struct file_operations via_dsp_fops = {
-	owner:		THIS_MODULE,
-	open:		via_dsp_open,
-	release:	via_dsp_release,
-	read:		via_dsp_read,
-	write:		via_dsp_write,
-	poll:		via_dsp_poll,
-	llseek: 	no_llseek,
-	ioctl:		via_dsp_ioctl,
-	mmap:		via_dsp_mmap,
+	.owner		= THIS_MODULE,
+	.open		= via_dsp_open,
+	.release	= via_dsp_release,
+	.read		= via_dsp_read,
+	.write		= via_dsp_write,
+	.poll		= via_dsp_poll,
+	.llseek		= no_llseek,
+	.ioctl		= via_dsp_ioctl,
+	.mmap		= via_dsp_mmap,
 };


@@ -2179,10 +2179,10 @@


 struct vm_operations_struct via_mm_ops = {
-	nopage:		via_mm_nopage,
+	.nopage		= via_mm_nopage,

 #ifndef VM_RESERVED
-	swapout:	via_mm_swapout,
+	.swapout	= via_mm_swapout,
 #endif
 };

diff -Nru linux-2.6.0-test2/sound/parisc/harmony.c linux-2.6.0-test2-bk1/sound/parisc/harmony.c
--- linux-2.6.0-test2/sound/parisc/harmony.c	2003-07-27 19:01:06.000000000 +0200
+++ linux-2.6.0-test2-bk1/sound/parisc/harmony.c	2003-08-03 09:10:17.000000000 +0200
@@ -1107,9 +1107,9 @@
  */

 static struct parisc_driver snd_card_harmony_driver = {
-	name:		"Lasi ALSA Harmony",
-	id_table:	snd_card_harmony_devicetbl,
-	probe:		snd_card_harmony_probe,
+	.name		= "Lasi ALSA Harmony",
+	.id_table	= snd_card_harmony_devicetbl,
+	.probe		= snd_card_harmony_probe,
 };

 static int __init alsa_card_harmony_init(void)

Re: C99 Initialisers

[Geert Uytterhoeven]

On Wed, 13 Aug 2003, Jeff Garzik wrote:
> > On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> >>enums are easy  putting direct references would be annoying, but I also 
> >>argue it's potentially broken and wrong to store and export that 
> >>information publicly anyway.  The use of enums instead of pointers is 
> >>practically required because there is a many-to-one relationship of ids 
> >>to board information structs.
> > 
> > The hard part is that it's actually many-to-many.  The same card can have
> > multiple drivers.  one driver can support many cards.
> 
> pci_device_tables are (and must be) at per-driver granularity.  Sure the 
> same card can have multiple drivers, but that doesn't really matter in 
> this context, simply because I/we cannot break that per-driver 
> granularity.  Any solution must maintain per-driver granularity.

Aren't there any `hidden multi-function in single-function' PCI devices out
there? E.g. cards with a serial and a parallel port?

At least for the Zorro bus, these exist. E.g. the Ariadne card contains both
Ethernet and 2 parallel ports, so the Ariadne Ethernet driver and the (still to
be written) Ariadne parallel port driver are both drivers for the same Zorro
device.

Gr{oetje,eeting}s,

						Geert

P.S. Yes, according to the IBM slides at LKS, m68k is dead ;-)
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: C99 Initialisers

[Gene Heskett]

On Thursday 14 August 2003 06:05, Geert Uytterhoeven wrote:
>On Wed, 13 Aug 2003, Jeff Garzik wrote:
>> > On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
>> >>enums are easy  putting direct references would be annoying, but
>> >> I also argue it's potentially broken and wrong to store and
>> >> export that information publicly anyway.  The use of enums
>> >> instead of pointers is practically required because there is a
>> >> many-to-one relationship of ids to board information structs.
>> >
>> > The hard part is that it's actually many-to-many.  The same card
>> > can have multiple drivers.  one driver can support many cards.
>>
>> pci_device_tables are (and must be) at per-driver granularity. 
>> Sure the same card can have multiple drivers, but that doesn't
>> really matter in this context, simply because I/we cannot break
>> that per-driver granularity.  Any solution must maintain
>> per-driver granularity.
>
>Aren't there any `hidden multi-function in single-function' PCI
> devices out there? E.g. cards with a serial and a parallel port?
>
>At least for the Zorro bus, these exist. E.g. the Ariadne card
> contains both Ethernet and 2 parallel ports, so the Ariadne
> Ethernet driver and the (still to be written) Ariadne parallel port
> driver are both drivers for the same Zorro device.

And don't forget the MFC-III, which as 2 more seriels and a parallel 
port, a quite popular card for the big box's.  But I can't, at this 
late date, certify the seriels were 16550 compliant.

>Gr{oetje,eeting}s,
>
>						Geert
>
>P.S. Yes, according to the IBM slides at LKS, m68k is dead ;-)
>--
>Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert@linux-m68k.org
>
>In personal conversations with technical people, I call myself a
> hacker. But when I'm talking to journalists I just say "programmer"
> or something like that. -- Linus Torvalds
>
>-
>To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/

-- 
Cheers, Gene
AMD K6-III@500mhz 320M
Athlon1600XP@1400mhz  512M
99.27% setiathome rank, not too shabby for a WV hillbilly
Yahoo.com attornies please note, additions to this message
by Gene Heskett are:
Copyright 2003 by Maurice Eugene Heskett, all rights reserved.

Re: C99 Initialisers

[jw schultz]

On Thu, Aug 14, 2003 at 12:05:28PM +0200, Geert Uytterhoeven wrote:
> On Wed, 13 Aug 2003, Jeff Garzik wrote:
> > > On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> > >>enums are easy  putting direct references would be annoying, but I also 
> > >>argue it's potentially broken and wrong to store and export that 
> > >>information publicly anyway.  The use of enums instead of pointers is 
> > >>practically required because there is a many-to-one relationship of ids 
> > >>to board information structs.
> > > 
> > > The hard part is that it's actually many-to-many.  The same card can have
> > > multiple drivers.  one driver can support many cards.
> > 
> > pci_device_tables are (and must be) at per-driver granularity.  Sure the 
> > same card can have multiple drivers, but that doesn't really matter in 
> > this context, simply because I/we cannot break that per-driver 
> > granularity.  Any solution must maintain per-driver granularity.
> 
> Aren't there any `hidden multi-function in single-function' PCI devices out
> there? E.g. cards with a serial and a parallel port?
> 
> At least for the Zorro bus, these exist. E.g. the Ariadne card contains both
> Ethernet and 2 parallel ports, so the Ariadne Ethernet driver and the (still to
> be written) Ariadne parallel port driver are both drivers for the same Zorro
> device.

I'm not sure but i think most of those look like multiple
pci devices rather than one device with multiple functions.
I've got an Initio 9520UW: One PCI card with two ini9x00 UW
SCSI HBAs sharing one interrupt and one EEPro100 on another
interrupt.  During scan it seems to me to be three devices
sitting behind a bridge.

This is on 2.4.18 so 2.6 may look a little different.

$ lspci -tvv
-[00]-+-00.0  Intel Corp. 440BX/ZX/DX - 82443BX/ZX/DX Host bridge
[snip +- a bunch of devices]
      \-0c.0-[02]--+-04.0  Initio Corporation 360P
                   +-08.0  Initio Corporation 360P
                   \-09.0  Intel Corp. 82557/8/9 [Ethernet Pro 100]

$ lspci -vv
[snip]
00:0c.0 PCI bridge: Digital Equipment Corporation DECchip 21152 (rev 02) (prog-if 00 [Normal decode])
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
        Status: Cap- 66Mhz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
        Latency: 32, cache line size 08
        Bus: primary=00, secondary=02, subordinate=02, sec-latency=32
        I/O behind bridge: 0000c000-0000cfff
        Memory behind bridge: de800000-dfffffff
        Prefetchable memory behind bridge: 00000000e2f00000-00000000e3e00000
        BridgeCtl: Parity- SERR- NoISA+ VGA- MAbort- >Reset- FastB2B-

02:04.0 SCSI storage controller: Initio Corporation 360P (rev 01)
        Subsystem: Unknown device 9292:0202
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
        Status: Cap- 66Mhz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
        Latency: 32, cache line size 08
        Interrupt: pin A routed to IRQ 9
        Region 0: I/O ports at c800 [size=256]
        Region 1: Memory at df800000 (32-bit, non-prefetchable) [size=4K]
        Expansion ROM at <unassigned> [disabled] [size=32K]

02:08.0 SCSI storage controller: Initio Corporation 360P (rev 01)
        Subsystem: Unknown device 9292:0202
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
        Status: Cap- 66Mhz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
        Latency: 32, cache line size 08
        Interrupt: pin A routed to IRQ 9
        Region 0: I/O ports at c400 [size=256]
        Region 1: Memory at df000000 (32-bit, non-prefetchable) [size=4K]
        Expansion ROM at <unassigned> [disabled] [size=32K]

02:09.0 Ethernet controller: Intel Corp. 82557/8/9 [Ethernet Pro 100] (rev 02)
        Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B-
        Status: Cap- 66Mhz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort
- <MAbort- >SERR- <PERR-
        Latency: 32 (2000ns min, 14000ns max)
        Interrupt: pin A routed to IRQ 11
        Region 0: Memory at e3000000 (32-bit, prefetchable) [size=4K]
        Region 1: I/O ports at c000 [size=32]
        Region 2: Memory at de800000 (32-bit, non-prefetchable) [size=1M]
        Expansion ROM at <unassigned> [disabled] [size=1M]


-- 
________________________________________________________________
	J.W. Schultz            Pegasystems Technologies
	email address:		jw@pegasys.ws

		Remember Cernan and Schmitt

Re: C99 Initialisers

[Geert Uytterhoeven]

On Thu, 14 Aug 2003, jw schultz wrote:
> On Thu, Aug 14, 2003 at 12:05:28PM +0200, Geert Uytterhoeven wrote:
> > On Wed, 13 Aug 2003, Jeff Garzik wrote:
> > > > On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> > > >>enums are easy  putting direct references would be annoying, but I also 
> > > >>argue it's potentially broken and wrong to store and export that 
> > > >>information publicly anyway.  The use of enums instead of pointers is 
> > > >>practically required because there is a many-to-one relationship of ids 
> > > >>to board information structs.
> > > > 
> > > > The hard part is that it's actually many-to-many.  The same card can have
> > > > multiple drivers.  one driver can support many cards.
> > > 
> > > pci_device_tables are (and must be) at per-driver granularity.  Sure the 
> > > same card can have multiple drivers, but that doesn't really matter in 
> > > this context, simply because I/we cannot break that per-driver 
> > > granularity.  Any solution must maintain per-driver granularity.
> > 
> > Aren't there any `hidden multi-function in single-function' PCI devices out
> > there? E.g. cards with a serial and a parallel port?
> > 
> > At least for the Zorro bus, these exist. E.g. the Ariadne card contains both
> > Ethernet and 2 parallel ports, so the Ariadne Ethernet driver and the (still to
> > be written) Ariadne parallel port driver are both drivers for the same Zorro
> > device.
> 
> I'm not sure but i think most of those look like multiple
> pci devices rather than one device with multiple functions.
> I've got an Initio 9520UW: One PCI card with two ini9x00 UW
> SCSI HBAs sharing one interrupt and one EEPro100 on another
> interrupt.  During scan it seems to me to be three devices
> sitting behind a bridge.

In most cases it is.

Just found a PCI example myself: there exists iDTV chips that connect to a PCI
bus. It's one device, but internally it has graphics, video, USB, IDE, ...
So you'll have different drivers.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: C99 Initialisers

[Andrey Panin]

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

On 226, 08 14, 2003 at 12:05:28PM +0200, Geert Uytterhoeven wrote:
> On Wed, 13 Aug 2003, Jeff Garzik wrote:
> > > On Wed, Aug 13, 2003 at 03:44:44PM -0400, Jeff Garzik wrote:
> > >>enums are easy  putting direct references would be annoying, but I also 
> > >>argue it's potentially broken and wrong to store and export that 
> > >>information publicly anyway.  The use of enums instead of pointers is 
> > >>practically required because there is a many-to-one relationship of ids 
> > >>to board information structs.
> > > 
> > > The hard part is that it's actually many-to-many.  The same card can have
> > > multiple drivers.  one driver can support many cards.
> > 
> > pci_device_tables are (and must be) at per-driver granularity.  Sure the 
> > same card can have multiple drivers, but that doesn't really matter in 
> > this context, simply because I/we cannot break that per-driver 
> > granularity.  Any solution must maintain per-driver granularity.
> 
> Aren't there any `hidden multi-function in single-function' PCI devices out
> there? E.g. cards with a serial and a parallel port?
 
Look at drivers/parport/parport-serial.c, it contains whole zoo of such beasts :)

-- 
Andrey Panin		| Linux and UNIX system administrator
pazke@donpac.ru		| PGP key: wwwkeys.pgp.net

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

Re: C99 Initialisers

[H. Peter Anvin]

Followup to:  <Pine.GSO.4.21.0308141202410.12289-100000@vervain.sonytel.be>
By author:    Geert Uytterhoeven <geert@linux-m68k.org>
In newsgroup: linux.dev.kernel
> 
> Aren't there any `hidden multi-function in single-function' PCI devices out
> there? E.g. cards with a serial and a parallel port?
> 

There probably are.  The easiest way to represent these is as a
"pseudo-bridge"; treat them as a bridge device with "ISA-like" serial
ports and parallel ports on the other side.

	-hpa

-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
If you send me mail in HTML format I will assume it's spam.
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64

Re: C99 Initialisers

[Sam Ravnborg]

On Thu, Aug 14, 2003 at 12:24:28AM +0200, Roman Zippel wrote:
> Something I really want to avoid is Makefile specific syntax in Kconfig.
I do not see the point in this.
Kconfig should treat this as a block of text - like the help section.
Only action to take is to:
1: Find all symbols enclosed in $()
	a: Check that it exists
	b: Append CONFIG_

Then Kconfig in each directory should generate a Kconfig.make file,
that will be included when kbuild reaches that directory.

> IMO it should somehow like this:
> 
> module maxtorsata MAXTOR_SATA
> 	{tristate|prompt} "SATA for Maxtor IDE"
> 	depends on LIB_SATA
> 	source smaxtor.c
> 	source maxtorlog.c if MAXTOR_VERBOSE

If using the above syntax - where do you see the rules being translated
to makefile syntax?
kbuild could do this - yes. But I do not see the point in having
the extra abstraction layer. Maybe you have something in mind I have
not envisioned?

	Sam

Re: C99 Initialisers

[Krzysztof Halasa]

Greg KH <greg@kroah.com> writes:

> > +	{ PCI_DEVICE(BROADCOM, TIGON3_5700) },
> > +	{ PCI_DEVICE(BROADCOM, TIGON3_5701) },
> > 
> Someone else mentioned that to me, but that's just mean as this file
> shows that not all device ids are in the pci id table.

PCI_VENDOR_ID_xxx can be #defined in the driver as well, no problem here.
Grepping - yes, real problem.

Using one 32-bit value for two 16-bit vendor and device IDs may be
worth considering, too. Some potential problems, though, not 2.6 I think.
-- 
Krzysztof Halasa
Network Administrator

Re: C99 Initialisers

[Greg KH]

On Wed, Aug 13, 2003 at 02:19:36PM -0700, junkio@cox.net wrote:
>  (1) If .subvendor and .subdevice are always PCI_ANY_ID, are
>      there any reason to keep them in the structure in the first
>      place?  I imagine there are some devices but not in the
>      tg3_pci_tbl list that need to have different values there,
>      but if that is the case we may want to generalize the macro
>      PCI_DEVICE like this:
> 
>         #define PCI_DEVICE(vend, dev) \
>             PCI_DEVICE_WITH_SUB(vend, dev, PCI_ANY_ID, PCI_ANY_ID)
>         #define PCI_DEVICE_WITH_SUB(vend, dev, subv, subd) \
>          .vendor = (vend), \
>          .device = (dev), \
>          .subvendor = (subv), \
>          .subdevice = (subd)

Patches always are gladly accepted :)

>  (2) PCI_VENDOR_ID_ and PCI_DEVICE_ID_ seem to be common prefix,
>      so how about doing something like this?
> 
>      #define PCI_DEVICE(vend,dev) \
>          .vendor = (PCI_VENDOR_ID_ ## vend), \
>          .device = (PCI_DEVICE_ID_ ## dev), \
>          .subvendor = PCI_ANY_ID, \
>          .subdevice = PCI_ANY_ID
> 
>      Then the table becomes much shorter:
> 
>      static struct pci_device_id tg3_pci_tbl[] = {
>      ...
>        { PCI_DEVICE(BROADCOM, TIGON3_5700) },
>        { PCI_DEVICE(BROADCOM, TIGON3_5701) },
>      ...

As has been responded before, this isn't a good idea right now.

thanks,

greg k-h

Re: C99 Initialisers

[junkio]

>>>>> "GKH" == Greg KH <greg@kroah.com> writes:

GKH> How about this patch?  If you like it I'll add the pci.h change to the
GKH> tree and let you take the tg3.c part.

Two comments:

GKH> ...
GKH> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5704S) },
GKH> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5702A3) },
GKH> +	{ PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, PCI_DEVICE_ID_TIGON3_5703A3) },
GKH> ...

GKH> +#define PCI_DEVICE(vend,dev) \
GKH> +	.vendor = (vend), .device = (dev), \
GKH> +	.subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID
 
 (1) If .subvendor and .subdevice are always PCI_ANY_ID, are
     there any reason to keep them in the structure in the first
     place?  I imagine there are some devices but not in the
     tg3_pci_tbl list that need to have different values there,
     but if that is the case we may want to generalize the macro
     PCI_DEVICE like this:

        #define PCI_DEVICE(vend, dev) \
            PCI_DEVICE_WITH_SUB(vend, dev, PCI_ANY_ID, PCI_ANY_ID)
        #define PCI_DEVICE_WITH_SUB(vend, dev, subv, subd) \
         .vendor = (vend), \
         .device = (dev), \
         .subvendor = (subv), \
         .subdevice = (subd)

 (2) PCI_VENDOR_ID_ and PCI_DEVICE_ID_ seem to be common prefix,
     so how about doing something like this?

     #define PCI_DEVICE(vend,dev) \
         .vendor = (PCI_VENDOR_ID_ ## vend), \
         .device = (PCI_DEVICE_ID_ ## dev), \
         .subvendor = PCI_ANY_ID, \
         .subdevice = PCI_ANY_ID

     Then the table becomes much shorter:

     static struct pci_device_id tg3_pci_tbl[] = {
     ...
       { PCI_DEVICE(BROADCOM, TIGON3_5700) },
       { PCI_DEVICE(BROADCOM, TIGON3_5701) },
     ...


    PCI_DEVICE_ID_xxx definitions for some devices that
    currently lack them need to be added, of course,
    e.g. SYSKONNECT 0x4400.

RE: C99 Initialisers

[Shureih, Tariq]

I personally see a difference and an advantage in making it easier to
read therefore easier to manage and maintain.

Sure it's a whole conversion process and may be a bit shocking to
existing drivers and people used to the struct in this format, but
that's nothing new to the kernel to yank something we're all so used to
and replace it with something that we would all benefit from in the long
term.


--
Tariq Shureih
 

*Opinions are my own and don't reflect those of my employer*
*But my two cents and what's left of my stock options :P*

> -----Original Message-----
> From: Matthew Wilcox [mailto:willy@debian.org]
> Sent: Tuesday, August 12, 2003 4:27 AM
> To: Greg KH
> Cc: Matthew Wilcox; Robert Love; CaT; linux-kernel@vger.kernel.org;
> kernel-janitor-discuss@lists.sourceforge.net
> Subject: Re: C99 Initialisers
> 
> On Mon, Aug 11, 2003 at 10:38:27PM -0700, Greg KH wrote:
> > On Tue, Aug 12, 2003 at 03:39:36AM +0100, Matthew Wilcox wrote:
> > > I don't think anyone would appreciate you converting that to:
> > >
> > > static struct pci_device_id tg3_pci_tbl[] __devinitdata = {
> > > 	{
> > > 		.vendor		= PCI_VENDOR_ID_BROADCOM,
> > > 		.device		= PCI_DEVICE_ID_TIGON3_5700,
> > > 		.subvendor	= PCI_ANY_ID,
> > > 		.subdevice	= PCI_ANY_ID,
> > > 		.class		= 0,
> > > 		.class_mask	= 0,
> > > 		.driver_data	= 0,
> > > 	},
> >
> > I sure would.  Oh, you can drop the .class, .class_mask, and
> > .driver_data lines, and then it even looks cleaner.
> >
> > I would love to see that kind of change made for pci drivers.
> 
> I really strongly disagree.  For a struct that's as well-known as
> pci_device_id, the argument of making it clearer doesn't make sense.
> It's also not subject to change, unlike say file_operations, so the
> argument of adding new elements without breaking anything is also not
> relevant.
> 
> In tg3, the table definition is already 32 lines long with 2 lines per
> device_id.  In the scheme you favour so much, that becomes 92 lines,
for
> no real gain that I can see.
> 
> --
> "It's not Hollywood.  War is real, war is primarily not about defeat
or
> victory, it is about death.  I've seen thousands and thousands of dead
> bodies.
> Do you think I want to have an academic debate on this subject?" --
Robert
> Fisk
> -
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/