Re: [PATCH v2] tty: serial: Add RS422 flag to struct serial_rs485

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

Hi,

On 06.11.23 08:19, Crescent CY Hsieh wrote:
> On Sat, Nov 04, 2023 at 08:53:18PM +0100, Lino Sanfilippo wrote:
>> On 01.11.23 07:44, Crescent CY Hsieh wrote:
>>> @@ -1371,11 +1371,26 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>>>  {
>>>  	u32 supported_flags = port->rs485_supported.flags;
>>>
>>> -	if (!(rs485->flags & SER_RS485_ENABLED)) {
>>> +	if (!(rs485->flags & (SER_RS485_ENABLED | SER_RS422_ENABLED))) {
>>>  		memset(rs485, 0, sizeof(*rs485));
>>>  		return;
>>>  	}
>>>
>>> +	/* Pick sane setting if the user enables both interfaces */
>>> +	if (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS422_ENABLED) {
>>> +		dev_warn_ratelimited(port->dev,
>>> +			"%s (%d): Invalid serial interface setting, using RS485 instead\n",
>>> +			port->name, port->line);
>>> +		rs485->flags &= ~SER_RS422_ENABLED;
>>> +	}
>>> +
>>> +	/* Clear other bits and return if RS422 is enabled */
>>> +	if (rs485->flags & SER_RS422_ENABLED) {
>>> +		memset(rs485, 0, sizeof(*rs485));
>>
>> Why are all flags cleared but SER_RS422_ENABLED?
>
> IMO, RS422 and RS485 are distinct serial interfaces. Therefore, when
> RS422 is enabled, the other RS485 flags should be cleared, and vice
> versa.
>
>>> +		rs485->flags |= SER_RS422_ENABLED;
>>> +		return;
>>> +	}
>>
>> What about all the other code places that check for SER_RS485_ENABLED?
>> For example uart_update_mctrl(), uart_suspend_port() and uart_resume_port() check this flag
>> to decide whether to set the modem control lines or not. Should this not also apply to
>> SER_RS422_ENABLED?
>
> After reviewing the code in serial_core.c, there are actually some codes
> that check for "SER_RS485_ENABLED" flag before setting the modem control
> lines.
>
> It also appears that these codes can only be done when disabling RS485.
>
> So yes, I will apply "SER_RS422_ENABLED" flag to these locations in the
> next patch.
>
>>
>> Maybe it would be better to change the meaning of the flag: Instead of being a substitution for
>> SER_RS485_ENABLED, it could be used to mark a special mode.
>> So if both SER_RS485_ENABLED and SER_RS485_MODE_RS422 are set it would mean that we have RS422.
>
> RS422 is not a mode of RS485, so I think using two flags to represent
> them is much more reasonable, even though they are both included in the
> "struct serial_rs485".

Yes, RS422 is not a mode of RS485, but you are already using the rs485 (and not a rs422) structure.
And treating RS422 as a different mode in the existing code would make things much easier and keep the code
clean. For example you would not have to alter all the code places that check for SER_RS485_ENABLED.
Also SER_RS485_ENABLED and SER_RS422_ENABLED would have the exact same effect, so why use two
different flags, when the effect is the same?

Regards,
Lino