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

[Simon Glass]

Hi Masahiro,

On 18 January 2016 at 19:01, Masahiro Yamada
<yamada.masahiro at socionext.com> wrote:
> 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.

That sounds fine to me. I've sent an update to that one patch.

Regards,
Simon