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?
> Continuation lines (e.g. the broken if statement) should be indented
> substantially to the right, which some subsystems interpret as at least
> two tabs further (you used just one).
>
> if (........................................... &&
> ...) {
> return;
> }
Ah, that's what is meant by that (i.e., to the right relative to the body,
not (just) the surrounding code).
> 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).
But assigning the result to a variable inline seems fine to me as that
doesn't introduce any indirection overhead.
> > 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.
> > 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.
> > However, in any case, the part of setting particular control characters
> > only when IXON is set seems like a very bad idea, in that it potentially
> > changes settings where no change was requested, so userspace code cannot
> > really be expected to check for that (and certainly not based on POSIX,
> > which explicitly states that other fields have to stay unchanged). So, if
> > we want to go with indicating lack of support, I think the correct strategy
> > would be to always force the control characters to what the hardware
> > supports.
>
> You're right, but since the defaults termios control characters match
> what the device uses, you can still reset them whenever user space tries
> to change them to indicate lack of support (instead of silently
> disabling sw flow control while leaving IXON set).
Well, yeah, that's what I suggested--while you had suggested to do so only
when IXON was set, which would be problematic IMO. Making them completely
unchangeable should be fine I'd think.
Regards, Florian
--
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
