Comments inline, not comprehensive by any measure... > -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Martyn Welch > Sent: Wednesday, January 13, 2016 06:31 > To: Johan Hovold; Linus Walleij; Alexandre Courbot > Cc: Greg Kroah-Hartman; linux-usb@xxxxxxxxxxxxxxx; linux- > gpio@xxxxxxxxxxxxxxx; Martyn Welch > Subject: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105 > ... > +#include <linux/gpio/driver.h> > +#include <linux/bitops.h> Enclose this in CONFIG_GPIOLIB? ... > @@ -201,6 +207,10 @@ MODULE_DEVICE_TABLE(usb, id_table); > struct cp210x_port_private { > __u8 bInterfaceNumber; > bool has_swapped_line_ctl; > +#ifdef CONFIG_GPIOLIB > + struct usb_serial *serial; If you have port_private (and port), you can always get usb_serial from port->serial, so why store it here? > + struct gpio_chip gc; > +#endif > }; ... > > static struct usb_serial_driver cp210x_device = { > @@ -219,6 +229,7 @@ static struct usb_serial_driver cp210x_device = { > .tx_empty = cp210x_tx_empty, > .tiocmget = cp210x_tiocmget, > .tiocmset = cp210x_tiocmset, > + .probe = cp210x_probe, Enclose this in CONFIG_GPIOLIB? ... > +static int cp210x_gpio_get(struct gpio_chip *gc, unsigned gpio) > +{ > + struct cp210x_port_private *port_priv = > + container_of(gc, struct cp210x_port_private, gc); > + struct usb_serial *serial = port_priv->serial; > + __le32 *buf; > + int result, i, length; > + int size = 1; > + unsigned int data[size]; > + > + /* Number of integers required to contain the array */ > + length = (((size - 1) | 3) + 1) / 4; usb_control_msg can deal with any size of the buffer, so use __le16 and remove these length calculations. > + > + buf = kcalloc(length, sizeof(__le32), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + /* Issue the request, attempting to read 'size' bytes */ > + result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, > 0), > + CP210X_VENDOR_SPECIFIC, > + REQTYPE_INTERFACE_TO_HOST, > CP210X_READ_LATCH, > + port_priv->bInterfaceNumber, buf, size, > + USB_CTRL_GET_TIMEOUT); > + if (result != size) { print err message here and any other time this function fails. > + result = 0; > + goto err; > + } ... > +static int cp210x_shared_gpio_init(struct usb_serial_port *port) > +{ > + struct usb_serial *serial = port->serial; > + struct cp210x_port_private *port_priv = > usb_get_serial_port_data(port); > + __le32 *buf; > + unsigned long tmp; Wouldn't "chip_features" be better? > + int result, mode; mode is unsigned. ... > static int cp210x_port_probe(struct usb_serial_port *port) > { > struct usb_serial *serial = port->serial; > @@ -1008,12 +1198,21 @@ static int cp210x_port_probe(struct > usb_serial_port *port) > usb_set_serial_port_data(port, port_priv); > > ret = cp210x_detect_swapped_line_ctl(port); > - if (ret) { > - kfree(port_priv); > - return ret; > - } > + if (ret) > + goto err_ctl; > + > +#ifdef CONFIG_GPIOLIB > + ret = cp210x_shared_gpio_init(port); > + if (ret < 0) > + goto err_ctl; put kfree and return here ... -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
