From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752767AbbKZHeu (ORCPT <rfc822;w@1wt.eu>);
	Thu, 26 Nov 2015 02:34:50 -0500
Received: from mail-lf0-f47.google.com ([209.85.215.47]:35762 "EHLO
	mail-lf0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751423AbbKZHeo (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 26 Nov 2015 02:34:44 -0500
Date: Thu, 26 Nov 2015 08:35:12 +0100
From: Johan Hovold <johan@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>, johan@kernel.org,
        USB <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] USB: serial: cp210x: Add tx_empty()
Message-ID: <20151126073512.GD11484@localhost>
References: <1448404121-12964-1-git-send-email-konstantin.shkolnyy@gmail.com>
 <CAHp75VcU5YeB3YMfk8JrmhJVjFCZ0ity6eACefkK1kQVTyJStQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VcU5YeB3YMfk8JrmhJVjFCZ0ity6eACefkK1kQVTyJStQ@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Nov 25, 2015 at 10:26:12PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 12:28 AM, Konstantin Shkolnyy
> <konstantin.shkolnyy@gmail.com> wrote:
> > Added tx_empty callback needed for generic wait-until-sent support.
> > Without this function, when the port is closed usbserial can't know that
> > there are still data in the chip's transmit FIFO. The chip gets disabled
> > and untransmitted data lost. When the actual byte count is reported by
> > tx-empty the close can be delayed until all data are sent.
> >
> 
> >  /*
> > + * Read how many bytes are waiting in the TX queue.
> > + */
> 
> One line?

Not necessarily for a function header.

> > +static bool cp210x_tx_empty(struct usb_serial_port *port)
> > +{
> > +       int err;
> > +       u32 count;
> > +
> > +       err = cp210x_get_tx_queue_byte_count(port, &count);
> 
> > +       if (!err && count)
> > +               return false;
> > +
> > +       return true;
> 
> return err || !count;

Nah, I don't consider that an improvement.

I prefer separating the error and success paths, for example:

	if (err)
		return true;

	return !count;

but the current code is also ok.

> Btw, can be count left uninitialized?

Yes, but that's not an issue. Clearly separating the success and errors
paths as I suggested above would make that more obvious however.

I'll fix that up before applying.

Thanks,
Johan