From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751069AbcCAQZO (ORCPT ); Tue, 1 Mar 2016 11:25:14 -0500 Received: from mail-lb0-f193.google.com ([209.85.217.193]:34731 "EHLO mail-lb0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750722AbcCAQZJ (ORCPT ); Tue, 1 Mar 2016 11:25:09 -0500 Message-ID: <1456849504.23036.108.camel@gmail.com> Subject: Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x From: Sergei Ianovich To: Andy Shevchenko , 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 19:25:04 +0300 In-Reply-To: <1456830401.13244.189.camel@linux.intel.com> References: <1456589675-25377-1-git-send-email-ynvich@gmail.com> <1456781209-11390-1-git-send-email-ynvich@gmail.com> <1456830401.13244.189.camel@linux.intel.com> 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 On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote: > 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; Great point. > > + len &= 3; > > + len <<= 3; > > Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT). OK > > + > > + 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(termi > > os > > , > > + baud, baud); > > I think you can call this unconditionally together with case > > 115200. The calls are orthogonal. This one deals with the case when BOTHER is defined and set, and we have non-zero rate with BOTHER, but we have zero rate after BOTHER is cleared. So we set 9600 as a sane default speed. > > + } > > + } > > + } > > +#endif > > + > > + /* We only support up to 115200 */ > > + if (baud > 115200) { > > + baud = 115200; > > + tty_termios_encode_baud_rate(termios, baud, baud); > > + } This one deals with the case when the rate is over 115200. If the previous case has been triggered, this one won't be. > Btw, can we use uart_get_baud_rate() here? uart_get_baud_rate() has already been called in serial8250_do_set_termios(). uart_get_baud_rate() calls tty_termios_encode_baud_rate(). uart_get_baud_rate() won't help us if BOTHER is set. Once BOTHER is cleared, we don't need any special processing of uart_get_baud_rate(). > > +static int lp8841_serial_probe(struct platform_device *pdev) > > +{ > > +     struct uart_8250_port uart = {}; > > {0} --- drivers/tty/serial/8250/8250_lp8841.c: In function 'lp8841_serial_probe': drivers/tty/serial/8250/8250_lp8841.c:124:32: warning: excess elements in struct initializer   struct uart_8250_port uart = {0};                                 ^ drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near initialization for 'uart.port.lock..rlock.raw_lock') --- Zero triggers a warning. I'll use memset(). > > +     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. OK Thanks for lightning fast reviews. I'll resubmit v8 if there is no objections to the points above.