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
