Re: [PATCH 1/3] usb: host: st-hcd: Add USB HCD support for STi SoCs

[Lee Jones]

On Fri, 11 Jul 2014, Alan Stern wrote:
> On Fri, 11 Jul 2014, Peter Griffin wrote:
> > On Thu, 10 Jul 2014, Alan Stern wrote:
> > 
> > > On Thu, 10 Jul 2014, Peter Griffin wrote:
> > > 
> > > > This driver adds support for the USB HCD present in STi
> > > > SoC's from STMicroelectronics. It has been tested on the
> > > > stih416-b2020 board.
> > > 
> > > This driver file, along with the Kconfig changes, belongs in the
> > > arch/platform-specific source directory.  Not in drivers/usb/host/.  
> > > It is, after all, a platform driver that registers two platform
> > > devices.
> > 
> > Why do think that?
> 
> Because it is true.  One can easily see that st-hcd.c is a platform
> driver: It contains a module_platform_driver() line and a struct
> platform_driver definition near the end.  And it registers a platform
> device by calling platform_device_add() in st_hcd_device_create(),
> which is called twice by st_hcd_probe().

You are correct that this is indeed a platform driver and in the 'old
days' these would have been stuffed into the ARM sub-architecture
directories.  However, these became far too bloated and created too
much churn which angered Linus.  A great deal has changed since his
ARM rant [1].  All drivers (platform or otherwise) are now to live in
'drivers', which makes a great deal of sense really, doesn't it?

Did you grep kernel wide for module_platform_driver()?

$ git grep module_platform_driver -- arch/ | wc -l
12

$ git grep module_platform_driver -- drivers/ | wc -l
1138

Most platform drivers have already been moved.

> > AFAIK certainly for ARM we are trying NOT to add code
> > under the arch/platform directorys and get the code into the relevant subsystems.
> 
> This does not agree with my experiences.

Then you haven't been following what's been going on for the past 3
years or so.  The majority of platform drivers have been moved out to
drivers.  Hence the creation of some fantastic new subsystems, such as
clk and pinctrl, to name but two.  These (and others) were realised as
part of a push to consolidate and remove all driver code out of the
arch directories.

> > In order to prove the above if you look in drivers/usb/host/ there are many other 
> > similar platform drivers for other SoC families: -
> > bcma-hcd.c
> > ehci-atmel.c
> > ssb-hcd.c
> > ehci-exynos.c
> > ehci-fsl.c
> > ehci-mxc.c 
> > ehci-omap.c
> > ehci-orion.c
> > ohci-nxp.c
> > and more, but you get the idea. In short I believe this to be the
> > correct directory :-)
> 
> No.  bcma-hcd.c and ssb-hcd.c are similar to your st-hcd.c, but the
> others aren't.
> 
> Take ehci-atmel.c as a typical example.  It does not define any
> ehci_pdata structure and does not call platform_device_add(); instead
> it calls usb_add_hcd().  The others work the same way.  I would suggest
> that bcma-hcd.c and ssb-hcd.c don't belong in drivers/usb/host either.
> 
> For examples of drivers that _do_ resemble st-hcd.c, look in:
> 
> arch/arm/mach-cns3xxx/cns3420vb.c
> arch/arm/mach-cns3xxx/core.c
> arch/arm/mach-shmobile/setup-r8a7778.c
> arch/arm/mach-shmobile/setup-r8a7779.c
> arch/arm/mach-omap2/board-cm-t3517.c
> arch/mips/netlogic/xlr/platform.c
> arch/mips/ath79/dev-usb.c
> arch/mips/loongson1/common/platform.c
> arch/mips/alchemy/common/platform.c
> 
> These files all define ehci_pdata structures and register platform 
> devices.  And they obviously are all located in arch/platform-specific 
> directories.

Right, these are all 'old' platforms.  There is no way these drivers
would be accepted into the architecture directories now days.

Drivers live in 'drivers'.

New platforms also have to have Device Tree support, so even platform
data (which I agree should live in arch/arm) doesn't live in
arch/arm/{mach,plat}-* anymore.  For many of the more modern
architectures the aforementioned directories are bare.

> > > > +++ b/drivers/usb/host/Kconfig
> > > > @@ -753,6 +753,23 @@ config USB_HCD_SSB
> > > >  
> > > >  	  If unsure, say N.
> > > >  
> > > > +config USB_HCD_ST
> > > > +	tristate "STMicroelectronics STi family HCD support"
> > > 
> > > Why does this need to be tristate?  Why not always build it into the 
> > > kernel?  Or at least make it boolean rather than tristate.
> > 
> > Building as a module is useful on these embedded SoCs usually as a mechanism for speeding up boot time.
> > Anything which isn't critical to getting the core functionality of the device going (in this case decoding
> > TV and outputting on HDMI), can be deffered to a later point. Things like USB drivers, can then be loaded in
> > after that point (deffered module loading), to give the appearence of 'instant on' (which in consumer electronics
> > is what everyone wishes to achieve).
> 
> But st-hcd.c takes very little time to run.  ehci-platform will cause a 
> delay, so it makes sense for ehci-platform to be a module.  But there's 
> no reason for st-hcd to be a module.
> 
> > Going back to my examples above, most of these platforms are also defined as tristate.
> 
> Probably for historical reasons.  They don't need to be tristate now.

I'm not going to get into this too much, but it is _my oppinion_ that
if a driver can be a module, the option for it to be a module should
exist.

[...]

[1] https://lkml.org/lkml/2011/3/17/492

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/