[U-Boot] [PATCH 01/50] dm: clk: Add support for decoding clocks from the device tree

[Masahiro Yamada]

Hi Simon,


2016-01-15 22:21 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 14 January 2016 at 03:11, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>>
>>> @@ -12,6 +12,8 @@
>>>  #include <dm/lists.h>
>>>  #include <dm/root.h>
>>>
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>>  ulong clk_get_rate(struct udevice *dev)
>>>  {
>>>         struct clk_ops *ops = clk_get_ops(dev);
>>> @@ -62,6 +64,32 @@ int clk_get_id(struct udevice *dev, int args_count, uint32_t *args)
>>>         return ops->get_id(dev, args_count, args);
>>>  }
>>>
>>> +int clk_get_by_index(struct udevice *dev, int index, struct udevice **clk_devp,
>>> +                    int *periphp)
>>
>>
>> This function causes NULL pointer access
>> if called with clk_devp == NULL.
>
> Yes, don't do that. Do you think the function comment is unclear?


This would not be a problem in this case,
but I thought NULL-pointer checking is a boilterplate
when we return a pointer filled into a function argument.



>>
>>
>> You can decrease the number of arguments
>> if this function returns periph ID.
>
> Right, but how to handle 0?


>= 0  means peripheral ID.

< 0 means error code.


>>
>>
>>> +{
>>> +       struct fdtdec_phandle_args args;
>>> +       int ret;
>>> +
>>> +       ret = fdtdec_parse_phandle_with_args(gd->fdt_blob, dev->of_offset,
>>> +                                            "clocks", "#clock-cells", 0, index,
>>> +                                            &args);
>>> +       if (ret) {
>>> +               debug("%s: fdtdec_parse_phandle_with_args failed: err=%d\n",
>>> +                     __func__, ret);
>>> +               return ret;
>>> +       }
>>> +
>>> +       ret = uclass_get_device_by_of_offset(UCLASS_CLK, args.node, clk_devp);
>>> +       if (ret) {
>>> +               debug("%s: uclass_get_device_by_of_offset failed: err=%d\n",
>>> +                     __func__, ret);
>>> +               return ret;
>>> +       }
>>> +       *periphp = args.args_count > 0 ? args.args[0] : -1;
>>
>>
>> Do you want to let this function fail against #clock-cells == 0?
>
> At present it doesn't. That's why I didn't have it return the
> peripheral ID. What could we return to signify that the function
> succeeded but there was no peripheral ID?


I think any clock provider should have at least one ID.

If "clock-cells == 0", this clock has ID 0.

Please correct me if I am misunderstanding.




-- 
Best Regards
Masahiro Yamada