Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752280AbcCAROk (ORCPT <rfc822;w@1wt.eu>);
	Tue, 1 Mar 2016 12:14:40 -0500
Received: from mail-lb0-f193.google.com ([209.85.217.193]:34891 "EHLO
	mail-lb0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751273AbcCAROh (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 1 Mar 2016 12:14:37 -0500
Message-ID: <1456852472.23036.124.camel@gmail.com>
Subject: Re: [PATCH v7] serial: support for 16550A serial ports on LP-8x4x
From: Sergei Ianovich <ynvich@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-kernel@vger.kernel.org
Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>, Arnd Bergmann <arnd@arndb.de>,
        Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Paul Burton <paul.burton@imgtec.com>, Mans Rullgard <mans@mansr.com>,
        Joachim Eastwood <manabian@gmail.com>,
        Scott Wood <scottwood@freescale.com>,
        Paul Gortmaker <paul.gortmaker@windriver.com>,
        Peter Ujfalusi <peter.ujfalusi@ti.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>
Date: Tue, 01 Mar 2016 20:14:32 +0300
In-Reply-To: <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>
	 <1456850782.13244.208.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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2187
Lines: 76

On Tue, 2016-03-01 at 18:46 +0200, Andy Shevchenko wrote:
> 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.

Sure.

> So, but if you support only fixed rates, why do you care about BOTHER
> at all?

If BOTHER is defined, tty_termios_baud_rate()
and tty_termios_encode_baud_rate() allow non-standard baud rates. I
should clear it from c_cflag to indicate I don't support it.

> > > >  
> > > 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.

> > 
> > 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.

I see. It saves a few lines.

> > ---
> > 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.<anonymous>.rlock.raw_lock')
> 
> Do you have any warning verbosity enabled? I see a lot of stuff like
> this in the code

Plain `make`.

The warning seems to be the result of initializing a spinlock with
zero. Spinlocks are intentionally obfuscated, but I didn't investigate
further.

> $ git grep -n 'struct .* = {0};' | wc -l
> 338
> 
> $ git grep -n 'struct .* = { \?0 \?};' | wc -l
> 550
> 
> ( '… = { 0 };' included)

The first structure member is most likely not a spinlock in those
cases.

> > ---
> > 
> > Zero triggers a warning. I'll use memset().
> 
> Either will work.

OK

The only remaining open point is BOTHER handling.