Re: [PATCHv7 00/19] First round in OTG rework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Nov 08, 2011 at 03:55:15PM +0200, Heikki Krogerus wrote:
> One more change. Sorry about this.
> 
> Changes since v6:
> - Rename struct usb_otg member "xceiv" to "phy"
> 
> 
> Heikki Krogerus (19):
>   usb: otg: Rename otg_transceiver to usb_phy
>   usb: otg: Rename usb_otg and usb_xceiv to usb_phy
>   usb: otg: Separate otg members from usb_phy
>   usb: otg: ab8500: Start using struct usb_otg
>   usb: otg: fsl: Start using struct usb_otg
>   usb: otg: gpio_vbus: Start using struct usb_otg
>   usb: otg: isp1301_omap: Start using struct usb_otg
>   usb: otg: msm: Start using struct usb_otg
>   usb: otg: langwell: Start using struct usb_otg
>   usb: otg: nop: Start using struct usb_otg
>   usb: otg: twl4030: Start using struct usb_otg
>   usb: otg: twl6030: Start using struct usb_otg
>   usb: otg: ulpi: Start using struct usb_otg
>   arm: imx: Start using struct usb_otg
>   usb: musb: Start using struct usb_otg
>   power_supply: Convert all users to new usb_phy
>   usb: Convert all users to new usb_phy
>   usb: otg: Remove OTG specific members from usb_phy
>   usb: otg: Convert all users to pass struct usb_otg for OTG functions

All in all, except for a few comments I had, the series is ok. But I
truly don't see the big benefit we have out of this. You're just
renaming a bunch of structures, enumerations and functions without a
real reason behind it.

What we need in the long run, like I had told you back in Nokia, is to
be able to compile several transceiver drivers into the same zImage or
as dynamically loadable modules and still have it working somehow. Which
means that we need a way for a Link (dwc3, musb, omap_udc, renesas_udc,
etc) to request a transceiver, or that transceiver has to be assigned to
a link as a resource.

Also keep in mind that on USB3 device controllers, we will have at least
two PHYs: one which will be handling USB2 signalling and another which
will be handling USB3 signalling.

I say "at least" because OMAP boards generally have split PHY
functionality between internal (into the SoC) entities and an external
PMIC. If you take OMAP4, for example, we have an internal IP handling
the data parts and communicating VBUS/ID levels to MUSB while the
actual VBUS/ID comparators are inside the PMIC.

With this series of yours I don't see any way we could easily model this
split PHY functionality.

Some OEMs might also decide to ditch completely the internal and PMIC
PHY functionality in exchange for their own solution of an all-in-one
PHY (e.g. N900).

So all of that is still a pain in the ass to get sorted out and your
series isn't helping at all towards that goal so I can't see the real
benefit in accepting these patches other than having better matching
structure names.

You didn't even cleanup the <linux/usb/otg.h> header. There's a bunch of
stuff in there which isn't OTG-related at all and you seem to not have
cared too much about it.

At the end of the day we need to solve the problems we have and make the
Embedded part of the USB suport on linux (mainly the gadget framework
and OTG support) as easy as possible to generate a product out of it.

If you remember the amount of patches we had to get N900 sorted out, you
know what I'm talking about. To prevent our users from ever rebasing
their own hacks on their internal trees, we need to provide solutions
which are flexible enough to have boards with USB2-only, USB3,
Host-only, device-only, OTG, with and without USB Battery Charging.

As of now we can't.

I hope you have some other patches/ideas behind this series, otherwise
there's not enough reason to merge it.

I suggest you start laying down your ideas and, where possible, code so
we can come up with a solution to help everybody.

-- 
balbi

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux