Re: [PATCH 20/22] USB: gadget: udc: pxa27x_udc: no need to check return value of debugfs_create functions

[Robert Jarzmik]

Greg Kroah-Hartman  writes:

> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
>
> There is also no need to keep the file dentries around at all, so remove
> those variables from the device structure.
>
> Cc: Daniel Mack 
> Cc: Haojian Zhuang 
> Cc: Robert Jarzmik 
> Cc: Felipe Balbi 
> Signed-off-by: Greg Kroah-Hartman 
Acked-by: Robert Jarzmik 

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 07/14] USB: gadget: pxa27x: Re-use DEFINE_SHOW_ATTRIBUTE() macro

[Robert Jarzmik]

Andy Shevchenko <andriy.shevche...@linux.intel.com> writes:

> ...instead of open coding file operations followed by custom ->open()
> callbacks per each attribute.
>
> Cc: Daniel Mack <dan...@zonque.org>
> Cc: Haojian Zhuang <haojian.zhu...@gmail.com>
> Cc: Robert Jarzmik <robert.jarz...@free.fr>
> Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 06/14] USB: gadget: pxa25x: Re-use DEFINE_SHOW_ATTRIBUTE() macro

[Robert Jarzmik]

Andy Shevchenko <andriy.shevche...@linux.intel.com> writes:

> ...instead of open coding file operations followed by custom ->open()
> callbacks per each attribute.
>
> Cc: Daniel Mack <dan...@zonque.org>
> Cc: Haojian Zhuang <haojian.zhu...@gmail.com>
> Cc: Robert Jarzmik <robert.jarz...@free.fr>
> Signed-off-by: Andy Shevchenko <andriy.shevche...@linux.intel.com>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][V2] usb: gadget: pxa27x: Remove redundant assignment to is_short and dev

[Robert Jarzmik]

Colin King <colin.k...@canonical.com> writes:

> From: Colin Ian King <colin.k...@canonical.com>
>
> Variable is_short is set to zero but this value is never read as it is
> overwritten with a new value later on, hence it is a redundant
> assignment and can be removed.  Pointer dev is assigned a value that
> is not read and it is updated a few statements later, this too is
> redundant and can be removed. Cleans up clan warnings:
>
> drivers/usb/gadget/udc/pxa27x_udc.c:986:3: warning: Value stored
> to 'is_short' is never read
> drivers/usb/gadget/udc/pxa27x_udc.c:1141:2: warning: Value stored
> to 'dev' is never read

Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 22/24] USB: gadget: udc: Remove redundant license text

[Robert Jarzmik]

Greg Kroah-Hartman <gre...@linuxfoundation.org> writes:

> Now that the SPDX tag is in all USB files, that identifies the license
> in a specific and legally-defined manner.  So the extra GPL text wording
> can be removed as it is no longer needed at all.
>
> This is done on a quest to remove the 700+ different ways that files in
> the kernel describe the GPL license text.  And there's unneeded stuff
> like the address (sometimes incorrect) for the FSF which is never
> needed.
>
> No copyright headers or other non-license-description text was removed.
>
> Cc: Felipe Balbi <ba...@kernel.org>
> Cc: Nicolas Ferre <nicolas.fe...@microchip.com>
> Cc: Kevin Cernekee <cerne...@gmail.com>
> Cc: Florian Fainelli <f.faine...@gmail.com>
> Cc: Li Yang <leoyang...@nxp.com>
> Cc: Vladimir Zapolskiy <v...@mleia.com>
> Cc: Sylvain Lemieux <slemieux.t...@gmail.com>
> Cc: Daniel Mack <dan...@zonque.org>
> Cc: Haojian Zhuang <haojian.zhu...@gmail.com>
> Cc: Robert Jarzmik <robert.jarz...@free.fr>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: "Sören Brinkmann" <soren.brinkm...@xilinx.com>
> Cc: Raviteja Garimella <raviteja.garime...@broadcom.com>
> Cc: Romain Perier <romain.per...@collabora.com>
> Cc: Johan Hovold <jo...@kernel.org>
> Cc: Al Cooper <alcoop...@gmail.com>
> Cc: Srinath Mannam <srinath.man...@broadcom.com>
> Cc: Roger Quadros <rog...@ti.com>
> Cc: Krzysztof Opasiak <k.opas...@samsung.com>
> Cc: Stefan Agner <ste...@agner.ch>
> Cc: Alan Stern <st...@rowland.harvard.edu>
> Cc: "Felix Hädicke" <felixhaedi...@web.de>
> Cc: Peter Chen <peter.c...@nxp.com>
> Cc: Allen Pais <allen.l...@gmail.com>
> Cc: Yuyang Du <yuyang...@intel.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> ---

For :
>  drivers/usb/gadget/udc/pxa25x_udc.c |  5 -
>  drivers/usb/gadget/udc/pxa25x_udc.h |  6 --
>  drivers/usb/gadget/udc/pxa27x_udc.c |  5 -
>  drivers/usb/gadget/udc/pxa27x_udc.h |  5 -

Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] usb: gadget: pxa27x: Test for a valid argument pointer

[Robert Jarzmik]

Petr Cvek <petr.c...@tul.cz> writes:

> A call usb_put_phy(udc->transceiver) must be tested for a valid pointer.
> Use an already existing test for usb_unregister_notifier call.
>
> Reported-by: Robert Jarzmik <robert.jarz...@free.fr>
> Signed-off-by: Petr Cvek <petr.c...@tul.cz>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] usb: gadget: pxa27x: Remove duplicate function prototype

[Robert Jarzmik]

Petr Cvek <petr.c...@tul.cz> writes:

> Functions udc_enable() and udc_disable() have a duplicated prototype.
> Remove it.
>
> Signed-off-by: Petr Cvek <petr.c...@tul.cz>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] [TRIVIAL] usb: gadget: Remove duplicated function declaration in pxa27x_udc

[Robert Jarzmik]

Petr Cvek  writes:

> Remove duplicated function declaration for udc_enable() and udc_disable().

The patch title looks a bit tool long to me, and that also applied to your
second one.

Moreover the correct prefix for this driver (for the title) is :
 usb: gadget: pxa27x: MySuperSyntheticTitle

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: Add test if argument pointer has a valid value in pxa27x_udc

[Robert Jarzmik]

Petr Cvek <petr.c...@tul.cz> writes:

> Move call usb_put_phy(udc->transceiver) inside a valid pointer test.
>
> Reported-by: robert.jarz...@free.fr
I'd rather have my normal signature here :
Reported-by: Robert Jarzmik <robert.jarz...@free.fr>

> Signed-off-by: Petr Cvek <petr.c...@tul.cz>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] usb: gadget: scheduling while atomic in pxa27x_udc, IS_ERR_OR_NULL test, duplicated definition

[Robert Jarzmik]

Petr Cvek <petr.c...@tul.cz> writes:

Hi Petr,

> I found a few problems with the PXA27x UDC.
>
>   usb_function_activate() in drivers/usb/gadget/composite.c
>
> does spin_lock_irqsave() and then calls 
>
>   gadget->ops->pullup() in drivers/usb/gadget/udc/core.c
>
> which is set to pxa_udc_pullup(), which should be called not in interrupt
>
>   /**
>* pxa_udc_pullup - Offer manual D+ pullup control
>* @_gadget: usb gadget using the control
>* @is_active: 0 if disconnect, else connect D+ pullup resistor
>* Context: !in_interrupt()
>*
>* Returns 0 if OK, -EOPNOTSUPP if udc driver doesn't handle D+ pullup
>*/
>
> This finally causes fail at
>
>   udc_enable() in drivers/usb/gadget/udc/pxa27x_udc.c
>   
> at code
>
>   /*
>* Caller must be able to sleep in order to cope with startup transients
>*/
>   msleep(100);
>
> with a following error (with CONFIG_DEBUG_PREEMPT on):
>
>   BUG: scheduling while atomic: v4l_id/360/0x0002
>
> With the msleep changed to mdelay, the code (specified as !in_interrupt()) 
> seems to work fine
> (after torture reloads). Can the caller (udc core) be changed to be able to
> sleep?

This question is for Felipe. From the several years back when I wrote this code
I think it was granted that the pullup() callback was not in interrupt context,
but Felipe knows better.

> Second bug was discovered by Robert Jarzmik during discussion in
Please go ahead and submit a patch.

> And as we talking about it, is this return correct?
I think so.

>   if (of_have_populated_dt()) {
>   udc->transceiver =
>   devm_usb_get_phy_by_phandle(udc->dev, "phys", 0);
>
>   if (IS_ERR(udc->transceiver))
>   return PTR_ERR(udc->transceiver);
>   } else {
>   udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
>   }
>
> One branch returns on error and second one is fine, udc->transceiver then can 
> hold an error,
> but this is fine for the rest of driver (tested). Question is does it have to 
> return from a first
> branch (e.g. my device does not have phy)?
In the devicetree context (first branch), even if you don't have a phy, you'll
instanciate a "nop" phy, ie. "usb-nop-xceiv". From a hardware perspective, you
have a phy, it's just that you don't have to do anything from a software
perspective to activate your phy.

In the platform_data context (second branch), an error is the common path, as
there is no phy in many old platforms, and pxa27x are old ... hence no check of
error.

> And finally it seems definitions from drivers/usb/gadget/udc/pxa27x_udc.c are 
> duplicated:
>
>   static void udc_enable(struct pxa_udc *udc);
>   static void udc_disable(struct pxa_udc *udc);
>
> I will send patch series as soon as we agree on solutions.
Excellent.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Robert Jarzmik]

Petr Cvek <petr.c...@tul.cz> writes:

> Dne 12.2.2017 v 13:02 Robert Jarzmik napsal(a):
> That's weird I even removed pxa_set_udc_info() from magician machine init and 
> it still fails.
> Host cable and/or actual camera is not required. It fails without them.
>
> So you binded pxa27x-udc as UDC controller (= activated it) and then rmmoded
> it and nobody complained?
No, that usecase actually fails. I think you could submit a proper patch with
the diff in [1], with which the pxa27x-udc unloading will work.

But it's not _your_ testcase, as per your provided callstack :
[ 2152.826529] [] (udc_bind_to_driver [udc_core]) from [] 
(usb_add_gadget_udc_release+0x138/0x21c [udc_core])
[ 2152.826731] [] (usb_add_gadget_udc_release [udc_core]) from 
[] (pxa_udc_probe+0x290/0x2fc [pxa27x_udc])
[ 2152.833554] [] (pxa_udc_probe [pxa27x_udc]) from [] 
(platform_drv_probe+0x38/0x84)
[ 2152.833602] [] (platform_drv_probe) from [] 
(driver_probe_device+0x1e0/0x3f4)

Your problem seems in the pxa_udc_probe(), which I would presume you're doing
for a second time, ie. after modprobe pxa27x_udc + bind UDC controller + rmmod
pxa27x_udc + modprobe pxa27x_udc.

I suspect that in this case, the problem is that the rmmod works while udc is
still bound to the composite. The lsmod seems to prove that the refcount is
still 0 while usb_f_uvc is is bound to pxa27x_udc.

[@Felipe and @Laurent]
I have no knowledge of usb_f_uvc, so would you tell me if binding usb_f_uvc to
pxa27_udc should have incremented the module refcount of pxa27x_udc or not ?

> Can you try v4.10? Or send me exact commit on what you tested it so I can test
> it too.
My top commit (not counting yours) is : 61c04572de40 ("Merge branch 'for-rc' of
git://git.kernel.org/pub/scm/linux/kernel/git/rzhang/linux")

Cheers.

-- 
Robert

[1]
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 9fb103348b5d..a239eb836652 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2512,7 +2512,8 @@ static int pxa_udc_remove(struct platform_device *_dev)
usb_del_gadget_udc(>gadget);
pxa_cleanup_debugfs(udc);
 
-   usb_put_phy(udc->transceiver);
+   if (!IS_ERR(udc->transceiver))
+   usb_put_phy(udc->transceiver);
 
udc->transceiver = NULL;
the_controller = NULL;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Robert Jarzmik]

Petr Cvek  writes:

> Hi
>
> any news?

Yeah, I don't have the error on my mainstone, which kind of hinders my debug
capability.

I have tried your script, and this is what I get :
  - dmesg | tail
random: crng init done
gadgetfs: USB Gadget filesystem, version 24 Aug 2004
configfs-gadget gadget: uvc_function_bind
[DEBUG] opts->streaming_maxpacket=256
name: ep1in-int, mp=16
name: ep1in-int, mp=16
name: ep2in-int, mp=16
name: ep3out-int, mp=16
name: ep4in-iso, mp=256
  - lsmod
Module  Size  Used byNot tainted
usb_f_uvc  40752  6 
libcomposite   38099 14 usb_f_uvc
pxa27x_udc 21131  0 
gadgetfs   16173  0 
videobuf2_vmalloc   4659  1 usb_f_uvc
videobuf2_memops1421  1 videobuf2_vmalloc

This was tried on a mainstone board, upon mainline kernel v4.8-rc3 with your
patch. but without an actual camera nor the USB cable attached to a host USB.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] usb: gadget: Kernel oops with UVC USB gadget and configfs

[Robert Jarzmik]

Petr Cvek  writes:

> Setting the UVC gadget with configfs and then reloading UDC controler driver
> (pxa27x_udc) causes kernel to fail.
>
> UDC subsystem was patched only in decreasing maxpacket size for UVC, addition
> of more predefined endpoints for pxa27x_udc and addition of some debugging 
> pr_info.
>
> Practically same behaviour was observed on 4.7.0 too.
Hi Petr,

If you could provide me your "debug patch" for the new endpoints, packet size
and pr_infos, I could try to reproduce on my platform and have a look.

I must say that I'm not familiar with uvc, so I can only suppose you're trying
to stream the magician camera to USB to "act" like a Webcam, is it what you're
trying to achieve ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: udc: constify usb_ep_ops structures

[Robert Jarzmik]

Bhumika Goyal <bhumi...@gmail.com> writes:

> Declare usb_ep_ops structures as const as they are only stored in the
> ops field of an usb_ep structure. This field is of type const, so
> usb_ep_ops structures having this property can be made const too.
> Done using Coccinelle( A smaller version of the script)

For pxa27x_udc.c :
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: Convert pr_warning to pr_warn

[Robert Jarzmik]

Joe Perches <j...@perches.com> writes:

> Use the more common logging mechanism.
>
> Miscellanea:
>
> o Realign multiline statements
> o Coalesce format
>
> Signed-off-by: Joe Perches <j...@perches.com>
For pxa25x_udc.h:
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] usb: phy: generic: get rid of VBUS handling

[Robert Jarzmik]

Felipe Balbi <felipe.ba...@linux.intel.com> writes:

> Hi Robert,
>
> Robert Jarzmik <robert.jarz...@free.fr> writes:
>> Felipe Balbi <felipe.ba...@linux.intel.com> writes:
>>
>>> Hi,
>>>
>>> Robert Jarzmik <robert.jarz...@free.fr> writes:
>>>> I don't know if there are other users than pxa, I'll make the assumption 
>>>> there
>>>> aren't.
>>>
>>> yeah, we can keep patches in linux-next for as long as possible. If
>>> nobody complains, we're good.
>>
>> Hi Felipe,
>>
>> Just a small update. I made the change, but I'm not submitting it yet
>
> thanks for the update :-)
Euh I think you have even more backlog in your mails than I do :)

The latest update is here :
https://patchwork.kernel.org/patch/9221755

This one is fully functional AFAIK.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] usb: phy: generic: remove the vbus dependency

[Robert Jarzmik]

As the last known user, ie. pxa27x_udc relying on calls to
usb_gadget_xxx() was amended to use the phy notifier, remove a bit the
USB stack adherence.

Actually the driver still uses the gadget API for structures definition,
but the implementation of USB gadget specific function usb_gadget_*() is
not necessary anymore.

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
 drivers/usb/phy/phy-generic.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 41b8aff9b925..c14e767a3fbf 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -118,8 +118,6 @@ static irqreturn_t nop_gpio_vbus_thread(int irq, void *data)
status = USB_EVENT_VBUS;
otg->state = OTG_STATE_B_PERIPHERAL;
nop->phy.last_event = status;
-   if (otg->gadget)
-   usb_gadget_vbus_connect(otg->gadget);
 
