2007-05-16 20:54:16

by Ben Collins

[permalink] [raw]
Subject: [PATCH] Remove duplicate ID in ipaq driver


Cc: Ganesh Varadarajan <[email protected]>
Cc: [email protected]
Signed-off-by: Ben Collins <[email protected]>

diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
index 4df0ec7..7c85be4 100644
--- a/drivers/usb/serial/ipaq.c
+++ b/drivers/usb/serial/ipaq.c
@@ -92,7 +92,6 @@ static void ipaq_destroy_lists(struct usb_serial_port *port);

static struct usb_device_id ipaq_id_table [] = {
/* The first entry is a placeholder for the insmod-specified device */
- { USB_DEVICE(0x049F, 0x0003) },
{ USB_DEVICE(0x0104, 0x00BE) }, /* Socket USB Sync */
{ USB_DEVICE(0x03F0, 0x1016) }, /* HP USB Sync */
{ USB_DEVICE(0x03F0, 0x1116) }, /* HP USB Sync 1611 */

--
Ubuntu : http://www.ubuntu.com/
Linux1394: http://wiki.linux1394.org/


2007-05-16 21:00:43

by Roland Dreier

[permalink] [raw]
Subject: Re: [PATCH] Remove duplicate ID in ipaq driver

> /* The first entry is a placeholder for the insmod-specified device */
> - { USB_DEVICE(0x049F, 0x0003) },

Is it obvious why this patch is correct? Especially given the
comment just before the line you delete, and the code

if (vendor) {
ipaq_id_table[0].idVendor = vendor;
ipaq_id_table[0].idProduct = product;
}

in ipaq_init()?

- R.

2007-05-16 21:04:34

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] Remove duplicate ID in ipaq driver

Ben Collins napsal(a):
> Cc: Ganesh Varadarajan <[email protected]>
> Cc: [email protected]
> Signed-off-by: Ben Collins <[email protected]>
>
> diff --git a/drivers/usb/serial/ipaq.c b/drivers/usb/serial/ipaq.c
> index 4df0ec7..7c85be4 100644
> --- a/drivers/usb/serial/ipaq.c
> +++ b/drivers/usb/serial/ipaq.c
> @@ -92,7 +92,6 @@ static void ipaq_destroy_lists(struct usb_serial_port *port);
>
> static struct usb_device_id ipaq_id_table [] = {
> /* The first entry is a placeholder for the insmod-specified device */

I wonder if the people ever read the comments? ^^^^^^^^^
It's used for parameter from command line when modprobing. See ipaq_init.

> - { USB_DEVICE(0x049F, 0x0003) },
> { USB_DEVICE(0x0104, 0x00BE) }, /* Socket USB Sync */
> { USB_DEVICE(0x03F0, 0x1016) }, /* HP USB Sync */
> { USB_DEVICE(0x03F0, 0x1116) }, /* HP USB Sync 1611 */
>

regards,
--
http://www.fi.muni.cz/~xslaby/ Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8 22A0 32CC 55C3 39D4 7A7E

2007-05-16 21:37:00

by Ben Collins

[permalink] [raw]
Subject: Re: [PATCH] Remove duplicate ID in ipaq driver

On Wed, 2007-05-16 at 13:59 -0700, Roland Dreier wrote:
> > /* The first entry is a placeholder for the insmod-specified device */
> > - { USB_DEVICE(0x049F, 0x0003) },
>
> Is it obvious why this patch is correct? Especially given the
> comment just before the line you delete, and the code
>
> if (vendor) {
> ipaq_id_table[0].idVendor = vendor;
> ipaq_id_table[0].idProduct = product;
> }
>
> in ipaq_init()?

My mistake, quick on the patching going through this dupe list.

Might I add that this is terrible use of the device table, though.
Clutters userspace, and adds processing to module-init-tools programs.

--
Ubuntu : http://www.ubuntu.com/
Linux1394: http://wiki.linux1394.org/

2007-05-17 12:45:12

by Greg KH

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] Remove duplicate ID in ipaq driver

