Re: [PATCH v2 1/2] serial: Keep rs485 settings for devices without firmware node

[Lukas Wunner <lukas@wunner.de>]

On Fri, Oct 17, 2025 at 04:42:08PM +0200, Gerhard Engleder wrote:
> Commit fe7f0fa43cef ("serial: 8250: Support rs485 devicetree properties")
> retrieves rs485 properties for 8250 drivers. These properties are read
> from the firmware node of the device. If the firmware node does not
> exist, then the rs485 flags are still reset. Thus, 8250 driver cannot
> set rs485 flags to enable a defined rs485 mode during driver loading.
> This is no problem so far, as no 8250 driver sets the rs485 flags.
> 
> If no firmware node exist, then it should be possible for the driver to
> set a reasonable default rs485 mode. Therefore, reset rs485 flags only
> if a firmware node exists.
[...]
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3533,7 +3533,13 @@ int uart_get_rs485_mode(struct uart_port *port)
>  	u32 rs485_delay[2];
>  	int ret;
>  
> -	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
> +	/*
> +	 * Retrieve properties only if rs485 is supported and if a firmware node
> +	 * exist. If no firmware node exist, then don't touch rs485 config and
> +	 * keep initial rs485 properties set by driver.
> +	 */
> +	if (!(port->rs485_supported.flags & SER_RS485_ENABLED) ||
> +	    !dev_fwnode(dev))
>  		return 0;
>  
>  	ret = device_property_read_u32_array(dev, "rs485-rts-delay",

Hm, this will also skip the call to uart_sanitize_serial_rs485_delays().

I'm wondering if a better approach might be to move the check for
!dev_fwnode(dev) further down, after the invocation of
uart_sanitize_serial_rs485_delays()?

It may be necessary then to change the else-branch for the delays to
"else if (ret != -EINVAL)" because -EINVAL is returned from
device_property_read_u32_array() if there's no fw_node.

If you decide to keep the check at the top of the function, then
style-wise it would seem cleaner to not insert it into the existing
if-condition, but add a separate if-condition.  It doesn't matter
IMO that they both return 0.  The way the patch is now, it creates
a little confusion to which of the two if-conditions the code
comment pertains.

Thanks,

Lukas