[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [PATCH 2/4] ch341: Detect HL340 variant
From:       Michael Hanselmann <public () hansmi ! ch>
Date:       2020-03-31 23:35:16
Message-ID: ae227be4-b730-a85d-dc28-3b6b31a8e72c () msgid ! hansmi ! ch
[Download RAW message or body]

On 24.03.20 11:31, Johan Hovold wrote:
> On Fri, Mar 06, 2020 at 07:00:43PM +0000, Michael Hanselmann wrote:
>> +static int ch341_detect_hl340(struct usb_device *dev)
> 
> Return bool? Rename ch341_is_xxx() ?

Changed the function to return the quirks flags, its name is now
"ch341_detect_quirks".

>> +	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.

Changed to use "usb_control_msg" directly.

>> +	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.

Not anymore after the change above: EPIPE is encountered for the limited
chips, 0 for normal chips and other errors should be reported.

>> +
>> +out:	kfree(buffer);
> 
> Line break after the label, please.

Tried to keep the style consistent (see "ch341_port_probe" for example).
Done.

>> +	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.

That's different now with the renaming to "ch341_detect_quirks". The
actual return value is still 0 as of this patch. Quirks are added in
later patches.

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

Done.


[prev in list] [next in list] [prev in thread] [next in thread]