From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [PATCH v6 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Date: Thu, 16 Jun 2016 21:59:50 -0700 Message-ID: <576383C6.3050005@roeck-us.net> References: <1466129353-48063-1-git-send-email-frank.wang@rock-chips.com> <1466129353-48063-3-git-send-email-frank.wang@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1466129353-48063-3-git-send-email-frank.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Frank Wang , heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org, dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, groeck-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, jwerner-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, kishon-l0cyMroinI0@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org Cc: huangtao-TNX95d0MmH7DzftRWevZcw@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, xzy.xu-TNX95d0MmH7DzftRWevZcw@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org List-Id: devicetree@vger.kernel.org On 06/16/2016 07:09 PM, Frank Wang wrote: > The newer SoCs (rk3366, rk3399) take a different usb-phy IP block > than rk3288 and before, and most of phy-related registers are also > different from the past, so a new phy driver is required necessarily. > > Signed-off-by: Frank Wang > Suggested-by: Guenter Roeck > Suggested-by: Doug Anderson > Reviewed-by: Heiko Stuebner > Tested-by: Heiko Stuebner > --- [ ... ] > + > +static int rockchip_usb2phy_resume(struct phy *phy) > +{ > + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy); > + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent); > + int ret; > + > + dev_dbg(&rport->phy->dev, "port resume\n"); > + > + ret = clk_prepare_enable(rphy->clk480m); > + if (ret) > + return ret; > + If suspend can be called multiple times, resume can be called multiple times as well. Doesn't this cause a clock imbalance if you call clk_prepare_enable() multiple times on resume, but clk_disable_unprepare() only once on suspend ? > + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false); > + if (ret) > + return ret; > + > + rport->suspended = false; > + return 0; > +} > + > +static int rockchip_usb2phy_suspend(struct phy *phy) > +{ > + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy); > + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent); > + int ret; > + > + dev_dbg(&rport->phy->dev, "port suspend\n"); > + > + if (rport->suspended) > + goto exit; > + I know I am nitpicking, but return 0; would be fine here, be more consistent with the rest of the code, > + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true); > + if (ret) > + return ret; > + > + rport->suspended = true; > + clk_disable_unprepare(rphy->clk480m); > + > +exit: > + return 0; and this label is really unnecessary. > +} > + [ ... ]