/* drawing a "unit load" is *always* OK, except for OTG */
nop_set_vbus_draw(nop, 100);
@@ -129,8 +127,6 @@ static irqreturn_t nop_gpio_vbus_thread(int irq, void *data)
} else {
nop_set_vbus_draw(nop, 0);
 
-   if (otg->gadget)
-   usb_gadget_vbus_disconnect(otg->gadget);
status = USB_EVENT_NONE;
otg->state = OTG_STATE_B_IDLE;
nop->phy.last_event = status;
@@ -187,7 +183,8 @@ static int nop_set_peripheral(struct usb_otg *otg, struct 
usb_gadget *gadget)
 
otg->gadget = gadget;
if (otg->state == OTG_STATE_B_PERIPHERAL)
-   usb_gadget_vbus_connect(gadget);
+   atomic_notifier_call_chain(>usb_phy->notifier,
+  USB_EVENT_VBUS, otg->gadget);
else
otg->state = OTG_STATE_B_IDLE;
return 0;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] usb: phy: generic: cope with initial state

[Robert Jarzmik]

In the gpio based case, the status of the phy is known at start by
reading the VBus gpio.

Actually, this is a fix, as this initial state, when not set up,
prevents a gadget to answer to the enumeration phase, as there is no
notification in this case (the VBus is already high when kernel boots)
so no interrupt is triggered, and the flow is :
 - gadget initializes
 - gadget gets its phy-generic with a xxx_get_phy_xxx() call type
 - gadget does a "set_peripheral()" call type
   => here if the otg->state is correctly filled, the proper vbus
   handling will be called, and the gadget will be aware it should
   answer enumeration and go forth

Without this fix, the USB cable must be removed and replugged for any
gadget relying on phy-generic and its gpio vbus handling to work.

The problem was seen on a pxa27x architecture based board on a
devicetree build.

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
 drivers/usb/phy/phy-generic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 980c9dee09eb..41b8aff9b925 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -322,6 +322,8 @@ static int usb_phy_generic_probe(struct platform_device 
*pdev)
gpiod_to_irq(nop->gpiod_vbus), err);
return err;
}
+   nop->phy.otg->state = gpiod_get_value(nop->gpiod_vbus) ?
+   OTG_STATE_B_PERIPHERAL : OTG_STATE_B_IDLE;
}
 
nop->phy.init   = usb_gen_phy_init;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usb: gadget: pxa27x: add phy notifier event handler

[Robert Jarzmik]

In the legacy behavior, and USB phy, upon detection a VBus signal, was
calling usb_gadget_vbus_(dis)connect().

This model doesn't work if the phy is generic and doesn't have an
adherence to the gadget API.

Instead of relying on the phy to call the gadget API, hook up the phy
notifier to report the VBus event, and upon it call the usb gadget API
ourselves.

This brings a new ordering problem, as before even if the usb_get_phy()
was failing because the UDC was probed before the phy, the phy would
call the gadget anyway, making the VBus connection event forwarded to
the gadget. Now we rely on the notifier, we have to ensure the
xxx_get_phy() does indeed work.

In order to cope with this, it is assumed that :
 - for legacy platform_data machine, as the ordering cannot be ensured,
   the phy must call usb_gadget_vbus_(dis)connect, such as
   phy-gpio-vbus-usb.c
 - for new devicetree platforms, we'll rely on the probe deferral, and
   the phy can be gadget API agnostic.

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 51 +++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 001a3b74a993..79643e25a5bb 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "pxa27x_udc.h"
 
@@ -1655,6 +1656,37 @@ static int pxa_udc_vbus_draw(struct usb_gadget *_gadget, 
unsigned mA)
return -EOPNOTSUPP;
 }
 
+/**
+ * pxa_udc_phy_event - Called by phy upon VBus event
+ * @nb: notifier block
+ * @action: phy action, is vbus connect or disconnect
+ * @data: the usb_gadget structure in pxa_udc
+ *
+ * Called by the USB Phy when a cable connect or disconnect is sensed.
+ *
+ * Returns 0
+ */
+static int pxa_udc_phy_event(struct notifier_block *nb, unsigned long action,
+void *data)
+{
+   struct usb_gadget *gadget = data;
+
+   switch (action) {
+   case USB_EVENT_VBUS:
+   usb_gadget_vbus_connect(gadget);
+   return NOTIFY_OK;
+   case USB_EVENT_NONE:
+   usb_gadget_vbus_disconnect(gadget);
+   return NOTIFY_OK;
+   default:
+   return NOTIFY_DONE;
+   }
+}
+
+static struct notifier_block pxa27x_udc_phy = {
+   .notifier_call = pxa_udc_phy_event,
+};
+
 static int pxa27x_udc_start(struct usb_gadget *g,
struct usb_gadget_driver *driver);
 static int pxa27x_udc_stop(struct usb_gadget *g);
@@ -2435,7 +2467,14 @@ static int pxa_udc_probe(struct platform_device *pdev)
return udc->irq;
 
udc->dev = >dev;
-   udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
+   if (of_have_populated_dt()) {
+   udc->transceiver =
+   devm_usb_get_phy_by_phandle(udc->dev, "phys", 0);
+   if (IS_ERR(udc->transceiver))
+   return PTR_ERR(udc->transceiver);
+   } else {
+   udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
+   }
 
if (IS_ERR(udc->gpiod)) {
dev_err(>dev, "Couldn't find or request D+ gpio : %ld\n",
@@ -2468,14 +2507,20 @@ static int pxa_udc_probe(struct platform_device *pdev)
goto err;
}
 
+   if (!IS_ERR_OR_NULL(udc->transceiver))
+   usb_register_notifier(udc->transceiver, _udc_phy);
retval = usb_add_gadget_udc(>dev, >gadget);
if (retval)
-   goto err;
+   goto err_add_gadget;
 
pxa_init_debugfs(udc);
if (should_enable_udc(udc))
udc_enable(udc);
return 0;
+
+err_add_gadget:
+   if (!IS_ERR_OR_NULL(udc->transceiver))
+   usb_unregister_notifier(udc->transceiver, _udc_phy);
 err:
clk_unprepare(udc->clk);
return retval;
@@ -2492,6 +2537,8 @@ static int pxa_udc_remove(struct platform_device *_dev)
usb_del_gadget_udc(>gadget);
pxa_cleanup_debugfs(udc);
 
+   if (!IS_ERR_OR_NULL(udc->transceiver))
+   usb_unregister_notifier(udc->transceiver, _udc_phy);
usb_put_phy(udc->transceiver);
 
udc->transceiver = NULL;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] usb: phy: generic: get rid of VBUS handling

[Robert Jarzmik]

Felipe Balbi  writes:

>>  - amend the drivers (for my part I have pxa27x_udc.c and probably 
>> pxa25x_udc.c)
>>  to subscribe to the notifier, and in the action function call either
>>  usb_gadget_vbus_connect() or usb_gadget_vbus_disconnect().
>>
>> Would that kind of approach solve the layering issue, what do you think ?
>
> I like second option of having PHY simply sending the notification and
> relying on UDC to actually call usb_gadget_vbus_*().
>
> I could write the patches, but I wouldn't have any means to test them
> :-s
>
> Let me know if you prefer to write them yourself or want me to give it a
> shot.
Hi Felipe,

I'll write the 3 patches and test them, as it's easier when you have the proper
hardware :
 - one for pxa27x_udc.c
 - one for pxa25x_udc.c
 - one for phy-generic.c

I don't know if there are other users than pxa, I'll make the assumption there
aren't.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] usb: phy: generic: get rid of VBUS handling

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi Robert,
>
> your commit 7acc9973e3c4 ("usb: phy: generic: add vbus support") added
> GPIO-based VBUS handling for phy-generic.c, but that ends up calling
> usb_gadget_vbus_connect() which forces NOP to depend on the gadget API.
>
> I was grepping around and couldn't find any users for that VBUS gpio
> _and_ we have phy-gpio-vbus.c which does the same thing.
Indeed, I remember creating this commit out of phy-gpio-vbus-usb.c
This is used in pxa device-tree platforms, that's why your grep didn't find them
as device-tree descriptions are out of tree.

> I'm wondering if we should drop the usb_gadget_vbus_connect() support so
> other phy-generic users (like ehci-omap) can rely on it without
> depending on the gadget API.
I understand, from a layering perspective that makes perfect sense.

> Thoughts?
Yeah, kind of. phy-generic has an atomic notifier for connection events. I think
we could leverage that and :
 - remove the usb_gadget_vbus_*() calls
 - amend the drivers (for my part I have pxa27x_udc.c and probably pxa25x_udc.c)
 to subscribe to the notifier, and in the action function call either
 usb_gadget_vbus_connect() or usb_gadget_vbus_disconnect().

Would that kind of approach solve the layering issue, what do you think ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] usb: pxa27x_udc: remove unused function argument

[Robert Jarzmik]

Arnd Bergmann <a...@arndb.de> writes:

> We get a warning for this when building with W=1 because the
> argument gets assigned to something else but never read:
>
> drivers/usb/gadget/udc/pxa27x_udc.c: In function 'stop_activity':
> drivers/usb/gadget/udc/pxa27x_udc.c:1828:74: error: parameter 'driver' set 
> but not used [-Werror=unused-but-set-parameter]
>
> This remove the argument entirely.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
Hi Arnd,

I wonder if it'd worth adding this ... even if the word "Completes" would be 
more
correct than "Fixes" :
Fixes: 70189a63d408 ("usb: gadget: pxa27x_udc: convert to udc_start/udc_stop")

Apart form that:
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio

[Robert Jarzmik]

Arnd Bergmann  writes:

> Coming back to the specific pxa25x_udc case: using __raw_* accessors
> in the driver would possibly end up breaking the PXA25x machines in
> the (very unlikely) case that someone wants to make it work with
> big-endian kernels, assuming it does not have the same hardware
> byteswap logic as ixp4xx.

As far as I know, pxa25x machine implies the kernel is little endian. From an
hardware perspective, pxa25x doesn't support big endian (old ARM platform).

So I don't think considering BE for pxa25x is ... necessary.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: host: ohci-pxa27x: propagate the irq error code

[Robert Jarzmik]

In several drivers in the pxa architecture, it was found that the
platform_get_irq() was not propagated. This breaks the the device-tree
probe deferral path, if -EPROBE_DEFER is returned. Unfortunately, the
error return in this case is transformed into -ENXIO, breaking the
deferral mechanism.

Even if in this specific case the driver was not broken, because the
interrupt controller is always probed before drivers, propagate the
proper return code.

Signed-off-by: Robert Jarzmik <robert.jarz...@free.fr>
---
 drivers/usb/host/ohci-pxa27x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
index e8c006e7a960..a667cf2d5788 100644
--- a/drivers/usb/host/ohci-pxa27x.c
+++ b/drivers/usb/host/ohci-pxa27x.c
@@ -435,7 +435,7 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, 
struct platform_device
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
pr_err("no resource of IORESOURCE_IRQ");
-   return -ENXIO;
+   return irq;
}
 
usb_clk = devm_clk_get(>dev, NULL);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] USB changes for rare warnings

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi Arnd,
>
>>  arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h | 198 -
>>  arch/arm/mach-pxa/include/mach/pxa25x-udc.h | 163 
>
> for arch/arm I need Acked-by from relevant folks.
You already have mine for arch/arm/mach-pxa/include/mach/pxa25x-udc.h I think,
or did my mailer betray me again ...

I cannot speak for ixp4xx though.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/7] usb: gadget: pxa25x_udc cleanup

[Robert Jarzmik]

Arnd Bergmann <a...@arndb.de> writes:

> This removes the dependency on the mach/hardware.h header file
> from the pxa25x_udc driver after the register definitions were
> already unified in the previous patch.
>
> Following the model of pxa27x_udc (and basically all other drivers
> in the kernel), we define the register numbers as offsets from
> the register base address and use accessor functions to read/write
> them.
>
> For the moment, this still leaves the direct pointer dereference
> in place, instead of using readl/writel, so this patch should
> not be changing the behavior of the driver, other than using
> ioremap() on the platform resource to replace the hardcoded
> virtual address pointers.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

It's too bad coccinelle cannot make that work more automatic, isn't it ? :)

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch

[Robert Jarzmik]

Arnd Bergmann  writes:

> ixp4xx and pxa25x both use this driver and provide a slightly
> different set of register definitions for it. Aside from that,
> the definition in the ixp4xx-regs.h header conflicts with the
> on in the pxa27x device driver when compile-testing that:
>
> In file included from ../drivers/usb/gadget/udc/pxa27x_udc.c:37:0:
> ../drivers/usb/gadget/udc/pxa27x_udc.h:26:0: warning: "UDCCR" redefined
>  #define UDCCR  0x  /* UDC Control Register */
>  ^
> In file included from ../arch/arm/mach-ixp4xx/include/mach/hardware.h:27:0,
>  from ../arch/arm/mach-ixp4xx/include/mach/io.h:18,
>  from ../arch/arm/include/asm/io.h:194,
>  from ../include/linux/io.h:25,
>  from ../include/linux/irq.h:24,
>  from ../drivers/usb/gadget/udc/pxa27x_udc.c:23:
> ../arch/arm/mach-ixp4xx/include/mach/ixp4xx-regs.h:415:0: note: this is the 
> location of the previous definition
>  #define UDCCR  IXP4XX_USB_REG(IXP4XX_USB_BASE_VIRT+0x)
>
> This addresses both issues by moving all the definitions into the
> pxa25x_udc driver itself. It turns out the only difference between
> them was 'UDCCS_IO_ROF', and that could well be a mistake when it
> was incorrectly copied from pxa25x to ixp4xx.
Hi Arnd,

Is there a reason to have chosen to move into pxa25_udc.c instead of
drivers/usb/gadget/udc/pxa25x_udc.h ? pxa27x_udc has a .h in the same directory
with register definitions, hence the question.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/7] usb: gadget: pxa25x_udc: use readl/writel for mmio

[Robert Jarzmik]

Arnd Bergmann <a...@arndb.de> writes:

> This converts the pxa25x udc driver to use readl/writel as normal
> driver should do, rather than dereferencing __iomem pointers
> themselves.
>
> Based on the earlier preparation work, we can now also pass
> the register start in the device pointer so we no longer need
> the global variable.
>
> The unclear part here is for IXP4xx, which supports both big-endian
> and little-endian configurations. So far, the driver has done
> no byteswap in either case. I suspect that is wrong and it would
> actually need to swap in one or the other case, but I don't know
> which. It's also possible that there is some magic setting in
> the chip that makes the endianess of the MMIO register match the
> CPU, and in that case, the code actually does the right thing
> for all configurations, both before and after this patch.
>
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
For pxa25x_udc:
Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] usb: gadget: pxa25x_udc: move register definitions from arch

[Robert Jarzmik]

Arnd Bergmann <a...@arndb.de> writes:

> On Friday 29 January 2016 10:32:07 Robert Jarzmik wrote:
>> > This addresses both issues by moving all the definitions into the
>> > pxa25x_udc driver itself. It turns out the only difference between
>> > them was 'UDCCS_IO_ROF', and that could well be a mistake when it
>> > was incorrectly copied from pxa25x to ixp4xx.
>> Hi Arnd,
>> 
>> Is there a reason to have chosen to move into pxa25_udc.c instead of
>> drivers/usb/gadget/udc/pxa25x_udc.h ? pxa27x_udc has a .h in the same 
>> directory
>> with register definitions, hence the question.
>
> Yes: The register definitions are only used in a single .c file and
> are not an interface between files, so it's better to have them in
> the file that uses them.
>
> I could have continued the cleanup and moved the two headers into the
> respective drivers as well, but I had to stop somewhere ;-)
Fair enough.

Acked-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: pxa27x: fix suspend callback

[Robert Jarzmik]

Felipe Balbi  writes:

