linux-mips
[Top] [All Lists]

Re: [PATCH 06/25] USB: ehci: allow need_io_watchdog to be passed to ehci

To: Alan Stern <stern@rowland.harvard.edu>
Subject: Re: [PATCH 06/25] USB: ehci: allow need_io_watchdog to be passed to ehci-platform driver
From: Florian Fainelli <florian@openwrt.org>
Date: Wed, 03 Oct 2012 18:12:29 +0200
Cc: linux-usb@vger.kernel.org, Ralf Baechle <ralf@linux-mips.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Gabor Juhos <juhosg@openwrt.org>, Hauke Mehrtens <hauke@hauke-m.de>, Kelvin Cheung <keguang.zhang@gmail.com>, Jayachandran C <jayachandranc@netlogicmicro.com>, Dan Carpenter <dan.carpenter@oracle.com>, Geert Uytterhoeven <geert@linux-m68k.org>, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id:organization:user-agent :in-reply-to:references:mime-version:content-transfer-encoding :content-type; bh=I13ichDw/Xl9kARXMJMZDELVNxwwdaSO7+2AP/MhrPI=; b=x0SBddbV6/y5v2weLJT72/i6A9Kb0NJ+vebaRU6V0ZxofX0X+TB3L7Di/2gHNNQn1G T3DELtaR0pXIyDd/qgggyN2MQpHDzCTlV8LVE/pqFCS/om7eUscSpq0p29AuFlXyTpTf CRdqV/pIS8LellGYOBKFx89doYMlfC9PZALHNaPU+B5vY1PBr5rq5mssDJtS5TQJbzXy dxzWzbIZTeSmj/CR5UZPfPVggbP162PNw8qYN+jOoelL8BrN6E1wgnTl9HeRonXHOazw MvX5ojuOgVpPWYEjR5ae/3i19734Ijlxu3rcwKiSnOXkrlMrsxj+VnZUZUTFufiET2/w IDVg==
In-reply-to: <Pine.LNX.4.44L0.1210031154400.1441-100000@iolanthe.rowland.org>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
Organization: OpenWrt
References: <Pine.LNX.4.44L0.1210031154400.1441-100000@iolanthe.rowland.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: KMail/4.8.5 (Linux/3.2.0-24-generic; KDE/4.8.5; x86_64; ; )
On Wednesday 03 October 2012 12:01:22 Alan Stern wrote:
> On Wed, 3 Oct 2012, Florian Fainelli wrote:
> 
> > And convert all the existing users of ehci-platform to specify a correct
> > need_io_watchdog value.
> 
> IMO (and I realize that not everybody agrees), the patch description 
> should not be considered an extension of the patch title, as though the 
> title were the first sentence and the description the remainder of the 
> same paragraph.  Descriptions should stand on their own and be 
> comprehensible even to somebody who hasn't read the title.
> 
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
> > ---
> >  arch/mips/ath79/dev-usb.c             |    2 ++
> >  arch/mips/loongson1/common/platform.c |    1 +
> >  arch/mips/netlogic/xlr/platform.c     |    1 +
> >  drivers/usb/host/bcma-hcd.c           |    1 +
> >  drivers/usb/host/ehci-platform.c      |    1 +
> >  drivers/usb/host/ssb-hcd.c            |    1 +
> >  include/linux/usb/ehci_pdriver.h      |    3 +++
> >  7 files changed, 10 insertions(+)
> 
> More importantly...  Nearly every driver will have need_io_watchdog
> set.  Only a few of them explicitly turn it off.  The approach you're 
> using puts an extra burden on most of the drivers.
> 
> > diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-
platform.c
> > index 764e010..08d5dec 100644
> > --- a/drivers/usb/host/ehci-platform.c
> > +++ b/drivers/usb/host/ehci-platform.c
> > @@ -32,6 +32,7 @@ static int ehci_platform_reset(struct usb_hcd *hcd)
> >     ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
> >     ehci->big_endian_desc = pdata->big_endian_desc;
> >     ehci->big_endian_mmio = pdata->big_endian_mmio;
> > +   ehci->need_io_watchdog = pdata->need_io_watchdog;
> 
> > --- a/include/linux/usb/ehci_pdriver.h
> > +++ b/include/linux/usb/ehci_pdriver.h
> > @@ -29,6 +29,8 @@
> >   *                 initialization.
> >   * @port_power_off:        set to 1 if the controller needs to be powered 
> > down
> >   *                 after initialization.
> > + * @need_io_watchdog:      set to 1 if the controller needs the I/O 
watchdog to
> > + *                 run.
> 
> Instead, how about adding a no_io_watchdog flag?  Then after 
> ehci-platform.c calls ehci_setup(), it can see whether to turn off 
> ehci->need_io_watchdog.

Sounds good, you are right,  this also greatly reduces the chances to miss one 
user of this "feature".

> 
> This way, most of this patch would become unnecessary.
> 
> Alan Stern
> 

<Prev in Thread] Current Thread [Next in Thread>