From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CBE17C4828F for ; Fri, 9 Feb 2024 19:02:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:CC:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=cm0kdtnouJcERDX3tuEiG1M5ulQNVfkDXhqFNVnohTc=; b=wQp7skN7hctP3F PL3BPRm8/HOF5xBLZizxXSN858D3Tk8E6QFxdFGSL5LSh70ytR1SAYKrfGQjzZbqJDaqPrHQGn5+7 PjhzT4TsFQdNE+zmkqcFnjfpIYO0H3UeOvfRdovnBF4l5tmMqtAUSSRt7YmqIsJ4iGSnYSR0qmwJS d2dL67OTN3+dhf97rjVp7fllE+th7R/tufOVKnvbTRFk1Y/UNYsEKMFGX7j1ayS/oVhP866S7BwDe hDsxwCVZ9TyHHsp5j2K5UbYFTA8o9PyhkQzVVbVBQIRCOiqWPRKBGhtwvDhoqLTB17bnYL1hJjHiZ lXbm3A4CzPCEHzk9i8BQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rYW90-00000000L91-0KRf; Fri, 09 Feb 2024 19:02:38 +0000 Received: from lelv0143.ext.ti.com ([198.47.23.248]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rYW8w-00000000L6n-2Cy4 for linux-arm-kernel@lists.infradead.org; Fri, 09 Feb 2024 19:02:35 +0000 Received: from lelv0266.itg.ti.com ([10.180.67.225]) by lelv0143.ext.ti.com (8.15.2/8.15.2) with ESMTP id 419J2SYe103057; Fri, 9 Feb 2024 13:02:28 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1707505348; bh=tZ4wb1vqLQzLJ/2I94UyZzE0C21f3pHsPwQpBOLxMMM=; h=Date:From:To:CC:Subject:References:In-Reply-To; b=oOcBYN0dhrskMXoQ3859F78yI7ShYNnYtlXpADGgcsVNj1oCKzMFU08aAt5Y37YmD CceXDGPkMqs33NhBmw6yn1ac5iLLLg1aQnn540LDQWouH9DOptdsizXttsqKSwOTu+ 2EWM2bjgX8ISnhUzqk65Z5ShpPNUus86kYRQ4O/g= Received: from DFLE104.ent.ti.com (dfle104.ent.ti.com [10.64.6.25]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 419J2SfS114492 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 9 Feb 2024 13:02:28 -0600 Received: from DFLE112.ent.ti.com (10.64.6.33) by DFLE104.ent.ti.com (10.64.6.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Fri, 9 Feb 2024 13:02:27 -0600 Received: from lelvsmtp6.itg.ti.com (10.180.75.249) by DFLE112.ent.ti.com (10.64.6.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Fri, 9 Feb 2024 13:02:27 -0600 Received: from localhost (uda0133052.dhcp.ti.com [128.247.81.232]) by lelvsmtp6.itg.ti.com (8.15.2/8.15.2) with ESMTP id 419J2Rr5002005; Fri, 9 Feb 2024 13:02:27 -0600 Date: Fri, 9 Feb 2024 13:02:27 -0600 From: Nishanth Menon To: Kamlesh Gurudasani CC: "Kumar, Udit" , , , , , , , , , , Subject: Re: [PATCH v3] clk: keystone: sci-clk: Adding support for non contiguous clocks Message-ID: <20240209190227.lboi6n5s2oy7kxgc@pristine> References: <20240207091100.4001428-1-u-kumar1@ti.com> <20240207125410.r2q3jcplvif7dvt2@tumbling> <3a7c4a18-5463-414f-82df-39aaed861148@ti.com> <20240209172555.fxlxijhhbgrkyibo@agreeing> <87y1btlb66.fsf@kamlesh.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87y1btlb66.fsf@kamlesh.i-did-not-set--mail-host-address--so-tickle-me> X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240209_110234_692469_302E18A3 X-CRM114-Status: GOOD ( 26.45 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 00:25-20240210, Kamlesh Gurudasani wrote: > >> > > > >> > > diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c > >> > > index 35fe197dd303..31b7df05d7bb 100644 > >> > > --- a/drivers/clk/keystone/sci-clk.c > >> > > +++ b/drivers/clk/keystone/sci-clk.c > >> > > @@ -516,6 +516,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > >> > > struct sci_clk *sci_clk, *prev; > >> > > int num_clks = 0; > >> > > int num_parents; > >> > > [..] /* Check if this clock id is valid */ > >> > > + ret = provider->ops->is_auto(provider->sci, > >> > > + sci_clk->dev_id, ++clk_id, &state); > >> > A bit too nice coding ;) => I had been confused momentarily by clk_id = args.args[1] > >> > change just above till I saw that you are pre-incrementing > >> > clk_id - Is there a harm in leaving the original clk_id increment logic > >> > alone (it was much simpler to read up)? > >> > >> No warm in using original code but want to avoid, two statement for > >> increment in case of failure and success. > >> > >> Let me know, if i need to add few comments around this > >> > >> or if you think, code is confusing I can move to original one > > > > Yes, please drop the un-necessary changes. In this case, original > > increment code should work just fine. > I wouldn't call it unnecessary, If I have to track increment/addition at > 3 different places just to understand the loop, it is hard. On other > hand, pre-increment code is solving the problem by having increment at > only one place(easier to track). On the plus side, every clk_id belonging to > parent is handled completely inside the loop. > > For a new person looking at this code, pre-increment code would be > actually easier to undertsand. > > Also, Udit feels the same. > > Would you please explain why do you think the original increment code > make more sense? It's not simple to understand or track, that's for sure. the context of the fix is the is_auto call to know what parent options are valid or not. Do the absolutely what is necessary in the change. if you want to beautify etc, move it to some other patch and debate about it. So, this is un-necessary change in this patch. -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel