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

[Crescent CY Hsieh]

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".

---
Sincerely,
Crescent CY Hsieh