* Re: [PATCH v3] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-06 8:20 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-06 8:20 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh
Hi Phil,
Thank you for the patch.
On Thursday 02 July 2015 11:26:33 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>
> ---
> v3:
> - Changed how indirect vbus is plumbed in.
> - Removed unnecessary (void) on call to otg_set_peripheral.
> - Moved code that connects to bus through transceiver so it
> is before setting gpriv->driver.
>
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
> ---
> drivers/usb/renesas_usbhs/common.c | 8 +++++--
> drivers/usb/renesas_usbhs/common.h | 2 ++
> drivers/usb/renesas_usbhs/mod_gadget.c | 38 +++++++++++++++++++++++++++++++
> 3 files changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/renesas_usbhs/common.c
> b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
> /*
> * get vbus status from platform
> */
> - enable = usbhs_platform_call(priv, get_vbus, pdev);
> + if (priv->vbus_is_indirect)
> + enable = priv->vbus_indirect_value;
> + else
> + enable = usbhs_platform_call(priv, get_vbus, pdev);
>
> /*
> * get id from platform
> @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev)
> pm_runtime_enable(&pdev->dev);
> if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
> usbhsc_power_ctrl(priv, 1);
> - usbhs_mod_autonomy_mode(priv);
> + if (!priv->vbus_is_indirect)
Isn't this racy ? The vbus_session operation is called by transceivers from
interrupt handlers or work queues. Do we have a guarantee that it will be
called at least once before this code is reached ? Please see below for a
possible way to fix this.
> + usbhs_mod_autonomy_mode(priv);
> }
>
> /*
> diff --git a/drivers/usb/renesas_usbhs/common.h
> b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644
> --- a/drivers/usb/renesas_usbhs/common.h
> +++ b/drivers/usb/renesas_usbhs/common.h
> @@ -255,6 +255,8 @@ struct usbhs_priv {
> struct renesas_usbhs_driver_param dparam;
>
> struct delayed_work notify_hotplug_work;
> + bool vbus_is_indirect;
> + bool vbus_indirect_value;
> struct platform_device *pdev;
>
> struct extcon_dev *edev;
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> #include "common.h"
>
> /*
> @@ -50,6 +51,7 @@ struct usbhsg_gpriv {
> int uep_size;
>
> struct usb_gadget_driver *driver;
> + struct usb_phy *transceiver;
>
> u32 status;
> #define USBHSG_STATUS_STARTED (1 << 0)
> @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
> struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
>
> if (!driver ||
> !driver->setup ||
> driver->max_speed < USB_SPEED_FULL)
> return -EINVAL;
>
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);
> + return ret;
> + }
> + }
> +
How is this change related to the topic ? I'm not too familiar with this
driver so I might just have missed something, but then others could as well,
and the commit message could do with a bit more information.
> /* first hook up the driver ... */
> gpriv->driver = driver;
>
> @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
>
> usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> +
> gpriv->driver = NULL;
>
> return 0;
> @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
> }
>
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + priv->vbus_is_indirect = 1;
vbus_is_indirect is never reset to 0. Shouldn't you set it to 1 (or rather to
true) in usbhs_mod_gadget_probe() if an external transceiver is found ?
What would you think of implementing and wiring up the get_vbus operation in
usbhs_mod_gadget_probe() in the same way that usbhs_mod_autonomy_mode() does,
and return the value of vbus_indirect_value there ? You could then get rid of
the vbus_is_indirect field and move the vbus_indirect_value field to struct
usbhsg_gpriv, removing the need to modify drivers/usb/renesas_usbhs/common.c.
It should also solve the race condition mentioned above.
> + priv->vbus_indirect_value = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops usbhsg_gadget_ops = {
> .get_frame = usbhsg_get_frame,
> .set_selfpowered = usbhsg_set_selfpowered,
> .udc_start = usbhsg_gadget_start,
> .udc_stop = usbhsg_gadget_stop,
> .pullup = usbhsg_pullup,
> + .vbus_session = usbhsg_vbus_session,
> };
>
> static int usbhsg_start(struct usbhs_priv *priv)
> @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
> goto usbhs_mod_gadget_probe_err_gpriv;
> }
>
> + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> + dev_info(dev, "%s transceiver found\n",
> + gpriv->transceiver ? "" : "No");
> +
> /*
> * CAUTION
> *
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v3] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-06 8:20 ` Laurent Pinchart
@ 2015-07-07 9:12 ` Phil Edworthy
-1 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-07 9:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 06 July 2015 09:20, Laurent wrote:
> Hi Phil,
>
> Thank you for the patch.
Thanks for your review!
> On Thursday 02 July 2015 11:26:33 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > ---
> > v3:
> > - Changed how indirect vbus is plumbed in.
> > - Removed unnecessary (void) on call to otg_set_peripheral.
> > - Moved code that connects to bus through transceiver so it
> > is before setting gpriv->driver.
> >
> > v2:
> > - vbus variables changed from int to bool.
> > - dev_info() changed to dev_err()
> > ---
> > drivers/usb/renesas_usbhs/common.c | 8 +++++--
> > drivers/usb/renesas_usbhs/common.h | 2 ++
> > drivers/usb/renesas_usbhs/mod_gadget.c | 38
> +++++++++++++++++++++++++++++++
> > 3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/renesas_usbhs/common.c
> > b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
> > @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
> > /*
> > * get vbus status from platform
> > */
> > - enable = usbhs_platform_call(priv, get_vbus, pdev);
> > + if (priv->vbus_is_indirect)
> > + enable = priv->vbus_indirect_value;
> > + else
> > + enable = usbhs_platform_call(priv, get_vbus, pdev);
> >
> > /*
> > * get id from platform
> > @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev)
> > pm_runtime_enable(&pdev->dev);
> > if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
> > usbhsc_power_ctrl(priv, 1);
> > - usbhs_mod_autonomy_mode(priv);
> > + if (!priv->vbus_is_indirect)
>
> Isn't this racy ? The vbus_session operation is called by transceivers from
> interrupt handlers or work queues. Do we have a guarantee that it will be
> called at least once before this code is reached ? Please see below for a
> possible way to fix this.
You are right, it is racy... :(
> > + usbhs_mod_autonomy_mode(priv);
> > }
> >
> > /*
> > diff --git a/drivers/usb/renesas_usbhs/common.h
> > b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644
> > --- a/drivers/usb/renesas_usbhs/common.h
> > +++ b/drivers/usb/renesas_usbhs/common.h
> > @@ -255,6 +255,8 @@ struct usbhs_priv {
> > struct renesas_usbhs_driver_param dparam;
> >
> > struct delayed_work notify_hotplug_work;
> > + bool vbus_is_indirect;
> > + bool vbus_indirect_value;
> > struct platform_device *pdev;
> >
> > struct extcon_dev *edev;
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -21,6 +21,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/usb/ch9.h>
> > #include <linux/usb/gadget.h>
> > +#include <linux/usb/otg.h>
> > #include "common.h"
> >
> > /*
> > @@ -50,6 +51,7 @@ struct usbhsg_gpriv {
> > int uep_size;
> >
> > struct usb_gadget_driver *driver;
> > + struct usb_phy *transceiver;
> >
> > u32 status;
> > #define USBHSG_STATUS_STARTED (1 << 0)
> > @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, {
> > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct device *dev = usbhs_priv_to_dev(priv);
> > + int ret;
> >
> > if (!driver ||
> > !driver->setup ||
> > driver->max_speed < USB_SPEED_FULL)
> > return -EINVAL;
> >
> > + /* connect to bus through transceiver */
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > + &gpriv->gadget);
> > + if (ret) {
> > + dev_err(dev, "%s: can't bind to transceiver\n",
> > + gpriv->gadget.name);
> > + return ret;
> > + }
> > + }
> > +
>
> How is this change related to the topic ? I'm not too familiar with this
> driver so I might just have missed something, but then others could as well,
> and the commit message could do with a bit more information.
I'm not sure what you mean, this code is needed to tell the phy driver
if it is in host or gadget mode. I would have thought this is clear as it calls
otg_set_peripheral.
> > /* first hook up the driver ... */
> > gpriv->driver = driver;
> >
> > @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> >
> > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> > +
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> > + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> > +
> > gpriv->driver = NULL;
> >
> > return 0;
> > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> > *gadget, int is_self) return 0;
> > }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > + priv->vbus_is_indirect = 1;
>
> vbus_is_indirect is never reset to 0. Shouldn't you set it to 1 (or rather to
> true) in usbhs_mod_gadget_probe() if an external transceiver is found ?
Yes, I see what you say.
> What would you think of implementing and wiring up the get_vbus operation in
> usbhs_mod_gadget_probe() in the same way that
> usbhs_mod_autonomy_mode() does,
> and return the value of vbus_indirect_value there ? You could then get rid of
> the vbus_is_indirect field and move the vbus_indirect_value field to struct
> usbhsg_gpriv, removing the need to modify
> drivers/usb/renesas_usbhs/common.c.
> It should also solve the race condition mentioned above.
Ok, I'll have a look at that.
> > + priv->vbus_indirect_value = !!is_active;
> > +
> > + renesas_usbhs_call_notify_hotplug(pdev);
> > +
> > + return 0;
> > +}
> > +
> > static const struct usb_gadget_ops usbhsg_gadget_ops = {
> > .get_frame = usbhsg_get_frame,
> > .set_selfpowered = usbhsg_set_selfpowered,
> > .udc_start = usbhsg_gadget_start,
> > .udc_stop = usbhsg_gadget_stop,
> > .pullup = usbhsg_pullup,
> > + .vbus_session = usbhsg_vbus_session,
> > };
> >
> > static int usbhsg_start(struct usbhs_priv *priv)
> > @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv
> *priv)
> > goto usbhs_mod_gadget_probe_err_gpriv;
> > }
> >
> > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> > + dev_info(dev, "%s transceiver found\n",
> > + gpriv->transceiver ? "" : "No");
> > +
> > /*
> > * CAUTION
> > *
>
> --
> Regards,
>
> Laurent Pinchart
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v3] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-07 9:12 ` Phil Edworthy
0 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-07 9:12 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 06 July 2015 09:20, Laurent wrote:
> Hi Phil,
>
> Thank you for the patch.
Thanks for your review!
> On Thursday 02 July 2015 11:26:33 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > ---
> > v3:
> > - Changed how indirect vbus is plumbed in.
> > - Removed unnecessary (void) on call to otg_set_peripheral.
> > - Moved code that connects to bus through transceiver so it
> > is before setting gpriv->driver.
> >
> > v2:
> > - vbus variables changed from int to bool.
> > - dev_info() changed to dev_err()
> > ---
> > drivers/usb/renesas_usbhs/common.c | 8 +++++--
> > drivers/usb/renesas_usbhs/common.h | 2 ++
> > drivers/usb/renesas_usbhs/mod_gadget.c | 38
> +++++++++++++++++++++++++++++++
> > 3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/renesas_usbhs/common.c
> > b/drivers/usb/renesas_usbhs/common.c index 0f7e850..15e67d8 100644
> > --- a/drivers/usb/renesas_usbhs/common.c
> > +++ b/drivers/usb/renesas_usbhs/common.c
> > @@ -379,7 +379,10 @@ static void usbhsc_hotplug(struct usbhs_priv *priv)
> > /*
> > * get vbus status from platform
> > */
> > - enable = usbhs_platform_call(priv, get_vbus, pdev);
> > + if (priv->vbus_is_indirect)
> > + enable = priv->vbus_indirect_value;
> > + else
> > + enable = usbhs_platform_call(priv, get_vbus, pdev);
> >
> > /*
> > * get id from platform
> > @@ -662,7 +665,8 @@ static int usbhs_probe(struct platform_device *pdev)
> > pm_runtime_enable(&pdev->dev);
> > if (!usbhsc_flags_has(priv, USBHSF_RUNTIME_PWCTRL)) {
> > usbhsc_power_ctrl(priv, 1);
> > - usbhs_mod_autonomy_mode(priv);
> > + if (!priv->vbus_is_indirect)
>
> Isn't this racy ? The vbus_session operation is called by transceivers from
> interrupt handlers or work queues. Do we have a guarantee that it will be
> called at least once before this code is reached ? Please see below for a
> possible way to fix this.
You are right, it is racy... :(
> > + usbhs_mod_autonomy_mode(priv);
> > }
> >
> > /*
> > diff --git a/drivers/usb/renesas_usbhs/common.h
> > b/drivers/usb/renesas_usbhs/common.h index 8c5fc12..478700c 100644
> > --- a/drivers/usb/renesas_usbhs/common.h
> > +++ b/drivers/usb/renesas_usbhs/common.h
> > @@ -255,6 +255,8 @@ struct usbhs_priv {
> > struct renesas_usbhs_driver_param dparam;
> >
> > struct delayed_work notify_hotplug_work;
> > + bool vbus_is_indirect;
> > + bool vbus_indirect_value;
> > struct platform_device *pdev;
> >
> > struct extcon_dev *edev;
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..93ad4ea 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -21,6 +21,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/usb/ch9.h>
> > #include <linux/usb/gadget.h>
> > +#include <linux/usb/otg.h>
> > #include "common.h"
> >
> > /*
> > @@ -50,6 +51,7 @@ struct usbhsg_gpriv {
> > int uep_size;
> >
> > struct usb_gadget_driver *driver;
> > + struct usb_phy *transceiver;
> >
> > u32 status;
> > #define USBHSG_STATUS_STARTED (1 << 0)
> > @@ -882,12 +884,25 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, {
> > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct device *dev = usbhs_priv_to_dev(priv);
> > + int ret;
> >
> > if (!driver ||
> > !driver->setup ||
> > driver->max_speed < USB_SPEED_FULL)
> > return -EINVAL;
> >
> > + /* connect to bus through transceiver */
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > + &gpriv->gadget);
> > + if (ret) {
> > + dev_err(dev, "%s: can't bind to transceiver\n",
> > + gpriv->gadget.name);
> > + return ret;
> > + }
> > + }
> > +
>
> How is this change related to the topic ? I'm not too familiar with this
> driver so I might just have missed something, but then others could as well,
> and the commit message could do with a bit more information.
I'm not sure what you mean, this code is needed to tell the phy driver
if it is in host or gadget mode. I would have thought this is clear as it calls
otg_set_peripheral.
> > /* first hook up the driver ... */
> > gpriv->driver = driver;
> >
> > @@ -900,6 +915,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> >
> > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> > +
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> > + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> > +
> > gpriv->driver = NULL;
> >
> > return 0;
> > @@ -947,12 +966,27 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> > *gadget, int is_self) return 0;
> > }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > + priv->vbus_is_indirect = 1;
>
> vbus_is_indirect is never reset to 0. Shouldn't you set it to 1 (or rather to
> true) in usbhs_mod_gadget_probe() if an external transceiver is found ?
Yes, I see what you say.
> What would you think of implementing and wiring up the get_vbus operation in
> usbhs_mod_gadget_probe() in the same way that
> usbhs_mod_autonomy_mode() does,
> and return the value of vbus_indirect_value there ? You could then get rid of
> the vbus_is_indirect field and move the vbus_indirect_value field to struct
> usbhsg_gpriv, removing the need to modify
> drivers/usb/renesas_usbhs/common.c.
> It should also solve the race condition mentioned above.
Ok, I'll have a look at that.
> > + priv->vbus_indirect_value = !!is_active;
> > +
> > + renesas_usbhs_call_notify_hotplug(pdev);
> > +
> > + return 0;
> > +}
> > +
> > static const struct usb_gadget_ops usbhsg_gadget_ops = {
> > .get_frame = usbhsg_get_frame,
> > .set_selfpowered = usbhsg_set_selfpowered,
> > .udc_start = usbhsg_gadget_start,
> > .udc_stop = usbhsg_gadget_stop,
> > .pullup = usbhsg_pullup,
> > + .vbus_session = usbhsg_vbus_session,
> > };
> >
> > static int usbhsg_start(struct usbhs_priv *priv)
> > @@ -994,6 +1028,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv
> *priv)
> > goto usbhs_mod_gadget_probe_err_gpriv;
> > }
> >
> > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> > + dev_info(dev, "%s transceiver found\n",
> > + gpriv->transceiver ? "" : "No");
> > +
> > /*
> > * CAUTION
> > *
>
> --
> Regards,
>
> Laurent Pinchart
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-06 8:20 ` Laurent Pinchart
@ 2015-07-07 11:52 ` Phil Edworthy
-1 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-07 11:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh, Phil Edworthy
These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v5:
- Avoid race when vbus_is_indirect may or may not be read
before the phy has called vbus_session. In doing so, the
changes have also been isolated to mod_gadget.c
v4:
- Use true/false with bool vars.
- Clean up "transceiver found" message.
v3:
- Changed how indirect vbus is plumbed in.
- Removed unnecessary (void) on call to otg_set_peripheral.
- Moved code that connects to bus through transceiver so it
is before setting gpriv->driver.
v2:
- vbus variables changed from int to bool.
- dev_info() changed to dev_err()
---
drivers/usb/renesas_usbhs/mod_gadget.c | 62 ++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..19a22a3 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
#include <linux/platform_device.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
#include "common.h"
/*
@@ -50,6 +51,8 @@ struct usbhsg_gpriv {
int uep_size;
struct usb_gadget_driver *driver;
+ struct usb_phy *transceiver;
+ bool vbus_indirect_value;
u32 status;
#define USBHSG_STATUS_STARTED (1 << 0)
@@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32 status)
}
/*
+ * VBUS provided by the PHY
+ */
+static int usbhsm_phy_get_vbus(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
+
+ return gpriv->vbus_indirect_value;
+}
+
+static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
+{
+ struct usbhs_mod_info *info = &priv->mod_info;
+
+ info->irq_vbus = NULL;
+ priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
+
+ usbhs_irq_callback_update(priv, NULL);
+}
+
+/*
*
* linux usb function
*
@@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
{
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct device *dev = usbhs_priv_to_dev(priv);
+ int ret;
if (!driver ||
!driver->setup ||
driver->max_speed < USB_SPEED_FULL)
return -EINVAL;
+ /* connect to bus through transceiver */
+ if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+ ret = otg_set_peripheral(gpriv->transceiver->otg,
+ &gpriv->gadget);
+ if (ret) {
+ dev_err(dev, "%s: can't bind to transceiver\n",
+ gpriv->gadget.name);
+ return ret;
+ }
+
+ /* get vbus using phy versions */
+ usbhs_mod_phy_mode(priv);
+ }
+
/* first hook up the driver ... */
gpriv->driver = driver;
@@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+ if (!IS_ERR_OR_NULL(gpriv->transceiver))
+ otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
return 0;
@@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget *gadget, int is_self)
return 0;
}
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+ struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+ struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+ gpriv->vbus_indirect_value = !!is_active;
+
+ renesas_usbhs_call_notify_hotplug(pdev);
+
+ return 0;
+}
+
static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame = usbhsg_get_frame,
.set_selfpowered = usbhsg_set_selfpowered,
.udc_start = usbhsg_gadget_start,
.udc_stop = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+ .vbus_session = usbhsg_vbus_session,
};
static int usbhsg_start(struct usbhs_priv *priv)
@@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}
+ gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+ dev_info(dev, "%stransceiver found\n",
+ gpriv->transceiver ? "" : "no ");
+
/*
* CAUTION
*
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-07 11:52 ` Phil Edworthy
0 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-07 11:52 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh, Phil Edworthy
These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v5:
- Avoid race when vbus_is_indirect may or may not be read
before the phy has called vbus_session. In doing so, the
changes have also been isolated to mod_gadget.c
v4:
- Use true/false with bool vars.
- Clean up "transceiver found" message.
v3:
- Changed how indirect vbus is plumbed in.
- Removed unnecessary (void) on call to otg_set_peripheral.
- Moved code that connects to bus through transceiver so it
is before setting gpriv->driver.
v2:
- vbus variables changed from int to bool.
- dev_info() changed to dev_err()
---
drivers/usb/renesas_usbhs/mod_gadget.c | 62 ++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..19a22a3 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
#include <linux/platform_device.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
#include "common.h"
/*
@@ -50,6 +51,8 @@ struct usbhsg_gpriv {
int uep_size;
struct usb_gadget_driver *driver;
+ struct usb_phy *transceiver;
+ bool vbus_indirect_value;
u32 status;
#define USBHSG_STATUS_STARTED (1 << 0)
@@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32 status)
}
/*
+ * VBUS provided by the PHY
+ */
+static int usbhsm_phy_get_vbus(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
+
+ return gpriv->vbus_indirect_value;
+}
+
+static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
+{
+ struct usbhs_mod_info *info = &priv->mod_info;
+
+ info->irq_vbus = NULL;
+ priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
+
+ usbhs_irq_callback_update(priv, NULL);
+}
+
+/*
*
* linux usb function
*
@@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
{
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct device *dev = usbhs_priv_to_dev(priv);
+ int ret;
if (!driver ||
!driver->setup ||
driver->max_speed < USB_SPEED_FULL)
return -EINVAL;
+ /* connect to bus through transceiver */
+ if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+ ret = otg_set_peripheral(gpriv->transceiver->otg,
+ &gpriv->gadget);
+ if (ret) {
+ dev_err(dev, "%s: can't bind to transceiver\n",
+ gpriv->gadget.name);
+ return ret;
+ }
+
+ /* get vbus using phy versions */
+ usbhs_mod_phy_mode(priv);
+ }
+
/* first hook up the driver ... */
gpriv->driver = driver;
@@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+ if (!IS_ERR_OR_NULL(gpriv->transceiver))
+ otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
return 0;
@@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget *gadget, int is_self)
return 0;
}
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+ struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+ struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+ gpriv->vbus_indirect_value = !!is_active;
+
+ renesas_usbhs_call_notify_hotplug(pdev);
+
+ return 0;
+}
+
static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame = usbhsg_get_frame,
.set_selfpowered = usbhsg_set_selfpowered,
.udc_start = usbhsg_gadget_start,
.udc_stop = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+ .vbus_session = usbhsg_vbus_session,
};
static int usbhsg_start(struct usbhs_priv *priv)
@@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}
+ gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+ dev_info(dev, "%stransceiver found\n",
+ gpriv->transceiver ? "" : "no ");
+
/*
* CAUTION
*
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-07 11:52 ` Phil Edworthy
@ 2015-07-07 23:08 ` Laurent Pinchart
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-07 23:08 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh
Hi Phil,
Thank you for the patch.
On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
This looks much better to me. I just have two comments, please see below.
> ---
> v5:
> - Avoid race when vbus_is_indirect may or may not be read
> before the phy has called vbus_session. In doing so, the
> changes have also been isolated to mod_gadget.c
>
> v4:
> - Use true/false with bool vars.
> - Clean up "transceiver found" message.
>
> v3:
> - Changed how indirect vbus is plumbed in.
> - Removed unnecessary (void) on call to otg_set_peripheral.
> - Moved code that connects to bus through transceiver so it
> is before setting gpriv->driver.
>
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
> ---
> drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> #include "common.h"
>
> /*
> @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> int uep_size;
>
> struct usb_gadget_driver *driver;
> + struct usb_phy *transceiver;
> + bool vbus_indirect_value;
How about naming this vbus_connected ? A boolean "value" doesn't mean much.
>
> u32 status;
> #define USBHSG_STATUS_STARTED (1 << 0)
> @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> status) }
>
> /*
> + * VBUS provided by the PHY
> + */
> +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> +{
> + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> +
> + return gpriv->vbus_indirect_value;
> +}
> +
> +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> +{
> + struct usbhs_mod_info *info = &priv->mod_info;
> +
> + info->irq_vbus = NULL;
> + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> +
> + usbhs_irq_callback_update(priv, NULL);
> +}
> +
> +/*
> *
> * linux usb function
> *
> @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
> struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
>
> if (!driver ||
> !driver->setup ||
> driver->max_speed < USB_SPEED_FULL)
> return -EINVAL;
>
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);
> + return ret;
> + }
> +
> + /* get vbus using phy versions */
> + usbhs_mod_phy_mode(priv);
Given that the presence of an external PHY is known at probe time, couldn't
this call be moved to the probe function ? It would then probably conflict
with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which would need
to be fixed.
> + }
> +
> /* first hook up the driver ... */
> gpriv->driver = driver;
>
> @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
>
> usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> +
> gpriv->driver = NULL;
>
> return 0;
> @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
> }
>
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + gpriv->vbus_indirect_value = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops usbhsg_gadget_ops = {
> .get_frame = usbhsg_get_frame,
> .set_selfpowered = usbhsg_set_selfpowered,
> .udc_start = usbhsg_gadget_start,
> .udc_stop = usbhsg_gadget_stop,
> .pullup = usbhsg_pullup,
> + .vbus_session = usbhsg_vbus_session,
> };
>
> static int usbhsg_start(struct usbhs_priv *priv)
> @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
> goto usbhs_mod_gadget_probe_err_gpriv;
> }
>
> + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> + dev_info(dev, "%stransceiver found\n",
> + gpriv->transceiver ? "" : "no ");
> +
> /*
> * CAUTION
> *
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-07 23:08 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-07 23:08 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh
Hi Phil,
Thank you for the patch.
On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
This looks much better to me. I just have two comments, please see below.
> ---
> v5:
> - Avoid race when vbus_is_indirect may or may not be read
> before the phy has called vbus_session. In doing so, the
> changes have also been isolated to mod_gadget.c
>
> v4:
> - Use true/false with bool vars.
> - Clean up "transceiver found" message.
>
> v3:
> - Changed how indirect vbus is plumbed in.
> - Removed unnecessary (void) on call to otg_set_peripheral.
> - Moved code that connects to bus through transceiver so it
> is before setting gpriv->driver.
>
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
> ---
> drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> #include "common.h"
>
> /*
> @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> int uep_size;
>
> struct usb_gadget_driver *driver;
> + struct usb_phy *transceiver;
> + bool vbus_indirect_value;
How about naming this vbus_connected ? A boolean "value" doesn't mean much.
>
> u32 status;
> #define USBHSG_STATUS_STARTED (1 << 0)
> @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> status) }
>
> /*
> + * VBUS provided by the PHY
> + */
> +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> +{
> + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> +
> + return gpriv->vbus_indirect_value;
> +}
> +
> +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> +{
> + struct usbhs_mod_info *info = &priv->mod_info;
> +
> + info->irq_vbus = NULL;
> + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> +
> + usbhs_irq_callback_update(priv, NULL);
> +}
> +
> +/*
> *
> * linux usb function
> *
> @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
> struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
>
> if (!driver ||
> !driver->setup ||
> driver->max_speed < USB_SPEED_FULL)
> return -EINVAL;
>
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);
> + return ret;
> + }
> +
> + /* get vbus using phy versions */
> + usbhs_mod_phy_mode(priv);
Given that the presence of an external PHY is known at probe time, couldn't
this call be moved to the probe function ? It would then probably conflict
with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which would need
to be fixed.
> + }
> +
> /* first hook up the driver ... */
> gpriv->driver = driver;
>
> @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
>
> usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> +
> gpriv->driver = NULL;
>
> return 0;
> @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
> }
>
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + gpriv->vbus_indirect_value = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops usbhsg_gadget_ops = {
> .get_frame = usbhsg_get_frame,
> .set_selfpowered = usbhsg_set_selfpowered,
> .udc_start = usbhsg_gadget_start,
> .udc_stop = usbhsg_gadget_stop,
> .pullup = usbhsg_pullup,
> + .vbus_session = usbhsg_vbus_session,
> };
>
> static int usbhsg_start(struct usbhs_priv *priv)
> @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
> goto usbhs_mod_gadget_probe_err_gpriv;
> }
>
> + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> + dev_info(dev, "%stransceiver found\n",
> + gpriv->transceiver ? "" : "no ");
> +
> /*
> * CAUTION
> *
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-07 23:08 ` Laurent Pinchart
@ 2015-07-08 8:08 ` Phil Edworthy
-1 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-08 8:08 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 08 July 2015 00:08, Laurent wrote:
> Hi Phil,
>
> Thank you for the patch.
>
> On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>
> This looks much better to me. I just have two comments, please see below.
>
> > ---
> > v5:
> > - Avoid race when vbus_is_indirect may or may not be read
> > before the phy has called vbus_session. In doing so, the
> > changes have also been isolated to mod_gadget.c
> >
> > v4:
> > - Use true/false with bool vars.
> > - Clean up "transceiver found" message.
> >
> > v3:
> > - Changed how indirect vbus is plumbed in.
> > - Removed unnecessary (void) on call to otg_set_peripheral.
> > - Moved code that connects to bus through transceiver so it
> > is before setting gpriv->driver.
> >
> > v2:
> > - vbus variables changed from int to bool.
> > - dev_info() changed to dev_err()
> > ---
> > drivers/usb/renesas_usbhs/mod_gadget.c | 62
> +++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -21,6 +21,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/usb/ch9.h>
> > #include <linux/usb/gadget.h>
> > +#include <linux/usb/otg.h>
> > #include "common.h"
> >
> > /*
> > @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> > int uep_size;
> >
> > struct usb_gadget_driver *driver;
> > + struct usb_phy *transceiver;
> > + bool vbus_indirect_value;
>
> How about naming this vbus_connected ? A boolean "value" doesn't mean much.
Hmm, vbus_connected isn't right either. This variable directly maps to the vbus
signal, i.e. this variable is true when the signal is high. I'll change it to
vbus_active.
> >
> > u32 status;
> > #define USBHSG_STATUS_STARTED (1 << 0)
> > @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> > status) }
> >
> > /*
> > + * VBUS provided by the PHY
> > + */
> > +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> > +{
> > + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> > + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> > +
> > + return gpriv->vbus_indirect_value;
> > +}
> > +
> > +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> > +{
> > + struct usbhs_mod_info *info = &priv->mod_info;
> > +
> > + info->irq_vbus = NULL;
> > + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> > +
> > + usbhs_irq_callback_update(priv, NULL);
> > +}
> > +
> > +/*
> > *
> > * linux usb function
> > *
> > @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, {
> > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct device *dev = usbhs_priv_to_dev(priv);
> > + int ret;
> >
> > if (!driver ||
> > !driver->setup ||
> > driver->max_speed < USB_SPEED_FULL)
> > return -EINVAL;
> >
> > + /* connect to bus through transceiver */
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > + &gpriv->gadget);
> > + if (ret) {
> > + dev_err(dev, "%s: can't bind to transceiver\n",
> > + gpriv->gadget.name);
> > + return ret;
> > + }
> > +
> > + /* get vbus using phy versions */
> > + usbhs_mod_phy_mode(priv);
>
> Given that the presence of an external PHY is known at probe time, couldn't
> this call be moved to the probe function ? It would then probably conflict
> with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which would
> need to be fixed.
You could do this, but as you say it would conflict with usbhs_probe(). I can't
see a simple way to check for the external phy connection, so I would prefer to
keep it here.
Is that ok?
> > + }
> > +
> > /* first hook up the driver ... */
> > gpriv->driver = driver;
> >
> > @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> >
> > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> > +
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> > + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> > +
> > gpriv->driver = NULL;
> >
> > return 0;
> > @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> > *gadget, int is_self) return 0;
> > }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > + gpriv->vbus_indirect_value = !!is_active;
> > +
> > + renesas_usbhs_call_notify_hotplug(pdev);
> > +
> > + return 0;
> > +}
> > +
> > static const struct usb_gadget_ops usbhsg_gadget_ops = {
> > .get_frame = usbhsg_get_frame,
> > .set_selfpowered = usbhsg_set_selfpowered,
> > .udc_start = usbhsg_gadget_start,
> > .udc_stop = usbhsg_gadget_stop,
> > .pullup = usbhsg_pullup,
> > + .vbus_session = usbhsg_vbus_session,
> > };
> >
> > static int usbhsg_start(struct usbhs_priv *priv)
> > @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv
> *priv)
> > goto usbhs_mod_gadget_probe_err_gpriv;
> > }
> >
> > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> > + dev_info(dev, "%stransceiver found\n",
> > + gpriv->transceiver ? "" : "no ");
> > +
> > /*
> > * CAUTION
> > *
>
> --
> Regards,
>
> Laurent Pinchart
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-08 8:08 ` Phil Edworthy
0 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-08 8:08 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 08 July 2015 00:08, Laurent wrote:
> Hi Phil,
>
> Thank you for the patch.
>
> On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>
> This looks much better to me. I just have two comments, please see below.
>
> > ---
> > v5:
> > - Avoid race when vbus_is_indirect may or may not be read
> > before the phy has called vbus_session. In doing so, the
> > changes have also been isolated to mod_gadget.c
> >
> > v4:
> > - Use true/false with bool vars.
> > - Clean up "transceiver found" message.
> >
> > v3:
> > - Changed how indirect vbus is plumbed in.
> > - Removed unnecessary (void) on call to otg_set_peripheral.
> > - Moved code that connects to bus through transceiver so it
> > is before setting gpriv->driver.
> >
> > v2:
> > - vbus variables changed from int to bool.
> > - dev_info() changed to dev_err()
> > ---
> > drivers/usb/renesas_usbhs/mod_gadget.c | 62
> +++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> > @@ -21,6 +21,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/usb/ch9.h>
> > #include <linux/usb/gadget.h>
> > +#include <linux/usb/otg.h>
> > #include "common.h"
> >
> > /*
> > @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> > int uep_size;
> >
> > struct usb_gadget_driver *driver;
> > + struct usb_phy *transceiver;
> > + bool vbus_indirect_value;
>
> How about naming this vbus_connected ? A boolean "value" doesn't mean much.
Hmm, vbus_connected isn't right either. This variable directly maps to the vbus
signal, i.e. this variable is true when the signal is high. I'll change it to
vbus_active.
> >
> > u32 status;
> > #define USBHSG_STATUS_STARTED (1 << 0)
> > @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> > status) }
> >
> > /*
> > + * VBUS provided by the PHY
> > + */
> > +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> > +{
> > + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> > + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> > +
> > + return gpriv->vbus_indirect_value;
> > +}
> > +
> > +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> > +{
> > + struct usbhs_mod_info *info = &priv->mod_info;
> > +
> > + info->irq_vbus = NULL;
> > + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> > +
> > + usbhs_irq_callback_update(priv, NULL);
> > +}
> > +
> > +/*
> > *
> > * linux usb function
> > *
> > @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> > *gadget, {
> > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct device *dev = usbhs_priv_to_dev(priv);
> > + int ret;
> >
> > if (!driver ||
> > !driver->setup ||
> > driver->max_speed < USB_SPEED_FULL)
> > return -EINVAL;
> >
> > + /* connect to bus through transceiver */
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > + &gpriv->gadget);
> > + if (ret) {
> > + dev_err(dev, "%s: can't bind to transceiver\n",
> > + gpriv->gadget.name);
> > + return ret;
> > + }
> > +
> > + /* get vbus using phy versions */
> > + usbhs_mod_phy_mode(priv);
>
> Given that the presence of an external PHY is known at probe time, couldn't
> this call be moved to the probe function ? It would then probably conflict
> with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which would
> need to be fixed.
You could do this, but as you say it would conflict with usbhs_probe(). I can't
see a simple way to check for the external phy connection, so I would prefer to
keep it here.
Is that ok?
> > + }
> > +
> > /* first hook up the driver ... */
> > gpriv->driver = driver;
> >
> > @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> > *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> >
> > usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> > +
> > + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> > + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> > +
> > gpriv->driver = NULL;
> >
> > return 0;
> > @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> > *gadget, int is_self) return 0;
> > }
> >
> > +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> > +{
> > + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> > +
> > + gpriv->vbus_indirect_value = !!is_active;
> > +
> > + renesas_usbhs_call_notify_hotplug(pdev);
> > +
> > + return 0;
> > +}
> > +
> > static const struct usb_gadget_ops usbhsg_gadget_ops = {
> > .get_frame = usbhsg_get_frame,
> > .set_selfpowered = usbhsg_set_selfpowered,
> > .udc_start = usbhsg_gadget_start,
> > .udc_stop = usbhsg_gadget_stop,
> > .pullup = usbhsg_pullup,
> > + .vbus_session = usbhsg_vbus_session,
> > };
> >
> > static int usbhsg_start(struct usbhs_priv *priv)
> > @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv
> *priv)
> > goto usbhs_mod_gadget_probe_err_gpriv;
> > }
> >
> > + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> > + dev_info(dev, "%stransceiver found\n",
> > + gpriv->transceiver ? "" : "no ");
> > +
> > /*
> > * CAUTION
> > *
>
> --
> Regards,
>
> Laurent Pinchart
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-08 8:08 ` Phil Edworthy
@ 2015-07-09 1:03 ` Laurent Pinchart
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-09 1:03 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Phil,
On Wednesday 08 July 2015 08:08:27 Phil Edworthy wrote:
> On 08 July 2015 00:08, Laurent wrote:
> > On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> > > These changes allow a PHY driver to trigger a VBUS interrupt and
> > > to provide the value of VBUS.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > This looks much better to me. I just have two comments, please see below.
> >
> > > ---
> > >
> > > v5:
> > > - Avoid race when vbus_is_indirect may or may not be read
> > >
> > > before the phy has called vbus_session. In doing so, the
> > > changes have also been isolated to mod_gadget.c
> > >
> > > v4:
> > > - Use true/false with bool vars.
> > > - Clean up "transceiver found" message.
> > >
> > > v3:
> > > - Changed how indirect vbus is plumbed in.
> > > - Removed unnecessary (void) on call to otg_set_peripheral.
> > > - Moved code that connects to bus through transceiver so it
> > >
> > > is before setting gpriv->driver.
> > >
> > > v2:
> > > - vbus variables changed from int to bool.
> > > - dev_info() changed to dev_err()
> > >
> > > ---
> > >
> > > drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
[snip]
> > > @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> > > *gadget,
> > > {
> > > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > > + struct device *dev = usbhs_priv_to_dev(priv);
> > > + int ret;
> > >
> > > if (!driver ||
> > > !driver->setup ||
> > > driver->max_speed < USB_SPEED_FULL)
> > > return -EINVAL;
> > >
> > > + /* connect to bus through transceiver */
> > > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > > + &gpriv->gadget);
> > > + if (ret) {
> > > + dev_err(dev, "%s: can't bind to transceiver\n",
> > > + gpriv->gadget.name);
> > > + return ret;
> > > + }
> > > +
> > > + /* get vbus using phy versions */
> > > + usbhs_mod_phy_mode(priv);
> >
> > Given that the presence of an external PHY is known at probe time,
> > couldn't this call be moved to the probe function ? It would then probably
> > conflict with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which
> > would need to be fixed.
>
> You could do this, but as you say it would conflict with usbhs_probe(). I
> can't see a simple way to check for the external phy connection, so I would
> prefer to keep it here.
> Is that ok?
I can live with that, but I can't help feeling that it's not the best
architecture. Coming to think about it, why do we get the transceiver (by
calling usb_get_phy) in the gadget code ? Isn't the transceiver shared between
host and peripheral modes ? Shouldn't it be handled by core code then ? I
might miss something as my knowledge of the USB subsystem isn't perfect.
If we decide to refactor the code it can be done in a follow-up patch, this
patch has been delayed for long enough.
> > > + }
> > > +
> > >
> > > /* first hook up the driver ... */
> > > gpriv->driver = driver;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-09 1:03 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-09 1:03 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Phil,
On Wednesday 08 July 2015 08:08:27 Phil Edworthy wrote:
> On 08 July 2015 00:08, Laurent wrote:
> > On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> > > These changes allow a PHY driver to trigger a VBUS interrupt and
> > > to provide the value of VBUS.
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > This looks much better to me. I just have two comments, please see below.
> >
> > > ---
> > >
> > > v5:
> > > - Avoid race when vbus_is_indirect may or may not be read
> > >
> > > before the phy has called vbus_session. In doing so, the
> > > changes have also been isolated to mod_gadget.c
> > >
> > > v4:
> > > - Use true/false with bool vars.
> > > - Clean up "transceiver found" message.
> > >
> > > v3:
> > > - Changed how indirect vbus is plumbed in.
> > > - Removed unnecessary (void) on call to otg_set_peripheral.
> > > - Moved code that connects to bus through transceiver so it
> > >
> > > is before setting gpriv->driver.
> > >
> > > v2:
> > > - vbus variables changed from int to bool.
> > > - dev_info() changed to dev_err()
> > >
> > > ---
> > >
> > > drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++
> > > 1 file changed, 62 insertions(+)
> > >
> > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3 100644
> > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
[snip]
> > > @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> > > *gadget,
> > > {
> > > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > > + struct device *dev = usbhs_priv_to_dev(priv);
> > > + int ret;
> > >
> > > if (!driver ||
> > > !driver->setup ||
> > > driver->max_speed < USB_SPEED_FULL)
> > > return -EINVAL;
> > >
> > > + /* connect to bus through transceiver */
> > > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > > + &gpriv->gadget);
> > > + if (ret) {
> > > + dev_err(dev, "%s: can't bind to transceiver\n",
> > > + gpriv->gadget.name);
> > > + return ret;
> > > + }
> > > +
> > > + /* get vbus using phy versions */
> > > + usbhs_mod_phy_mode(priv);
> >
> > Given that the presence of an external PHY is known at probe time,
> > couldn't this call be moved to the probe function ? It would then probably
> > conflict with the usbhs_mod_autonomy_mode() call from usbhs_probe(), which
> > would need to be fixed.
>
> You could do this, but as you say it would conflict with usbhs_probe(). I
> can't see a simple way to check for the external phy connection, so I would
> prefer to keep it here.
> Is that ok?
I can live with that, but I can't help feeling that it's not the best
architecture. Coming to think about it, why do we get the transceiver (by
calling usb_get_phy) in the gadget code ? Isn't the transceiver shared between
host and peripheral modes ? Shouldn't it be handled by core code then ? I
might miss something as my knowledge of the USB subsystem isn't perfect.
If we decide to refactor the code it can be done in a follow-up patch, this
patch has been delayed for long enough.
> > > + }
> > > +
> > >
> > > /* first hook up the driver ... */
> > > gpriv->driver = driver;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-09 1:03 ` Laurent Pinchart
(?)
@ 2015-07-13 15:22 ` Phil Edworthy
2015-07-13 15:30 ` Phil Edworthy
-1 siblings, 1 reply; 34+ messages in thread
From: Phil Edworthy @ 2015-07-13 15:22 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 09 July 2015 02:03, Laurent wrote:
> Hi Phil,
>
> On Wednesday 08 July 2015 08:08:27 Phil Edworthy wrote:
> > On 08 July 2015 00:08, Laurent wrote:
> > > On Tuesday 07 July 2015 12:52:43 Phil Edworthy wrote:
> > > > These changes allow a PHY driver to trigger a VBUS interrupt and
> > > > to provide the value of VBUS.
> > > >
> > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > >
> > > This looks much better to me. I just have two comments, please see below.
> > >
> > > > ---
> > > >
> > > > v5:
> > > > - Avoid race when vbus_is_indirect may or may not be read
> > > >
> > > > before the phy has called vbus_session. In doing so, the
> > > > changes have also been isolated to mod_gadget.c
> > > >
> > > > v4:
> > > > - Use true/false with bool vars.
> > > > - Clean up "transceiver found" message.
> > > >
> > > > v3:
> > > > - Changed how indirect vbus is plumbed in.
> > > > - Removed unnecessary (void) on call to otg_set_peripheral.
> > > > - Moved code that connects to bus through transceiver so it
> > > >
> > > > is before setting gpriv->driver.
> > > >
> > > > v2:
> > > > - vbus variables changed from int to bool.
> > > > - dev_info() changed to dev_err()
> > > >
> > > > ---
> > > >
> > > > drivers/usb/renesas_usbhs/mod_gadget.c | 62
> +++++++++++++++++++++++++++
> > > > 1 file changed, 62 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..19a22a3
> 100644
> > > > --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> > > > +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
>
> [snip]
>
> > > > @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> > > > *gadget,
> > > > {
> > > > struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> > > > struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> > > > + struct device *dev = usbhs_priv_to_dev(priv);
> > > > + int ret;
> > > >
> > > > if (!driver ||
> > > > !driver->setup ||
> > > > driver->max_speed < USB_SPEED_FULL)
> > > > return -EINVAL;
> > > >
> > > > + /* connect to bus through transceiver */
> > > > + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> > > > + ret = otg_set_peripheral(gpriv->transceiver->otg,
> > > > + &gpriv->gadget);
> > > > + if (ret) {
> > > > + dev_err(dev, "%s: can't bind to transceiver\n",
> > > > + gpriv->gadget.name);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + /* get vbus using phy versions */
> > > > + usbhs_mod_phy_mode(priv);
> > >
> > > Given that the presence of an external PHY is known at probe time,
> > > couldn't this call be moved to the probe function ? It would then probably
> > > conflict with the usbhs_mod_autonomy_mode() call from usbhs_probe(),
> which
> > > would need to be fixed.
> >
> > You could do this, but as you say it would conflict with usbhs_probe(). I
> > can't see a simple way to check for the external phy connection, so I would
> > prefer to keep it here.
> > Is that ok?
>
> I can live with that, but I can't help feeling that it's not the best
> architecture. Coming to think about it, why do we get the transceiver (by
> calling usb_get_phy) in the gadget code ? Isn't the transceiver shared between
> host and peripheral modes ? Shouldn't it be handled by core code then ? I
> might miss something as my knowledge of the USB subsystem isn't perfect.
I suspect your knowledge of it is much better than mine!
> If we decide to refactor the code it can be done in a follow-up patch, this
> patch has been delayed for long enough.
Ok, thanks. I'll fix the variable name and re-post.
> > > > + }
> > > > +
> > > >
> > > > /* first hook up the driver ... */
> > > > gpriv->driver = driver;
>
> --
> Regards,
>
> Laurent Pinchart
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread* [PATCH v6] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-13 15:22 ` Phil Edworthy
@ 2015-07-13 15:30 ` Phil Edworthy
0 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-13 15:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh, Phil Edworthy
These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v6:
- Rename vbus_indirect_value to vbus_active
v5:
- Avoid race when vbus_is_indirect may or may not be read
before the phy has called vbus_session. In doing so, the
changes have also been isolated to mod_gadget.c
v4:
- Use true/false with bool vars.
- Clean up "transceiver found" message.
v3:
- Changed how indirect vbus is plumbed in.
- Removed unnecessary (void) on call to otg_set_peripheral.
- Moved code that connects to bus through transceiver so it
is before setting gpriv->driver.
v2:
- vbus variables changed from int to bool.
- dev_info() changed to dev_err()
usb: renesas_usbhs: fixes
---
drivers/usb/renesas_usbhs/mod_gadget.c | 62 ++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..54f916f 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
#include <linux/platform_device.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
#include "common.h"
/*
@@ -50,6 +51,8 @@ struct usbhsg_gpriv {
int uep_size;
struct usb_gadget_driver *driver;
+ struct usb_phy *transceiver;
+ bool vbus_active;
u32 status;
#define USBHSG_STATUS_STARTED (1 << 0)
@@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32 status)
}
/*
+ * VBUS provided by the PHY
+ */
+static int usbhsm_phy_get_vbus(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
+
+ return gpriv->vbus_active;
+}
+
+static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
+{
+ struct usbhs_mod_info *info = &priv->mod_info;
+
+ info->irq_vbus = NULL;
+ priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
+
+ usbhs_irq_callback_update(priv, NULL);
+}
+
+/*
*
* linux usb function
*
@@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
{
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct device *dev = usbhs_priv_to_dev(priv);
+ int ret;
if (!driver ||
!driver->setup ||
driver->max_speed < USB_SPEED_FULL)
return -EINVAL;
+ /* connect to bus through transceiver */
+ if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+ ret = otg_set_peripheral(gpriv->transceiver->otg,
+ &gpriv->gadget);
+ if (ret) {
+ dev_err(dev, "%s: can't bind to transceiver\n",
+ gpriv->gadget.name);
+ return ret;
+ }
+
+ /* get vbus using phy versions */
+ usbhs_mod_phy_mode(priv);
+ }
+
/* first hook up the driver ... */
gpriv->driver = driver;
@@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+ if (!IS_ERR_OR_NULL(gpriv->transceiver))
+ otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
return 0;
@@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget *gadget, int is_self)
return 0;
}
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+ struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+ struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+ gpriv->vbus_active = !!is_active;
+
+ renesas_usbhs_call_notify_hotplug(pdev);
+
+ return 0;
+}
+
static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame = usbhsg_get_frame,
.set_selfpowered = usbhsg_set_selfpowered,
.udc_start = usbhsg_gadget_start,
.udc_stop = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+ .vbus_session = usbhsg_vbus_session,
};
static int usbhsg_start(struct usbhs_priv *priv)
@@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}
+ gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+ dev_info(dev, "%stransceiver found\n",
+ gpriv->transceiver ? "" : "no ");
+
/*
* CAUTION
*
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v6] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-13 15:30 ` Phil Edworthy
0 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-13 15:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh, Phil Edworthy
These changes allow a PHY driver to trigger a VBUS interrupt and
to provide the value of VBUS.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v6:
- Rename vbus_indirect_value to vbus_active
v5:
- Avoid race when vbus_is_indirect may or may not be read
before the phy has called vbus_session. In doing so, the
changes have also been isolated to mod_gadget.c
v4:
- Use true/false with bool vars.
- Clean up "transceiver found" message.
v3:
- Changed how indirect vbus is plumbed in.
- Removed unnecessary (void) on call to otg_set_peripheral.
- Moved code that connects to bus through transceiver so it
is before setting gpriv->driver.
v2:
- vbus variables changed from int to bool.
- dev_info() changed to dev_err()
usb: renesas_usbhs: fixes
---
drivers/usb/renesas_usbhs/mod_gadget.c | 62 ++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index dc2aa32..54f916f 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -21,6 +21,7 @@
#include <linux/platform_device.h>
#include <linux/usb/ch9.h>
#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
#include "common.h"
/*
@@ -50,6 +51,8 @@ struct usbhsg_gpriv {
int uep_size;
struct usb_gadget_driver *driver;
+ struct usb_phy *transceiver;
+ bool vbus_active;
u32 status;
#define USBHSG_STATUS_STARTED (1 << 0)
@@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32 status)
}
/*
+ * VBUS provided by the PHY
+ */
+static int usbhsm_phy_get_vbus(struct platform_device *pdev)
+{
+ struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+ struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
+
+ return gpriv->vbus_active;
+}
+
+static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
+{
+ struct usbhs_mod_info *info = &priv->mod_info;
+
+ info->irq_vbus = NULL;
+ priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
+
+ usbhs_irq_callback_update(priv, NULL);
+}
+
+/*
*
* linux usb function
*
@@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget *gadget,
{
struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct device *dev = usbhs_priv_to_dev(priv);
+ int ret;
if (!driver ||
!driver->setup ||
driver->max_speed < USB_SPEED_FULL)
return -EINVAL;
+ /* connect to bus through transceiver */
+ if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
+ ret = otg_set_peripheral(gpriv->transceiver->otg,
+ &gpriv->gadget);
+ if (ret) {
+ dev_err(dev, "%s: can't bind to transceiver\n",
+ gpriv->gadget.name);
+ return ret;
+ }
+
+ /* get vbus using phy versions */
+ usbhs_mod_phy_mode(priv);
+ }
+
/* first hook up the driver ... */
gpriv->driver = driver;
@@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget *gadget)
struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
+
+ if (!IS_ERR_OR_NULL(gpriv->transceiver))
+ otg_set_peripheral(gpriv->transceiver->otg, NULL);
+
gpriv->driver = NULL;
return 0;
@@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget *gadget, int is_self)
return 0;
}
+static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
+{
+ struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
+ struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
+ struct platform_device *pdev = usbhs_priv_to_pdev(priv);
+
+ gpriv->vbus_active = !!is_active;
+
+ renesas_usbhs_call_notify_hotplug(pdev);
+
+ return 0;
+}
+
static const struct usb_gadget_ops usbhsg_gadget_ops = {
.get_frame = usbhsg_get_frame,
.set_selfpowered = usbhsg_set_selfpowered,
.udc_start = usbhsg_gadget_start,
.udc_stop = usbhsg_gadget_stop,
.pullup = usbhsg_pullup,
+ .vbus_session = usbhsg_vbus_session,
};
static int usbhsg_start(struct usbhs_priv *priv)
@@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
goto usbhs_mod_gadget_probe_err_gpriv;
}
+ gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
+ dev_info(dev, "%stransceiver found\n",
+ gpriv->transceiver ? "" : "no ");
+
/*
* CAUTION
*
--
1.9.1
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v6] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-13 15:30 ` Phil Edworthy
@ 2015-07-13 15:50 ` Laurent Pinchart
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-13 15:50 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh
Hi Phil,
Thank you for the patch.
On Monday 13 July 2015 16:30:18 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
I don't see any further show stopper for this patch, so
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Do you plan to submit a patch to move PHY handling from gadget code to core
code ?
> ---
> v6:
> - Rename vbus_indirect_value to vbus_active
>
> v5:
> - Avoid race when vbus_is_indirect may or may not be read
> before the phy has called vbus_session. In doing so, the
> changes have also been isolated to mod_gadget.c
>
> v4:
> - Use true/false with bool vars.
> - Clean up "transceiver found" message.
>
> v3:
> - Changed how indirect vbus is plumbed in.
> - Removed unnecessary (void) on call to otg_set_peripheral.
> - Moved code that connects to bus through transceiver so it
> is before setting gpriv->driver.
>
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
>
> usb: renesas_usbhs: fixes
> ---
> drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..54f916f 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> #include "common.h"
>
> /*
> @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> int uep_size;
>
> struct usb_gadget_driver *driver;
> + struct usb_phy *transceiver;
> + bool vbus_active;
>
> u32 status;
> #define USBHSG_STATUS_STARTED (1 << 0)
> @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> status) }
>
> /*
> + * VBUS provided by the PHY
> + */
> +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> +{
> + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> +
> + return gpriv->vbus_active;
> +}
> +
> +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> +{
> + struct usbhs_mod_info *info = &priv->mod_info;
> +
> + info->irq_vbus = NULL;
> + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> +
> + usbhs_irq_callback_update(priv, NULL);
> +}
> +
> +/*
> *
> * linux usb function
> *
> @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
> struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
>
> if (!driver ||
> !driver->setup ||
> driver->max_speed < USB_SPEED_FULL)
> return -EINVAL;
>
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);
> + return ret;
> + }
> +
> + /* get vbus using phy versions */
> + usbhs_mod_phy_mode(priv);
> + }
> +
> /* first hook up the driver ... */
> gpriv->driver = driver;
>
> @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
>
> usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> +
> gpriv->driver = NULL;
>
> return 0;
> @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
> }
>
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + gpriv->vbus_active = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops usbhsg_gadget_ops = {
> .get_frame = usbhsg_get_frame,
> .set_selfpowered = usbhsg_set_selfpowered,
> .udc_start = usbhsg_gadget_start,
> .udc_stop = usbhsg_gadget_stop,
> .pullup = usbhsg_pullup,
> + .vbus_session = usbhsg_vbus_session,
> };
>
> static int usbhsg_start(struct usbhs_priv *priv)
> @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
> goto usbhs_mod_gadget_probe_err_gpriv;
> }
>
> + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> + dev_info(dev, "%stransceiver found\n",
> + gpriv->transceiver ? "" : "no ");
> +
> /*
> * CAUTION
> *
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v6] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
@ 2015-07-13 15:50 ` Laurent Pinchart
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Pinchart @ 2015-07-13 15:50 UTC (permalink / raw)
To: Phil Edworthy
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel, linux-usb, linux-sh
Hi Phil,
Thank you for the patch.
On Monday 13 July 2015 16:30:18 Phil Edworthy wrote:
> These changes allow a PHY driver to trigger a VBUS interrupt and
> to provide the value of VBUS.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
I don't see any further show stopper for this patch, so
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Do you plan to submit a patch to move PHY handling from gadget code to core
code ?
> ---
> v6:
> - Rename vbus_indirect_value to vbus_active
>
> v5:
> - Avoid race when vbus_is_indirect may or may not be read
> before the phy has called vbus_session. In doing so, the
> changes have also been isolated to mod_gadget.c
>
> v4:
> - Use true/false with bool vars.
> - Clean up "transceiver found" message.
>
> v3:
> - Changed how indirect vbus is plumbed in.
> - Removed unnecessary (void) on call to otg_set_peripheral.
> - Moved code that connects to bus through transceiver so it
> is before setting gpriv->driver.
>
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
>
> usb: renesas_usbhs: fixes
> ---
> drivers/usb/renesas_usbhs/mod_gadget.c | 62 +++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..54f916f 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -21,6 +21,7 @@
> #include <linux/platform_device.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> #include "common.h"
>
> /*
> @@ -50,6 +51,8 @@ struct usbhsg_gpriv {
> int uep_size;
>
> struct usb_gadget_driver *driver;
> + struct usb_phy *transceiver;
> + bool vbus_active;
>
> u32 status;
> #define USBHSG_STATUS_STARTED (1 << 0)
> @@ -873,6 +876,27 @@ static int usbhsg_try_stop(struct usbhs_priv *priv, u32
> status) }
>
> /*
> + * VBUS provided by the PHY
> + */
> +static int usbhsm_phy_get_vbus(struct platform_device *pdev)
> +{
> + struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> + struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
> +
> + return gpriv->vbus_active;
> +}
> +
> +static void usbhs_mod_phy_mode(struct usbhs_priv *priv)
> +{
> + struct usbhs_mod_info *info = &priv->mod_info;
> +
> + info->irq_vbus = NULL;
> + priv->pfunc.get_vbus = usbhsm_phy_get_vbus;
> +
> + usbhs_irq_callback_update(priv, NULL);
> +}
> +
> +/*
> *
> * linux usb function
> *
> @@ -882,12 +906,28 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, {
> struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct device *dev = usbhs_priv_to_dev(priv);
> + int ret;
>
> if (!driver ||
> !driver->setup ||
> driver->max_speed < USB_SPEED_FULL)
> return -EINVAL;
>
> + /* connect to bus through transceiver */
> + if (!IS_ERR_OR_NULL(gpriv->transceiver)) {
> + ret = otg_set_peripheral(gpriv->transceiver->otg,
> + &gpriv->gadget);
> + if (ret) {
> + dev_err(dev, "%s: can't bind to transceiver\n",
> + gpriv->gadget.name);
> + return ret;
> + }
> +
> + /* get vbus using phy versions */
> + usbhs_mod_phy_mode(priv);
> + }
> +
> /* first hook up the driver ... */
> gpriv->driver = driver;
>
> @@ -900,6 +940,10 @@ static int usbhsg_gadget_stop(struct usb_gadget
> *gadget) struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
>
> usbhsg_try_stop(priv, USBHSG_STATUS_REGISTERD);
> +
> + if (!IS_ERR_OR_NULL(gpriv->transceiver))
> + otg_set_peripheral(gpriv->transceiver->otg, NULL);
> +
> gpriv->driver = NULL;
>
> return 0;
> @@ -947,12 +991,26 @@ static int usbhsg_set_selfpowered(struct usb_gadget
> *gadget, int is_self) return 0;
> }
>
> +static int usbhsg_vbus_session(struct usb_gadget *gadget, int is_active)
> +{
> + struct usbhsg_gpriv *gpriv = usbhsg_gadget_to_gpriv(gadget);
> + struct usbhs_priv *priv = usbhsg_gpriv_to_priv(gpriv);
> + struct platform_device *pdev = usbhs_priv_to_pdev(priv);
> +
> + gpriv->vbus_active = !!is_active;
> +
> + renesas_usbhs_call_notify_hotplug(pdev);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops usbhsg_gadget_ops = {
> .get_frame = usbhsg_get_frame,
> .set_selfpowered = usbhsg_set_selfpowered,
> .udc_start = usbhsg_gadget_start,
> .udc_stop = usbhsg_gadget_stop,
> .pullup = usbhsg_pullup,
> + .vbus_session = usbhsg_vbus_session,
> };
>
> static int usbhsg_start(struct usbhs_priv *priv)
> @@ -994,6 +1052,10 @@ int usbhs_mod_gadget_probe(struct usbhs_priv *priv)
> goto usbhs_mod_gadget_probe_err_gpriv;
> }
>
> + gpriv->transceiver = usb_get_phy(USB_PHY_TYPE_UNDEFINED);
> + dev_info(dev, "%stransceiver found\n",
> + gpriv->transceiver ? "" : "no ");
> +
> /*
> * CAUTION
> *
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 34+ messages in thread* RE: [PATCH v6] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
2015-07-13 15:50 ` Laurent Pinchart
(?)
@ 2015-07-13 16:15 ` Phil Edworthy
-1 siblings, 0 replies; 34+ messages in thread
From: Phil Edworthy @ 2015-07-13 16:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Yoshihiro Shimoda, Kuninori Morimoto, Greg Kroah-Hartman,
Felipe Balbi, Ulrich Hecht, Kishon Vijay Abraham I,
Sergei Shtylyov, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org, linux-sh@vger.kernel.org
Hi Laurent,
On 13 July 2015 16:51, Laurent wrote:
> Hi Phil,
>
> Thank you for the patch.
>
> On Monday 13 July 2015 16:30:18 Phil Edworthy wrote:
> > These changes allow a PHY driver to trigger a VBUS interrupt and
> > to provide the value of VBUS.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
>
> I don't see any further show stopper for this patch, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Do you plan to submit a patch to move PHY handling from gadget code to core
> code ?
Not at the moment... I am concentrating on the usb PHY driver and max3355
extcon driver.
<snip>
Thanks
Phil
^ permalink raw reply [flat|nested] 34+ messages in thread