Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753395AbbKQJYz (ORCPT ); Tue, 17 Nov 2015 04:24:55 -0500 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:34498 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752179AbbKQJYw (ORCPT ); Tue, 17 Nov 2015 04:24:52 -0500 Date: Tue, 17 Nov 2015 10:24:42 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: "Matwey V. Kornilov" Cc: gregkh@linuxfoundation.org, jslaby@suse.com, peter@hurleysoftware.com, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org Subject: Re: [PATCH v3 5/5] tty: Add software emulated RS485 support for 8250 Message-ID: <20151117092442.GL22022@pengutronix.de> References: <1447338836-8785-1-git-send-email-matwey@sai.msu.ru> <1447338836-8785-6-git-send-email-matwey@sai.msu.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1447338836-8785-6-git-send-email-matwey@sai.msu.ru> User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1682 Lines: 50 Hello, On Thu, Nov 12, 2015 at 05:33:56PM +0300, Matwey V. Kornilov wrote: > +static void serial8250_start_tx(struct uart_port *port) > +{ > + struct uart_8250_port *up = up_to_u8250p(port); > + __u32 delay; > + > + serial8250_rpm_get_tx(up); > + > + if (timer_pending(&up->rs485_start_tx_timer)) > + return; I think this is wrong (or at least suboptimal). The .start_tx callback can be called even if transmission is already ongoing. In this case you don't want to restart the rs485_start_tx_timer. > + if ((delay = serial8250_rs485_start_tx(up))) { > + mod_timer(&up->rs485_start_tx_timer, jiffies + delay * HZ / 1000); > + } else { > + __start_tx(port); > + } > +} > + > [...] > diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h > index faa0e03..c4905e7 100644 > --- a/include/linux/serial_8250.h > +++ b/include/linux/serial_8250.h > @@ -86,6 +86,8 @@ struct uart_8250_ops { > struct uart_8250_port { > struct uart_port port; > struct timer_list timer; /* "no irq" timer */ > + struct timer_list rs485_start_tx_timer; /* "rs485 start tx" timer */ > + struct timer_list rs485_stop_tx_timer; /* "rs485 stop tx" timer */ Do you really need two timers here? At each point in time there should only be at most one of them active. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/