Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1309572

Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 4/4] USB: serial: cp210x: Switch to new register access functions for large registers Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com> - 2016-01-02 04:20 +0100
  Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access  functions for large registers Martyn Welch <martyn.welch@collabora.co.uk> - 2016-01-14 19:00 +0100
    RE: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access  functions for large registers Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com> - 2016-01-14 19:20 +0100
      Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access  functions for large registers Martyn Welch <martyn.welch@collabora.co.uk> - 2016-01-14 19:30 +0100
      RE: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access  functions for large registers Konstantin Shkolnyy <Konstantin.Shkolnyy@silabs.com> - 2016-01-14 19:40 +0100
      Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register  access functions for large registers Johan Hovold <johan@kernel.org> - 2016-01-14 20:00 +0100
      Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register  access functions for large registers Johan Hovold <johan@kernel.org> - 2016-01-18 21:40 +0100

csiph-web