usbserial: pl2303 tx xon/xoff flow control

[Johan Hovold <johan@kernel.org>]

On Fri, May 18, 2018 at 06:06:09AM +0200, Florian Zumbiehl wrote:
> Hi,
> 
> > That looks like an HX device according to the current way we identify
> > these types (which may be wrong).
> 
> That at least matches the labeling on the chip, so I guess that might be
> correct?

Then it's an HX (even if the driver algorithm for detecting the type may
be wrong).

> > Depends on how you name your helper, I'd say. Another option could be to
> > use an intermediate bool. Either way, the above is hardly readable and
> > must be fixed.
> 
> I don't think a descriptive name would make up for the overhead of the
> indirection as it necessarily still hides what exactly is being tested from
> view, but doesn't add anything to readability as the basic function of the
> condition is obvious from the code itself (a list of fields being checked
> for changes from old to new).

The compiler would inline the function, and this isn't a hot path
anyway.

> But assigning the result to a variable inline seems fine to me as that
> doesn't introduce any indirection overhead.

Great, then please do that.

> > > I don't know how to enable IXANY or change the control characters, so I am
> > > working with what I do know (also, I guess the screenshot of the "ROM
> > > programming tool" in the "datasheet" suggests that none of that is
> > > supported).
> > 
> > Fair enough, that's why I asked. My device exhibits the same behaviour
> > with regards to IXANY by the way.
> 
> I am not sure I understand. Are you saying your device exhibits IXANY
> behaviour if you enable IXON with the patch applied? Mine definitely does
> not, I just re-checked, it receives other data just fine, but only resumes
> transmission when ^Q is received.

Mine exhibits the same behaviour *as yours*.

> > > Now, I agree that signaling lack of support would be better in general, but
> > > it does change what the code does for (still) unsupported cases. After all,
> > > usbserial in general does not signal that it doesn't support IXON/IXANY,
> > > but just pretends that it does. So, if there is a good reason why usbserial
> > > ignores POSIX on this point, I would think that would still apply for the
> > > remaining unsupported cases after partial support is implemented?!
> > 
> > There are historical reasons for a lot of things, but that's not
> > necessarily a reason to continue taking shortcuts.
> 
> Well, sure, I just assumed that there was a *good* reason for why it is the
> way it is, for lack of any information to the contrary, and so just
> preserved the existing behaviour for cases that I wasn't obviously
> improving.

Yep, and after taking a closer look at this, that seems sensible.

The usbserial code has behaved this way for decades without anyone
really complaining, and fixing this up properly would require quite a
bit of work.

I just don't think that should block this patch.

Johan
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html