Skip to contentSkip to conversation

*** SUBJECT HERE ***

PATCH In review Anders Hammarquist 3 participants 18 messages Jun 19, 2013 – Jul 25, 2013

Subsystems (3)
Stable branch USB USB serial
lore
Share & tools
lore

Anders Hammarquist

author
#0 lore raw

cover *** SUBJECT HERE ***

The USB cable to read out data from the Abbott FreeStyle Precision meters, known as the Abbott stip port cable, uses the TI 3410 chip, just as the already added stereo port cable. They are essestially the same cable, just with different connectors at the end.

This patch set adds the product id to the driver, and makes the product type more explicit. Arguably, the ABBOTT_PRODUCT_ID define could be removed, but I left it on the off chance that someone other that the TI 3410 driver uses it.

/Anders

Anders Hammarquist (2):
Add product id for Abbott strip port cable for Precision meter
which uses the TI 3410 chip.
Be explicit about the Abbott product ids being product ids.

 drivers/usb/serial/ti_usb_3410_5052.c |    3 ++-
 drivers/usb/serial/ti_usb_3410_5052.h |    4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.10.4
maintainer
#1 lore raw

I think you forgot a Subject :)

Quoted 9 lines · Anders Hammarquist

The USB cable to read out data from the Abbott FreeStyle Precision meters, known as the Abbott stip port cable, uses the TI 3410 chip, just as the already added stereo port cable. They are essestially the same cable, just with different connectors at the end.

This patch set adds the product id to the driver, and makes the product type more explicit. Arguably, the ABBOTT_PRODUCT_ID define could be removed, but I left it on the off chance that someone other that the TI 3410 driver uses it.

No, it doesn't, please fix up that last patch and resend it.

thanks,

greg k-h

author
#2 lore raw
Quoted 1 line · Greg KH

I think you forgot a Subject :)

Indeed. I blame it on my first fight with git format-patch ;-)

Quoted 7 lines · Anders Hammarquist
Quoted 4 lines

This patch set adds the product id to the driver, and makes the product type more explicit. Arguably, the ABBOTT_PRODUCT_ID define could be removed, but I left it on the off chance that someone other that the TI 3410 driver uses it.

No, it doesn't, please fix up that last patch and resend it.

Right, and in removing it I actually found that it was used in two places in the driver. I lost my second fight with format-patch so I'll just enclose the fixed patch below.

/Anders

---8<---
ti_usb_3410_5052:
* Remove unspecific ABBOTT_PRODUCT_ID
* Fix size of statically sized arrays

Signed-off-by Anders Hammarquist <[email protected]>
drivers/usb/serial/ti_usb_3410_5052.c +6 -5
index e581c25..26c1161 100644
@@ -158,7 +158,7 @@ static unsigned int product_5052_count;
 /* the array dimension is the number of default entries plus */
 /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
 /* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -172,8 +172,8 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 };
 

@@ -184,7 +184,7 @@ static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -202,7 +202,8 @@ static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 	{ }
 };
drivers/usb/serial/ti_usb_3410_5052.h +2 -3
index 4a2423e..d3ff470 100644
@@ -52,9 +52,8 @@
 
 /* Abbott Diabetics vendor and product ids */
 #define ABBOTT_VENDOR_ID		0x1a61
-#define ABBOTT_STEREO_PLUG_ID		0x3410
-#define ABBOTT_PRODUCT_ID		ABBOTT_STEREO_PLUG_ID
-#define ABBOTT_STRIP_PORT_ID		0x3420
+#define ABBOTT_STEREO_PLUG_PRODUCT_ID	0x3410
+#define ABBOTT_STRIP_PORT_PRODUCT_ID	0x3420
 
 /* Commands */
 #define TI_GET_VERSION			0x01
maintainer
#3 lore raw
Quoted 17 lines · Anders Hammarquist
Quoted 1 line · Greg KH

I think you forgot a Subject :)

Indeed. I blame it on my first fight with git format-patch ;-)

Quoted 7 lines · Anders Hammarquist
Quoted 4 lines

This patch set adds the product id to the driver, and makes the product type more explicit. Arguably, the ABBOTT_PRODUCT_ID define could be removed, but I left it on the off chance that someone other that the TI 3410 driver uses it.

