Re: [PATCH v2 2/6] USB: serial: ch341: Add basis for quirk detection

[Johan Hovold <johan@kernel.org>]

On Tue, Mar 31, 2020 at 11:37:18PM +0000, Michael Hanselmann wrote:
> A subset of CH341 devices does not support all features, namely the
> prescaler is limited to a reduced precision and there is no support for
> sending a RS232 break condition. This patch adds a detection function
> which will be extended to return quirk flags as they're implemented.
> 
> The author's affected device has an imprint of "340" on the
> turquoise-colored plug, but not all such devices appear to be affected.
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>

I've applied this one now after making some minor changes.

> ---
>  drivers/usb/serial/ch341.c | 49 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 85e7125d0194..9c839f67c3d4 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -87,6 +87,7 @@ struct ch341_private {
>  	u8 mcr;
>  	u8 msr;
>  	u8 lcr;
> +	u8 quirks;

I used unsigned long to avoid any false sharing with the locked fields
even if that's likely not an issue here.

>  };
>  
>  static void ch341_set_termios(struct tty_struct *tty,
> @@ -330,6 +331,45 @@ out:	kfree(buffer);
>  	return r;
>  }
>  
> +static int ch341_detect_quirks(struct usb_device *dev)
> +{
> +	const unsigned int size = 2;
> +	char *buffer;
> +	int r;
> +
> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	/*
> +	 * A subset of CH34x devices does not support all features. The
> +	 * prescaler is limited and there is no support for sending a RS232
> +	 * break condition. A read failure when trying to set up the latter is
> +	 * used to detect these devices.
> +	 */
> +	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), CH341_REQ_READ_REG,
> +			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
> +			    CH341_REG_BREAK, 0, buffer, size, DEFAULT_TIMEOUT);
> +	if (r == -EPIPE) {
> +		dev_dbg(&dev->dev, "%s - reading break condition register"
> +			" failed (%d)\n", __func__, r);

Log message strings should not be broken (even if that violates the 80
column rule) so that they are easier to grep for.

No need need to include the errno, here and reworded the message.

I also used passed in the port structure and used the port->dev for
logging (I know, the driver isn't consistent here).

> +		r = 0;
> +		goto out;
> +	}
> +
> +	if (r < 0) {

We generally need to check for short transfers as well when reading.

> +		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
> +			__func__, r);
> +		goto out;
> +	}
> +
> +	r = 0;
> +
> +out:
> +	kfree(buffer);
> +	return r;
> +}
> +
>  static int ch341_port_probe(struct usb_serial_port *port)
>  {
>  	struct ch341_private *priv;
> @@ -351,6 +391,15 @@ static int ch341_port_probe(struct usb_serial_port *port)
>  	if (r < 0)
>  		goto error;
>  
> +	r = ch341_detect_quirks(port->serial->dev);
> +	if (r < 0)
> +		goto error;
> +	if (r != 0) {
> +		dev_dbg(&port->serial->dev->dev,
> +			"%s - enabling quirks flags %08x\n", __func__, r);
> +		priv->quirks |= r;

And finally I moved this bit into the helper itself.

> +	}
> +
>  	usb_set_serial_port_data(port, priv);
>  	return 0;

Johan