> From: Martyn Welch [mailto:martyn.welch@xxxxxxxxxxxxxxx] > Sent: Thursday, January 14, 2016 04:23 > To: Konstantin Shkolnyy; Johan Hovold; Linus Walleij; Alexandre Courbot > Cc: Greg Kroah-Hartman; linux-usb@xxxxxxxxxxxxxxx; linux- > gpio@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105 > > On 14/01/16 00:27, Konstantin Shkolnyy wrote: > > 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? > > ... > > Is there any point? I took a look at a few other drivers which > optionally support GPIO and they don't ifdef the headers. OK, according to other comment, I need to learn about local ifdef policies. I'm sorry. To me, knowing *why * a header is included seems beneficial, but obviously there are other considerations. > I think the contents of the headers will effectively be ignored if not > used and this won't affect module size. I think a good linker will throw away anything that is not referenced anyway. ... > >> +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. > > > > OK. (This is the process used in the other calls. Was wondering why they > are in the first place, any ideas?) Johan Hovold previously said he didn't like these functions either. I actually submitted a patch to replace them with simpler ones. ... > >> 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 > > ... > > > > Why not use a shared error path? I don't have a strong opinion here. Just felt like insulating the code insertion into ifdef CONFIG_GPIOLIB. But you also have a good point... -- 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