No, it doesn't, please fix up that last patch and resend it.

Right, and in removing it I actually found that it was used in two places in the driver. I lost my second fight with format-patch so I'll just enclose the fixed patch below.

Quoted 8 lines

/Anders

---8<---
ti_usb_3410_5052:
* Remove unspecific ABBOTT_PRODUCT_ID
* Fix size of statically sized arrays

Signed-off-by Anders Hammarquist <[email protected]>

Please resend this in a format that I can apply it in (i.e. one that does not require me to edit it by hand...)

Also, please cc: the [email protected] mailing list for usb patches.

drivers/usb/serial/ti_usb_3410_5052.c static unsigned int product_5052_count; · line 161

Quoted 10 lines
drivers/usb/serial/ti_usb_3410_5052.c +1 -1
index e581c25..26c1161 100644
@@ -158,7 +158,7 @@ static unsigned int product_5052_count;
 /* the array dimension is the number of default entries plus */
 /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
 /* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {

That's a mess, why have it be a static array at all? Just include an empty one at the end.

Quoted 15 lines
drivers/usb/serial/ti_usb_3410_5052.c +2 -2
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -172,8 +172,8 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 };

drivers/usb/serial/ti_usb_3410_5052.c static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = { · line 187

Quoted 5 lines
drivers/usb/serial/ti_usb_3410_5052.c +1 -1
@@ -184,7 +184,7 @@ static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {

Same here. Although that might take another patch, to handle it all correctly, separate from this "change the device id names" patch.

thanks,

greg k-h

author
#4 lore raw
Quoted 2 lines · Greg KH

Please resend this in a format that I can apply it in (i.e. one that does not require me to edit it by hand...)

After more fighting with git, I belive I now made it spit out what I wanted. Patch 1/2 ahead.

Quoted 5 lines

drivers/usb/serial/ti_usb_3410_5052.c static unsigned int product_5052_count; · line 161

Quoted 2 lines
drivers/usb/serial/ti_usb_3410_5052.c +1 -1
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {

That's a mess, why have it be a static array at all? Just include an empty one at the end.

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

/Anders

maintainer
#5 lore raw
Quoted 13 lines · Anders Hammarquist
Quoted 2 lines · Greg KH

Please resend this in a format that I can apply it in (i.e. one that does not require me to edit it by hand...)

After more fighting with git, I belive I now made it spit out what I wanted. Patch 1/2 ahead.

Quoted 5 lines

drivers/usb/serial/ti_usb_3410_5052.c static unsigned int product_5052_count; · line 161

Quoted 2 lines
drivers/usb/serial/ti_usb_3410_5052.c +1 -1
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {

That's a mess, why have it be a static array at all? Just include an empty one at the end.

Quoted 4 lines

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

Why don't we just drop the extra id thing entirely? The usb-serial subsystem handles new device ids being added dynamically from sysfs for a long time now. Removing this module option would clean up the code a lot, and prevent these errors from ever happening again.

thanks,

greg k-h

author
#6 lore raw
Quoted 9 lines · Greg KH
Quoted 4 lines

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

Why don't we just drop the extra id thing entirely? The usb-serial subsystem handles new device ids being added dynamically from sysfs for a long time now. Removing this module option would clean up the code a lot, and prevent these errors from ever happening again.

Aha, yes, I'm all for that (had I only known I'd have done that to start with). I'll look in to it.

/Anders

#7 lore raw
Quoted 6 lines · Anders Hammarquist
Quoted 5 lines · Greg KH
Quoted 4 lines

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

Quoted 7 lines
Quoted 4 lines

Why don't we just drop the extra id thing entirely? The usb-serial subsystem handles new device ids being added dynamically from sysfs for a long time now. Removing this module option would clean up the code a lot, and prevent these errors from ever happening again.

Aha, yes, I'm all for that (had I only known I'd have done that to start with). I'll look in to it.

I already have a few patches here (part of a larger 3.11 clean-up series) which removes the vid/pid module parameters from all usb-serial modules including ti_usb_3410_5052.

I hope to be able to submit the whole series a later tonight, but here's the ti_usb_3410_5052 part if anyone's interested.

Thanks,
Johan

From: Johan Hovold <[email protected]>
Subject: [PATCH] USB: ti_usb_3410_5052: remove vendor/product module parameters

Remove the vendor and product module parameters which were added a long time ago when we did not have the dynamic sysfs interface to add new device ids (and which isn't limited to five new vid/pid pair).

A vid/pid pair can be added dynamically using sysfs, for example:

echo 0451 1234 >/sys/bus/usb-serial/drivers/ti_usb_3410_5052_1/new_id

for 1-port adapters, or

echo 0451 1234 >/sys/bus/usb-serial/drivers/ti_usb_3410_5052_2/new_id

for 2-port adapters.

Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 72 ++++-------------------------------
 1 file changed, 7 insertions(+), 65 deletions(-)
drivers/usb/serial/ti_usb_3410_5052.c +7 -65
index f3e21f5..5585b20 100644
@@ -141,20 +141,9 @@ static int ti_download_firmware(struct ti_device *tdev);
 
 /* module parameters */
 static int closing_wait = TI_DEFAULT_CLOSING_WAIT;
