Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753279AbcCALGE (ORCPT ); Tue, 1 Mar 2016 06:06:04 -0500 Received: from mga14.intel.com ([192.55.52.115]:54955 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752460AbcCALGA (ORCPT ); Tue, 1 Mar 2016 06:06:00 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,522,1449561600"; d="scan'208";a="914312404" Message-ID: <1456830401.13244.189.camel@linux.intel.com> Subject: Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x From: Andy Shevchenko To: Sergei Ianovich , linux-kernel@vger.kernel.org Cc: Alan Cox , Arnd Bergmann , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Greg Kroah-Hartman , Jiri Slaby , Heikki Krogerus , Peter Hurley , Masahiro Yamada , Paul Burton , Mans Rullgard , Joachim Eastwood , Scott Wood , Paul Gortmaker , Peter Ujfalusi , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "open list:SERIAL DRIVERS" Date: Tue, 01 Mar 2016 13:06:41 +0200 In-Reply-To: <1456781209-11390-1-git-send-email-ynvich@gmail.com> References: <1456589675-25377-1-git-send-email-ynvich@gmail.com> <1456781209-11390-1-git-send-email-ynvich@gmail.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3809 Lines: 155 On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote: > The patch adds support for 3 additional LP-8x4x built-in serial > ports. > > The device can also host up to 8 extension cards with 4 serial ports > on each card for a total of 35 ports. However, I don't have > the hardware to test extension cards, so they are not supported, yet. > > Signed-off-by: Sergei Ianovich > Reviewed-by: Heikki Krogerus Few more comments, mostly about style. > +static void lp8841_serial_set_termios(struct uart_port *port, > + struct ktermios *termios, struct ktermios *old) > +{ > +#ifdef BOTHER > + unsigned int cbaud; > +#endif > + unsigned int baud; > + unsigned int len; Since you are writing this to the register at the end maybe   - unsigned int -> u8 (write*b* — exactly one byte)  - len -> value (it's not only about data length) > + unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8; > + struct lp8841_serial_data *data = port->private_data; > + > + /* We only support CS7 and CS8 */ > + while ((termios->c_cflag & CSIZE) != CS7 && > +        (termios->c_cflag & CSIZE) != CS8) { > + termios->c_cflag &= ~CSIZE; > + termios->c_cflag |= old_csize; > + old_csize = CS8; > + } > + > + serial8250_do_set_termios(port, termios, old); > + > + if ((termios->c_cflag & CSIZE) == CS7) > + len = 9; > + else > + len = 10; > + > + if (termios->c_cflag & CSTOPB) > + len++; > + if (termios->c_cflag & PARENB) > + len++; > + if (!(termios->c_cflag & PARODD)) > + len++; > +#ifdef CMSPAR > + if (termios->c_cflag & CMSPAR) > + len++; > +#endif I don't know if someone likes it or not (up to you), but for me looks better to have ternary operators here: value += (termios->c_cflag & CSTOPB) ? 1 : 0; value += (termios->c_cflag & PARENB) ? 1 : 0; value += (termios->c_cflag & PARODD) ? 0 : 1; #ifdef CMSPAR value += (termios->c_cflag & CMSPAR) ? 1 : 0; #endif > + len -= 9; This one could be part of previous evaluation:  value = (termios->c_cflag & CSIZE) == CS7 ? 0 : 1; > + len &= 3; > + len <<= 3; Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT). > + > + baud = tty_termios_baud_rate(termios); > + > +#ifdef BOTHER > + /* We only support fixed rates */ > + cbaud = termios->c_cflag & CBAUD; > + > + if (cbaud == BOTHER) { > + termios->c_cflag &= ~BOTHER; > + > + /* Don't rewrite B0 */ > + if (baud) { > + tty_termios_encode_baud_rate(termios, baud, > baud); > + baud = tty_termios_baud_rate(termios); > + > + /* Set sane default speed if we get 0 */ > + if (!baud) { > + baud = 9600; > + tty_termios_encode_baud_rate(termios > , > + baud, baud); I think you can call this unconditionally together with case > 115200. > + } > + } > + } > +#endif > + > + /* We only support up to 115200 */ > + if (baud > 115200) { > + baud = 115200; > + tty_termios_encode_baud_rate(termios, baud, baud); > + } Btw, can we use uart_get_baud_rate() here? > + writeb(len, data->ios_mem); > + > +} > +static int lp8841_serial_probe(struct platform_device *pdev) > +{ > + struct uart_8250_port uart = {}; {0} > + struct lp8841_serial_data *data; > + struct resource *mmres, *mires; > + int ret; > + > + mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mires = platform_get_resource(pdev, IORESOURCE_MEM, 1); Perhaps move it down to be closer to devm_ioremap_resource() call. > + if (!mmres) > + return -ENODEV; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + + mires = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + data->ios_mem = devm_ioremap_resource(&pdev->dev, mires); > + if (IS_ERR(data->ios_mem)) > + return PTR_ERR(data->ios_mem); -- Andy Shevchenko Intel Finland Oy