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

Masahiro Yamada
2016-01-19T02:01:20Z

Hi Simon,

2016-01-15 22:21 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 14 January 2016 at 03:11, Masahiro Yamada
> <yamada.masahiro@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