Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934121AbdGKRDv (ORCPT ); Tue, 11 Jul 2017 13:03:51 -0400 Received: from mail-wr0-f182.google.com ([209.85.128.182]:33692 "EHLO mail-wr0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933338AbdGKRDg (ORCPT ); Tue, 11 Jul 2017 13:03:36 -0400 MIME-Version: 1.0 In-Reply-To: <1499486629-9659-2-git-send-email-david.wu@rock-chips.com> References: <1499486629-9659-1-git-send-email-david.wu@rock-chips.com> <1499486629-9659-2-git-send-email-david.wu@rock-chips.com> From: Doug Anderson Date: Tue, 11 Jul 2017 10:03:34 -0700 X-Google-Sender-Auth: Anj1YGAHwcW09Rkz95jMusrDArY Message-ID: Subject: Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support To: David Wu 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" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2529 Lines: 65 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. > @@ -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. -Doug