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.