-static ushort vendor_3410[TI_EXTRA_VID_PID_COUNT];
-static unsigned int vendor_3410_count;
-static ushort product_3410[TI_EXTRA_VID_PID_COUNT];
-static unsigned int product_3410_count;
-static ushort vendor_5052[TI_EXTRA_VID_PID_COUNT];
-static unsigned int vendor_5052_count;
-static ushort product_5052[TI_EXTRA_VID_PID_COUNT];
-static unsigned int product_5052_count;
 
 /* supported devices */
-/* the array dimension is the number of default entries plus */
-/* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
-/* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -171,16 +160,18 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
+	{ }	/* terminator */
 };
 
-static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_5052[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5152_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_EEPROM_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
+	{ }	/* terminator */
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -200,7 +191,7 @@ static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
-	{ }
+	{ }	/* terminator */
 };
 
 static struct usb_serial_driver ti_1port_device = {

@@ -289,61 +280,12 @@ module_param(closing_wait, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(closing_wait,
     "Maximum wait for data to drain in close, in .01 secs, default is 4000");
 
-module_param_array(vendor_3410, ushort, &vendor_3410_count, S_IRUGO);
-MODULE_PARM_DESC(vendor_3410,
-		"Vendor ids for 3410 based devices, 1-5 short integers");
-module_param_array(product_3410, ushort, &product_3410_count, S_IRUGO);
-MODULE_PARM_DESC(product_3410,
-		"Product ids for 3410 based devices, 1-5 short integers");
-module_param_array(vendor_5052, ushort, &vendor_5052_count, S_IRUGO);
-MODULE_PARM_DESC(vendor_5052,
-		"Vendor ids for 5052 based devices, 1-5 short integers");
-module_param_array(product_5052, ushort, &product_5052_count, S_IRUGO);
-MODULE_PARM_DESC(product_5052,
-		"Product ids for 5052 based devices, 1-5 short integers");
-
 MODULE_DEVICE_TABLE(usb, ti_id_table_combined);
 
+module_usb_serial_driver(serial_drivers, ti_id_table_combined);
 
 /* Functions */
 
-static int __init ti_init(void)
-{
-	int i, j, c;
-
-	/* insert extra vendor and product ids */
-	c = ARRAY_SIZE(ti_id_table_combined) - 2 * TI_EXTRA_VID_PID_COUNT - 1;
-	j = ARRAY_SIZE(ti_id_table_3410) - TI_EXTRA_VID_PID_COUNT - 1;
-	for (i = 0; i < min(vendor_3410_count, product_3410_count); i++, j++, c++) {
-		ti_id_table_3410[j].idVendor = vendor_3410[i];
-		ti_id_table_3410[j].idProduct = product_3410[i];
-		ti_id_table_3410[j].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-		ti_id_table_combined[c].idVendor = vendor_3410[i];
-		ti_id_table_combined[c].idProduct = product_3410[i];
-		ti_id_table_combined[c].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-	}
-	j = ARRAY_SIZE(ti_id_table_5052) - TI_EXTRA_VID_PID_COUNT - 1;
-	for (i = 0; i < min(vendor_5052_count, product_5052_count); i++, j++, c++) {
-		ti_id_table_5052[j].idVendor = vendor_5052[i];
-		ti_id_table_5052[j].idProduct = product_5052[i];
-		ti_id_table_5052[j].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-		ti_id_table_combined[c].idVendor = vendor_5052[i];
-		ti_id_table_combined[c].idProduct = product_5052[i];
-		ti_id_table_combined[c].match_flags = USB_DEVICE_ID_MATCH_DEVICE;
-	}
-
-	return usb_serial_register_drivers(serial_drivers, KBUILD_MODNAME, ti_id_table_combined);
-}
-
-static void __exit ti_exit(void)
-{
-	usb_serial_deregister_drivers(serial_drivers);
-}
-
-module_init(ti_init);
-module_exit(ti_exit);
-
-
 static int ti_startup(struct usb_serial *serial)
 {
 	struct ti_device *tdev;
Signature · 1 line
1.8.2.1
author
#8 lore raw
Quoted 15 lines · Johan Hovold
Quoted 6 lines · Anders Hammarquist
Quoted 5 lines · Greg KH
Quoted 4 lines

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

Quoted 7 lines
Quoted 4 lines

Why don't we just drop the extra id thing entirely? The usb-serial subsystem handles new device ids being added dynamically from sysfs for a long time now. Removing this module option would clean up the code a lot, and prevent these errors from ever happening again.

Aha, yes, I'm all for that (had I only known I'd have done that to start with). I'll look in to it.

Quoted 6 lines

I already have a few patches here (part of a larger 3.11 clean-up series) which removes the vid/pid module parameters from all usb-serial modules including ti_usb_3410_5052.

I hope to be able to submit the whole series a later tonight, but here's the ti_usb_3410_5052 part if anyone's interested.

I did a quick check of adding the device id though sysfs, and although it partly works, it doesn't find the correct firmware (it ends up trying to load 5052 firmware for a 3410 device. Looking at the code it seems (struct ti_device) td_is_3410 isn't set properly.)

I can take a stab at fixing it in the next few days.

/Anders

#9 lore raw
Quoted 20 lines · Anders Hammarquist
Quoted 19 lines · Johan Hovold
Quoted 6 lines · Anders Hammarquist
Quoted 5 lines · Greg KH
Quoted 4 lines

Indeed. I'd already had some (failed) thoughts about how to handle it nicely. Now I've had another think through, and I have something which deals with it and at least complains if TI_EXTRA_VID_PID_COUNT is changed without changing the initializer. Patch 2/2

Quoted 7 lines
Quoted 4 lines

Why don't we just drop the extra id thing entirely? The usb-serial subsystem handles new device ids being added dynamically from sysfs for a long time now. Removing this module option would clean up the code a lot, and prevent these errors from ever happening again.

Aha, yes, I'm all for that (had I only known I'd have done that to start with). I'll look in to it.

