From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753334AbcIBL1f (ORCPT ); Fri, 2 Sep 2016 07:27:35 -0400 Received: from mga01.intel.com ([192.55.52.88]:32944 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751752AbcIBL1d (ORCPT ); Fri, 2 Sep 2016 07:27:33 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,270,1470726000"; d="scan'208";a="756493814" Date: Fri, 2 Sep 2016 14:21:06 +0300 From: Heikki Krogerus To: Kefeng Wang Cc: gregkh@linuxfoundation.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Shevchenko , Jiri Slaby , guohanjun@huawei.com, xuwei5@hisilicon.com, graeme.gregory@linaro.org, z.liuxinliang@hisilicon.com, linux-acpi@vger.kernel.org Subject: Re: [PATCH v4 1/2] serial: 8250_dw: make dw8250_set_termios as default set_termios callback Message-ID: <20160902112106.GB21737@kuha.fi.intel.com> References: <1472614152-43136-1-git-send-email-wangkefeng.wang@huawei.com> <1472614152-43136-2-git-send-email-wangkefeng.wang@huawei.com> <20160831120756.GG1751@kuha.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160831120756.GG1751@kuha.fi.intel.com> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 31, 2016 at 03:07:56PM +0300, Heikki Krogerus wrote: > Hi, > > On Wed, Aug 31, 2016 at 11:29:11AM +0800, Kefeng Wang wrote: > > Make dw8250_set_termios() handle all cases of dw8250_data->clk properly, > > then we can safely use dw8250_set_termios() as the default set_termios > > callback instead of serial8250_do_set_termios(), so do it. > > > > Signed-off-by: Kefeng Wang > > --- > > drivers/tty/serial/8250/8250_dw.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > > index 5c0c123..adecece 100644 > > --- a/drivers/tty/serial/8250/8250_dw.c > > +++ b/drivers/tty/serial/8250/8250_dw.c > > @@ -235,7 +235,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, > > unsigned int rate; > > int ret; > > > > - if (IS_ERR(d->clk) || !old) > > + if (IS_ERR_OR_NULL(d->clk) || !old) > > goto out; > > This part is fine, but... > > > clk_disable_unprepare(d->clk); > > @@ -308,13 +308,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) > > p->serial_in = dw8250_serial_in32; > > data->uart_16550_compatible = true; > > } > > - p->set_termios = dw8250_set_termios; > > } > > > > /* Platforms with iDMA */ > > if (platform_get_resource_byname(to_platform_device(p->dev), > > IORESOURCE_MEM, "lpss_priv")) { > > - p->set_termios = dw8250_set_termios; > > data->dma.rx_param = p->dev->parent; > > data->dma.tx_param = p->dev->parent; > > data->dma.fn = dw8250_idma_filter; > > @@ -391,6 +389,7 @@ static int dw8250_probe(struct platform_device *pdev) > > p->iotype = UPIO_MEM; > > p->serial_in = dw8250_serial_in; > > p->serial_out = dw8250_serial_out; > > + p->set_termios = dw8250_set_termios; > > I'm a bit worried about this. If I remember correctly, we didn't set > the set_termios hook by default in the past because somebody was using > a type of clk that can't handle rate rounding and ended up using > improper clock frequency for some baud rates. Unfortunately I can't > remember who had that problem and on which platform. > > On the other hand, I'm not sure if drivers should worry about things > like that. This should be done the other way around. The platform > with the problem should unset the p->set_termios hook in a quirk, but > by default everybody would use dw8250_set_termios. No comments... I would suggest we skip this for now. I don't think it's necessary for Hisilicon Hip05 SoC support, so perhaps you can just propose this change separately. Thanks, -- heikki