Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751144AbbKVGDu (ORCPT ); Sun, 22 Nov 2015 01:03:50 -0500 Received: from conssluserg001.nifty.com ([202.248.44.39]:23842 "EHLO conssluserg001-v.nifty.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750724AbbKVGDs (ORCPT ); Sun, 22 Nov 2015 01:03:48 -0500 X-Nifty-SrcIP: [209.85.160.175] MIME-Version: 1.0 In-Reply-To: <20151120174509.GQ32672@codeaurora.org> References: <1448004981-11133-1-git-send-email-yamada.masahiro@socionext.com> <1448004981-11133-2-git-send-email-yamada.masahiro@socionext.com> <20151120174509.GQ32672@codeaurora.org> Date: Sun, 22 Nov 2015 15:03:38 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices From: Masahiro Yamada To: Stephen Boyd Cc: linux-clk@vger.kernel.org, Michael Turquette , Linux Kernel Mailing List , Ben Dooks Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3450 Lines: 116 Hi Stephen, 2015-11-21 2:45 GMT+09:00 Stephen Boyd : > 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 >> @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/