Hi Lukas
After a closed look to the driver, I believe that it is not wrong, or
at least it is only badly documented/commented.
The user case is: the user wants delay_rts_before_send and/or
delay_rts_after_send but does not want automatic handling of RTS.
If that is the case, RTS_INVERT cannot be used, because it only works
when RS485_URA is enabled (hw limination), and therefore we need to
clear all the flags except SER_RS485_ENABLED, because if that flag is
0, delay_rts_* are ignored.
The datasheet in this case is not very clarifying, perhaps Peter Hong
has some internal information regarding the hardware here, and can
tell us if TXW4C_IRA/RXW4C_IRA does not work without RS485_URA.
It that is the case we should move the check at the beginning something like:
Replace:
if (rs485->flags & SER_RS485_ENABLED)
memset(rs485->padding, 0, sizeof(rs485->padding));
else
memset(rs485, 0, sizeof(*rs485));
with:
/* Check for rs485 enabled and invalid rts configuration*/
if (!(rs485->flags & SER_RS485_ENABLED) ||
(!(rs485->flags & SER_RS485_RTS_ON_SEND)) ==
!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
memset(rs485, 0, sizeof(*rs485));
}
else {
memset(rs485->padding, 0, sizeof(rs485->padding));
config |= RS485_URA;
}
Thanks for your patch and sorry for the delay
PS: to make it official
Nacked-by: Ricardo Ribalda <ricardo.ribalda@xxxxxxxxx> (Until Peter
Hong has a response)
On Fri, Oct 27, 2017 at 10:14 AM, Lukas Wunner <lukas@xxxxxxxxx> wrote:
> On Thu, Oct 26, 2017 at 10:26:27AM +0200, Ricardo Ribalda Delgado wrote:
>> On Tue, Oct 24, 2017 at 8:37 PM, Lukas Wunner <lukas@xxxxxxxxx> wrote:
>> > This driver's ->rs485_config callback checks if SER_RS485_RTS_ON_SEND
>> > and SER_RS485_RTS_AFTER_SEND have the same value. If they do, it means
>> > the user has passed in nonsensical data with the TIOCSRS485 ioctl()
>> > since RTS must have a different polarity when sending and when not
>> > sending. In this case, rs485 mode is not enabled (the RS485_URA bit
>> > is not set in the RS485 Enable Register) and this is supposed to be
>> > signaled back to the user by clearing the SER_RS485_ENABLED bit in
>> > struct serial_rs485 ... except a missing tilde character is preventing
>> > that from happening.
>>
>> Thanks for your patch. Hope you haven't lost much time debugging it.
>
> I haven't, I don't even have the hardware, just stumbled across the bug
> while reading the rs485 code of various drivers. I did have to chase
> down the datasheet of this chip though to find out what the RS485_URA bit
> exactly does. :-) There's an older version of the datasheet which doesn't
> specify the RS485 bits yet.
>
>
>> Your patch looks good to me. I would only replace "nonsensical" with
>> "invalid" on the subject of the patch.
>
> Happy to respin. Could you provide an Acked-by for the patch? Thanks!
>
> Lukas
--
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