On Wed, May 16, 2007 at 05:36:48PM -0400, Ben Collins wrote:
> On Wed, 2007-05-16 at 13:59 -0700, Roland Dreier wrote:
> > > /* The first entry is a placeholder for the insmod-specified device */
> > > - { USB_DEVICE(0x049F, 0x0003) },
> >
> > Is it obvious why this patch is correct? Especially given the
> > comment just before the line you delete, and the code
> >
> > if (vendor) {
> > ipaq_id_table[0].idVendor = vendor;
> > ipaq_id_table[0].idProduct = product;
> > }
> >
> > in ipaq_init()?
>
> My mistake, quick on the patching going through this dupe list.
>
> Might I add that this is terrible use of the device table, though.
> Clutters userspace, and adds processing to module-init-tools programs.

It's a hold-over from the times when we didn't have the sysfs "add a new
id" interface for usb-serial drivers, which only recently was created.

So we just have to live with it, and the infinitesimal speed hit it
creates :)

thanks,

greg k-h

2007-05-17 13:02:47

by Ben Collins

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] Remove duplicate ID in ipaq driver

On Thu, 2007-05-17 at 05:43 -0700, Greg KH wrote:
> On Wed, May 16, 2007 at 05:36:48PM -0400, Ben Collins wrote:
> > On Wed, 2007-05-16 at 13:59 -0700, Roland Dreier wrote:
> > > > /* The first entry is a placeholder for the insmod-specified device */
> > > > - { USB_DEVICE(0x049F, 0x0003) },
> > >
> > > Is it obvious why this patch is correct? Especially given the
> > > comment just before the line you delete, and the code
> > >
> > > if (vendor) {
> > > ipaq_id_table[0].idVendor = vendor;
> > > ipaq_id_table[0].idProduct = product;
> > > }
> > >
> > > in ipaq_init()?
> >
> > My mistake, quick on the patching going through this dupe list.
> >
> > Might I add that this is terrible use of the device table, though.
> > Clutters userspace, and adds processing to module-init-tools programs.
>
> It's a hold-over from the times when we didn't have the sysfs "add a new
> id" interface for usb-serial drivers, which only recently was created.
>
> So we just have to live with it, and the infinitesimal speed hit it
> creates :)

Any objection to adding it to planned-for-removal and spitting out a
printk when someone uses the "feature"?

--
Ubuntu : http://www.ubuntu.com/
Linux1394: http://wiki.linux1394.org/

2007-05-17 13:33:30

by Valdis Klētnieks

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] Remove duplicate ID in ipaq driver

On Thu, 17 May 2007 09:02:20 EDT, Ben Collins said:
> > So we just have to live with it, and the infinitesimal speed hit it
> > creates :)
>
> Any objection to adding it to planned-for-removal and spitting out a
> printk when someone uses the "feature"?

Do we have any good reason to believe that this will eventually lead to
a clean-up? ISTR that we did a similar printk for sysctl (or as Dr. Phil
likes to say, "How's that working for you?")


Attachments:
(No filename) (226.00 B)

2007-05-17 13:35:17

by Greg KH

[permalink] [raw]
Subject: Re: [linux-usb-devel] [PATCH] Remove duplicate ID in ipaq driver

On Thu, May 17, 2007 at 09:02:20AM -0400, Ben Collins wrote:
> On Thu, 2007-05-17 at 05:43 -0700, Greg KH wrote:
> > On Wed, May 16, 2007 at 05:36:48PM -0400, Ben Collins wrote:
> > > On Wed, 2007-05-16 at 13:59 -0700, Roland Dreier wrote:
> > > > > /* The first entry is a placeholder for the insmod-specified device */
> > > > > - { USB_DEVICE(0x049F, 0x0003) },
> > > >
> > > > Is it obvious why this patch is correct? Especially given the
> > > > comment just before the line you delete, and the code
> > > >
> > > > if (vendor) {
> > > > ipaq_id_table[0].idVendor = vendor;
> > > > ipaq_id_table[0].idProduct = product;
> > > > }
> > > >
> > > > in ipaq_init()?
> > >
> > > My mistake, quick on the patching going through this dupe list.
> > >
> > > Might I add that this is terrible use of the device table, though.
> > > Clutters userspace, and adds processing to module-init-tools programs.
> >
> > It's a hold-over from the times when we didn't have the sysfs "add a new
> > id" interface for usb-serial drivers, which only recently was created.
> >
> > So we just have to live with it, and the infinitesimal speed hit it
> > creates :)
>
> Any objection to adding it to planned-for-removal and spitting out a
> printk when someone uses the "feature"?

No, it's a module parameter and it's much easier to use for some systems
than the sysfs file way. It's just not worth breaking userspace APIs
for no good reason.

thanks,

greg k-h