[U-Boot] [PATCH] dm: clk: Remove simple version of clk_get_by_index()

[Simon Glass]

Hi Michal,

On 15 July 2016 at 00:01, Michal Simek <michal.simek at xilinx.com> wrote:
> On 14.7.2016 20:17, Stephen Warren wrote:
>> On 07/14/2016 05:24 AM, Michal Simek wrote:
>>> Simple version of clk_get_by_index() added by:
>>> "dm: clk: Add a simple version of clk_get_by_index()"
>>> (sha1: a4b10c088c4f6ef2e2bba33e8cfea369bcbbce44)
>>> is not sufficient if you use multiple clocks in the system
>>> because clk->id is phandle id which for example fixed-clock
>>> is not able to handle. Use the same implementation as is used
>>> in full version.
>>
>> It took me a while to work out what failure case you were describing. It
>> might be worth more explicitly pointing out that the existing simple
>> implementation fails in any case where #clock-cells=<0>, or for larger
>> #clock-cells, where the clock ID isn't in the first cell.
>
> TBH I didn't try to understand if this is related to clock-cells size
> because I thought that fixed-clock are tested in mainline and my testing
> platform is ZynqMP where we don't have own clock driver.
> That's why this is not described in commit message.
>
>
>> To be honest, I'd be inclined to always include the real
>> clk_get_by_name() in SPL builds too. If it's never called, the function
>> will be dropped by the linker. If it is called, the dummy implementation
>> probably actively causes failures that we should avoid by using the real
>> implementation. I'm not sure why the original SPL-specific code existed,
>> unless the 773 byte code increase you mention is actually problematic
>> for some specific build?
>
> Agree.
>
> The next part I found is that when I was playing with mmc and serial
> driver that I do call the same clk sequence in both drivers and will be
> good to introduce own function to ensure that the same code is not
> copied in several places which increase just size.
>
> For my testing the sequence is like this (clock should be probably
> return value).
>
> +       int ret;
> +       struct clk clk;
> +       unsigned long clock;
> +
> +       ret = clk_get_by_index(dev, 0, &clk);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to get clock\n");
> +               return ret;
> +       }
> +
> +       clock = clk_get_rate(&clk);
> +       if (IS_ERR_VALUE(clock)) {
> +               dev_err(dev, "failed to get rate\n");
> +               return clock;
> +       }
> +       debug("%s: CLK %ld\n", __func__, clock);
> +
> +       ret = clk_enable(&clk);
> +       if (ret && ret != -ENOSYS) {
> +               dev_err(dev, "failed to enable clock\n");
> +               return ret;
> +       }


Yes I think a helper function in the uclass would be good.

Regards,
Simon