linux-kernel - Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

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: <CAD=FV=Vqkueo_cqU8bJXXT3pV5d1dZR5LF+c=O-Ka1qR-Ff-WQ@mail.gmail.com>
Date:   Tue, 11 Jul 2017 10:03:34 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     David Wu <david.wu@...k-chips.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Heiko Stübner <heiko@...ech.de>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Rob Herring <robh+dt@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Brian Norris <briannorris@...omium.org>,
        Mark Rutland <mark.rutland@....com>,
        黄涛 <huangtao@...k-chips.com>,
        linux-pwm <linux-pwm@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

Hi,

On Fri, Jul 7, 2017 at 9:03 PM, David Wu <david.wu@...k-chips.com> 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