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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