I already have a few patches here (part of a larger 3.11 clean-up series) which removes the vid/pid module parameters from all usb-serial modules including ti_usb_3410_5052.

Quoted 7 lines
Quoted 2 lines

I hope to be able to submit the whole series a later tonight, but here's the ti_usb_3410_5052 part if anyone's interested.

I did a quick check of adding the device id though sysfs, and although it partly works, it doesn't find the correct firmware (it ends up trying to load 5052 firmware for a 3410 device. Looking at the code it seems (struct ti_device) td_is_3410 isn't set properly.)

Turns out that the drivers device-type detection has never worked with the dynamic id interface (all devices were detected as 2-port devices).

I'm responding to this mail with a fix. Care to give it a try?

Thanks,
Johan

#10 lore raw

PATCH 1/1 USB: ti_usb_3410_5052: fix dynamic-id matching

The driver failed to take the dynamic ids into account when determining the device type and therefore all devices were detected as 2-port devices when using the dynamic-id interface.

Match on the usb-serial-driver field instead of doing redundant id-table searches.

Reported-by Anders Hammarquist <[email protected]>
Cc stable <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/serial/ti_usb_3410_5052.c +1 -1
index 5585b20..5c07d55 100644
@@ -309,7 +309,7 @@ static int ti_startup(struct usb_serial *serial)
 	usb_set_serial_data(serial, tdev);
 
 	/* determine device type */
