| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 02 Jul 2015 11:15:10 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Phil Edworthy <phil.edworthy@...esas.com>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Felipe Balbi <balbi@...com>,
Ulrich Hecht <ulrich.hecht@...il.com>,
Kishon Vijay Abraham I <kishon@...com>,
Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
linux-sh@...r.kernel.org, kuninori.morimoto.gx@...esas.com
Subject: Re: [PATCH v2] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS
Hi Phil,
(CC'ing Morimoto-san)
Thank you for the patch.
On Thursday 02 July 2015 08:36:42 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@...esas.com>
>
> ---
> v2:
> - vbus variables changed from int to bool.
> - dev_info() changed to dev_err()
> ---
> drivers/usb/renesas_usbhs/common.h | 2 ++
> drivers/usb/renesas_usbhs/mod.c | 3 +++
> drivers/usb/renesas_usbhs/mod_gadget.c | 38 +++++++++++++++++++++++++++++++
> 3 files changed, 43 insertions(+)
>
> 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.c
> b/drivers/usb/renesas_usbhs/mod.c index 9a705b1..8a2d3dd 100644
> --- a/drivers/usb/renesas_usbhs/mod.c
> +++ b/drivers/usb/renesas_usbhs/mod.c
> @@ -42,6 +42,9 @@ static int usbhsm_autonomy_get_vbus(struct platform_device
> *pdev) {
> struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
>
> + if (priv->vbus_is_indirect)
> + return priv->vbus_indirect_value;
> +
Something is bothering me. The comment block right before this function states
that "these functions are used if platform doesn't have external phy".
However, the mechanism you implement here is aimed at letting a PHY control
vbus. I have a feeling this isn't the right mechanism.
> return VBSTS & usbhs_read(priv, INTSTS0);
> }
>
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c
> b/drivers/usb/renesas_usbhs/mod_gadget.c index dc2aa32..51b63f9 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,6 +884,8 @@ 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 ||
> @@ -891,6 +895,17 @@ static int usbhsg_gadget_start(struct usb_gadget
> *gadget, /* first hook up the driver ... */
> gpriv->driver = driver;
>
> + /* 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);
Shouldn't gpriv->driver be set to NULL here ? Or possibly better, this code
block moved before setting gpriv->driver ?
> + return ret;
> + }
> + }
> +
> return usbhsg_try_start(priv, USBHSG_STATUS_REGISTERD);
> }
>
> @@ -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))
> + (void) otg_set_peripheral(gpriv->transceiver->otg, NULL);
Is there really a need for (void) ?
> +
> 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;
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists