On Thu, Jan 14, 2016 at 06:15:42PM +0000, Konstantin Shkolnyy wrote:
> The patching procedure enforced by maintainers dictates that each
> separate patch addresses 1 issue.
> It's much easier to review changes this way. So this particular patch
> just converts from one register access function to another.
> The bugfix patch will come later.
Do one logical change per patch is a good rule of thumb, yes. But we
must never introduce regression by taking that rule too far.
I haven't had time to review your patches yet, but I remember thinking
that those FIXMEs looked odd. How about adding the missing error
handling before you introduce the new helpers?
> While doing this cleanup, I also noticed another bug - this function
> will always set the low 2 bits of byte 0 to 01b: "modem_ctl[0] |=
> 0x01".
> This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held
> inactive"). The function will only write it when CRTSCTS changes.
> So the device will start with 0, then, if CRTSCTS ever changes, it'll
> become 1 and stay 1 forever. Looks wrong to me.
> I'm still researching the subject when and how it should be set.
>
> * Wikipedia: "DTR and DSR are usually on all the time and, per the
> * RS-232 standard and its successors, are used to signal from each
> * end that the other equipment is actually present and powered-up."
> * So perhaps DTR should be turned ON in open() and OFF in close()?
>
> I'm waiting on this patch series to be accepted, then submit other
> improvements. Or it may be better to submit a longer patch series that
> has further improvements appended... I'm new here and not really sure.
Generally, you should wait for a series to be reviewed before sending
(too many) follow ups. Unless you find any issues with it and ask for it
to be dropped, that is.
Thanks,
Johan
--
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
