From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751450AbcCAQpo (ORCPT ); Tue, 1 Mar 2016 11:45:44 -0500 Received: from mga02.intel.com ([134.134.136.20]:59959 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751154AbcCAQpm (ORCPT ); Tue, 1 Mar 2016 11:45:42 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,523,1449561600"; d="scan'208";a="57710063" Message-ID: <1456850782.13244.208.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 18:46:22 +0200 In-Reply-To: <1456849504.23036.108.camel@gmail.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> <1456849504.23036.108.camel@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 On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote: > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote: > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote: > > > + len &= 3; Mask as well to be defined. > > > + 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 */ So, but if you support only fixed rates, why do you care about BOTHER at all? > > >  > > 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. > + > > > + /* 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. Yeah, but I meant to unconditionally call it just once here every time.  tty_termios_encode_baud_rate(termios, baud, baud); > +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') Do you have any warning verbosity enabled? I see a lot of stuff like this in the code $ git grep -n 'struct .* = {0};' | wc -l 338 $ git grep -n 'struct .* = { \?0 \?};' | wc -l 550 ( '… = { 0 };' included) > --- > > Zero triggers a warning. I'll use memset(). Either will work. > Thanks for lightning fast reviews. I'll resubmit v8 if there is no > objections to the points above. See above. -- Andy Shevchenko Intel Finland Oy From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Shevchenko Subject: Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x Date: Tue, 01 Mar 2016 18:46:22 +0200 Message-ID: <1456850782.13244.208.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> <1456849504.23036.108.camel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <1456849504.23036.108.camel@gmail.com> Sender: linux-kernel-owner@vger.kernel.org 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" List-Id: devicetree@vger.kernel.org On Tue, 2016-03-01 at 19:25 +0300, Sergei Ianovich wrote: > On Tue, 2016-03-01 at 13:06 +0200, Andy Shevchenko wrote: > > On Tue, 2016-03-01 at 00:26 +0300, Sergei Ianovich wrote: > > > + len &=3D 3; Mask as well to be defined. > > > + len <<=3D 3; > >=20 > > Perhaps to define magic number (e.g. LP8841_DATA_LEN_SHIFT). >=20 > OK > + baud =3D tty_termios_baud_rate(termios); > > > + > > > +#ifdef BOTHER > > > + /* We only support fixed rates */ So, but if you support only fixed rates, why do you care about BOTHER at all? > > >=C2=A0 > > I think you can call this unconditionally together with case > > > 115200. >=20 > 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. > + > > > + /* We only support up to 115200 */ > > > + if (baud > 115200) { > > > + baud =3D 115200; > > > + tty_termios_encode_baud_rate(termios, baud, > > > baud); > > > + } >=20 > This one deals with the case when the rate is over 115200. If the > previous case has been triggered, this one won't be. Yeah, but I meant to unconditionally call it just once here every time. =C2=A0tty_termios_encode_baud_rate(termios, baud, baud); > +static int lp8841_serial_probe(struct platform_device *pdev) > > > +{ > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct uart_8250_port uart =3D {}; > >=20 > > {0} >=20 > --- > 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 > =C2=A0 struct uart_8250_port uart =3D {0}; > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0^ > drivers/tty/serial/8250/8250_lp8841.c:124:32: note: (near > initialization for 'uart.port.lock..rlock.raw_lock') Do you have any warning verbosity enabled? I see a lot of stuff like this in the code $ git grep -n 'struct .* =3D {0};' | wc -l 338 $ git grep -n 'struct .* =3D { \?0 \?};' | wc -l 550 ( '=E2=80=A6 =3D { 0 };' included) > --- >=20 > Zero triggers a warning. I'll use memset(). Either will work. > Thanks for lightning fast reviews. I'll resubmit v8 if there is no > objections to the points above. See above. --=20 Andy Shevchenko Intel Finland Oy