-	if (usb_match_id(serial->interface, ti_id_table_3410))
+	if (serial->type == &ti_1port_device)
 		tdev->td_is_3410 = 1;
 	dev_dbg(&dev->dev, "%s - device type is %s\n", __func__,
 		tdev->td_is_3410 ? "3410" : "5052");
Signature · 1 line
1.8.2.1
author
#11 lore raw
In reply to Johan Hovold
Quoted 9 lines · Johan Hovold
Quoted 4 lines

I did a quick check of adding the device id though sysfs, and although it partly works, it doesn't find the correct firmware (it ends up trying to load 5052 firmware for a 3410 device. Looking at the code it seems (struct ti_device) td_is_3410 isn't set properly.)

Turns out that the drivers device-type detection has never worked with the dynamic id interface (all devices were detected as 2-port devices).

I'm responding to this mail with a fix. Care to give it a try?

Yes, this works fine.

/Anders

#12 lore raw
Quoted 6 lines · Anders Hammarquist
Quoted 5 lines · Johan Hovold
Quoted 4 lines

I did a quick check of adding the device id though sysfs, and although it partly works, it doesn't find the correct firmware (it ends up trying to load 5052 firmware for a 3410 device. Looking at the code it seems (struct ti_device) td_is_3410 isn't set properly.)

Quoted 6 lines
Quoted 4 lines

Turns out that the drivers device-type detection has never worked with the dynamic id interface (all devices were detected as 2-port devices).

I'm responding to this mail with a fix. Care to give it a try?

Yes, this works fine.

Thanks for testing.

Johan

author
#13 lore raw

PATCH 1/2 * Remove unused and overly generic ABBOTT_PRODUCT_ID * Fix sizes of statically sized usb_debvice_id tables

In reply to Greg KH
Signed-off-by Anders Hammarquist <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c |   11 ++++++-----
 drivers/usb/serial/ti_usb_3410_5052.h |    5 ++---
 2 files changed, 8 insertions(+), 8 deletions(-)
drivers/usb/serial/ti_usb_3410_5052.c +6 -5
index e581c25..26c1161 100644
@@ -158,7 +158,7 @@ static unsigned int product_5052_count;
 /* the array dimension is the number of default entries plus */
 /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
 /* null entry */
-static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -172,8 +172,8 @@ static struct usb_device_id ti_id_table_3410[15+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 };
 

@@ -184,7 +184,7 @@ static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
 };
 
-static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -202,7 +202,8 @@ static struct usb_device_id ti_id_table_combined[19+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_4543_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454B_PRODUCT_ID) },
 	{ USB_DEVICE(IBM_VENDOR_ID, IBM_454C_PRODUCT_ID) },
-	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
+	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
 	{ }
 };
drivers/usb/serial/ti_usb_3410_5052.h +2 -3
index 4a2423e..d3ff470 100644
@@ -52,9 +52,8 @@
 
 /* Abbott Diabetics vendor and product ids */
 #define ABBOTT_VENDOR_ID		0x1a61
-#define ABBOTT_STEREO_PLUG_ID		0x3410
-#define ABBOTT_PRODUCT_ID		ABBOTT_STEREO_PLUG_ID
-#define ABBOTT_STRIP_PORT_ID		0x3420
+#define ABBOTT_STEREO_PLUG_PRODUCT_ID	0x3410
+#define ABBOTT_STRIP_PORT_PRODUCT_ID	0x3420
 
 /* Commands */
 #define TI_GET_VERSION			0x01
Signature · 1 line
1.7.10.4
author
#14 lore raw

PATCH 2/2 Remove static sizing of usb_device_id arrays

Signed-off-by Anders Hammarquist <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
drivers/usb/serial/ti_usb_3410_5052.c +24 -5
index 26c1161..441c788 100644
@@ -60,7 +60,20 @@
 #define TI_READ_URB_STOPPED	2
 
 #define TI_EXTRA_VID_PID_COUNT	5
