Re: [PATCH] HSO: add option hso driver

[Jan Engelhardt]

On Tuesday 2008-05-13 23:51, Greg KH wrote:
>+#define MOD_AUTHOR			"Option Wireless"
>+#define MOD_DESCRIPTION			"USB High Speed Option driver"
>+#define MOD_LICENSE			"GPL"
>...
That MODULE_DESCRIPTION(MOD_DESCRIPTION) looks so redundant. [Same
for MOD_AUTHOR and MOD_LICENSE.]
Just write MODULE_DESCRIPTION("USB High Speed Option driver"), no?

Also, "Option" sounds a bit misleading. Is Option a company name,
a product name, or the original English word "option"? It's confusing.

>+struct hso_device {
>+	union {
>+		struct hso_serial *dev_serial;
>+		struct hso_net *dev_net;
>+	} port_data;

This does not really need a name, since it is an anon union anyway.

>+};
>+

The usual bunch of \s.. er, has this gone through checkpatch? [Seems 
mostly clean though.]

>+/* Type of interface */
>+#define HSO_INTF_MASK		0xFF00
>+#define	HSO_INTF_MUX		0x0100
>+#define	HSO_INTF_BULK   	0x0200
>+
>+/* Type of port */
>+#define HSO_PORT_MASK		0xFF
>+#define HSO_PORT_NO_PORT	0x0
>+#define	HSO_PORT_CONTROL	0x1
>+#define	HSO_PORT_APP		0x2
>+#define	HSO_PORT_GPS		0x3
>+#define	HSO_PORT_PCSC		0x4
>+#define	HSO_PORT_APP2		0x5
>+#define HSO_PORT_GPS_CONTROL	0x6
>+#define HSO_PORT_MSD		0x7
>+#define HSO_PORT_VOICE		0x8
>+#define HSO_PORT_DIAG2		0x9
>+#define	HSO_PORT_DIAG		0x10
>+#define	HSO_PORT_MODEM		0x11
>+#define	HSO_PORT_NETWORK	0x12
>+
>+/* Additional device info */
>+#define HSO_INFO_MASK		0xFF000000
>+#define HSO_INFO_CRC_BUG	0x01000000
>+
>+/* Sysfs attribute */
>+static ssize_t hso_sysfs_show_porttype(struct device *dev,
>+				       struct device_attribute *attr,
>+				       char *buf)
>+{
>+	struct hso_device *hso_dev = dev->driver_data;
>+	char *port_name;
	const char *port_name;

>...
>+	case HSO_PORT_DIAG2:
>+		port_name = "Diagnostic2";
>+		break;
>+	case HSO_PORT_MODEM:
>+		port_name = "Modem";
>+		break;
>+	case HSO_PORT_NETWORK:
>+		port_name = "Network";
>+		break;
>+	default:
>+		port_name = "Unknown";
>+		break;
>+	}
>+
>+	return sprintf(buf, "%s\n", port_name);
>+}

You could refactor this using a static array mapping from port to 
string. Same for the following three functions (hso_port_to_mux and 
hso_mux_to_port).

	static const char *const names[] = {
		[HSO_PORT_xxx] = "xxx";
	};

>+static struct ethtool_ops ops = {
const?

>+	.get_drvinfo = hso_get_drvinfo,
>+	.get_link = ethtool_op_get_link
>+};
>+
>+static void hso_serial_set_termios(struct tty_struct *tty, struct ktermios *old)
>+{
[...]
>+	/* done */
>+	return;
>+}

I would have never thought! (Also elsewhere.)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/