From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754698AbcFQA54 (ORCPT ); Thu, 16 Jun 2016 20:57:56 -0400 Received: from regular1.263xmail.com ([211.150.99.139]:52108 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754021AbcFQA5x (ORCPT ); Thu, 16 Jun 2016 20:57:53 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: frank.wang@rock-chips.com X-FST-TO: frank.wang@rock-chips.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: frank.wang@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy To: Guenter Roeck References: <1465783810-18756-1-git-send-email-frank.wang@rock-chips.com> <7225720.0AClDW7eQ6@diego> <4065185.1IWsBDlcMJ@diego> <24acf224-e792-6648-fcdb-8729ded6df84@rock-chips.com> <328485b0-2c5d-5d5e-a21b-6a26e487a923@rock-chips.com> <5762A5AE.5030400@roeck-us.net> Cc: =?UTF-8?Q?Heiko_St=c3=bcbner?= , Douglas Anderson , Guenter Roeck , jwerner@chromium.org, kishon@ti.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, Kumar Gala , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-usb@vger.kernel.org, linux-rockchip@lists.infradead.org, Ziyuan Xu , Kever Yang , Tao Huang , william.wu@rock-chips.com, frank.wang@rock-chips.com From: Frank Wang Message-ID: Date: Fri, 17 Jun 2016 08:57:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <5762A5AE.5030400@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Guenter, On 2016/6/16 21:12, Guenter Roeck wrote: > On 06/15/2016 06:47 PM, Frank Wang wrote: >> Hi Guenter & Heiko, >> >> On 2016/6/15 23:47, Guenter Roeck wrote: >>> On Tue, Jun 14, 2016 at 6:14 PM, Frank Wang >>> wrote: >>>> Hi Heiko & Guenter, >>>> >>>> >>>> On 2016/6/14 22:00, Heiko Stübner wrote: >>>>> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck: >>>>>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner >>>>>> wrote: >>>>>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang: >>>>>>>> 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 >>>>>>>> --- >>>>> [...] >>>>> >>>>>>>> +static int rockchip_usb2phy_init(struct phy *phy) >>>>>>>> +{ >>>>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy); >>>>>>>> + struct rockchip_usb2phy *rphy = >>>>>>>> dev_get_drvdata(phy->dev.parent); >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> >>>>>>> if (!rport->port_cfg) >>>>>>> return 0; >>>>>>> >>>>>>> Otherwise the currently empty otg-port will cause null-pointer >>>>>>> dereferences >>>>>>> when it gets assigned in the devicetree already. >>>>>> Not really, at least not here - that port should not have port_id >>>>>> set >>>>>> to USB2PHY_PORT_HOST. >>>>>> >>>>>> Does it even make sense to instantiate the otg port ? Is it going to >>>>>> do anything without port configuration ? >>>>> Ok, that would be the other option - not creating the phy in the >>>>> driver. >>>> >>>> Well, I will put this conditional inside *_host_port_init(), if it >>>> is an >>>> empty, the phy-device should not be created. >>>> Something like the following: >>>> >>>> --- a/drivers/phy/phy-rockchip-inno-usb2.c >>>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c >>>> @@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct >>>> rockchip_usb2phy *rphy, >>>> { >>>> int ret; >>>> >>>> - rport->port_id = USB2PHY_PORT_HOST; >>>> rport->port_cfg = >>>> &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST]; >>>> + if (!rport->port_cfg) { >>>> + dev_err(rphy->dev, "no host port-config provided.\n"); >>>> + return -EINVAL; >>>> + } >>> This would never be NULL. At issue is that you don't assign port_cfg >>> if the port is _not_ a host port. >> >> Sorry, I made a mistake. How about something like the following: >> > Yes, that should work. Just keep in mind that there could always be > a port named "something-port", so you'll always need some kind of check > (and possibly return an error if a port with a wrong name is provided). > > OK, thanks for your reminding, I am going to send out it with a new version later. BR. Frank > >> @@ -574,6 +579,15 @@ static int rockchip_usb2phy_probe(struct >> platform_device *pdev) >> struct rockchip_usb2phy_port *rport = >> &rphy->ports[index]; >> struct phy *phy; >> >> + /* >> + * This driver aim to support both otg-port and >> host-port, >> + * but unfortunately, the otg part is not ready in >> current, >> + * so this comments and below codes are interim, >> which should >> + * be removed after otg-port is supplied soon. >> + */ >> + if (of_node_cmp(child_np->name, "host-port")) >> + goto next_child; >> + >> phy = devm_phy_create(dev, child_np, >> &rockchip_usb2phy_ops); >> if (IS_ERR(phy)) { >> dev_err(dev, "failed to create phy\n"); >> @@ -582,17 +596,13 @@ static int rockchip_usb2phy_probe(struct >> platform_device *pdev) >> } >> >> rport->phy = phy; >> - >> - /* initialize otg/host port separately */ >> - if (!of_node_cmp(child_np->name, "host-port")) { >> - ret = rockchip_usb2phy_host_port_init(rphy, >> rport, >> - child_np); >> - if (ret) >> - goto put_child; >> - } >> - >> phy_set_drvdata(rport->phy, rport); >> >> + ret = rockchip_usb2phy_host_port_init(rphy, rport, >> child_np); >> + if (ret) >> + goto put_child; >> + >> +next_child: >> /* to prevent out of boundary */ >> if (++index >= rphy->phy_cfg->num_ports) >> break; >> >> >> >