On Sat, Nov 27, 2004 at 09:05:40PM -0500, Edwin Olson wrote:
> Hi folks,
>
> Being able to set the event character and latency timer can greatly
> improve performance for some applications which use the ftdi_sio module
> (a serial->USB converter). The following patch adds sysfs attributes,
> allowing easy run-time configuration.
>
> This is my first ever submitted patch. I've tried to follow all of the
> instructions, but I probably did at least one thing badly! Your comments
> (style, patch format, hints for kernel hacking) all readily solicited.
You forgot the "Signed-off-by:" line.
> This patch is against 2.6.9.
Also, against 2.6.10-rc2 would be nice to see.
> diff -uprN -X dontdiff linux-2.6.9-orig/drivers/usb/serial/ftdi_sio.c
> linux-2.6.9/drivers/usb/serial/ftdi_sio.c
> --- linux-2.6.9-orig/drivers/usb/serial/ftdi_sio.c 2004-10-18
> 17:54:55.000000000 -0400
> +++ linux-2.6.9/drivers/usb/serial/ftdi_sio.c 2004-11-27 20:55:58.737667469
> -0500
> @@ -816,7 +816,7 @@ static struct usb_serial_device_type ftd
> .num_bulk_in = 1,
> .num_bulk_out = 1,
> .num_ports = 1,
> - .open = ftdi_open,
> +.open = ftdi_open,
Why did you change this line?
> +/* Sysfs-related declarations */
> +static void create_sysfs_attrs(struct usb_serial *serial);
> +static void remove_sysfs_attrs(struct usb_serial *serial);
> +
> +static ssize_t show_latency_timer(struct device *dev, char *buf);
> +static ssize_t store_latency_timer(struct device *dev, const char *valbuf,
> size_t count);
> +static DEVICE_ATTR(latency_timer, S_IWUGO | S_IRUGO, show_latency_timer,
> store_latency_timer);
> +
> +static ssize_t show_event_char(struct device *dev, char *buf);
> +static ssize_t store_event_char(struct device *dev, const char *buf, size_t
> count);
> +static DEVICE_ATTR(event_char, S_IWUGO | S_IRUGO, show_event_char,
> store_event_char);
If you put these before where they are called, you don't need a forward
declaration at all, right?
>
> #define WDR_TIMEOUT (HZ * 5 ) /* default urb timeout */
>
> @@ -1263,6 +1274,7 @@ static int ftdi_8U232AM_startup (struct
>
> /* Startup for the FT232BM chip */
> /* Called from usbserial:serial_probe */
> +
Why the extra line?
> static int ftdi_FT232BM_startup (struct usb_serial *serial)
> { /* ftdi_FT232BM_startup */
> struct ftdi_private *priv;
> @@ -1278,6 +1290,9 @@ static int ftdi_FT232BM_startup (struct
> priv->chip_type = FT232BM;
> priv->baud_base = 48000000 / 2; /* Would be / 16, but FT232BM supports
> multiple of 0.125 divisor fractions! */
>
> + dbg("adding sysfs attributes");
> + create_sysfs_attrs(serial);
> +
> return (0);
> } /* ftdi_FT232BM_startup */
>
> @@ -1371,14 +1386,16 @@ static void ftdi_shutdown (struct usb_se
>
> dbg("%s", __FUNCTION__);
>
> + remove_sysfs_attrs(serial);
> +
> /* all open ports are closed at this point
> * (by usbserial.c:__serial_close, which calls ftdi_close)
> */
> -
> if (priv) {
> usb_set_serial_port_data(port, NULL);
> kfree(priv);
> }
> +
Why the extra line?
> } /* ftdi_shutdown */
>
>
> @@ -2340,10 +2357,140 @@ static void __exit ftdi_exit (void)
> usb_serial_deregister (&ftdi_FT232BM_device);
> usb_serial_deregister (&ftdi_8U232AM_device);
> usb_serial_deregister (&ftdi_SIO_device);
> +}
> +
> +
> +void create_sysfs_attrs(struct usb_serial *serial)
> +{
> + struct ftdi_private *priv;
> + struct usb_device *udev;
> +
> + priv = usb_get_serial_port_data(serial->port[0]);
> + udev = serial->dev;
> +
> + if (priv->chip_type == FT232BM) {
> + dbg("sysfs attributes for FT232BM");
> + device_create_file(&udev->dev, &dev_attr_event_char);
> + device_create_file(&udev->dev, &dev_attr_latency_timer);
> + }
> +}
> +
> +void remove_sysfs_attrs(struct usb_serial *serial)
> +{
> + struct ftdi_private *priv;
> + struct usb_device *udev;
> +
> + priv = usb_get_serial_port_data(serial->port[0]);
> + udev = serial->dev;
> +
> + if (priv->chip_type == FT232BM) {
> + device_remove_file(&udev->dev, &dev_attr_event_char);
> + device_remove_file(&udev->dev, &dev_attr_latency_timer);
> + }
> +
> +}
> +
> +ssize_t show_latency_timer(struct device *dev, char *buf)
> +{
> + struct usb_serial_port *port = to_usb_serial_port(dev);
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_device *udev;
> +
> + unsigned short latency=0;
> +
> + int rv=0;
> +
> + udev=to_usb_device(dev);
Please use spaces around your '=' characters.
> +
> + dbg("%s",__FUNCTION__);
> +
> + rv = usb_control_msg(udev,
> + usb_rcvctrlpipe(udev, 0),
> + FTDI_SIO_GET_LATENCY_TIMER_REQUEST,
> + FTDI_SIO_GET_LATENCY_TIMER_REQUEST_TYPE,
> + 0, priv->interface,
> + (char*) &latency, 1, WDR_TIMEOUT);
> +
> + if (rv<0) {
spaces around the '<' is also good.
> + dbg("error: %i", rv);
A more descriptive message might be nice to have. Also, what's wrong
with the dev_err() macro?
> + return EIO;
> + }
> + return sprintf(buf, "%i\n", latency);
> +}
>
> +/* Write a new value of the latency timer, in units of milliseconds. */
> +ssize_t store_latency_timer(struct device *dev, const char *valbuf, size_t
> count)
> +{
> + struct usb_serial_port *port = to_usb_serial_port(dev);
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_device *udev;
> +
> + char buf[1];
> + int v=simple_strtoul(valbuf, NULL, 10);
> + int rv=0;
> +
> + udev=to_usb_device(dev);
> +
> + dbg("%s: setting latency timer = %i", __FUNCTION__, v);
> +
> + rv = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
> + FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
> + v, priv->interface,
> + buf, 0, WDR_TIMEOUT);
> +
> + if (rv<0) {
> + dbg("error: %i", rv);
> + return EIO;
> + }
> +
> + return count;
> }
>
>
> +ssize_t show_event_char(struct device *dev, char *buf)
> +{
> + dbg("%s",__FUNCTION__);
> +
> + /* we're not able to read the event_char back from the FTDI. I
> + suppose we could cache the value, but it'd be hard to be sure
> + we were returning valid data. */
> +
> + return sprintf(buf, "-1\n");
> +}
Just set the read value to NULL and then you don't need this as you will
not be able to read from the file. That sounds like what you want,
right?
> +
> +/* Write an event character directly to the FTDI register. The ASCII
> + value is in the low 8 bits, with the enable bit in the 9th bit. */
> +ssize_t store_event_char(struct device *dev, const char *valbuf, size_t
> count)
> +{
> + struct usb_serial_port *port = to_usb_serial_port(dev);
> + struct ftdi_private *priv = usb_get_serial_port_data(port);
> + struct usb_device *udev;
> +
> + char buf[1];
> + int v=simple_strtoul(valbuf, NULL, 10);
> + int rv=0;
> +
> + udev=to_usb_device(dev);
> +
> + dbg("%s: setting event char = %i", __FUNCTION__, v);
> +
> + rv = usb_control_msg(udev,
> + usb_sndctrlpipe(udev, 0),
> + FTDI_SIO_SET_EVENT_CHAR_REQUEST,
> + FTDI_SIO_SET_EVENT_CHAR_REQUEST_TYPE,
> + v, priv->interface,
> + buf, 0, WDR_TIMEOUT);
> +
> + if (rv<0) {
> + dbg("error: %i", rv);
> + return EIO;
> + }
> +
> + return count;
> +}
> +
> module_init(ftdi_init);
> module_exit(ftdi_exit);
>
> diff -uprN -X dontdiff linux-2.6.9-orig/drivers/usb/serial/ftdi_sio.h
> linux-2.6.9/drivers/usb/serial/ftdi_sio.h
> --- linux-2.6.9-orig/drivers/usb/serial/ftdi_sio.h 2004-10-18
> 17:55:06.000000000 -0400
> +++ linux-2.6.9/drivers/usb/serial/ftdi_sio.h 2004-11-27 20:55:59.384571029
> -0500
> @@ -234,6 +234,8 @@
> #define FTDI_SIO_GET_MODEM_STATUS 5 /* Retrieve current value of modern
> status register */
> #define FTDI_SIO_SET_EVENT_CHAR 6 /* Set the event character */
> #define FTDI_SIO_SET_ERROR_CHAR 7 /* Set the error character */
> +#define FTDI_SIO_SET_LATENCY_TIMER 9 /* Set the latency timer */
> +#define FTDI_SIO_GET_LATENCY_TIMER 10 /* Get the latency timer */
Please use tabs like the other #defines here use.
Other than these minor comments the patch looks like a very good start.
thanks,
greg k-h
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
