Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

Felipe Balbi
2014-12-26T04:49:59Z

[-- Attachment #1: Type: text/plain, Size: 7183 bytes --]

Hi,

On Wed, Dec 24, 2014 at 02:43:27PM -0800, David Cohen wrote:
> Hi Felipe,
>
> Thanks replying.
>
> On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
> > Hi,
> >
> > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > GPIOs:
> > >
> > > (1) USB ID is connected directly to GPIO
> > >
> > > Optionally:
> > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> >
> > ok, so a fixed regulator with a GPIO enable pin.
>
> Pretty much yes.

ok

> > > (3) Platform has 2 USB controllers connected to same port: one for
> > > device and one for host role. D+/- are switched between phys
> > > by GPIO.
> >
> > so you have discrete mux with a GPIO select signal, like below ?
> >
> >
> > .-------.----------------. .--------.
> > | | | | | D+
> > | | | | |<-------------.
> > | | | | | |
> > | | USB Host -->| P | |
> > | | | | H | |
> > | | | | Y | D- |
> > | '----------------' | 0 |<--------. |
> > | | | | | |
> > | | '--------' .--------. D+
> > | | | |------>
> > | SOC GPIO | ----------------->| |
> > | | | MUX |
> > | | | |------>
> > | | .--------. '--------' D-
> > | .----------------. | | D- | |
> > | | | | P |<------' |
> > | | | | H | |
> > | | | | Y | |
> > | | USB Device -->| 1 | |
> > | | | | | D+ |
> > | | | | |<-------------'
> > | | | | |
> > '-------'----------------' '--------'
>
> Nice ASCII pic :)

asciio ftw \o/

> Yes, that's the case.

alright

> > I have been on and off about writing a pinctrl-gpio.c driver which would
> > allow us to hide this detail from users. It shouldn't really matter
> > which modes are available behind the mux, but we should be able to tell
> > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > don't really know if pinctrl would be able to handle discrete muxes.
> >
> > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > which different modes hidden behind discrete muxes.
>
> An input from Linus would fine in this case.
>
> >
> > > As per initial version, this driver has the duty to control whether
> > > USB-Host cable is plugged in or not:
> > > - If yes, OTG port is configured for host role
> > > - If no, by standard, the OTG port is configured for device role
> >
> > correct, this default-B is mandated by OTG spec anyway.
> >
> > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > ---
> > >
> > > Hi,
> > >
> > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > - The USB ID pin is connected directly to GPIO on SoC
> > > - When in host role, VBUS is provided by enabling a GPIO
> > > - Device and host roles are supported by 2 independent controllers with D+/-
> > > pins from port switched between different phys according a GPIO level.
> > >
> > > The ACPI table describes this USB port as a (virtual) device with all the
> > > necessary GPIOs. This driver implements support to this virtual device as an
> > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > PMIC, charge detection) will listen to extcon events.
> >
> > Right I think you're almost there, but I still think that this needs to
> > be a bit broken down. First, we need some generic DRD library under
> > drivers/usb/common, and that should be the one listening to extcon cable
> > events. But your extcon driver should be doing only that: checking which
> > cable was attached, it shouldn't be doing the switch by itself. That
> > should be part of the DRD library.
> >
> > That DRD library would also be the one enabling the (optional) VBUS
> > regulator.
> >
> > George Cherian tried to implement a generic DRD library but I think
> > there are still too many changes happening on usbcore and udc-core. Most
> > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > know how to properly unload an hcd/udc), but there are details missing,
> > no doubt.
> >
> > If we can find a way to broadcast (probably not the best term, but
> > whatever) a "Hey ID pin was just grounded" message, we can get things
> > working.
> >
> > That message, btw, shouldn't really depend on extcon-gpio alone because
> > other platforms might use non-gpio methods to verify ID pin level. In
> > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > that a generic DRD library can listen to and load/unload the correct
> > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
>
> IMHO extcon is the correct way to broadcast it, as long as we define a
> standard for the cable names. E.g. DRD library could listen to
> "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> it just matters that whoever is controlling it broadcast via this cable.

right, the likelyhood that someone would not use a GPIO is also quite
minimal and for such cases, the controller would likely switch roles
automatically (like with MUSB).

> > With that in mind, I think you can use extcon-gpio.c for your purposes
> > if the other pieces can be fullfilled by regulator and pinctrl.
>
> In my case we have all gpios listed in a single ACPI device. In order to
> be backwards compatible with products already on market, we'd need
> something like a single mfd to register platform devices for this
> smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).

correct.

> > > + ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> > > + vuport_isr, vuport_thread_isr,
> > > + IRQF_SHARED | IRQF_TRIGGER_RISING |
> > > + IRQF_TRIGGER_FALLING,
> > > + dev_name(dev), vup);
> > > + if (ret < 0) {
> > > + dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> > > + ret);
> > > + goto irq_err;
> > > + }
> > > + vuport_do_usb_id(vup);
> > > +
> > > + platform_set_drvdata(pdev, vup);
> > > +
> > > + dev_info(dev, "driver successfully probed\n");
> >
> > this will just make boot noisier, make it dev_dbg() ? Or even
> > dev_vdbg() ?
>
> dev_dgb() perhaps.

sure, why not :-)

--
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]