diff mbox

[v1,1/3] usb: phy: device-tree documentation for gpio-vbus

Message ID 1414951910-16075-1-git-send-email-robert.jarzmik@free.fr
State New, archived
Headers show

Commit Message

Robert Jarzmik Nov. 2, 2014, 6:11 p.m. UTC
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

Comments

Philipp Zabel Nov. 4, 2014, 8:01 a.m. UTC | #1
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
Philipp Zabel Nov. 4, 2014, 8:20 a.m. UTC | #2
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
Philipp Zabel Nov. 4, 2014, 8:28 a.m. UTC | #3
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
Robert Jarzmik Nov. 4, 2014, 9:27 p.m. UTC | #4
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.
Sergei Shtylyov Nov. 5, 2014, 12:59 p.m. UTC | #5
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
Felipe Balbi Nov. 5, 2014, 7:29 p.m. UTC | #6
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
Felipe Balbi Nov. 5, 2014, 7:31 p.m. UTC | #7
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.
Robert Jarzmik Nov. 5, 2014, 7:46 p.m. UTC | #8
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.
Felipe Balbi Nov. 5, 2014, 7:50 p.m. UTC | #9
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.
Robert Jarzmik Nov. 5, 2014, 8:02 p.m. UTC | #10
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.
Felipe Balbi Nov. 5, 2014, 8:09 p.m. UTC | #11
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
Robert Jarzmik Nov. 8, 2014, 5:45 p.m. UTC | #12
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.
Felipe Balbi Nov. 8, 2014, 6:06 p.m. UTC | #13
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 mbox

Patch

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;
+	};