Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162726AbbKTRpO (ORCPT ); Fri, 20 Nov 2015 12:45:14 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:59979 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759694AbbKTRpL (ORCPT ); Fri, 20 Nov 2015 12:45:11 -0500 Date: Fri, 20 Nov 2015 09:45:09 -0800 From: Stephen Boyd To: Masahiro Yamada Cc: linux-clk@vger.kernel.org, Michael Turquette , linux-kernel@vger.kernel.org, Ben Dooks Subject: Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448004981-11133-2-git-send-email-yamada.masahiro@socionext.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2274 Lines: 79 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? > > 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 ? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/