Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753334AbbGIBDY (ORCPT ); Wed, 8 Jul 2015 21:03:24 -0400 Received: from galahad.ideasonboard.com ([185.26.127.97]:59384 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751663AbbGIBDM (ORCPT ); Wed, 8 Jul 2015 21:03:12 -0400 From: Laurent Pinchart 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" Subject: Re: [PATCH v5] usb: renesas_usbhs: Allow an OTG PHY driver to provide VBUS Date: Thu, 09 Jul 2015 04:03:23 +0300 Message-ID: <8728955.GfUuMsjafi@avalon> User-Agent: KMail/4.14.8 (Linux/4.0.5-gentoo; KDE/4.14.8; x86_64; ; ) In-Reply-To: References: <3446210.Evs1k3c1oP@avalon> <1989113.aQDiRBvWpG@avalon> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3717 Lines: 108 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 > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/