Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Mon, 6 Nov 2023 15:19:47 +0800
From:   Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
To:     Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH v2] tty: serial: Add RS422 flag to struct serial_rs485
Message-ID: <ZUiTkyZSbbmTUGYQ@moxa-ThinkCentre-M90t>
References: <20231101064404.45711-1-crescentcy.hsieh@moxa.com>
 <0d203024-ba5c-4258-b549-b9304622978b@gmx.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0d203024-ba5c-4258-b549-b9304622978b@gmx.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BIjAAyk2J5x4MViFWxmYX4FkKx8ScfqHrmO1XC2SK2apZOUHInKDkgUj6tTD?=
 =?us-ascii?Q?jiY4WYPwQJsXqLWmz6pbyeNvymbF0k5RNEhS7QYeaOam5hjRpwzBsD6OxMfT?=
 =?us-ascii?Q?E9eg0uDpHmNex7IEKY+gUtq+hMBjEBgjyR4qakJiIdutkw2BNOmoCqZZjm0G?=
 =?us-ascii?Q?tBGPXq2e6OtlCZr7a3llodKfObvHBRkY6ND6EvoZXeA9yNYhCH7hc0zP71K/?=
 =?us-ascii?Q?AylMsa0JmmiEJTYFUYJNeChp3NSyaVDRQgjFLQ1wax0NRvPTOh5Ng4VbwhPW?=
 =?us-ascii?Q?2PSFHPTIerPQc9fSF+H2lKhmt+Oib4sxtIfxJQ/RoGPGVvYvuMz03sbr6dIe?=
 =?us-ascii?Q?qbiydDt3LjPjy/s9E0FE08bXzWvSeCD7zHkenm7r7r4Z+mbcrI8MAn27jOPC?=
 =?us-ascii?Q?cynIgvRdK7tTjhrp8236FhjUPle6CqA8POQ2yPL9Ma0t5/+Sn8UBMuwmHAMg?=
 =?us-ascii?Q?zvRzwQ16Feew64EbQ1K2h2DDsrGuqntWEdGbwo4B2FzAOzkQTick1nmeNyfc?=
 =?us-ascii?Q?RjYQ1uh6CowS39NpoDJQqGH6IW+iipze+/X4EMYIohfncuP8Ad39Z0RBi1ky?=
 =?us-ascii?Q?LEVxD4UrCtbjdUXmDWokXnap2JsGEh6vPx6yoHBsaEgnyVuT66wSgqyWyTzP?=
 =?us-ascii?Q?V4458gKhYs/N86ic2BnV+l5u+BXEIfFBbo9vBdhT3Cq7+5qVthDKDlPrihyi?=
 =?us-ascii?Q?/FQgDZ5gNtch5DLbZxq3J3Ysr5K1EA6K20thIoHQ10ruknMOc6shMadolpgI?=
 =?us-ascii?Q?Ag6obQo7KcRmhPUMO8klct1P6AjoPCuYI1z4r4jTKdDwPWx9/yeQZTg2CKH8?=
 =?us-ascii?Q?BK3By0pthDIVfXSqlXuh0FpkAWBa0OWVxA9pBoED59+JapwWhX425SDC1pOa?=
 =?us-ascii?Q?/rlZWkZEpHrMU1gaXDooYQFcei622EP6O+xy06e3Ep5OLgPSnywohkNQxqwC?=
 =?us-ascii?Q?oJp8wUzMG42I9jQ2ysVe+QXVUmjXUJhozbEmanDBkFzk2Hykto5yRSGB2Iq5?=
 =?us-ascii?Q?5u2mv3klwpE7g5fWiM//1TtZ174oLbFI19Z70c10kBOK27jdBW1cR4RTXHF4?=
 =?us-ascii?Q?MykFB1T5hZCQQcJvI/FN/ajBchcr4QWVPrNg1kSVNnZYP64dWMATSxToyMzg?=
 =?us-ascii?Q?jcxsNLCEeAl8h2oaDtQzc+bb5hIzEH/C692PIEZ6dSIMrnEoeYzR8Bf6wE+1?=
 =?us-ascii?Q?i2/Qr4+DusITD3ToNBxtW1idN0Tvv53RdQ1Y1BwV7Q0rGz1PgEcydjtfsaRO?=
 =?us-ascii?Q?MQKtC6I4GbJneenFPT/ieySDEJ0d/6oTlBOFg5TGuX7W9H4fKT/YK1E1HFLs?=
 =?us-ascii?Q?CVl7sa42iRFPIEuf6ep3vOqow1pcLff2+6ZBWiJGlV2TvkybqXUwS9LyhJBO?=
 =?us-ascii?Q?Pf2iHIDR8NSEVxNQZjEi5TXhqNraWWAn4g3T9hsLJUzn+iGU2PNOJkOHFahG?=
 =?us-ascii?Q?Ab8aa3hvUWtbzSaxzORlYs8+h3QgAb7zIDdSd40w09x161L7rlWpG3HpWqgi?=
 =?us-ascii?Q?9K9Jm+VpkJhs0jiw/vRsWMhSvDBQTPOG6/ILz48li43rfyJGzOCcY9Q4AMi0?=
 =?us-ascii?Q?g39MMNyettiOFWvH8VWyYb3d0jOk+wjC1d2W42AXnw7uIGxaWhcQx266r8ek?=
 =?us-ascii?Q?bQ=3D=3D?=
X-OriginatorOrg: moxa.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d525f548-23ff-4e64-046d-08dbde98c709
X-MS-Exchange-CrossTenant-AuthSource: SEYPR01MB5387.apcprd01.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2023 07:19:56.0534
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5571c7d4-286b-47f6-9dd5-0aa688773c8e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ADqhA0O9iqKJ6XF694DmjRhEZq1XJXFxl48OmwG1qr9sJaCbXdx+ix4UAUvKAO0hmjdYuPRTGPZHrJrnvpAhbqy98n7z5Edt/6hxZZ0dPAE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: JH0PR01MB6380
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Sun, 05 Nov 2023 23:20:19 -0800 (PST)

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