Re: [PATCH 2/4] ch341: Detect HL340 variant

[Johan Hovold <johan@kernel.org>]

On Fri, Mar 06, 2020 at 07:00:43PM +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.
> 
> These devices can usually be identified by an imprint of "340" on the
> turquoise-colored plug. They're called "HL340" in this driver.

As I mentioned in my reply to the cover letter, you need to come up with
a different designation as this does not apply to all HL340 devices.
 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 518209072c50..0523f46f53c7 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -89,6 +89,7 @@ struct ch341_private {
>  	u8 mcr;
>  	u8 msr;
>  	u8 lcr;
> +	u8 flags;
>  };
>  
>  static void ch341_set_termios(struct tty_struct *tty,
> @@ -315,6 +316,43 @@ out:	kfree(buffer);
>  	return r;
>  }
>  
> +/*
> + * A subset of CH341 devices, called "HL340" in this driver, 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.
> + */
> +static int ch341_detect_hl340(struct usb_device *dev)

Return bool? Rename ch341_is_xxx() ?

> +{
> +	const unsigned int size = 2;
> +	char *buffer;
> +	int r;
> +
> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	r = ch341_control_in(dev, CH341_REQ_READ_REG,
> +			     CH341_REG_BREAK, 0, buffer, size);

This helper would already have logged an error message, which perhaps is
ok, but you could consider using usb_control_msg() directly here.

> +	if (r == -EPIPE) {
> +		dev_dbg(&dev->dev, "%s - Chip is a HL340 variant\n",
> +			__func__);
> +		r = 1;
> +		goto out;
> +	}
> +
> +	if (r < 0) {
> +		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
> +			__func__, r);
> +		goto out;
> +	}

So this is currently redundant.

> +
> +	r = 0;

Not needed either, right?

> +
> +out:	kfree(buffer);

Line break after the label, please.

> +	return r;
> +}
> +
>  static int ch341_port_probe(struct usb_serial_port *port)
>  {
>  	struct ch341_private *priv;
> @@ -336,6 +374,10 @@ static int ch341_port_probe(struct usb_serial_port *port)
>  	if (r < 0)
>  		goto error;
>  
> +	r = ch341_detect_hl340(port->serial->dev);
> +	if (r < 0)
> +		goto error;

You never store the return value (and the "flags" variable you add is
unused) which indicates your series needs to be restructured.

At least update flags in this patch. Perhaps consider renaming it
"quirks" depending on how it ends up being used.

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

Johan