> pxa27x disconnects pullups on suspend but doesn't
> notify the gadget driver about it, so gadget driver
> can't disable the endpoints it was using.
>
> This causes problems on resume because gadget core
> will think endpoints are still enabled and just
> ignore the following usb_ep_enable().
>
> Fix this problem by calling
> gadget_driver->disconnect().
Thanks for doing this for me.
> @@ -2535,6 +2535,7 @@ static int pxa_udc_suspend(struct platform_device 
> *_dev, pm_message_t state)
>   udc_disable(udc);
>   udc->pullup_resume = udc->pullup_on;
>   dplus_pullup(udc, 0);
> + udc->driver->disconnect(>gadget);
If no driver is bound, this will segfault, right ?
Shouldn't an "if (udc->driver)" protect this line ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: pxa27x: fix suspend callback

[Robert Jarzmik]

Felipe Balbi <ba...@ti.com> writes:

> pxa27x disconnects pullups on suspend but doesn't
> notify the gadget driver about it, so gadget driver
> can't disable the endpoints it was using.
>
> This causes problems on resume because gadget core
> will think endpoints are still enabled and just
> ignore the following usb_ep_enable().
>
> Fix this problem by calling
> gadget_driver->disconnect().
>
> Cc: <sta...@vger.kernel.org> # v3.10+
> Signed-off-by: Felipe Balbi <ba...@ti.com>
Tested-by: Robert Jarzmik <robert.jarz...@free.fr>

Cheers.

--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Robert Jarzmik <robert.jarz...@free.fr> writes:

> Felipe Balbi <ba...@ti.com> writes:
>
>> Hi,
>>
>> without any sort of logs it a bit difficult :-) Care to send some output
>> of the failure ? Are there any oopses or what exactly happens ?
> Ah well, the UDC is the only way to "speak" to the board (no UART), so I don't
> have any written feedback available. All I have are the logs displayed on the
> phone's screen in the framebuffer screen.

I can have logs if I kinda ... revert the patch, applying the diff in [1].
This enables the suspend/resume to work again, and I can gather the logs when
[1] is applied :

[   63.649528] Suspending console(s) (use no_console_suspend to debug)
[   63.688100] PM: suspend of devices complete after 37.275 msecs
[   63.690351] PM: late suspend of devices complete after 2.191 msecs
[   63.692595] PM: noirq suspend of devices complete after 2.196 msecs
[   63.694387] PM: noirq resume of devices complete after 1.482 msecs
[   63.696711] PM: early resume of devices complete after 1.533 msecs
[   63.802930] PM: resume of devices complete after 106.122 msecs
[   63.804976] Restarting tasks ... 
[   63.821371] pxa27x-udc pxa27x-udc: USB reset
[   63.822988] done.
[   63.933666] pxa27x-udc pxa27x-udc: USB reset
[   64.064026] g_ether gadget: full-speed config #1: CDC Subset/SAFE
[   64.069692] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk 
already enabled, doing nothing
[   64.075389] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk 
already enabled, doing nothing
[   64.082081] g_ether gadget: full-speed config #1: CDC Subset/SAFE
[   64.087876] pxa27x-udc pxa27x-udc: ep4:pxa_ep_enable:usb_ep ep2in-bulk 
already enabled, doing nothing
[   64.093358] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk 
already enabled, doing nothing
[   64.099092] pxa27x-udc pxa27x-udc: ep4:pxa_ep_enable:usb_ep ep2in-bulk 
already enabled, doing nothing
[   64.104835] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk 
already enabled, doing nothing

I don't know if that can help debug further ...

Cheers.

--
Robert

[1] Stupid diff to renable the suspend/resume to work on 4.4-rc1
rj@belgarion:~/mio_linux/kernel$ git diff
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a10b926..2b8dcf546d03 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -267,8 +267,8 @@ static inline int usb_ep_enable(struct usb_ep *ep)
 {
int ret;
 
-   if (ep->enabled)
-   return 0;
+   /* if (ep->enabled) */
+   /*  return 0; */
 
ret = ep->ops->enable(ep, ep->desc);
if (ret)
@@ -295,8 +295,8 @@ static inline int usb_ep_disable(struct usb_ep *ep)
 {
int ret;
 
-   if (!ep->enabled)
-   return 0;
+   /* if (!ep->enabled) */
+   /*  return 0; */
 
ret = ep->ops->disable(ep);
if (ret)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi,
>
> without any sort of logs it a bit difficult :-) Care to send some output
> of the failure ? Are there any oopses or what exactly happens ?
Ah well, the UDC is the only way to "speak" to the board (no UART), so I don't
have any written feedback available. All I have are the logs displayed on the
phone's screen in the framebuffer screen.

I know there is not panic/crash, as the screen show no stack/panic.
I also know that disconnecting and reconnecting the USB cable triggers the logs
of the phy vbus detection.
>From what I see the kernel is perfectly resumed, ie. key inputs trigger changes
on the framebuffer.

I have the host side logs :
1035787.154247] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
[1035787.154254] usb 1-1: New USB device strings: Mfr=1, Product=2, 
SerialNumber=0
[1035787.154257] usb 1-1: Product: Ethernet Gadget
[1035787.154259] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
[1035787.170423] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at 
usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
[1035822.149430] usb 1-1: USB disconnect, device number 69
[1035822.149564] cdc_subset 1-1:1.0 usb0: unregister 'cdc_subset'
usb-:00:14.0-1, Linux Device

--- here the board is gone to "Suspend to RAM" ---

[1035829.041922] usb 1-1: new full-speed USB device number 70 using xhci_hcd
[1035829.172194] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
[1035829.172197] usb 1-1: New USB device strings: Mfr=1, Product=2, 
SerialNumber=0
[1035829.172199] usb 1-1: Product: Ethernet Gadget
[1035829.172200] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
[1035829.187640] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at 
usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82

Here the board is back from suspend to RAM. It "looks" as if either it lost its
IP setup, or something else ... the final effect "felt" is that "ping mioa701"
doesn't work anymore.

I can try that on another board (ie. with an UART, ie. a mainstone), but I don't
have a working setup for suspend/resume, so it will probably take time.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi,
>
> this could be a bug in either g_ether or pxa27x... Seems like something
> is enabling endpoints which were already enabled. Not sure if this is
> pxa27x not _really_ disabling endpoints or g_ether being stupid.
You're probably right.
>From what I remember, g_ether is enabling already enabled endpoints upon 
>resume,
but that memory is pretty thin and goes back to 2008 when I created the driver,
so I'm not sure anymore.

> yeah, so something is not disabling endpoints when they should :-) So
> this could be a bug with your suspend/resume callbacks. If you're going
> to disconnect from the bus, you need to tell the gadget driver about it,
> which means after disabling pullups, you should call
> gadget_driver->disconnect().
Okay.

> Can you see if this *stupid* and *untested* diff helps :
...zip...
Yes it does. I don't understand why, but it does fix it, and the logs "already
enabled" are still gone. I wasn't aware that a disconnect was required in the
suspend, nor did I see it in another udc driver (in 2008 of course).


> I'm not 100% sure this is enough, as I'm not at all familiar with
> pxa27x, but that driver looks hugely unmaintained.
That would be my fault, as I'm maintaining it. That's the reason I'm seeing the
regression. If you have on mind improvements required just say so, I'll see what
I can do.

> Which device are you using for development/test ? Is it this Mio A701 ?
Yes it is.

> I can't find any sources of it :-s
What about arch/arm/mach-pxa/mioa701.c ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi,
>
>> Euh it's expected to work _after resume_.
>
> sure is, but you can't expect it to have a proper IP. From host's
> perspective this is a brand new usb0 network interface. Unless you have
> dhcp client running on the phone and a server on your PC, you can't
> assume it'll have IP ;-) (or statically configured, yada yada yada ;-)

On the board, it's very static, ie. in an init (think /etc/init.d/XX) file:
/sbin/ifconfig usb0 192.168.0.202 up

On the host, it's static also, but a bit more robust, as upon usb0 network
interface creation the same IP is always assigned.

Maybe the board's too "simple" assignement scheme has always been faulty
(ie. assigning only once an IP), and the recent commit revealed a long time
flaw, who knows ... if the interface is removed then added again, I might be
screwed (as the ip configuration is lost) ...

> I got that :-) My surprise is how you managed to get IP
> before. Statically configured ?
Yeah, once as I said on /init execution. That might be what is buggy, and which
I only see now ...

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Felipe Balbi  writes:

> Hi,
> you can increase fbcon verbosity, right ? I guess that was
> CONFIG_MESSAGE_LOGLEVEL_DEFAULT.
Sure.

> Set it to 9 and you should get everything. Another option is to change
> dev_* to dev_crit() and KERN_* to KERN_CRIT.
>
>> I know there is not panic/crash, as the screen show no stack/panic.  I
>> also know that disconnecting and reconnecting the USB cable triggers
>> the logs of the phy vbus detection.
>
> does it reenumerate fine when you unplug and replug ?
Yeah, I think the log below on host side show this.

>> From what I see the kernel is perfectly resumed, ie. key inputs
>> trigger changes on the framebuffer.
>>
>> I have the host side logs :
>> 1035787.154247] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
>> [1035787.154254] usb 1-1: New USB device strings: Mfr=1, Product=2, 
>> SerialNumber=0
>> [1035787.154257] usb 1-1: Product: Ethernet Gadget
>> [1035787.154259] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
>> [1035787.170423] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at 
>> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
>> [1035822.149430] usb 1-1: USB disconnect, device number 69
>> [1035822.149564] cdc_subset 1-1:1.0 usb0: unregister 'cdc_subset'
>> usb-:00:14.0-1, Linux Device
>
> is pxa27x always full-speed ? Seems like it, but I see that max_speed is
> never set there. That's a bug, but a very minor one.
Yes, always full-speed, right. As for max_speed, I will look into it.

>> --- here the board is gone to "Suspend to RAM" ---
>>
>> [1035829.041922] usb 1-1: new full-speed USB device number 70 using xhci_hcd
>> [1035829.172194] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
>> [1035829.172197] usb 1-1: New USB device strings: Mfr=1, Product=2, 
>> SerialNumber=0
>> [1035829.172199] usb 1-1: Product: Ethernet Gadget
>> [1035829.172200] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
>> [1035829.187640] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at 
>> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
>
> so it actually connects again just fine, this means suspend/resume works
> as before.
>From host side, yes. From device side, it's a bit different, as you can see in
the other mail I sent : all the log containing "already enabled, doing nothing"
are in v4.3 (ie. in working case), and absent in v4.4-rc1 (ie. broken case).

>> Here the board is back from suspend to RAM. It "looks" as if either it
>> lost its IP setup, or something else ... the final effect "felt" is
>
> But you still have your usb0 interface, right ? Does that have an IP
> address when you look at ifconfig ?
Yes, just as before, on host side I see no difference. The interface reappears
and is IP configured automaticaly (my host PC setup takes care of that).

>> that "ping mioa701" doesn't work anymore.
>
> that's kind of expected considering pxa27x disabled the UDC and
> disconnects from the bus on suspend:
Euh it's expected to work _after resume_.
>
> static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
> {
>   struct pxa_udc *udc = platform_get_drvdata(_dev);
>   struct pxa_ep *ep;
>
>   ep = >pxa_ep[0];
>   udc->udccsr0 = udc_ep_readl(ep, UDCCSR);
>
>   udc_disable(udc);
>   udc->pullup_resume = udc->pullup_on;
>   dplus_pullup(udc, 0);
>
>   return 0;
> }
>
> I'm actually surprised that it ever worked before :-)
I don't follow you : upon resume, ie. after pxa_udc_resume(), it worked
before. The case that bothers me is after resume, not between suspend() and
resume().

>> I can try that on another board (ie. with an UART, ie. a mainstone),
>> but I don't have a working setup for suspend/resume, so it will
>> probably take time.
> yeah, let's try to avoid that if possible :-)
Agreed.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[4.4-rc1 regression] pxa27x_udc and suspend/resume

[Robert Jarzmik]

Hi Robert and Felipe,

I experience a regression in the 4.4-rc1 kernel which appeared between v4.3 and
v4.4-rc1.
The test :
  - boot the mioa701 kernel (ie. pxa27x_udc driver)
  - echo mem > /sys/power/state
  - resume from "Suspend to RAM" with a key press
  - try to use the udc (in my case, g_ether => ping mioa701)

I bissected the issue to this :
commit b0bac2581c19 (refs/bisect/bad)
Author: Robert Baldyga 
Date:   Wed Sep 16 12:10:42 2015 +0200

usb: gadget: introduce 'enabled' flag in struct usb_ep

Could either of you tell me what is happening and how this commit could explain
the suspend/resume breakage ?

Cheers.

-- 
Robert

PS: Bisect log for reference
git bisect start
# good: [6a13feb9c82803e2b815eca72fa7a9f5561d7861] Linux 4.3
git bisect good 6a13feb9c82803e2b815eca72fa7a9f5561d7861
# bad: [8005c49d9aea74d382f474ce11afbbc7d7130bec] Linux 4.4-rc1
git bisect bad 8005c49d9aea74d382f474ce11afbbc7d7130bec
# bad: [118c216e16c5ccb028cd03a0dcd56d17a07ff8d7] Merge tag 'staging-4.4-rc1' 
of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging
git bisect bad 118c216e16c5ccb028cd03a0dcd56d17a07ff8d7
# good: [e627078a0cbdc0c391efeb5a2c4eb287328fd633] Merge branch 'for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
git bisect good e627078a0cbdc0c391efeb5a2c4eb287328fd633
# good: [c17c6da659571a115c7b4983da6c6ac464317c34] staging: wilc1000: rename 
pfScanResult of struct scan_attr
git bisect good c17c6da659571a115c7b4983da6c6ac464317c34
# good: [7bdb7d554e0e433b92b63f3472523cc3067f8ab4] Staging: rtl8192u: 
ieee80211: corrected indent
git bisect good 7bdb7d554e0e433b92b63f3472523cc3067f8ab4
# good: [ac322de6bf5416cb145b58599297b8be73cd86ac] Merge tag 'md/4.4' of 
git://neil.brown.name/md
git bisect good ac322de6bf5416cb145b58599297b8be73cd86ac
# bad: [a4d8e93c3182a54d8d21a4d1cec6538ae1be9e16] Merge tag 'usb-for-v4.4' of 
git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb into usb-next
git bisect bad a4d8e93c3182a54d8d21a4d1cec6538ae1be9e16
# bad: [5390d438e6f4aaf3555acc72aff155660a48cd28] usb: dwc2: gadget: kill ep0 
requests before reinitializing core
git bisect bad 5390d438e6f4aaf3555acc72aff155660a48cd28
# bad: [58efd4b06df4a421652cb2c8a850a9697a37915c] usb: phy: change some comments
git bisect bad 58efd4b06df4a421652cb2c8a850a9697a37915c
# good: [f871cb9b2e178667a351a6fae9d930826ec10e95] usb: gadget: fix few 
outdated comments
git bisect good f871cb9b2e178667a351a6fae9d930826ec10e95
# bad: [101382ffb3838d68bf6d6d675c66a2f84ed3cb90] usb: gadget: f_phonet: 
eliminate abuse of ep->driver data
git bisect bad 101382ffb3838d68bf6d6d675c66a2f84ed3cb90
# bad: [34422a5e5a884c8680ce9144cf270ae08b1472be] usb: gadget: f_eem: eliminate 
abuse of ep->driver data
git bisect bad 34422a5e5a884c8680ce9144cf270ae08b1472be
# bad: [b0bac2581c1918cc4ab0aca01977ad69f0bc127a] usb: gadget: introduce 
'enabled' flag in struct usb_ep
git bisect bad b0bac2581c1918cc4ab0aca01977ad69f0bc127a
# good: [b67f628c84329a9ce82dbff5fde196dc4624e7c2] usb: gadget: epautoconf: add 
usb_ep_autoconfig_release() function
git bisect good b67f628c84329a9ce82dbff5fde196dc4624e7c2
# first bad commit: [b0bac2581c1918cc4ab0aca01977ad69f0bc127a] usb: gadget: 
introduce 'enabled' flag in struct usb_ep
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Robert Jarzmik]

Alan Stern st...@rowland.harvard.edu writes:

