linux-kernel - RE: [PATCH 7/7] usb: phy: registering tegra USB PHY as platform driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D958900912E20642BCBC71664EFECE3E6E5092FFFA@BGMAIL02.nvidia.com>
Date:	Wed, 20 Mar 2013 18:13:07 +0530
From:	Venu Byravarasu <vbyravarasu@...dia.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
	"balbi@...com" <balbi@...com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>
Subject: RE: [PATCH 7/7] usb: phy: registering tegra USB PHY as platform
 driver

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@...dotorg.org]
> Sent: Wednesday, March 20, 2013 1:51 AM
> To: Venu Byravarasu
> Cc: gregkh@...uxfoundation.org; stern@...land.harvard.edu;
> balbi@...com; linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-tegra@...r.kernel.org; devicetree-discuss@...ts.ozlabs.org
> Subject: Re: [PATCH 7/7] usb: phy: registering tegra USB PHY as platform
> driver
> 
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > Registered tegra USB PHY as a separate platform driver.
> >
> > diff --git a/drivers/usb/phy/tegra_usb_phy.c
> b/drivers/usb/phy/tegra_usb_phy.c
> 
> >  static void tegra_usb_phy_close(struct usb_phy *x)
> >  {
> >  	if (phy->is_ulpi_phy)
> >  		clk_put(phy->clk);
> 
> phy->clk is obtained using devm_clk_get(). This typically means you
> never need to clk_put() it, and if for some reason you really have to,
> you should use devm_clk_put() instead of plain clk_put().

Agree, will remove clk_put.
 
> 
> > @@ -774,23 +667,53 @@ struct tegra_usb_phy
> *tegra_usb_phy_open(struct device *dev, int instance,
> 
> > +		err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
> 
> I think you can use devm_gpio_request() here to simplify the error-handling.

Sure, will do. 

> 
> I wonder why in the ULPI case, all the code is inline here, whereas in
> the UTMI case, this simply calls a function. Wouldn't it be more
> consistent to have the following code here:
> 
> 	if (phy->is_ulpi_phy)
> 		err = ulpi_open();
> 	else
> 		err = utmip_open();
> 	if (err)
> 		goto fail;

Sure, will take care of this in next patch. 

> 
> > +static int tegra_usb_phy_probe(struct platform_device *pdev)
> 
> Hmmm. Note that in order to make deferred probe work correctly, all the
> gpio_request(), clk_get(), etc. calls that acquire resources from other
> drivers must happen here in probe() and not in tegra_usb_phy_open().
 
In present code tegra_usb_phy_open() is indirectly called via usb_phy_init() from ehci-tegra.c.
Between obtaining PHY handle (and hence getting into deferred probe, when it is not available)
and calling usb_phy_init, no other USB registers were accessed. Hence I was doing this way.

Do you still think it would be better to move gpio and clk APIs to probe?

> 
> > +	err = of_property_match_string(np, "dr_mode", "otg");
> > +	if (err < 0) {
> > +		err = of_property_match_string(np, "dr_mode", "gadget");
> 
> Again, use "peripheral", not "gadget".

Will do. 

> 
> > +struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
> > +{
> > +	struct device *dev;
> > +	struct tegra_usb_phy *tegra_phy;
> > +
> > +	dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
> > +				 tegra_usb_phy_match);
> > +	if (!dev)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	tegra_phy = dev_get_drvdata(dev);
> > +
> > +	return &tegra_phy->u_phy;
> > +}
> 
> I think you need a module_get() somewhere in there, and also need to add
> a tegra_usb_put_phy() function too, so you can call module_put() from it.

Am not clear on why to add module_get here. Can you plz provide more details?
 
Thanks Stephen, for the review.
--
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/