| Message ID | 1414951910-16075-1-git-send-email-robert.jarzmik@free.fr |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > In order to prepare device-tree conversion, port gpio_vbus to use > gpio_desc. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> Acked-by: Philipp Zabel <philipp.zabel@gmail.com> regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robert, On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Add documentation for device-tree binding of the generic gpio-vbus phy > controller. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: devicetree@vger.kernel.org > --- > .../devicetree/bindings/phy/gpio-vbus.txt | 23 ++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/gpio-vbus.txt > > diff --git a/Documentation/devicetree/bindings/phy/gpio-vbus.txt b/Documentation/devicetree/bindings/phy/gpio-vbus.txt > new file mode 100644 > index 0000000..ffcfd35 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/gpio-vbus.txt > @@ -0,0 +1,23 @@ > +GPIO VBus phy > +============= > + > +gpio-vbus is a phy controller supporting VBus detection and pullup activation on > +GPIOs. Maybe duplicate the comment from the driver here how his is about B peripheral only devices > +Required properties: > +- compatible : should be "generic,phy-gpio-vbus" I'm not sure about the compatible value. I have not seen the "generic," vendor prefix, and all the other generic gpio-something bindings don't use any prefix: "gpio-gate-clock", "gpio-leds", "gpio-beeper", "pps-gpio", etc. > +- #phy-cells : from the generic PHY bindings, must be 1. > +- gpios : set of 2 gpio properties (see gpio.txt for gpio properties format) > + first gpio is required, it's the VBus sensing input gpio > + second gpio is optional, it's the D+ pullup controlling output > + gpio > + > +Optional properties: > +- wakeup : boolean, if true the VBus gpio will wakeup the platform The vbus_draw regulator should be part of this binding, I think. > +Example: > + usb2phy : gpio-vbus@13 { > + compatible = "generic,phy-gpio-vbus"; > + gpios = <&gpio 13 GPIO_ACTIVE_LOW>; > + wakeup; This on the other hand might be too generic. I'd like to see just wakeup used here, but the other bindings prefix "linux," (or "gpio-key,"). regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Robert, On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Add device-tree support for the generic gpio-vbus driver. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: devicetree@vger.kernel.org > --- > drivers/usb/phy/phy-gpio-vbus-usb.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/usb/phy/phy-gpio-vbus-usb.c b/drivers/usb/phy/phy-gpio-vbus-usb.c > index d302ab2..6194728 100644 > --- a/drivers/usb/phy/phy-gpio-vbus-usb.c > +++ b/drivers/usb/phy/phy-gpio-vbus-usb.c > @@ -13,6 +13,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/gpio.h> > #include <linux/module.h> > +#include <linux/of_gpio.h> > #include <linux/slab.h> > #include <linux/interrupt.h> > #include <linux/usb.h> > @@ -254,6 +255,12 @@ static int gpio_vbus_probe(struct platform_device *pdev) > } > gpiod = gpio_to_desc(gpio); > wakeup = pdata->wakeup; > + } else { > + gpiod = devm_gpiod_get_index(&pdev->dev, NULL, 0, GPIOD_IN); > + if (pdev->dev.of_node && > + of_get_property(pdev->dev.of_node, > + "wakeup", NULL)) > + wakeup = 1; > } > if (!gpiod) > return -EINVAL; > @@ -309,6 +316,8 @@ static int gpio_vbus_probe(struct platform_device *pdev) > gpiod = gpio_to_desc(gpio); > gpiod_direction_output(gpiod, 0); > } > + } else { > + gpiod = devm_gpiod_get_index(&pdev->dev, NULL, 1, 0); The pdata path has gpiod_direction_output(gpiod, 0), so should't the flag here be GPIOD_OUT_LOW? > } > if (!IS_ERR(gpiod)) > gpio_vbus->gpiod_pullup = gpiod; > @@ -382,12 +391,18 @@ static const struct dev_pm_ops gpio_vbus_dev_pm_ops = { > }; > #endif > > +static struct of_device_id gpio_vbus_dt_ids[] = { > + { .compatible = "generic,phy-gpio-vbus" }, See my reply to the binding docs, I think the "generic," might have to go. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Philipp Zabel <philipp.zabel@gmail.com> writes: > Hi Robert, > > On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Maybe duplicate the comment from the driver here how his is about > B peripheral only devices Yeah sure, for v2. >> +Required properties: >> +- compatible : should be "generic,phy-gpio-vbus" > > I'm not sure about the compatible value. I have not seen the "generic," > vendor prefix, and all the other generic gpio-something bindings don't > use any prefix: "gpio-gate-clock", "gpio-leds", "gpio-beeper", > "pps-gpio", etc. Okay, so we'll probably end up on "phy-gpio-vbus" then. >> +- #phy-cells : from the generic PHY bindings, must be 1. >> +- gpios : set of 2 gpio properties (see gpio.txt for gpio properties format) >> + first gpio is required, it's the VBus sensing input gpio >> + second gpio is optional, it's the D+ pullup controlling output >> + gpio >> + >> +Optional properties: >> +- wakeup : boolean, if true the VBus gpio will wakeup the platform > > The vbus_draw regulator should be part of this binding, I think. Indeed. It should be optional, right ? Schedules for v2. >> +Example: >> + usb2phy : gpio-vbus@13 { >> + compatible = "generic,phy-gpio-vbus"; >> + gpios = <&gpio 13 GPIO_ACTIVE_LOW>; >> + wakeup; > > This on the other hand might be too generic. > I'd like to see just wakeup used here, but the other bindings prefix > "linux," (or "gpio-key,"). If I write this, would you change to better suit you ? usb2phy : gpio-vbus@13 { compatible = "phy-gpio-vbus"; regulator-vbus: regulator@0 { regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; regulator-always-on; }; vbus-gpio = <&gpio 13 GPIO_ACTIVE_LOW>; wakeup; }; Cheers.
Hello. On 11/2/2014 9:11 PM, Robert Jarzmik wrote: > Add documentation for device-tree binding of the generic gpio-vbus phy > controller. > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: devicetree@vger.kernel.org > --- > .../devicetree/bindings/phy/gpio-vbus.txt | 23 ++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/gpio-vbus.txt > diff --git a/Documentation/devicetree/bindings/phy/gpio-vbus.txt b/Documentation/devicetree/bindings/phy/gpio-vbus.txt > new file mode 100644 > index 0000000..ffcfd35 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/gpio-vbus.txt > @@ -0,0 +1,23 @@ > +GPIO VBus phy It's either VBUS or Vbus. > +============= > + > +gpio-vbus is a phy controller supporting VBus detection and pullup activation on s/phy/PHY/. > +GPIOs. > + > +Required properties: > +- compatible : should be "generic,phy-gpio-vbus" "generic," not needed. > +- #phy-cells : from the generic PHY bindings, must be 1. However, you don't specify it in the example. It's also not clear why you need 1, not 0. > +- gpios : set of 2 gpio properties (see gpio.txt for gpio properties format) > + first gpio is required, it's the VBus sensing input gpio > + second gpio is optional, it's the D+ pullup controlling output > + gpio s/gpio/GPIO/. > + > +Optional properties: > +- wakeup : boolean, if true the VBus gpio will wakeup the platform > + > +Example: > + usb2phy : gpio-vbus@13 { Please use the generic node name ("usb-phy") in order to comply with the ePAPR standard. > + compatible = "generic,phy-gpio-vbus"; > + gpios = <&gpio 13 GPIO_ACTIVE_LOW>; > + wakeup; > + }; WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sun, Nov 02, 2014 at 07:11:49PM +0100, Robert Jarzmik wrote: > In order to prepare device-tree conversion, port gpio_vbus to use > gpio_desc. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> Can we just convert users of this to a phy-generic.c with a regulator ? This is basically what gpio-vbus is doing, it's basically a regulator. We can figure out how to maintain backwards compatibility with older DTS too, no problem. cheers
On Sun, Nov 02, 2014 at 07:11:50PM +0100, Robert Jarzmik wrote: > Add device-tree support for the generic gpio-vbus driver. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: devicetree@vger.kernel.org > --- I would rather see you converting to a fixed-regulator with phy-generic.
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Sun, Nov 02, 2014 at 07:11:49PM +0100, Robert Jarzmik wrote: >> In order to prepare device-tree conversion, port gpio_vbus to use >> gpio_desc. >> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > Can we just convert users of this to a phy-generic.c with a regulator ? Maybe, let's see what is missing. > This is basically what gpio-vbus is doing, it's basically a regulator. And a detector too. The basic thing is that it request an interrupt, and upon this interrupt it schedules through a workqueue a usb_gadget_vbus_connect() and the regulator stuff. I don't see the interrupt+ usb_gadget_vbus_connect() stuff that in the phy-generic. Am I missing something ? Cheers.
On Wed, Nov 05, 2014 at 08:46:58PM +0100, Robert Jarzmik wrote: > Felipe Balbi <balbi@ti.com> writes: > > > Hi, > > > > On Sun, Nov 02, 2014 at 07:11:49PM +0100, Robert Jarzmik wrote: > >> In order to prepare device-tree conversion, port gpio_vbus to use > >> gpio_desc. > >> > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > > > Can we just convert users of this to a phy-generic.c with a regulator ? > Maybe, let's see what is missing. > > > This is basically what gpio-vbus is doing, it's basically a regulator. > And a detector too. The basic thing is that it request an interrupt, and upon > this interrupt it schedules through a workqueue a usb_gadget_vbus_connect() and > the regulator stuff. > > I don't see the interrupt+ usb_gadget_vbus_connect() stuff that in the > phy-generic. Am I missing something ? Well, let's add that :-) Just make it optional. It's pointless to have 80% duplicated code just because of 20% missing in phy-generic :-) Then we avoid adding gpio-vbus specific DT properties too.
Felipe Balbi <balbi@ti.com> writes: > On Wed, Nov 05, 2014 at 08:46:58PM +0100, Robert Jarzmik wrote: > Well, let's add that :-) Just make it optional. It's pointless to have > 80% duplicated code just because of 20% missing in phy-generic :-) > > Then we avoid adding gpio-vbus specific DT properties too. OK, got it. It will take me a couple of days. Philipp, am I missing something apart the detection and connect stuff ? While I'm at making my board work with phy-generic, let's thing ahead. Felipe, that will mean at least this for phy-generic : - usb_phy_gen_create_phy() will be enhanced => struct usb_phy_generic_platform_data will get a : - int gpio_vbus field (or whatever name you wish) - int gpio_vbus_inverted (or maybe we could go directly for gpio desc) - int gpio_pullup field (I'm not sure here, maybe we should just drop that) - bool wakeup field (or another name) => device tree will get : - a vbus-gpio (or another name) - a pullup-gpio (or nothing if we drop) - there will be a request_irq() and a workqueue (mostly taken from gpio-vbus) => will call usb_gadget_vbus_connect() => will call usb_gadget_vbus_disconnect() I'm writing all this just to be sure I have the good picture before I start coding. Cheers.
Hi, On Wed, Nov 05, 2014 at 09:02:04PM +0100, Robert Jarzmik wrote: > Felipe Balbi <balbi@ti.com> writes: > > > On Wed, Nov 05, 2014 at 08:46:58PM +0100, Robert Jarzmik wrote: > > Well, let's add that :-) Just make it optional. It's pointless to have > > 80% duplicated code just because of 20% missing in phy-generic :-) > > > > Then we avoid adding gpio-vbus specific DT properties too. > OK, got it. > > It will take me a couple of days. Philipp, am I missing something apart the > detection and connect stuff ? While I'm at making my board work with > phy-generic, let's thing ahead. > > Felipe, that will mean at least this for phy-generic : > - usb_phy_gen_create_phy() will be enhanced > => struct usb_phy_generic_platform_data will get a : > - int gpio_vbus field (or whatever name you wish) > - int gpio_vbus_inverted (or maybe we could go directly for gpio desc) Actually, you might want to first convert phy-generic to gpio_desc and avoid the inverted field. > - int gpio_pullup field (I'm not sure here, maybe we should just drop that) > - bool wakeup field (or another name) sonds good to me. > => device tree will get : > - a vbus-gpio (or another name) > - a pullup-gpio (or nothing if we drop) fine by me, as long as their all optional and agreed with devicetree folks. I think we still have time for v3.19 if you manage to finish this before next week's end. > - there will be a request_irq() and a workqueue (mostly taken from gpio-vbus) > => will call usb_gadget_vbus_connect() > => will call usb_gadget_vbus_disconnect() the workqueue should be unnecessary if you use devm_request_threaded_irq() without a top half. > I'm writing all this just to be sure I have the good picture before I > start coding. sounds good to me :-) When it comes to DT, let's try to keep things as generic as possible so we can just move phy-generic.c into drivers/phy/ later on without much effort ;-) I guess everything that you need already has existent bindings. cheers
Felipe Balbi <balbi@ti.com> writes: > fine by me, as long as their all optional and agreed with devicetree > folks. I think we still have time for v3.19 if you manage to finish this > before next week's end. I will try, no promise I'll succeed in this window. At least I should fire out within the next days the gpiod patch and the DT documentation patch if I want to respect the schedule. Cheers.
On Sat, Nov 08, 2014 at 06:45:59PM +0100, Robert Jarzmik wrote: > Felipe Balbi <balbi@ti.com> writes: > > > fine by me, as long as their all optional and agreed with devicetree dumb me: s/their/they're > > folks. I think we still have time for v3.19 if you manage to finish this > > before next week's end. > I will try, no promise I'll succeed in this window. sure, no pressure either :-) > At least I should fire out within the next days the gpiod patch and > the DT documentation patch if I want to respect the schedule. good, we can at least cut down some dependencies for v3.20.
diff --git a/Documentation/devicetree/bindings/phy/gpio-vbus.txt b/Documentation/devicetree/bindings/phy/gpio-vbus.txt new file mode 100644 index 0000000..ffcfd35 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/gpio-vbus.txt @@ -0,0 +1,23 @@ +GPIO VBus phy +============= + +gpio-vbus is a phy controller supporting VBus detection and pullup activation on +GPIOs. + +Required properties: +- compatible : should be "generic,phy-gpio-vbus" +- #phy-cells : from the generic PHY bindings, must be 1. +- gpios : set of 2 gpio properties (see gpio.txt for gpio properties format) + first gpio is required, it's the VBus sensing input gpio + second gpio is optional, it's the D+ pullup controlling output + gpio + +Optional properties: +- wakeup : boolean, if true the VBus gpio will wakeup the platform + +Example: + usb2phy : gpio-vbus@13 { + compatible = "generic,phy-gpio-vbus"; + gpios = <&gpio 13 GPIO_ACTIVE_LOW>; + wakeup; + };
Add documentation for device-tree binding of the generic gpio-vbus phy controller. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> Cc: devicetree@vger.kernel.org --- .../devicetree/bindings/phy/gpio-vbus.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/gpio-vbus.txt