Hi Alan,

 On Sat, 25 Jul 2015, Robert Jarzmik wrote:

 Petr Cvek petr.c...@tul.cz writes:
 
  On 23.7.2015 21:46, Alan Stern wrote:
  It seems that it allows using a BULK endpoint for requested INT
  endpoint. For my PXA27x machine the original code returns BULK EP
  even with valid INT endpoint definition (because BULK EPs are defined
  earlier than INT EPs).
  
  Yes, it does allow a bulk endpoint to be used when an interrupt 
  endpoint was requested.  However, it won't return a bulk endpoint if 
  all the bulk endpoints are already in use.
 This cannot work for pxa27x.

 Do you mean that on pxa27x, a bulk endpoint cannot be used as an
 interrupt endpoint?  Why not?  From the device controller's point of
 view, there is no difference between bulk and interrupt (except
 possibly for the maxpacket sizes and high-bandwidth usage when running
 at high speed).
That's the point, maxpacket size and priority.

As you said, it's not that it won't work, it won't work with the priority
expected by the software stack, ie. higher priority for ISO endpoint.

 The pxa27x IP has a hardware limitation which prevents an endpoint from 
 changing
 its type once the UDC is enabled (see the comment at the beginning of
 pxa27x_udc.c).
 
 If that patchset implies that for a requested INT endpoint a BULK endpoint 
 can
 be returned, that won't work. Felipe and Robert, is that what this patchset
 implies ?

 Sort of.  The matching code has always behaved that way and this
 patchset does not change the behavior.
Then all is fine I suppose, if it was working before and nothing changes, it
will continue to work, won't it ?

  A default PXA27x configuration returns BULK for requested INT. Which is
  unfortunate, because PXA27x supports INT endpoints and has one predefined, 
  but
  this function find BULK first (one BULK is allocated and INT is never 
  used).
 See above.

 See response above.

 Besides, let's say the pxa27x has one bulk and one interrupt endpoint.  
 Now suppose the gadget driver requests a bulk endpoint first.  The 
 matching code will allocate the single bulk endpoint.  Then the gadget 
 driver requests an interrupt endpoint.  The matching code cannot 
 allocate the bulk endpoint, because that endpoint is already allocated.  
 So it will allocate the interrupt endpoint.

 Thus, as you can see, under the right conditions everything will work 
 as desired.

Let me give you another example :
 - pxa27x_udc offers 3 endpoints : ep-in, ep-out, ep-iso-in
 - a gadget driver does :
   - request an ep-in
   - request an ep-out
   - request an ep-in
   - request an ep-iso-in
In that case, the ep-iso-in request will fail, right ? Yet I would have expected
the second ep-in request to fail, as that's the one which cannot be serviced.

Of course, this hypothetical case implies that pxa27x_udc is not compatible with
this gadget driver, so it's not really relevant, is it ...

  Because if they do, the ep_matches() function works poorly. It returns a 
  BULK
  for device (gadget) side, but host side (PC) thinks that this endpoint is 
  an INT
  and handles it in this way. But the PXA27x thinks the endpoint is a BULK 
  and
  handles it in its way (according to datasheet, settings for a BULK and an 
  INT
  transfers are not 100% compatible).

 How do they differ?
One example I have in mind is chapter 12.4.2 of pxa27x developer manual
Endpoint Memory Configuration, quote follows :
  If the USB host controller transmits more OUT data than the maximum
  size packet for a bulk or interrupt endpoint, the UDC does not send
  any handshake to the host controller causing the host controller to
  time-out. If the USB host controller transmits more OUT data than the
  maximum size packet for an isochronous endpoint, the UDC sets the data
  packet error (DPE) bit in the Endpoint Control/Status register,
  UDCCSRx[DPE].

 Perhaps you could submit a patch that adds a do not allocate a bulk 
 endpoint when an interrupt endpoint is requested quirk flag to the 
 usb_gadget structure, and modify the matching code to take the new flag 
 into account.
Well, if it was working that way already in the past, I don't see overloading
the code with a quirk a necessity. My only need is that it continues to work.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 42/46] usb: gadget: move ep_matches() from epautoconf to udc-core

[Robert Jarzmik]

Petr Cvek petr.c...@tul.cz writes:

 On 23.7.2015 21:46, Alan Stern wrote:
 It seems that it allows using a BULK endpoint for requested INT
 endpoint. For my PXA27x machine the original code returns BULK EP
 even with valid INT endpoint definition (because BULK EPs are defined
 earlier than INT EPs).
 
 Yes, it does allow a bulk endpoint to be used when an interrupt 
 endpoint was requested.  However, it won't return a bulk endpoint if 
 all the bulk endpoints are already in use.
This cannot work for pxa27x.

The pxa27x IP has a hardware limitation which prevents an endpoint from changing
its type once the UDC is enabled (see the comment at the beginning of
pxa27x_udc.c).

If that patchset implies that for a requested INT endpoint a BULK endpoint can
be returned, that won't work. Felipe and Robert, is that what this patchset
implies ?

 A default PXA27x configuration returns BULK for requested INT. Which is
 unfortunate, because PXA27x supports INT endpoints and has one predefined, but
 this function find BULK first (one BULK is allocated and INT is never used).
See above.

 Because if they do, the ep_matches() function works poorly. It returns a BULK
 for device (gadget) side, but host side (PC) thinks that this endpoint is an 
 INT
 and handles it in this way. But the PXA27x thinks the endpoint is a BULK and
 handles it in its way (according to datasheet, settings for a BULK and an INT
 transfers are not 100% compatible).

 I cannot test INT as BULK behavior for the gadget functions, because all
 gadgets which works on PXA27x does not use INT endpoints (some allocate the
 endpoint but never use it).
Ah a bit of history here.

At least gadget zero does, and it's my main testing point for pxa27x_udc.
Then there should be g_serial (no acm nor obex), but that's something I have not
tried since 2009 ...

For history also, there was already an attempt a long time ago for epautoconf
revamping, done by Rodolfo Giometti IIRC.

Anyway, I need pxa27x_udc to remain functional, so I'd like to understand if
something will stop working, Robert B.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: pass flags parameter to gpiod_get functions

[Robert Jarzmik]

Alexandre Courbot acour...@nvidia.com writes:

 On 03/28/2015 04:30 AM, Uwe Kleine-König wrote:
 Looks good, but can't we also merge the call to gpiod_direction to avoid using
 GPIOD_ASIS? The golden rule is that a direction should not remain in an 
 unknown
 state after being requested.

 This would also require adding a flag to the alternative calls to
 gpio_request_one(). At first sight it seems to be doable, but I don't know 
 these
 drivers well enough to have an informed opinion.
Is the following information enough to grant the doable ? :
 - gpiod_reset is always an output gpio
 - gpiod_vbus is aslways an input gpio

Moreover, I seem to remember there was a bit of back and forth around the reset
gpio, so there's something subtle I don't remember (Fabio maybe your memory is
better than mine here) ...

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: pass flags parameter to gpiod_get functions

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 Looks good to me:

 Acked-by: Felipe Balbi ba...@ti.com
Same for me:
Acked-by: Robert Jarzmik robert.jarz...@free.fr

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/27] pxa27x_udc: Remove use of seq_printf return value

[Robert Jarzmik]

Joe Perches j...@perches.com writes:

 The seq_printf return value, because it's frequently misused,
 will eventually be converted to void.

 See: commit 1f33c41c03da (seq_file: Rename seq_overflow() to
  seq_has_overflowed() and make public)

 While there, simplify the error handler logic by returning
 immediately and remove the unnecessary labels.

 Signed-off-by: Joe Perches j...@perches.com
Tested-by: Robert Jarzmik robert.jarz...@free.fr

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 On Thu, Jan 29, 2015 at 8:41 PM, Robert Jarzmik robert.jarz...@free.fr 
 wrote:
 All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
 and not prevent the driver from working. Fix the regression in the
 behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

 The usual way is to also specify the commit number.
Yes, if it's already upstream in Linus's tree, where it's not.
When Linus will pull, the commit id will change, and the commit message will
become wrong, that's why I omitted the commit id.

 +   if (nop-gpiod_reset)
 +   gpiod_direction_output(nop-gpiod_reset, 0);

 If you change it to:
 gpiod_direction_output(nop-gpiod_reset, 1);

 then you can add:

 Tested-by: Fabio Estevam fabio.este...@freescale.com
Thanks, on my way.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] usb: phy: generic: fix the vbus interrupt request

[Robert Jarzmik]

Declare the interrupt as one shot so that it is masked until the end
of the threaded handler. This prevents the irq core from spitting out an
error :
  Threaded irq requested with handler=NULL and !ONESHOT for irq 63

This was introduced by commit usb: phy: generic: add vbus support.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index bdb4cb3..48af068 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -41,7 +41,8 @@
 #include phy-generic.h
 
 #define VBUS_IRQ_FLAGS \
-   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
+   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | \
+   IRQF_ONESHOT)
 
 struct platform_device *usb_phy_generic_register(void)
 {
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 Since the power GPIO is mapped as active-low, its actual signal will be 0
 after this code. Contrary to the legacy integer GPIO interface, the active-low
 property is handled during mapping and is thus transparent to GPIO consumers.

 ,which is exactly what we want in the gpio reset case: we want it to
 output in its active level first.
I didn't know that.

 So your patch should do like this:

  +   if (nop-gpiod_reset)
  +   gpiod_direction_output(nop-gpiod_reset, 1);

 This will guarantee that we keep the same behaviour as we had prior to
 the introduction of gpiod.
That's true, the GPIOF_OUT_INIT_LOW/HIGH in the former code just struck me.

 I plan to submit a separate fix on top of yours that puts the delay in
 the correct position.

 This separate issue exists even prior to the gpiod api inclusion.

 What we need to do:

 - Force the GPIO in its active reset level state
I will take care of that, as it's a regression introduced by my patch.

 - Wait a bit
 - Put the GPIO out of reset.
Ah, that one will be for you :)

 ,but I can handle this one after your patch gets applied.
OK, let's do that.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/2] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
and not prevent the driver from working. Fix the regression in the
behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
Tested-by: Fabio Estevam fabio.este...@freescale.com

---
Since v1: correctly handle the error codes
  set the reset gpio as an output gpio
Since v2: set the reset gpio initial state to logically active
---
 drivers/usb/phy/phy-generic.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 9a826ff..bdb4cb3 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,12 +218,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
-   err = PTR_ERR(nop-gpiod_reset);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_reset);
if (!err) {
nop-gpiod_vbus = devm_gpiod_get_optional(dev,
-vbus-detect-gpio);
-   err = PTR_ERR(nop-gpiod_vbus);
+vbus-detect);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_vbus);
}
} else if (pdata) {
type = pdata-type;
@@ -242,9 +242,11 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
if (err == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (err) {
-   dev_err(dev, Error requesting RESET GPIO\n);
+   dev_err(dev, Error requesting RESET or VBUS GPIO\n);
return err;
}
+   if (nop-gpiod_reset)
+   gpiod_direction_output(nop-gpiod_reset, 1);
 
nop-phy.otg = devm_kzalloc(dev, sizeof(*nop-phy.otg),
GFP_KERNEL);
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 Why do you set it to zero independently of the GPIO active level flag?
I don't.

Please have a look at gpiod_direction_output() first, especially :
if (test_bit(FLAG_ACTIVE_LOW, desc-flags))

 ,which correctly puts the GPIO output to the correct initial level
 depending on the active level flag.
Same here.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] usb: phy: generic: fix the vbus interrupt request

[Robert Jarzmik]

Declare the interrupt as one shot so that it is masked until the end
of the threaded handler. This prevents the irq core from spitting out an
error :
  Threaded irq requested with handler=NULL and !ONESHOT for irq 63

This was introduced by commit usb: phy: generic: add vbus support.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 9f08c1e..af32ce2 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -41,7 +41,8 @@
 #include phy-generic.h
 
 #define VBUS_IRQ_FLAGS \
-   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
+   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | \
+   IRQF_ONESHOT)
 
 struct platform_device *usb_phy_generic_register(void)
 {
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
and not prevent the driver from working. Fix the regression in the
behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
Since v1: correctly handle the error codes
  set the reset gpio as an output gpio
---
 drivers/usb/phy/phy-generic.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 9a826ff..9f08c1e 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,12 +218,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
-   err = PTR_ERR(nop-gpiod_reset);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_reset);
if (!err) {
nop-gpiod_vbus = devm_gpiod_get_optional(dev,
-vbus-detect-gpio);
-   err = PTR_ERR(nop-gpiod_vbus);
+vbus-detect);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_vbus);
}
} else if (pdata) {
type = pdata-type;
@@ -242,9 +242,11 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
if (err == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (err) {
-   dev_err(dev, Error requesting RESET GPIO\n);
+   dev_err(dev, Error requesting RESET or VBUS GPIO\n);
return err;
}
+   if (nop-gpiod_reset)
+   gpiod_direction_output(nop-gpiod_reset, 0);
 
nop-phy.otg = devm_kzalloc(dev, sizeof(*nop-phy.otg),
GFP_KERNEL);
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
 if (gpiod_is_active_low(nop-gpiod_reset))
 gpiod_direction_output(nop-gpiod_reset, GPIOD_OUT_LOW);
 else
 gpiod_direction_output(nop-gpiod_reset, GPIOD_OUT_HIGH);

 won't the descriptor itself handle that for us ? Linus ?

 I want to hear from Linus W first.

Yes, so do I.

Let's add a bit of context for Linus :
 1) In the past, the driver was doing a ;
gpio_request_one()
  - gpiod_direction_output_raw()
 2) After the conversion to gpio descriptors, it is doing :
gpiod_get_optional(dev, reset);
  - ...
- __gpiod_get_index(dev, reset, 0, 0)
  - this of course doesn't call gpiod_direction()

The problem is that we cannot call:
  gpiod_get_optional(dev, reset, GPIOD_OUT_LOW);
because we don't know before the call if the GPIO is active high or low. The
former gpio_request_one() was clever and did a gpio_direction_output_raw(..,
(flags  GPIOF_INIT_HIGH) ? 1 : 0).

So what is the right way out ? :
 - is it to call gpiod_direction_output(nop-gpiod, 0) just after
   gpiod_get_optional() in probe code ?
 - is it when the gpio is used to call gpiod_direction_output(gpiod, x) instead
   of gpiod_set_value() ?
 - is it something else ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

  At the same time, declare the interrupt as one shot so that it is
  masked until the end of the threaded handler. This prevents the irq core
  from spitting out an error :
