Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934106AbdGLIi0 (ORCPT ); Wed, 12 Jul 2017 04:38:26 -0400 Received: from lucky1.263xmail.com ([211.157.147.132]:54935 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933960AbdGLIiX (ORCPT ); Wed, 12 Jul 2017 04:38:23 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: david.wu@rock-chips.com X-FST-TO: linux-kernel@vger.kernel.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: david.wu@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support To: Doug Anderson Cc: Thierry Reding , =?UTF-8?Q?Heiko_St=c3=bcbner?= , Boris Brezillon , Rob Herring , Catalin Marinas , Brian Norris , Mark Rutland , =?UTF-8?B?6buE5rab?= , linux-pwm , "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <1499486629-9659-1-git-send-email-david.wu@rock-chips.com> <1499486629-9659-2-git-send-email-david.wu@rock-chips.com> From: "David.Wu" Message-ID: Date: Wed, 12 Jul 2017 16:38:09 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: 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 Content-Length: 2840 Lines: 78 Hi Doug, 在 2017/7/12 1:03, Doug Anderson 写道: > Hi, > > On Fri, Jul 7, 2017 at 9:03 PM, David Wu wrote: > >> @@ -6,7 +6,13 @@ Required properties: >> "rockchip,rk3288-pwm": found on RK3288 SoC >> "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC >> - reg: physical base address and length of the controller's registers >> - - clocks: phandle and clock specifier of the PWM reference clock >> + - clocks: See ../clock/clock-bindings.txt >> + - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399): >> + - There is one clock that's used both to derive the functional clock >> + for the device and as the bus clock. >> + - For newer hardware (rk3328 and future socs): specified by name >> + - "pwm": This is used to derive the functional clock. >> + - "pclk": This is the APB bus clock. > > I'm pretty sure that that the above description doesn't quite match the code. > > * The above description says that for old hardware there is one clock > and 'clock-names' was not necessary (though as I understand it it's OK > if it's there). > > * The old code matched the old description. AKA: if there was no > "clock-names" then everything was OK. > > * The new code will not work if there was no "clock-names". > > Many of the old devices had a clock-names present (and it was "pwm"), > but not all. Specifically it looks like > "arch/arm/boot/dts/rk3xxx.dtsi" doesn't specify a clock-names. > So we can keep code: the pc->clk = devm_clk_get(&pdev->dev, NULL); If the name is NULL, we can get the first clk defined at DTB. >> @@ -343,13 +344,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev) >> if (IS_ERR(pc->base)) >> return PTR_ERR(pc->base); >> >> - pc->clk = devm_clk_get(&pdev->dev, NULL); >> - if (IS_ERR(pc->clk)) >> - return PTR_ERR(pc->clk); >> + pc->clk = devm_clk_get(&pdev->dev, "pwm"); >> + count = of_property_count_strings(pdev->dev.of_node, "clock-names"); >> + if (count == 2) >> + pc->pclk = devm_clk_get(&pdev->dev, "pclk"); >> + else >> + pc->pclk = pc->clk; >> + >> + if (IS_ERR(pc->clk)) { >> + ret = PTR_ERR(pc->clk); >> + if (ret != -EPROBE_DEFER) >> + dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret); >> + return ret; >> + } >> + >> + if (IS_ERR(pc->pclk)) { >> + ret = PTR_ERR(pc->pclk); >> + if (ret != -EPROBE_DEFER) >> + dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret); >> + return ret; >> + } > > In the above code you need to check the count _before_ trying to get > the clock by name. Okay, I will fix it. > > > -Doug > > >