Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752469AbbKXAx1 (ORCPT ); Mon, 23 Nov 2015 19:53:27 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:42354 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752296AbbKXAx0 (ORCPT ); Mon, 23 Nov 2015 19:53:26 -0500 Date: Mon, 23 Nov 2015 16:53:24 -0800 From: Stephen Boyd To: Masahiro Yamada Cc: linux-clk@vger.kernel.org, Michael Turquette , Linux Kernel Mailing List , Ben Dooks Subject: Re: [PATCH 2/3] clk: let of_clk_get_parent_name() fail for invalid clock-indices Message-ID: <20151124005324.GM19156@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: 2579 Lines: 99 On 11/22, Masahiro Yamada wrote: > 2015-11-21 2:45 GMT+09:00 Stephen Boyd : > > > > 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. > Ok. Thanks. > > 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. > Ok. Well if we don't want to count them again, perhaps a goto jump over an unconditional return NULL would be better? of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { if (index == pv) { index = count; goto found; } count++; } return NULL; found: Or since the macro for of_property_for_each_u32() tests the vp poitner for NULL, we can check that pointer too... of_property_for_each_u32(clkspec.np, "clock-indices", prop, vp, pv) { if (index == pv) { index = count; break; } count++; } /* We didn't find anything */ if (!vp) return NULL; I guess I prefer the latter approach here. -- 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/