Threaded irq requested with handler=NULL and !ONESHOT for irq 63
 
 Shouldn't this be a separate patch?
   #define VBUS_IRQ_FLAGS \
  -  (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
  +  (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | \
  +  IRQF_ONESHOT)
 
 IMHO, unrelated to the regression fix.

 +1
Sure, no problem.
As soon as there is a confirmation of the right final fix for the direction
issue, I'll post a V2 with 2 patches instead of 1.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: phy: generic: fix the gpios to be optional

[Robert Jarzmik]

All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
and not prevent the driver from working. Fix the regression in the
behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

At the same time, declare the interrupt as one shot so that it is
masked until the end of the threaded handler. This prevents the irq core
from spitting out an error :
  Threaded irq requested with handler=NULL and !ONESHOT for irq 63

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 9a826ff..49db411 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -41,7 +41,8 @@
 #include phy-generic.h
 
 #define VBUS_IRQ_FLAGS \
-   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
+   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | \
+   IRQF_ONESHOT)
 
 struct platform_device *usb_phy_generic_register(void)
 {
@@ -218,12 +219,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
-   err = PTR_ERR(nop-gpiod_reset);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_reset);
if (!err) {
nop-gpiod_vbus = devm_gpiod_get_optional(dev,
-vbus-detect-gpio);
-   err = PTR_ERR(nop-gpiod_vbus);
+vbus-detect);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_vbus);
}
} else if (pdata) {
type = pdata-type;
@@ -242,7 +243,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
if (err == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (err) {
-   dev_err(dev, Error requesting RESET GPIO\n);
+   dev_err(dev, Error requesting RESET or VBUS GPIO\n);
return err;
}
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[robert . jarzmik]

- Mail original -
De: Fabio Estevam feste...@gmail.com
À: Robert Jarzmik robert.jarz...@free.fr
Cc: Felipe Balbi ba...@ti.com, Linus Walleij linus.wall...@linaro.org, 
Philipp Zabel philipp.za...@gmail.com, USB list 
linux-usb@vger.kernel.org
Envoyé: Mardi 27 Janvier 2015 13:41:57
Objet: Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

 I agree, but 'reset-gpio' property exists in imx51-babbage.dts, but
 devm_gpiod_get_optional(dev, reset) is returning NULL.

 This is the problem that puzzles me at the moment.
Is in the right node, ie. the node that has compatible == usb-nop-xceiv ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 On Tue, Jan 27, 2015 at 12:16 PM, Fabio Estevam feste...@gmail.com wrote:
 On Tue, Jan 27, 2015 at 11:37 AM,  robert.jarz...@free.fr wrote:

 Is in the right node, ie. the node that has compatible == usb-nop-xceiv ?

 Yes, it is correct since this commit:
 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/arm/boot/dts/imx51-babbage.dts?id=7a9f0604bd56936b2b18f49824e0e392dc7878c3

 This is the patch I have applied:
...zip...

This is my device-tree blob:
usb2phy: gpio-vbus@13 {
compatible = usb-nop-xceiv;
vbus-detect-gpio = gpio 13 GPIO_ACTIVE_LOW;
wakeup;
};

And this is what I have with this latest patch :
[0.617927] usb_phy_generic pxabus:gpio-vbus@13: GPIO lookup for consumer 
reset
[0.618177] usb_phy_generic pxabus:gpio-vbus@13: using device tree for GPIO 
lookup
[0.618306] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of 
node '/pxabus/gpio-vbus@13[0]'
[0.618425] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of 
node '/pxabus/gpio-vbus@13[0]'
[0.618538] usb_phy_generic pxabus:gpio-vbus@13: using lookup tables for 
GPIO lookup
[0.618643] usb_phy_generic pxabus:gpio-vbus@13: lookup for GPIO reset failed
[0.618743] usb_phy_generic pxabus:gpio-vbus@13: GPIO lookup for consumer 
vbus-detect
[0.618842] usb_phy_generic pxabus:gpio-vbus@13: using device tree for GPIO 
lookup
[0.618943] of_get_named_gpiod_flags: can't parse 'vbus-detect-gpios' 
property of node '/pxabus/gpio-vbus@13[0]'
[0.619093] of_get_named_gpiod_flags: parsed 'vbus-detect-gpio' property of 
node '/pxabus/gpio-vbus@13[0]' - status (0)
[0.619221] RJK2: nop-gpiod_vbus = c06bf6dc

So I'll submit that one in [1]. It has to work in your case too, it works in
mine ...

Cheers.

--
Robert

---8---
[1] Patch
From 4005a6abf519272267399ac4030a5671f7877ca4 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik robert.jarz...@free.fr
Date: Tue, 27 Jan 2015 06:27:03 +0100
Subject: [PATCH] usb: phy: generic: fix the gpios to be optional

All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
and not prevent the driver from working. Fix the regression in the
behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index dd05254..012534a 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,12 +218,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
-   err = PTR_ERR(nop-gpiod_reset);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_reset);
if (!err) {
-   nop-gpiod_vbus = devm_gpiod_get(dev,
-vbus-detect-gpio);
-   err = PTR_ERR(nop-gpiod_vbus);
+   nop-gpiod_vbus = devm_gpiod_get_optional(dev,
+vbus-detect);
+   err = PTR_ERR_OR_ZERO(nop-gpiod_vbus);
}
} else if (pdata) {
type = pdata-type;
@@ -242,7 +242,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
if (err == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (err) {
-   dev_err(dev, Error requesting RESET GPIO\n);
+   dev_err(dev, Error requesting RESET or VBUS GPIO\n);
return err;
}
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
 index 9a826ff31951..34231c31cd1a 100644
 --- a/drivers/usb/phy/phy-generic.c
 +++ b/drivers/usb/phy/phy-generic.c
 @@ -219,11 +219,11 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
 usb_phy_generic *nop,
  
   needs_vcc = of_property_read_bool(node, vcc-supply);
   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
 - err = PTR_ERR(nop-gpiod_reset);
 + err = PTR_ERR_OR_ZERO(nop-gpiod_reset);
   if (!err) {
   nop-gpiod_vbus = devm_gpiod_get_optional(dev,
vbus-detect-gpio);
 - err = PTR_ERR(nop-gpiod_vbus);
 + err = PTR_ERR_OR_ZERO(nop-gpiod_vbus);
   }
   } else if (pdata) {
   type = pdata-type;
Funny, we ended up on the same solution ... almost, I add the reset-gpios
into reset and vbus-detect-gpio into vbus-detect also. But it's a good
sign we've ended up on the same code, isn't it ;) ?

Do you want me to rebase on your next tree and submit ? Or do you want to submit
the patch yourself ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[robert . jarzmik]

- Mail original -
De: Fabio Estevam feste...@gmail.com
À: Robert Jarzmik robert.jarz...@free.fr
Cc: Felipe Balbi ba...@ti.com, Linus Walleij linus.wall...@linaro.org, 
Philipp Zabel philipp.za...@gmail.com, USB list 
linux-usb@vger.kernel.org
Envoyé: Mardi 27 Janvier 2015 10:40:35
Objet: Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc
 -   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
 +   nop-gpiod_reset = devm_gpiod_get_optional(dev, 
 reset-gpios);

 According to Documentation/gpio/board.txt:

 GPIOs mappings are defined in the consumer device's node, in a
 property named function-gpios
 so this should be:
 nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);
Ah you're right, I see it now ... in of_find_gpio(), the match is done on 
%s-%s, where
the first one is the string in devm_gpiod_get(), and the second one is either 
gpio or
gpios ...

I'll amend my patch just as you said, my bad.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[robert . jarzmik]

- Mail original -
De: Fabio Estevam feste...@gmail.com
À: Robert Jarzmik robert.jarz...@free.fr
Cc: Felipe Balbi ba...@ti.com, Linus Walleij linus.wall...@linaro.org, 
Philipp Zabel philipp.za...@gmail.com, USB list 
linux-usb@vger.kernel.org
Envoyé: Mardi 27 Janvier 2015 12:23:43
Objet: Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

 I'll amend my patch just as you said, my bad.
 but the problem I see is that even if I use:
 nop-gpiod_reset = devm_gpiod_get_optional(dev, reset);

 The 'reset-gpio' pin cannot be retrieved. This is the point I don't
 understand currently.
If the reset-gpio device-tree property doesn't exist, devm_gpiod_get_optional()
will return a NULL pointer, and nop-gpiod_reset = NULL.

Then in nop_reset_set(), the first test will assert that
(nop-gpiod_reset == NULL) and return.

For the longer story, devm_gpiod_get_optional() returns NULL :
 - because gpiod_get_index_optional() returns NULL
 - because gpiod_get_index returns -ENOENT
 - because of_find_gpio() returns -ENOENT

Does that answer you question ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 On Sat, Dec 6, 2014 at 7:05 PM, Robert Jarzmik robert.jarz...@free.fr wrote:
 Change internal gpio handling from integer gpios into gpio
 descriptors. This change only addresses the internal API and
 device-tree/ACPI, while the legacy platform data remains integer space
 based.

 This change is only build compile tested, and very prone to error. I
 leave this comment for now in the commit message so that this patch gets
 some testing as I'm pretty sure it's buggy.

 I can confirm it is buggy. It makes the property 'reset-gpios' to be
 mandatory now and causes regression.

 -   gpio_set_value_cansleep(nop-gpio_reset, value);
 +   if (nop-gpiod_reset)
 +   gpiod_set_value(nop-gpiod_reset, asserted);

 Previously we had the active_low or active_high flag taken into
 account. Now we don't.
Is it something you're seing by testing of by code review ?
Because the active high/active low is taken into account by gpiod_set_value().

 -   nop-gpio_reset = of_get_named_gpio_flags(node, 
 reset-gpios,
 -   0, flags);
 -   if (nop-gpio_reset == -EPROBE_DEFER)
 -   return -EPROBE_DEFER;
 -
 -   nop-reset_active_low = flags  OF_GPIO_ACTIVE_LOW;
 -
 +   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);

 We should do 'devm_gpiod_get(dev, reset);' instead.
Why ? The previous call was of_get_named_gpio_flags(node, reset-gpios,
...). Why this name change into reset ?

Now if you say we should do a gpiod_get_optional() instead of
devm_gpiod_get(), and if you're not willing to make that patch, I can make it.

 This commit is now in linux-next as e9f2cefb0cdc2aea.
 Should we revert it as it has so many issues?
I'm not convinced of the so many issues. So far I see the
gpiod_get_optional() requirement, which is a one liner patch.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Robert Jarzmik robert.jarz...@free.fr writes:

 I'm not convinced of the so many issues. So far I see the
 gpiod_get_optional() requirement, which is a one liner patch.
Would you try the following patch to see if it fixes all your concerns please ?

Cheers.

-- 
Robert

---8---
From 7b34a3aa2a0717b53cd458611048f50edde53d0c Mon Sep 17 00:00:00 2001
From: Robert Jarzmik robert.jarz...@free.fr
Date: Tue, 27 Jan 2015 06:27:03 +0100
Subject: [PATCH] usb: phy: generic: fix the gpios to be optional

All the gpios, ie. reset-gpios and vbus-detect-gpio, should be optional
and not prevent the driver from working. Fix the regression in the
behavior introduced by commit usb: phy: generic: migrate to gpio_desc.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index dd05254..c767bf0 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -218,10 +218,10 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
+   nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
err = PTR_ERR(nop-gpiod_reset);
if (!err) {
-   nop-gpiod_vbus = devm_gpiod_get(dev,
+   nop-gpiod_vbus = devm_gpiod_get_optional(dev,
 vbus-detect-gpio);
err = PTR_ERR(nop-gpiod_vbus);
}
@@ -242,7 +242,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
if (err == -EPROBE_DEFER)
return -EPROBE_DEFER;
if (err) {
-   dev_err(dev, Error requesting RESET GPIO\n);
+   dev_err(dev, Error requesting RESET or VBUS GPIO\n);
return err;
}
 
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usb: phy: generic: Do not fail when 'reset-gpios' is not used

[Robert Jarzmik]

Fabio Estevam feste...@gmail.com writes:

 On Fri, Jan 23, 2015 at 2:12 AM, Fabio Estevam feste...@gmail.com wrote:
 Will need to debug more.

 The problem here is that:

 nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);

 Always return -2.

 On imx51-babbage.dts we pass 'reset-gpios' and it used to retrieve it
 fine prior to e9f2cefb0cdc2ae (usb: phy: generic: migrate to
 gpio_desc) by using of_get_named_gpio_flags().

 Any ideas as to why devm_gpiod_get() always fail now?
I think the right solution was proposed a week ago or so here :
  https://lkml.org/lkml/2015/1/14/1016

I added Paul to the discussion as he is the author of it.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: make GPIOs optional for the generic phy

[Robert Jarzmik]

Paul Zimmerman paul.zimmer...@synopsys.com writes:

 From 47bd18e210fecf701d493c27884e13c69bc449ea Mon Sep 17 00:00:00 2001
 From: Paul Zimmerman pa...@synopsys.com
 Date: Wed, 14 Jan 2015 18:15:58 -0800
 Subject: [PATCH] usb: phy: make GPIOs optional for the generic phy

 The use of GPIOs should be optional for the generic phy, otherwise
 the Altera SOCFPGA platform at least is broken.

 Fixes breakage caused by a combination of e9f2cefb0cd usb: phy: 
 generic: migrate to gpio_desc and 135b3c4304d usb: dwc2: platform: 
 add generic PHY framework support.

 Signed-off-by: Paul Zimmerman pa...@synopsys.com
Reviewed-by: Robert Jarzmik robert.jarz...@free.fr

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/5] usb: dwc2: add generic PHY framework support for dwc2 usb controler platform driver.

[Robert Jarzmik]

Paul Zimmerman paul.zimmer...@synopsys.com writes:
 The patch below fixes it. And it seems like the right thing to me,
 since GPIOs should be optional for a generic phy, I would think. But
 my device tree foo is very weak, so I'm not sure.

 CCing Robert, who touched the generic phy code last. Robert, what do
 you think?
I think your patch in [1] is correct, because
Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt states that
reset-gpios is optional, and because Felipe told me gpios for phy_generic are
optional.

Now the original code was written by Felipe, so better ask him first. The
original code was :
af9f51c55 phy-generic.c nop-gpio_reset = of_get_named_gpio_flags(node, 
reset-gpios,
af9f51c55 phy-generic.c 0, 
flags);
af9f51c55 phy-generic.c if (nop-gpio_reset == -EPROBE_DEFER)
af9f51c55 phy-generic.c return -EPROBE_DEFER;

Cheers.

--
Robert

[1]
 diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
 index dd05254..9a826ff 100644
 --- a/drivers/usb/phy/phy-generic.c
 +++ b/drivers/usb/phy/phy-generic.c
 @@ -218,10 +218,10 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
 usb_phy_generic *nop,
   clk_rate = 0;
  
   needs_vcc = of_property_read_bool(node, vcc-supply);
 - nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
 + nop-gpiod_reset = devm_gpiod_get_optional(dev, reset-gpios);
   err = PTR_ERR(nop-gpiod_reset);
   if (!err) {
 - nop-gpiod_vbus = devm_gpiod_get(dev,
 + nop-gpiod_vbus = devm_gpiod_get_optional(dev,
vbus-detect-gpio);
   err = PTR_ERR(nop-gpiod_vbus);
   }
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] usb: phy: generic: device-tree support

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 Hi,

Happy new year Felipe,

 sorry for the long delay, got caught up with other stuff. It's now in my
 testing/next and I'll start testing with the boards I have.
Excellent news.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] usb: phy: generic: device-tree support

[Robert Jarzmik]

Robert Jarzmik robert.jarz...@free.fr writes:

 Hi Felipe,

 This is the 2nd opus of this serie.
 For patches 1 and 2, all comments have been addressed.

 For patch 3, you had a question about device charging spec and the regulator
 timings. I read the specs, saw the 15mn timing you were speaking of, but 
 didn't
 find a nice way to implement it. As I'd like this patchset to move forward, I
 see these ways out :
  1) I leave it as is, making it as good as phy_gpio_vbus, but not better
  2) I remove all the regulator stuff, it you judge a partial implementation is
 worse that a complete one

 Just tell me which solution you prefer.

Ping ?

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] usb: phy: generic: device-tree support

[Robert Jarzmik]

Hi Felipe,

This is the 2nd opus of this serie.
For patches 1 and 2, all comments have been addressed.

For patch 3, you had a question about device charging spec and the regulator
timings. I read the specs, saw the 15mn timing you were speaking of, but didn't
find a nice way to implement it. As I'd like this patchset to move forward, I
see these ways out :
 1) I leave it as is, making it as good as phy_gpio_vbus, but not better
 2) I remove all the regulator stuff, it you judge a partial implementation is
worse that a complete one

Just tell me which solution you prefer.

Cheers.

--
Robert

Robert Jarzmik (3):
  usb: phy: generic: migrate to gpio_desc
  usb: phy: nop: device tree documentation for vbus
  usb: phy: generic: add vbus support

 .../devicetree/bindings/usb/usb-nop-xceiv.txt  |   8 ++
 drivers/usb/phy/phy-generic.c  | 146 +++--
 drivers/usb/phy/phy-generic.h  |  10 +-
 include/linux/usb/usb_phy_generic.h|   2 +
 4 files changed, 124 insertions(+), 42 deletions(-)

-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Change internal gpio handling from integer gpios into gpio
descriptors. This change only addresses the internal API and
device-tree/ACPI, while the legacy platform data remains integer space
based.

