On Fri, Oct 7, 2016 at 12:30 PM, Johan Hovold <johan@xxxxxxxxxx> wrote: > This series looks good to me, I just a couple of comments to this one. >> >> - r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, a); >> - if (!r) >> - r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x0f2c, b); >> + /* 0x9c is "enable SFR_UART Control register and timer" */ >> + r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0x9c | (ctrl << 8), a | 0x80); > > Please break this line or restructure the code to stay within 80 cols > (checkpatch.pl would have let you know). Sorry, will fix this. Think I got sidetracked by other patch-submission-related tasks and forgot to actually run checkpatch. >> return r; >> } >> @@ -218,16 +216,12 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv) >> if (r < 0) >> goto out; >> >> - r = ch341_set_baudrate(dev, priv); >> - if (r < 0) >> - goto out; >> - >> /* expect two bytes 0x56 0x00 */ >> r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x2518, 0, buffer, size); >> if (r < 0) >> goto out; >> >> - r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, 0x0050); >> + r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, 0x00c3); > > Why is this change needed? I see no write to this register in the > vendor driver. In principle, it might not be because the value written to register 0x18 is probably overwritten by ch341_init_set_baudrate anyway. It's there because it's in Grigori's original patch, it looks superficially reasonable (corresponds to ENABLE_RX|ENABLE_TX|CS8, as opposed to the rather implausible ENABLE_TX|PAR_EVEN|CS5), and given that some hardware revisions are apparently a little temperamental I was reluctant to remove it without fully understanding why it was added in the first place. As the vendor driver does without it might make sense to delete the write altogether, but it does do quite a few things differently from this driver. Depends what Grigori says and the results of actual testing. Thanks for looking at these patches by the way! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
