RE: [PATCH v2] USB: serial: cp210x: Adding GPIO support for CP2105

[Konstantin Shkolnyy <Konstantin.Shkolnyy@xxxxxxxxxx>]

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-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html