This change is only build compile tested, and very prone to error. I
leave this comment for now in the commit message so that this patch gets
some testing as I'm pretty sure it's buggy.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
Acked-by: Linus Walleij linus.wall...@linaro.org
---
Since v1: added Linus's ack
---
 drivers/usb/phy/phy-generic.c | 61 ++-
 drivers/usb/phy/phy-generic.h |  4 +--
 2 files changed, 21 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 7594e50..71e061d 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -61,16 +61,8 @@ static int nop_set_suspend(struct usb_phy *x, int suspend)
 
 static void nop_reset_set(struct usb_phy_generic *nop, int asserted)
 {
-   int value;
-
-   if (!gpio_is_valid(nop-gpio_reset))
-   return;
-
-   value = asserted;
-   if (nop-reset_active_low)
-   value = !value;
-
-   gpio_set_value_cansleep(nop-gpio_reset, value);
+   if (nop-gpiod_reset)
+   gpiod_set_value(nop-gpiod_reset, asserted);
 
if (!asserted)
usleep_range(1, 2);
@@ -145,35 +137,38 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
struct usb_phy_generic_platform_data *pdata)
 {
enum usb_phy_type type = USB_PHY_TYPE_USB2;
-   int err;
+   int err = 0;
 
u32 clk_rate = 0;
bool needs_vcc = false;
 
-   nop-reset_active_low = true;   /* default behaviour */
-
if (dev-of_node) {
struct device_node *node = dev-of_node;
-   enum of_gpio_flags flags = 0;
 
if (of_property_read_u32(node, clock-frequency, clk_rate))
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpio_reset = of_get_named_gpio_flags(node, reset-gpios,
-   0, flags);
-   if (nop-gpio_reset == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-
-   nop-reset_active_low = flags  OF_GPIO_ACTIVE_LOW;
-
+   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
+   err = PTR_ERR(nop-gpiod_reset);
} else if (pdata) {
type = pdata-type;
clk_rate = pdata-clk_rate;
needs_vcc = pdata-needs_vcc;
-   nop-gpio_reset = pdata-gpio_reset;
-   } else {
-   nop-gpio_reset = -1;
+   if (gpio_is_valid(gpio-gpio_reset)) {
+   err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
+   dev_name(dev));
+   if (!err)
+   nop-gpiod_reset =
+   gpio_to_desc(pdata-gpio_reset);
+   }
+   }
+
+   if (err == -EPROBE_DEFER)
+   return -EPROBE_DEFER;
+   if (err) {
+   dev_err(dev, Error requesting RESET GPIO\n);
+   return err;
}
 
nop-phy.otg = devm_kzalloc(dev, sizeof(*nop-phy.otg),
@@ -203,24 +198,6 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
return -EPROBE_DEFER;
}
 
-   if (gpio_is_valid(nop-gpio_reset)) {
-   unsigned long gpio_flags;
-
-   /* Assert RESET */
-   if (nop-reset_active_low)
-   gpio_flags = GPIOF_OUT_INIT_LOW;
-   else
-   gpio_flags = GPIOF_OUT_INIT_HIGH;
-
-   err = devm_gpio_request_one(dev, nop-gpio_reset,
-   gpio_flags, dev_name(dev));
-   if (err) {
-   dev_err(dev, Error requesting RESET GPIO %d\n,
-   nop-gpio_reset);
-   return err;
-   }
-   }
-
nop-dev= dev;
nop-phy.dev= nop-dev;
nop-phy.label  = nop-xceiv;
diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h
index d8feacc..09924fd 100644
--- a/drivers/usb/phy/phy-generic.h
+++ b/drivers/usb/phy/phy-generic.h
@@ -2,14 +2,14 @@
 #define _PHY_GENERIC_H_
 
 #include linux/usb/usb_phy_generic.h
+#include linux/gpio/consumer.h
 
 struct usb_phy_generic {
struct usb_phy phy;
struct device *dev;
struct clk *clk;
struct regulator *vcc;
-   int gpio_reset;
-   bool reset_active_low;
+   struct gpio_desc *gpiod_reset;
 };
 
 int usb_gen_phy_init(struct usb_phy *phy);
-- 
2.1.0

[PATCH v2 2/3] usb: phy: nop: device tree documentation for vbus

[Robert Jarzmik]

Enhance the phy documentation by adding 2 new optional bindings :
 - the vbus gpio, which detects usb insertion
 - the vbus regulator, which provides current drawn from the usb cable

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
Since v1: changed vbus-gpios to vbus-detect-gpio
---
 Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt 
b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
index 1bd37fa..ef1072b 100644
--- a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
+++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
@@ -17,6 +17,11 @@ Optional properties:
 
 - reset-gpios: Should specify the GPIO for reset.
 
+- vbus-detect-gpio: should specify the GPIO detecting a VBus insertion
+(see Documentation/devicetree/bindings/gpio/gpio.txt)
+- vbus-regulator : should specifiy the regulator supplying current drawn from
+  the VBus line (see 
Documentation/devicetree/bindings/regulator/regulator.txt).
+
 Example:
 
hsusb1_phy {
@@ -26,8 +31,11 @@ Example:
clock-names = main_clk;
vcc-supply = hsusb1_vcc_regulator;
reset-gpios = gpio1 7 GPIO_ACTIVE_LOW;
+   vbus-detect-gpio = gpio2 13 GPIO_ACTIVE_HIGH;
+   vbus-regulator = vbus_regulator;
};
 
 hsusb1_phy is a NOP USB PHY device that gets its clock from an oscillator
 and expects that clock to be configured to 19.2MHz by the NOP PHY driver.
 hsusb1_vcc_regulator provides power to the PHY and GPIO 7 controls RESET.
+GPIO 13 detects VBus insertion, and accordingly notifies the vbus-regulator.
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] usb: phy: generic: add vbus support

[Robert Jarzmik]

Add support for vbus detection and power supply. This code is more or
less stolen from phy-gpio-vbus-usb.c, and aims at providing a detection
mechanism for VBus (ie. usb cable plug) based on a GPIO line, and a
power supply activation which draws current from the VBus.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
Since v1: changed vbus-gpios to vbus-detect-gpio
---
 drivers/usb/phy/phy-generic.c   | 91 -
 drivers/usb/phy/phy-generic.h   |  6 +++
 include/linux/usb/usb_phy_generic.h |  2 +
 3 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 71e061d..f56a47b 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -29,6 +29,7 @@
 #include linux/module.h
 #include linux/platform_device.h
 #include linux/dma-mapping.h
+#include linux/usb/gadget.h
 #include linux/usb/otg.h
 #include linux/usb/usb_phy_generic.h
 #include linux/slab.h
@@ -41,6 +42,9 @@
 
 #include phy-generic.h
 
