Hi Masahiro,
On 18 January 2016 at 19:01, Masahiro Yamada
<yamada.masah...@socionext.com> wrote:
> Hi Simon,
>
>
> 2016-01-15 22:21 GMT+09:00 Simon Glass <s...@chromium.org>:
>> Hi Masahiro,
>>
>> On 14 January 2016 at 03:11, Masahiro Yamada
>> <yamada.masah...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