-
+#define TI_EXTRA_VID_PID_INITALIZER {0}, {0}, {0}, {0}, {0}
+
+/* Check that TI_EXTRA_VID_PID_COUNT and TI_EXTRA_VID_PID_INITALIZER match.
+   On x86, this wastes one byte of __init space to provide a compile-time
+   error if you do not match up the definitions of TI_EXTRA_VID_PID_COUNT and
+   TI_EXTRA_VID_PID_INITIALIZER. Expect space waste up to sizeof(void *) for
+   other architectures. */
+__init void __ti_extra_vid_pid_test(void)
+{
+	struct { char a; } ti_extra_vid_pid_initializer[] =
+        	{ TI_EXTRA_VID_PID_INITALIZER };
+	BUILD_BUG_ON(ARRAY_SIZE(ti_extra_vid_pid_initializer) !=
+        	TI_EXTRA_VID_PID_COUNT);
+}
 
 /* Structures */
 

@@ -158,7 +171,7 @@ static unsigned int product_5052_count;
 /* the array dimension is the number of default entries plus */
 /* TI_EXTRA_VID_PID_COUNT user defined entries plus 1 terminating */
 /* null entry */
-static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_3410[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -175,16 +188,20 @@ static struct usb_device_id ti_id_table_3410[16+TI_EXTRA_VID_PID_COUNT+1] = {
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
+	TI_EXTRA_VID_PID_INITALIZER, /* space for run-time VID-PID pairs */
+	{0} /* End of array maker */
 };
 
-static struct usb_device_id ti_id_table_5052[5+TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_5052[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5152_BOOT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_EEPROM_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_5052_FIRMWARE_PRODUCT_ID) },
+	TI_EXTRA_VID_PID_INITALIZER, /* space for run-time VID-PID pairs */
+	{0} /* End of array maker */
 };
 
-static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1] = {
+static struct usb_device_id ti_id_table_combined[] = {
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
 	{ USB_DEVICE(MTS_VENDOR_ID, MTS_GSM_NO_FW_PRODUCT_ID) },

@@ -205,7 +222,9 @@ static struct usb_device_id ti_id_table_combined[20+2*TI_EXTRA_VID_PID_COUNT+1]
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STEREO_PLUG_PRODUCT_ID) },
 	{ USB_DEVICE(ABBOTT_VENDOR_ID, ABBOTT_STRIP_PORT_PRODUCT_ID) },
 	{ USB_DEVICE(TI_VENDOR_ID, FRI2_PRODUCT_ID) },
-	{ }
+	TI_EXTRA_VID_PID_INITALIZER, /* space for run-time VID-PID pairs */
+	TI_EXTRA_VID_PID_INITALIZER, /* Two needed, combined 3410 and 5052 */
+	{0} /* End of array maker */
 };
 
 static struct usb_serial_driver ti_1port_device = {
Signature · 1 line
1.7.10.4
maintainer
#15 lore raw

Re: [PATCH 2/2] Remove static sizing of usb_device_id arrays

Quoted 5 lines · Anders Hammarquist
Signed-off-by Anders Hammarquist <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

This patch, and your previous one, are no longer needed, right?

thanks,

greg k-h

author
#16 lore raw

Re: [PATCH 2/2] Remove static sizing of usb_device_id arrays

Quoted 8 lines · Greg KH
Quoted 5 lines · Anders Hammarquist
Signed-off-by Anders Hammarquist <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

This patch, and your previous one, are no longer needed, right?

They are needed until the module params for adding vendor and product ids are removed, and it sounded like the consensus was to keep them for a while.

/Anders

maintainer
#17 lore raw

Re: [PATCH 2/2] Remove static sizing of usb_device_id arrays

Quoted 8 lines · Anders Hammarquist
Quoted 7 lines · Greg KH
Quoted 5 lines · Anders Hammarquist
Signed-off-by Anders Hammarquist <[email protected]>
---
 drivers/usb/serial/ti_usb_3410_5052.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)
Quoted 5 lines
Quoted 1 line

This patch, and your previous one, are no longer needed, right?

They are needed until the module params for adding vendor and product ids are removed, and it sounded like the consensus was to keep them for a while.

No, they will be removed for 3.12, the patches are already queued up.

thanks,

greg k-h