+#define VBUS_IRQ_FLAGS \
+   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
+
 struct platform_device *usb_phy_generic_register(void)
 {
return platform_device_register_simple(usb_phy_generic,
@@ -68,6 +72,73 @@ static void nop_reset_set(struct usb_phy_generic *nop, int 
asserted)
usleep_range(1, 2);
 }
 
+/* interface to regulator framework */
+static void nop_set_vbus_draw(struct usb_phy_generic *nop, unsigned mA)
+{
+   struct regulator *vbus_draw = nop-vbus_draw;
+   int enabled;
+   int ret;
+
+   if (!vbus_draw)
+   return;
+
+   enabled = nop-vbus_draw_enabled;
+   if (mA) {
+   regulator_set_current_limit(vbus_draw, 0, 1000 * mA);
+   if (!enabled) {
+   ret = regulator_enable(vbus_draw);
+   if (ret  0)
+   return;
+   nop-vbus_draw_enabled = 1;
+   }
+   } else {
+   if (enabled) {
+   ret = regulator_disable(vbus_draw);
+   if (ret  0)
+   return;
+   nop-vbus_draw_enabled = 0;
+   }
+   }
+   nop-mA = mA;
+}
+
+
+static irqreturn_t nop_gpio_vbus_thread(int irq, void *data)
+{
+   struct usb_phy_generic *nop = data;
+   struct usb_otg *otg = nop-phy.otg;
+   int vbus, status;
+
+   vbus = gpiod_get_value(nop-gpiod_vbus);
+   if ((vbus ^ nop-vbus) == 0)
+   return IRQ_HANDLED;
+   nop-vbus = vbus;
+
+   if (vbus) {
+   status = USB_EVENT_VBUS;
+   nop-phy.state = OTG_STATE_B_PERIPHERAL;
+   nop-phy.last_event = status;
+   usb_gadget_vbus_connect(otg-gadget);
+
+   /* drawing a unit load is *always* OK, except for OTG */
+   nop_set_vbus_draw(nop, 100);
+
+   atomic_notifier_call_chain(nop-phy.notifier, status,
+  otg-gadget);
+   } else {
+   nop_set_vbus_draw(nop, 0);
+
+   usb_gadget_vbus_disconnect(otg-gadget);
+   status = USB_EVENT_NONE;
+   nop-phy.state = OTG_STATE_B_IDLE;
+   nop-phy.last_event = status;
+
+   atomic_notifier_call_chain(nop-phy.notifier, status,
+  otg-gadget);
+   }
+   return IRQ_HANDLED;
+}
+
 int usb_gen_phy_init(struct usb_phy *phy)
 {
struct usb_phy_generic *nop = dev_get_drvdata(phy-dev);
@@ -151,17 +222,23 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
needs_vcc = of_property_read_bool(node, vcc-supply);
nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
err = PTR_ERR(nop-gpiod_reset);
+   if (!err) {
+   nop-gpiod_vbus = devm_gpiod_get(dev,
+vbus-detect-gpio);
+   err = PTR_ERR(nop-gpiod_vbus);
+   }
} else if (pdata) {
type = pdata-type;
clk_rate = pdata-clk_rate;
needs_vcc = pdata-needs_vcc;
-   if (gpio_is_valid(gpio-gpio_reset)) {
+   if (gpio_is_valid(pdata-gpio_reset)) {
err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
dev_name(dev));
if (!err)
nop-gpiod_reset =
gpio_to_desc(pdata-gpio_reset);
}
+   nop-gpiod_vbus = pdata-gpiod_vbus;
}
 
if (err == -EPROBE_DEFER)
@@ -226,6 +303,18 @@ static int usb_phy_generic_probe(struct platform_device 
*pdev)
err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(pdev-dev

Re: [PATCH v1] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Alexandre Courbot gnu...@gmail.com writes:

 I think what you want to do is explained in the Platform Data
 section of Documentation/gpio/board.txt
Ah yeah, already one year and I never checked again this documemtation.

Thanks.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Linus Walleij linus.wall...@linaro.org writes:

 This definately make things better so:
 Acked-by: Linus Walleij linus.wall...@linaro.org
Thanks.

 One comment though:

   if (dev-of_node) {
 (...)
 + nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
 + err = PTR_ERR(nop-gpiod_reset);
   } else if (pdata) {
 (...)
 + err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
 + dev_name(dev));
 + if (!err)
 + nop-gpiod_reset = gpio_to_desc(pdata-gpio_reset);
 + }

 So a next step would be to add support for getting the
 devm_gpiod_get(dev, reset-gpios); outside of the if (dev-of_node)
 clause, and possibly convert the board files for affected
 platforms to use descriptors, if they will not be replaced by
 device tree only.

OK, if we were to do this, is there a way to build a static platform data with a
gpio descriptor ?
Ie. can I write something like :
struct generic_phy_pdata {
struct gpio_desc *reset_gpio;
};

And in machine code :
static struct generic_phy_pdata usb_phy __initdata {
   .reset_gpio = XXX(19),
};

What should XXX be like to describe reset_gpio as a ACTIVE_HIGH gpio number 19
?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: phy: add lubbock phy driver

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Extract lubbock-specific code from pxa25x_udc driver. As a bonus, phy
 driver determines connector/VBUS status by reading CPLD register. Also
 it uses a work to call into udc stack, instead of pinging vbus session
 right from irq handler.
This comment is not accurate anymore, right ? The work call, etc ...

Moreover, I have this compile error:
drivers/built-in.o: In function `lubbock_vbus_remove':
/home/rj/mio_linux/kernel/drivers/usb/phy/phy-lubbock.c:200: undefined 
reference to `usb_remove_phy'
drivers/built-in.o: In function `lubbock_vbus_probe':
/home/rj/mio_linux/kernel/drivers/usb/phy/phy-lubbock.c:186: undefined 
reference to `usb_add_phy'
Makefile:922: recipe for target 'vmlinux' failed

A select in Kconfig is missing, right ?
And then :
genirq: Threaded irq requested with handler=NULL and !ONESHOT for irq 294
lubbock-vbus lubbock-vbus: can't request irq 294, err: -22
lubbock-vbus: probe of lubbock-vbus failed with error -22

 +static int is_vbus_powered(void)
 +{
 + return !(LUB_MISC_RD  BIT(9));
That BIT(9) is a bit ugly. Moreover the  is certainly wrong.
A define somewhere would be fine.

 +}
 +
 +static void lubbock_vbus_handle(struct lubbock_vbus_data *lubbock_vbus)
I have not reviewed that one thoroughly ...
 +
 +/* VBUS change IRQ handler */
 +static irqreturn_t lubbock_vbus_irq(int irq, void *data)
 +{
 + struct platform_device *pdev = data;
 + struct lubbock_vbus_data *lubbock_vbus = platform_get_drvdata(pdev);
 + struct usb_otg *otg = lubbock_vbus-phy.otg;
 +
 + dev_dbg(pdev-dev, VBUS %s (gadget: %s)\n,
 + is_vbus_powered() ? supplied : inactive,
 + otg-gadget ? otg-gadget-name : none);
 +
 + switch (irq) {
 + case LUBBOCK_USB_IRQ:
 + disable_irq(LUBBOCK_USB_IRQ);
 + enable_irq(LUBBOCK_USB_DISC_IRQ);
 + break;
 + case LUBBOCK_USB_DISC_IRQ:
 + disable_irq(LUBBOCK_USB_DISC_IRQ);
 + enable_irq(LUBBOCK_USB_IRQ);
 + break;
 + default:
 + return IRQ_NONE;
 + }
 +
 + /*
 +  * No need to use workqueue here - we are in a threded handler,
 +  * so we can sleep.
 +  */
What if a new interrupt occurs in here, and preempts this thread.

 + if (otg-gadget)
 + lubbock_vbus_handle(lubbock_vbus);
I think the enable_irq() call should be here. I can't have an ordering problem
at this point, right ?

 + err = devm_request_threaded_irq(pdev-dev, LUBBOCK_USB_DISC_IRQ,
 + NULL, lubbock_vbus_irq, 0, vbus disconnect, pdev);
 + if (err) {
 + dev_err(pdev-dev, can't request irq %i, err: %d\n,
 + LUBBOCK_USB_DISC_IRQ, err);
 + return err;
 + }
 +
 + err = devm_request_threaded_irq(pdev-dev, LUBBOCK_USB_IRQ,
 + NULL, lubbock_vbus_irq, 0, vbus connect, pdev);
 + if (err) {
 + dev_err(pdev-dev, can't request irq %i, err: %d\n,
 + LUBBOCK_USB_IRQ, err);
 + return err;
 + }
Here you have both interrupts enabled, this will mean one interrupt at least
will fire. And of course the other one will be enabled a second time, hence
imbalance.

If you want to have an initial status of disconnected gadget, just enable ti
connect interrupt at probing.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

OK Dmitry, I pulled through, the interrupts are working back.

Actually one of the blockers I have is in pxa25x_udc, and it is also in your
phy-lubbock.c. The bottom of the error is that disable_irq() is called from
within a irq handler, and it deadlocks. A disable_irq_nosync() should be used
...

... but a better approach would be to use a threaded irq for vbus handling. I
think that way disable_irq() can be used, no workqueue is needed anymore in
phy-lubbock.

Would you make that change, I'll test it and review it.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lubbock interrupts fix (was lubbock udc phy)

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 2014-11-27 1:12 GMT+03:00 Robert Jarzmik robert.jarz...@free.fr:
 OK Dmitry, I pulled through, the interrupts are working back.

 What was the problem? Hardware issues? Firmware in CPLD being of old
 revision?
There were basically 2 problems :
 - hardware: switch S13 was in a position that disabled interrupts all the time
   One other problem which fooled me was the incorrect gate schematics I had,
   and which make my understanding of linear function foo such as
   GPIO0 = foo(LUB_IRQ_EN, LUB_SET_CLR) false.

 - software: lubbock.c error
   Since gpio-pxa was ported to device/gpio, it is probed *after*
   lubbock_init_irq() is called. As lubbock_init_irq() installs
   lubbock_irq_handler() and sets the irq to falling edge detect before gpio-pxa
   is probed, gpio-pxa probe overwrites this by :
 - clearing any edge detection (probe state reset)
 - installing generic handle_edge_irq() instead of the lubbock irq handler

So the conclusion is that I have to rework lubbock_init_irq(), remove from it
the PXA_GPIO_TO_IRQ(0) stuff, and move it to a code in lubbock.c which will
provide the guarantee of being executed _after_ gpio-pxa is probed. When I'm
happy with my patch I'll submit it.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: udc: pxa25x: remove unnecessary NULL check

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 @@ -1136,8 +1136,7 @@ static const struct file_operations debug_fops = {
   } while (0)
  #define remove_debug_files(dev) \
   do { \
 - if (dev-debugfs_udc) \
 - debugfs_remove(dev-debugfs_udc); \
 + debugfs_remove(dev-debugfs_udc); \
   } while (0)

Why this simpler form ?
#define remove_debug_files(dev) debugfs_remove(dev-debugfs_udc)

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: udc: pxa25x: remove unnecessary NULL check

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 On Mon, Nov 24, 2014 at 10:45:03PM +0100, Robert Jarzmik wrote:
 Felipe Balbi ba...@ti.com writes:
 
  @@ -1136,8 +1136,7 @@ static const struct file_operations debug_fops = {
 } while (0)
   #define remove_debug_files(dev) \
 do { \
  -  if (dev-debugfs_udc) \
  -  debugfs_remove(dev-debugfs_udc); \
  +  debugfs_remove(dev-debugfs_udc); \
 } while (0)
 
 Why this simpler form ?
 #define remove_debug_files(dev) debugfs_remove(dev-debugfs_udc)

 sure, I'll do that. Do I get a Review-by ?
Of course.

Reviewed-by: Robert Jarzmik robert.jarz...@free.fr

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 2014-11-22 20:49 GMT+03:00 Robert Jarzmik robert.jarz...@free.fr:
 Next point would be (from my POV) to make sure that
 lubbock_unmask_irq() is called
 and works as expected.

Actually the problem is probably within the CPLD.
Irrespective of the CPLD register LUB_IRQ_MASK_EN, the GPIO0 is always low,
indicating an interrupt all the time.

The only thing that changes that is the switch SW13, which forces GPIO0 to high,
but prevents interrupts.

Normally, GPIO0 = (SW13 | ! (LUB_IRQ_MASK_EN  LUB_IRQ_SET_CLR))

This is why I don't have any interrupt, next stage will be to verify U46 and U54
on the IO board for GPIO_INT# and USB_INT# signals ...

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Robert Jarzmik robert.jarz...@free.fr writes:
 Okay, now my ADSL line is repaired, thank you Mr farmer I drive a tractor 
 but I
 don't care about telephone posts, I'll fetch your changes and make a
 test. Let's say I schedule this for saturday.

Removed people from the list for the tests aspect.

Well, my lubbock board seem to raise several interrupts in the linux kernel, all
the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which are the
2 interrupts for the UDC.

That will mean delay I'm afraid, as I have to find out what happens. I'm pretty
sure the hardware is OK, because the blob loader uses the ethernet card which
is amongst the LUBBOCK_IRQs.

There is probably a regression in the kernel not seen for several revisions, and
I have to fix it before I can test.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Hello,

 2014-11-22 20:18 GMT+03:00 Robert Jarzmik robert.jarz...@free.fr:
 Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 2014-11-22 16:56 GMT+03:00 Robert Jarzmik robert.jarz...@free.fr:
 Robert Jarzmik robert.jarz...@free.fr writes:
 Okay, now my ADSL line is repaired, thank you Mr farmer I drive a 
 tractor but I
 don't care about telephone posts, I'll fetch your changes and make a
 test. Let's say I schedule this for saturday.

 Removed people from the list for the tests aspect.

 Well, my lubbock board seem to raise several interrupts in the linux 
 kernel, all
 the LUBBOCK_IRQ interrupts multiplexed on GPIO0 actually, amongst which 
 are the
 2 interrupts for the UDC.
 Arg, what I meant to write was seem to *never* raise several interrupts.

 Understood. Just to make me sure - does the upstream kernel work?
Nope, that's the current situation with the upstream kernel.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: nop: device tree documentation for vbus

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 On Sun, Nov 09, 2014 at 02:02:18PM +0100, Robert Jarzmik wrote:
  hsusb1_phy {
 @@ -26,8 +31,11 @@ Example:
  clock-names = main_clk;
  vcc-supply = hsusb1_vcc_regulator;
  reset-gpios = gpio1 7 GPIO_ACTIVE_LOW;
 +vbus-gpios = gpio2 13 GPIO_ACTIVE_HIGH;
 +vbus-regulator = vbus_regulator;

 not sure why you need vbus-gpios here. You can pass the gpio to the
 regulator as enable-gpio, right ?

Euh no. From my understanding a regulator enable-gpio is an _output_ gpio
enabling the regulator. This vbus-gpio is an _input_ gpio detecting the vbus
assertion. Maybe a name like vbus-detect-gpio would be more accurante ?

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: generic: add vbus support

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 On Sun, Nov 09, 2014 at 03:58:14PM +0100, Robert Jarzmik wrote:
 +if (!enabled) {
 +ret = regulator_enable(vbus_draw);
 +if (ret  0)
 +return;
 +nop-vbus_draw_enabled = 1;

 do you really need this flag here ? How about:

   if (regulator_is_enabled(vbus_draw))
   foo();
Good question, I copy-pasted that code from gpio-vbus...
Philipp, do you have an insight here ?

 +if (vbus) {
 +status = USB_EVENT_VBUS;
 +nop-phy.state = OTG_STATE_B_PERIPHERAL;
 +nop-phy.last_event = status;
 +usb_gadget_vbus_connect(otg-gadget);
 +
 +/* drawing a unit load is *always* OK, except for OTG */
 +nop_set_vbus_draw(nop, 100);

 right, we need to take into considering the Battery Charging
 specification here, though. We might need to setup a timer for the
 amount of time we can draw 100mA before device enumerates. If the times
 expires we're only allowed to draw 8mA or 2mA, or something like that.

 The time gets cancelled as soon as we get enumerated.

 Another possibility would be to move this policy into userland, then
 kernel only provides the interfaces.
Ah ok.
Do you know exactly what the spec is telling in here ?

Is it something such as :
 - upon vbus insertion, a UDC can draw 100mA during ??? ms
   - if during ??? ms, the device is enumerated, it can continue drawing 100mA
   - if not, it should not draw more than 8mA after ??? ms

Is this right ? I can add a timer I think, if the spec if that simple. Now if
there are a lot of special cases then maybe it could be dropped to userland.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: nop: device tree documentation for vbus

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 On Fri, Nov 21, 2014 at 06:17:58PM +0100, Robert Jarzmik wrote:
 Felipe Balbi ba...@ti.com writes:
 
  On Sun, Nov 09, 2014 at 02:02:18PM +0100, Robert Jarzmik wrote:
hsusb1_phy {
  @@ -26,8 +31,11 @@ Example:
clock-names = main_clk;
vcc-supply = hsusb1_vcc_regulator;
reset-gpios = gpio1 7 GPIO_ACTIVE_LOW;
  + vbus-gpios = gpio2 13 GPIO_ACTIVE_HIGH;
  + vbus-regulator = vbus_regulator;
 
  not sure why you need vbus-gpios here. You can pass the gpio to the
  regulator as enable-gpio, right ?
 
 Euh no. From my understanding a regulator enable-gpio is an _output_ gpio
 enabling the regulator. This vbus-gpio is an _input_ gpio detecting the vbus
 assertion. Maybe a name like vbus-detect-gpio would be more accurante ?

 aha, that clears it up, yeah. So the GPIO is detecting if we have VBUS
 from the host side while the regulator supplies VBUS ?
Yup, exactly. I'll amend the name for v2.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 I have sketched a compile-tested only PHY for the Lubbock platform. Could you
 please take a look and test.
   git://git.infradead.org/users/dbaryshkov/zaurus.git lubbock

Okay, now my ADSL line is repaired, thank you Mr farmer I drive a tractor but I
don't care about telephone posts, I'll fetch your changes and make a
test. Let's say I schedule this for saturday.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unprepare().

 Signed-off-by: Dmitry Eremin-Solenikov dbarysh...@gmail.com
 ---
  drivers/usb/gadget/udc/pxa25x_udc.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

 diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c 
 b/drivers/usb/gadget/udc/pxa25x_udc.c
 index 42f7eeb..e4964ee 100644
 --- a/drivers/usb/gadget/udc/pxa25x_udc.c
 +++ b/drivers/usb/gadget/udc/pxa25x_udc.c
 @@ -103,8 +103,8 @@ static const char ep0name [] = ep0;
  
  /* IXP doesn't yet support linux/clk.h */
  #define clk_get(dev,name)NULL
 -#define clk_enable(clk)  do { } while (0)
 -#define clk_disable(clk) do { } while (0)
 +#define clk_prepare_enable(clk)  do { } while (0)
 +#define clk_disable_unprepare(clk)   do { } while (0)
  #define clk_put(clk) do { } while (0)
  
  #endif
 @@ -932,7 +932,7 @@ static int pullup(struct pxa25x_udc *udc)
   if (!udc-active) {
   udc-active = 1;
   /* Enable clock for USB device */
 - clk_enable(udc-clk);
 + clk_prepare_enable(udc-clk);

Guess what, Russell's remark on i2c and serial made me cross-check.  And there
is a case where this will be called in irq context too ...

See :
- do_IRQ
  - lubbock_vbus_irq()
- pxa25x_udc_vbus_session()
  - pullup()
- clk_prepare_enable()
  - CRACK

Note that your patch is not really the faulty one, I think a threaded irq
instead of the raw irq should do the trick. And it is granted on UDC api that
vbus function is called in a sleeping context (Felipe correct me if I'm
wrong), so a patch to fix this before your current code would be fine.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/5] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Hello,
 OK, I will take a look. It seems the correct way would be to strip this code
 away to a phy, like gpio-vbus or nop phys. Do you have access to lubbock
 (or maybe Daniel, Haojian or Russell has?)?
Actually Russell kindly gave me his, so I do have one for testing :)

 Also, as we are at it.  Imre, Krzysztof, IIRC ixp4xx platform does not provide
 clock api either in a form of COMMON_CLK or in a local hacked form
 (like sa1100 does). Do you plan to add any in future, or it just does not
 make sense to support this API for ixp4xx?
I don't have any plans for ixp4xx.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: generic: add vbus support

[Robert Jarzmik]

Robert Jarzmik robert.jarz...@free.fr writes:

 Add support for vbus detection and power supply. This code is more or
 less stolen from phy-gpio-vbus-usb.c, and aims at providing a detection
 mechanism for VBus (ie. usb cable plug) based on a GPIO line, and a
 power supply activation which draws current from the VBus.

Hi Felipe,

Am I still on time for the next merge window ?

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: gadget: pxa25x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unrepapre().
  = clk_disable_unprepare() (typo)

Otherwise, good for me, ack.

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: pxa27x_udc: prepare/unprepare clocks in pxa-ssp

[Robert Jarzmik]

Dmitry Eremin-Solenikov dbarysh...@gmail.com writes:

 Change clk_enable/disable() calls to clk_prepare_enable() and
 clk_disable_unrepapre().

No, there are already patches for this in Felipe's next tree :
git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next

Cheers.

--
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Change internal gpio handling from integer gpios into gpio
descriptors. This change only addresses the internal API and
device-tree/ACPI, while the legacy platform data remains integer space
based.

This change is only build compile tested, and very prone to error. I
leave this comment for now in the commit message so that this patch gets
some testing as I'm pretty sure it's buggy.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c | 58 ---
 drivers/usb/phy/phy-generic.h |  4 +--
 2 files changed, 18 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 7594e50..eba097a 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -61,16 +61,8 @@ static int nop_set_suspend(struct usb_phy *x, int suspend)
 
 static void nop_reset_set(struct usb_phy_generic *nop, int asserted)
 {
-   int value;
-
-   if (!gpio_is_valid(nop-gpio_reset))
-   return;
-
-   value = asserted;
-   if (nop-reset_active_low)
-   value = !value;
-
-   gpio_set_value_cansleep(nop-gpio_reset, value);
+   if (nop-gpiod_reset)
+   gpiod_set_value(nop-gpiod_reset, asserted);
 
if (!asserted)
usleep_range(1, 2);
@@ -145,35 +137,35 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
struct usb_phy_generic_platform_data *pdata)
 {
enum usb_phy_type type = USB_PHY_TYPE_USB2;
-   int err;
+   int err = 0;
 
u32 clk_rate = 0;
bool needs_vcc = false;
 
-   nop-reset_active_low = true;   /* default behaviour */
-
if (dev-of_node) {
struct device_node *node = dev-of_node;
-   enum of_gpio_flags flags = 0;
 
if (of_property_read_u32(node, clock-frequency, clk_rate))
clk_rate = 0;
 
needs_vcc = of_property_read_bool(node, vcc-supply);
-   nop-gpio_reset = of_get_named_gpio_flags(node, reset-gpios,
-   0, flags);
-   if (nop-gpio_reset == -EPROBE_DEFER)
-   return -EPROBE_DEFER;
-
-   nop-reset_active_low = flags  OF_GPIO_ACTIVE_LOW;
-
+   nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
+   err = PTR_ERR(nop-gpiod_reset);
} else if (pdata) {
type = pdata-type;
clk_rate = pdata-clk_rate;
needs_vcc = pdata-needs_vcc;
-   nop-gpio_reset = pdata-gpio_reset;
-   } else {
-   nop-gpio_reset = -1;
+   err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
+   dev_name(dev));
+   if (!err)
+   nop-gpiod_reset = gpio_to_desc(pdata-gpio_reset);
+   }
+
+   if (err == -EPROBE_DEFER)
+   return -EPROBE_DEFER;
+   if (err) {
+   dev_err(dev, Error requesting RESET GPIO\n);
+   return err;
}
 
nop-phy.otg = devm_kzalloc(dev, sizeof(*nop-phy.otg),
@@ -203,24 +195,6 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
return -EPROBE_DEFER;
}
 
-   if (gpio_is_valid(nop-gpio_reset)) {
-   unsigned long gpio_flags;
-
-   /* Assert RESET */
-   if (nop-reset_active_low)
-   gpio_flags = GPIOF_OUT_INIT_LOW;
-   else
-   gpio_flags = GPIOF_OUT_INIT_HIGH;
-
-   err = devm_gpio_request_one(dev, nop-gpio_reset,
-   gpio_flags, dev_name(dev));
-   if (err) {
-   dev_err(dev, Error requesting RESET GPIO %d\n,
-   nop-gpio_reset);
-   return err;
-   }
-   }
-
nop-dev= dev;
nop-phy.dev= nop-dev;
nop-phy.label  = nop-xceiv;
diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h
index d8feacc..09924fd 100644
--- a/drivers/usb/phy/phy-generic.h
+++ b/drivers/usb/phy/phy-generic.h
@@ -2,14 +2,14 @@
 #define _PHY_GENERIC_H_
 
 #include linux/usb/usb_phy_generic.h
+#include linux/gpio/consumer.h
 
 struct usb_phy_generic {
struct usb_phy phy;
struct device *dev;
struct clk *clk;
struct regulator *vcc;
-   int gpio_reset;
-   bool reset_active_low;
+   struct gpio_desc *gpiod_reset;
 };
 
 int usb_gen_phy_init(struct usb_phy *phy);
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] usb: phy: generic: migrate to gpio_desc

[Robert Jarzmik]

Robert Jarzmik robert.jarz...@free.fr writes:

   } else if (pdata) {
   type = pdata-type;
   clk_rate = pdata-clk_rate;
   needs_vcc = pdata-needs_vcc;
 - nop-gpio_reset = pdata-gpio_reset;
 - } else {
 - nop-gpio_reset = -1;


 + err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
 + dev_name(dev));
 + if (!err)
 + nop-gpiod_reset = gpio_to_desc(pdata-gpio_reset);
This 4 lines block should be conditionally encompassed with a :
 if (gpio_is_valid(pdata-gpio_reset))

When I reread our previous conversation the optional part hit me there.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1] usb: phy: nop: device tree documentation for vbus

[Robert Jarzmik]

Enhance the phy documentation by adding 2 new optional bindings :
 - the vbus gpio, which detects usb insertion
 - the vbus regulator, which provides current drawn from the usb cable

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt 
b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
index 1bd37fa..65dfe4b 100644
--- a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
+++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt
@@ -17,6 +17,11 @@ Optional properties:
 
 - reset-gpios: Should specify the GPIO for reset.
 
+- vbus-gpios: should specify the GPIO detecting a VBus insertion
+  (see Documentation/devicetree/bindings/gpio/gpio.txt)
+- vbus-regulator : should specifiy the regulator supplying current drawn from
+  the VBus line (see 
Documentation/devicetree/bindings/regulator/regulator.txt).
+
 Example:
 
hsusb1_phy {
@@ -26,8 +31,11 @@ Example:
clock-names = main_clk;
vcc-supply = hsusb1_vcc_regulator;
reset-gpios = gpio1 7 GPIO_ACTIVE_LOW;
+   vbus-gpios = gpio2 13 GPIO_ACTIVE_HIGH;
+   vbus-regulator = vbus_regulator;
};
 
 hsusb1_phy is a NOP USB PHY device that gets its clock from an oscillator
 and expects that clock to be configured to 19.2MHz by the NOP PHY driver.
 hsusb1_vcc_regulator provides power to the PHY and GPIO 7 controls RESET.
+GPIO 13 detects VBus insertion, and accordingly notifies the vbus-regulator.
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1] usb: phy: generic: add vbus support

[Robert Jarzmik]

Add support for vbus detection and power supply. This code is more or
less stolen from phy-gpio-vbus-usb.c, and aims at providing a detection
mechanism for VBus (ie. usb cable plug) based on a GPIO line, and a
power supply activation which draws current from the VBus.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-generic.c   | 90 -
 drivers/usb/phy/phy-generic.h   |  6 +++
 include/linux/usb/usb_phy_generic.h |  2 +
 3 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 71e061d..2f383a1 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -29,6 +29,7 @@
 #include linux/module.h
 #include linux/platform_device.h
 #include linux/dma-mapping.h
+#include linux/usb/gadget.h
 #include linux/usb/otg.h
 #include linux/usb/usb_phy_generic.h
 #include linux/slab.h
@@ -41,6 +42,9 @@
 
 #include phy-generic.h
 
+#define VBUS_IRQ_FLAGS \
+   (IRQF_SHARED | IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)
+
 struct platform_device *usb_phy_generic_register(void)
 {
return platform_device_register_simple(usb_phy_generic,
@@ -68,6 +72,73 @@ static void nop_reset_set(struct usb_phy_generic *nop, int 
asserted)
usleep_range(1, 2);
 }
 
+/* interface to regulator framework */
+static void nop_set_vbus_draw(struct usb_phy_generic *nop, unsigned mA)
+{
+   struct regulator *vbus_draw = nop-vbus_draw;
+   int enabled;
+   int ret;
+
+   if (!vbus_draw)
+   return;
+
+   enabled = nop-vbus_draw_enabled;
+   if (mA) {
+   regulator_set_current_limit(vbus_draw, 0, 1000 * mA);
+   if (!enabled) {
+   ret = regulator_enable(vbus_draw);
+   if (ret  0)
+   return;
+   nop-vbus_draw_enabled = 1;
+   }
+   } else {
+   if (enabled) {
+   ret = regulator_disable(vbus_draw);
+   if (ret  0)
+   return;
+   nop-vbus_draw_enabled = 0;
+   }
+   }
+   nop-mA = mA;
+}
+
+
+static irqreturn_t nop_gpio_vbus_thread(int irq, void *data)
+{
+   struct usb_phy_generic *nop = data;
+   struct usb_otg *otg = nop-phy.otg;
+   int vbus, status;
+
+   vbus = gpiod_get_value(nop-gpiod_vbus);
+   if ((vbus ^ nop-vbus) == 0)
+   return IRQ_HANDLED;
+   nop-vbus = vbus;
+
+   if (vbus) {
+   status = USB_EVENT_VBUS;
+   nop-phy.state = OTG_STATE_B_PERIPHERAL;
+   nop-phy.last_event = status;
+   usb_gadget_vbus_connect(otg-gadget);
+
+   /* drawing a unit load is *always* OK, except for OTG */
+   nop_set_vbus_draw(nop, 100);
+
+   atomic_notifier_call_chain(nop-phy.notifier, status,
+  otg-gadget);
+   } else {
+   nop_set_vbus_draw(nop, 0);
+
+   usb_gadget_vbus_disconnect(otg-gadget);
+   status = USB_EVENT_NONE;
+   nop-phy.state = OTG_STATE_B_IDLE;
+   nop-phy.last_event = status;
+
+   atomic_notifier_call_chain(nop-phy.notifier, status,
+  otg-gadget);
+   }
+   return IRQ_HANDLED;
+}
+
 int usb_gen_phy_init(struct usb_phy *phy)
 {
struct usb_phy_generic *nop = dev_get_drvdata(phy-dev);
@@ -151,17 +222,22 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
usb_phy_generic *nop,
needs_vcc = of_property_read_bool(node, vcc-supply);
nop-gpiod_reset = devm_gpiod_get(dev, reset-gpios);
err = PTR_ERR(nop-gpiod_reset);
+   if (!err) {
+   nop-gpiod_vbus = devm_gpiod_get(dev, vbus-gpios);
+   err = PTR_ERR(nop-gpiod_vbus);
+   }
} else if (pdata) {
type = pdata-type;
clk_rate = pdata-clk_rate;
needs_vcc = pdata-needs_vcc;
-   if (gpio_is_valid(gpio-gpio_reset)) {
+   if (gpio_is_valid(pdata-gpio_reset)) {
err = devm_gpio_request_one(dev, pdata-gpio_reset, 0,
dev_name(dev));
if (!err)
nop-gpiod_reset =
gpio_to_desc(pdata-gpio_reset);
}
+   nop-gpiod_vbus = pdata-gpiod_vbus;
}
 
if (err == -EPROBE_DEFER)
@@ -226,6 +302,18 @@ static int usb_phy_generic_probe(struct platform_device 
*pdev)
err = usb_phy_gen_create_phy(dev, nop, dev_get_platdata(pdev-dev));
if (err)
return err;
+   if (nop-gpiod_vbus) {
+   err

Re: [PATCH v1 2/3] usb: phy: convert gpio-vbus to gpio_desc

[Robert Jarzmik]

Felipe Balbi ba...@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.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: udc: pxa27x: fix build warning when !OF

[Robert Jarzmik]

Daniel Mack dan...@zonque.org writes:

 On 11/06/2014 05:55 AM, Felipe Balbi wrote:
 in case kernel is built without CONFIG_OF there
 will be a build warning (see below) due to of_match_ptr()
 being defined to NULL.
 
 Because of_match_ptr() is pretty pointless anyway,
 let's just remove it and fix the warning.

 The alternative, of course, would be to wrap the udc_pxa_dt_ids
 declaration in IS_ENABLED(CONFIG_OF), but I'm not sure whether it's
 worth it.

Ah, wasn't the patch named [PATCH v1] usb: gadget: pxa27x_udc: fix warning in
non device-tree build sent October the 29th fixing this issue (in [1]) ?

Cheers.

--
Robert

[1] Patch
From: Robert Jarzmik robert.jarz...@free.fr
Subject: [PATCH v1] usb: gadget: pxa27x_udc: fix warning in non device-tree 
build
To: Felipe Balbi ba...@ti.com
Cc: linux-usb@vger.kernel.org, Robert Jarzmik robert.jarz...@free.fr
Date: Wed, 29 Oct 2014 21:58:33 +0100 (1 week, 22 hours, 56 minutes ago)
Message-Id: 1414616313-8212-1-git-send-email-robert.jarz...@free.fr
X-Mailer: git-send-email 2.1.0

The recent change bringing device-tree support triggers a warning in a
non device-tree build :
  drivers/usb/gadget/udc/pxa27x_udc.c:2405:28: warning: 'udc_pxa_dt_ids'
  defined but not used [-Wunused-variable]

Fix the warning with a preprocessor condition.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/gadget/udc/pxa27x_udc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c 
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 69e7b816..9b03fab 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2400,11 +2400,13 @@ static struct pxa_udc memory = {
}
 };
 
+#if defined(CONFIG_OF)
 static struct of_device_id udc_pxa_dt_ids[] = {
{ .compatible = marvell,pxa270-udc },
{}
 };
 MODULE_DEVICE_TABLE(of, udc_pxa_dt_ids);
+#endif
 
 /**
  * pxa_udc_probe - probes the udc device
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: gadget: udc: pxa27x: fix build warning when !OF

[Robert Jarzmik]

Felipe Balbi ba...@ti.com writes:

 On Thu, Nov 06, 2014 at 04:17:48PM -0600, Felipe Balbi wrote:
 I seem to have missed that, let me replace the patches and use yours.

 now done.

Thanks.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] usb: phy: convert gpio-vbus to gpio_desc

[Robert Jarzmik]

Felipe Balbi ba...@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.jarz...@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.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/3] usb: phy: convert gpio-vbus to gpio_desc

[Robert Jarzmik]

Felipe Balbi ba...@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.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Robert Jarzmik]

Philipp Zabel philipp.za...@gmail.com writes:

 Hi Robert,

 On Sun, Nov 2, 2014 at 7:11 PM, Robert Jarzmik robert.jarz...@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 = 500;
regulator-max-microvolt = 500;
regulator-always-on;
};
vbus-gpio = gpio 13 GPIO_ACTIVE_LOW;
wakeup;
};   

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/3] usb: phy: convert gpio-vbus to gpio_desc

[Robert Jarzmik]

In order to prepare device-tree conversion, port gpio_vbus to use
gpio_desc.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
---
 drivers/usb/phy/phy-gpio-vbus-usb.c | 105 +++-
 1 file changed, 56 insertions(+), 49 deletions(-)

diff --git a/drivers/usb/phy/phy-gpio-vbus-usb.c 
b/drivers/usb/phy/phy-gpio-vbus-usb.c
index f4b14bd..d302ab2 100644
--- a/drivers/usb/phy/phy-gpio-vbus-usb.c
+++ b/drivers/usb/phy/phy-gpio-vbus-usb.c
@@ -10,6 +10,7 @@
 
 #include linux/kernel.h
 #include linux/platform_device.h
+#include linux/gpio/consumer.h
 #include linux/gpio.h
 #include linux/module.h
 #include linux/slab.h
@@ -40,6 +41,8 @@ struct gpio_vbus_data {
struct delayed_work work;
int vbus;
int irq;
+   struct gpio_desc*gpiod_vbus;
+   struct gpio_desc*gpiod_pullup;
 };
 
 
@@ -86,28 +89,21 @@ static void set_vbus_draw(struct gpio_vbus_data *gpio_vbus, 
unsigned mA)
gpio_vbus-mA = mA;
 }
 
-static int is_vbus_powered(struct gpio_vbus_mach_info *pdata)
+static int is_vbus_powered(struct gpio_vbus_data *gpio_vbus)
 {
-   int vbus;
-
-   vbus = gpio_get_value(pdata-gpio_vbus);
-   if (pdata-gpio_vbus_inverted)
-   vbus = !vbus;
-
-   return vbus;
+   return gpiod_get_value(gpio_vbus-gpiod_vbus);
 }
 
 static void gpio_vbus_work(struct work_struct *work)
 {
struct gpio_vbus_data *gpio_vbus =
container_of(work, struct gpio_vbus_data, work.work);
-   struct gpio_vbus_mach_info *pdata = dev_get_platdata(gpio_vbus-dev);
-   int gpio, status, vbus;
+   int status, vbus;
 
if (!gpio_vbus-phy.otg-gadget)
return;
 
-   vbus = is_vbus_powered(pdata);
+   vbus = is_vbus_powered(gpio_vbus);
if ((vbus ^ gpio_vbus-vbus) == 0)
return;
gpio_vbus-vbus = vbus;
@@ -117,8 +113,6 @@ static void gpio_vbus_work(struct work_struct *work)
 * isp1301_omap::b_peripheral() does and enable the pullup here... 
although
 * that may complicate usb_gadget_{,dis}connect() support.
 */
-   gpio = pdata-gpio_pullup;
-
if (vbus) {
status = USB_EVENT_VBUS;
gpio_vbus-phy.state = OTG_STATE_B_PERIPHERAL;
@@ -129,15 +123,15 @@ static void gpio_vbus_work(struct work_struct *work)
set_vbus_draw(gpio_vbus, 100);
 
/* optionally enable D+ pullup */
-   if (gpio_is_valid(gpio))
-   gpio_set_value(gpio, !pdata-gpio_pullup_inverted);
+   if (gpio_vbus-gpiod_pullup)
+   gpiod_set_value(gpio_vbus-gpiod_pullup, 1);
 
atomic_notifier_call_chain(gpio_vbus-phy.notifier,
   status, gpio_vbus-phy.otg-gadget);
} else {
/* optionally disable D+ pullup */
-   if (gpio_is_valid(gpio))
-   gpio_set_value(gpio, pdata-gpio_pullup_inverted);
+   if (gpio_vbus-gpiod_pullup)
+   gpiod_set_value(gpio_vbus-gpiod_pullup, 0);
 
set_vbus_draw(gpio_vbus, 0);
 
@@ -155,12 +149,11 @@ static void gpio_vbus_work(struct work_struct *work)
 static irqreturn_t gpio_vbus_irq(int irq, void *data)
 {
struct platform_device *pdev = data;
-   struct gpio_vbus_mach_info *pdata = dev_get_platdata(pdev-dev);
struct gpio_vbus_data *gpio_vbus = platform_get_drvdata(pdev);
struct usb_otg *otg = gpio_vbus-phy.otg;
 
dev_dbg(pdev-dev, VBUS %s (gadget: %s)\n,
-   is_vbus_powered(pdata) ? supplied : inactive,
+   is_vbus_powered(gpio_vbus) ? supplied : inactive,
otg-gadget ? otg-gadget-name : none);
 
if (otg-gadget)
@@ -176,22 +169,18 @@ static int gpio_vbus_set_peripheral(struct usb_otg *otg,
struct usb_gadget *gadget)
 {
struct gpio_vbus_data *gpio_vbus;
-   struct gpio_vbus_mach_info *pdata;
struct platform_device *pdev;
-   int gpio;
 
gpio_vbus = container_of(otg-phy, struct gpio_vbus_data, phy);
pdev = to_platform_device(gpio_vbus-dev);
-   pdata = dev_get_platdata(gpio_vbus-dev);
-   gpio = pdata-gpio_pullup;
 
if (!gadget) {
dev_dbg(pdev-dev, unregistering gadget '%s'\n,
otg-gadget-name);
 
/* optionally disable D+ pullup */
-   if (gpio_is_valid(gpio))
-   gpio_set_value(gpio, pdata-gpio_pullup_inverted);
+   if (gpio_vbus-gpiod_pullup)
+   gpiod_set_value(gpio_vbus-gpiod_pullup, 0);
 
set_vbus_draw(gpio_vbus, 0);
 
@@ -246,12 +235,29 @@ static int gpio_vbus_probe(struct platform_device *pdev)
struct gpio_vbus_mach_info *pdata = dev_get_platdata(pdev-dev);
struct gpio_vbus_data *gpio_vbus

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

[Robert Jarzmik]

Add documentation for device-tree binding of the generic gpio-vbus phy
controller.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
Cc: devicet...@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 000..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;
+   };
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 3/3] usb: phy: add device-tree support for gpio-vbus

[Robert Jarzmik]

Add device-tree support for the generic gpio-vbus driver.

Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
Cc: devicet...@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);
}
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 },
+   {}
+};
+MODULE_DEVICE_TABLE(of, gpio_vbus_dt_ids);
 MODULE_ALIAS(platform:gpio-vbus);
 
 static struct platform_driver gpio_vbus_driver = {
.driver = {
.name  = gpio-vbus,
.owner = THIS_MODULE,
+   .of_match_table = of_match_ptr(gpio_vbus_dt_ids),
 #ifdef CONFIG_PM
.pm = gpio_vbus_dev_pm_ops,
 #endif
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html