Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices

[Masahiro Yamada]

Hi Stephen,


2015-11-21 2:45 GMT+09:00 Stephen Boyd <sboyd@xxxxxxxxxxxxxx>:
> On 11/20, Masahiro Yamada wrote:
>> Currently, of_clk_get_parent_name() returns a wrong parent clock name
>> when "clock-indices" property exists and the given index is not found
>> in the property.  In this case, NULL should be returned.
>>
>> For example,
>>
>>         oscillator {
>>                 compatible = "myclocktype";
>>                 #clock-cells = <1>;
>>                 clock-indices = <1>, <3>;
>>                 clock-output-names = "clka", "clkb";
>>         };
>>
>> Currently, of_clk_get_parent_name(np, 0) returns "clka", but should
>> return NULL because "clock-indices" does not contain <0>.
>
> What is np pointing at? Something like:
>
>         consumer {
>                 clocks = <&oscillator 0>;
>         };
>
> Which would be invalid DT because oscillator doesn't have an
> output for index 0?


You are right.  My example was confusing.



         oscillator: oscillator {
                 compatible = "myclocktype";
                 #clock-cells = <1>;
                 clock-indices = <1>, <3>;
                 clock-output-names = "clka", "clkb";
         };

         consumer {
                 compatible = "myclockconsumer";
                 clocks = <&oscillator 0>;
         };

Currently, of_clk_get_parent_name(consumer_np, 0) returns "clks", but
should return NULL;

I will rephrase the git-log in v2.




>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> @@ -3068,17 +3065,20 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
>>               return NULL;
>>
>>       index = clkspec.args_count ? clkspec.args[0] : 0;
>> -     count = 0;
>>
>>       /* if there is an indices property, use it to transfer the index
>>        * specified into an array offset for the clock-output-names property.
>>        */
>> -     of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
>> -             if (index == pv) {
>> -                     index = count;
>> -                     break;
>> -             }
>> -             count++;
>> +     list = of_get_property(clkspec.np, "clock-indices", &len);
>> +     if (list) {
>> +             len /= sizeof(*list);
>> +             for (i = 0; i < len; i++)
>> +                     if (index == be32_to_cpup(list++)) {
>> +                             index = i;
>> +                             break;
>> +                     }
>> +             if (i == len)
>> +                     return NULL;
>>       }
>
> Why can't we leave everything in place and check count == len at
> the end? i.e.
>
>         of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) {
>                 if (index == pv) {
>                         index = count;
>                         break;
>                 }
>                 count++;
>         }
>
>         if (count == of_property_count_u32_elems(clkspec.np, "clock-indices"))
>                 return NULL
>


Of course we can, although we have to mention "clock-indices" twice.

A good thing for of_get_property() is that we can get both the value
and the length
at the same time.



-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/