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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 49BA0C433EF for ; Tue, 12 Oct 2021 21:21:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 29DB6610CB for ; Tue, 12 Oct 2021 21:21:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234191AbhJLVXe (ORCPT ); Tue, 12 Oct 2021 17:23:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42184 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233635AbhJLVXd (ORCPT ); Tue, 12 Oct 2021 17:23:33 -0400 Received: from mail-ed1-x533.google.com (mail-ed1-x533.google.com [IPv6:2a00:1450:4864:20::533]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 67970C061570; Tue, 12 Oct 2021 14:21:31 -0700 (PDT) Received: by mail-ed1-x533.google.com with SMTP id r18so1347236edv.12; Tue, 12 Oct 2021 14:21:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gOFNBJr+5UGYcXXM52CBBPVCPAsy4cPdDGbGieRT3Pw=; b=TjyrNYrAJjITZgQqDIhNBw1waTJz03vnWMl+Y/NcIBBjvX+/eQQKKsaNJr5LF9vE/m U7HF99K3buQSlliPH7Y57gW8fcassAWYG8KLcminqJ3HpgDIA4QOzM1ttT4Toam8i+zj loUEwm7f4uGNYNP+lEiX76DKTTWoRBqMX6MQtDuOpXW5WdtCCRfhhZbqwC9bVl/Jx137 6zn0fRNrLwrgXBQPfibscSDKJ0TyA2u1+2N4tdOIJTICaaYl9RtiuyGYH01Vxh3gKJJe sbjeY31IlJkRviou4RLSupw9CNHNivQ+REiV0LHNpTHNQiiTBtoh9FtRbcU6L9o6M1md S3NQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gOFNBJr+5UGYcXXM52CBBPVCPAsy4cPdDGbGieRT3Pw=; b=aDRHj+pxJ3CklbQzyOnYKQLXev7NIxyZOegulFp+sT3Lry5/PdYo9GWtGiPN/POiza YcZmnnC3QgSYSbvfY5CmZfszCJLjMBAfc8b7qyXfijhn2h6zTFXqmH9MtHWfIegkFD0A QokH56p6mMZWLMmDh+rv8ZxOCxHZn9NTOiUv9IZPMLKuZd+XzMqkv2MRq+f2GmkxGoSR amEus9Yx3wZENFNQ6OfLjrYv9Mfr0G+XdxRYnWTaifIcz/BscvVKF9OFU2RxOXELp2K7 7PvQTWimZqN6KzDVN9UoJD3vsBGFQtcmOSb65uevFk2/+ZAA1TB5jXzrfb313Uct3b1G ebSg== X-Gm-Message-State: AOAM5302Zj3GgQ5/XRgRLQrKYUvilzw4vAUGHIASwmYoAIeMZapHGjBw DAU7nPTKANKEnzQqK5weQT2eK0OzfyTZGOfcio0= X-Google-Smtp-Source: ABdhPJwEOXZ3Sz3Z+75oIHMBoXtzmDYFRay0T/yAdFIEl1UtVqX3R3fR7pveB81mNdmIUdJ/kDbGrP2U5ErDIePd6pc= X-Received: by 2002:a17:906:5a47:: with SMTP id my7mr35357939ejc.128.1634073689974; Tue, 12 Oct 2021 14:21:29 -0700 (PDT) MIME-Version: 1.0 References: <20211012134027.684712-1-kernel@esmil.dk> <20211012134027.684712-7-kernel@esmil.dk> In-Reply-To: From: Andy Shevchenko Date: Wed, 13 Oct 2021 00:20:53 +0300 Message-ID: Subject: Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver To: Emil Renner Berthing Cc: linux-riscv , devicetree , linux-clk , "open list:GPIO SUBSYSTEM" , "open list:SERIAL DRIVERS" , Geert Uytterhoeven , Palmer Dabbelt , Paul Walmsley , Rob Herring , Michael Turquette , Stephen Boyd , Thomas Gleixner , Marc Zyngier , Philipp Zabel , Linus Walleij , Greg Kroah-Hartman , Daniel Lezcano , Andy Shevchenko , Jiri Slaby , Maximilian Luz , Sagar Kadam , Drew Fustini , Anup Patel , Atish Patra , Matteo Croce , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Tue, Oct 12, 2021 at 11:08 PM Emil Renner Berthing wrote: > On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko wrote: > > On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing wrote: ... > > > + value |= readl_relaxed(reg) & ~mask; > > > > value is not masked, is it okay? > > > > Usual pattern for this kind of operations is > > > > value = (current & ~mask) | (value & mask); > > This function is only ever called with constants, already masked > values or the parent number from the clk framework, so it should be > ok. Up to you, but I think it's better to have a usual pattern. > > > + writel_relaxed(value, reg); ... > > > + rate = parent / div; > > > + if (rate < req->min_rate && div > 1) { > > > + div -= 1; > > > + rate = parent / div; > > > + } > > > > Seems like homegrown DIV_ROUND_UP() or so. Who will guarantee that > > decreasing div by 1 will satisfy the conditional again? > > Maths unless I'm mistaken: div = DIV_ROUND_UP(parent, target), so in > rational numbers > div - 1 < parent / target > But the target is clamped by min_rate and max_rate, so > min_rate <= target < parent / (div - 1) = rate > > Sorry, re-using the rate varable for both the target and result is > confusing. I'll fix that. Also needs a comment, I believe. ... > > > +#ifdef CONFIG_DEBUG_FS > > > > Perhaps __maybe_unused? > > I can definitely use __maybe_unused for the function declaration, but > then I'll need a conditional every time clk_ops.debug_init needs to be > initialized to either the function or NULL depending on > CONFIG_DEBUG_FS below. Is that better? Actually, why can't you always initialize the field? Shouldn't CLK core take care about this conditional? > > > +#else > > > +#define jh7100_clk_debug_init NULL > > > +#endif ... > > > + while (idx > 0) > > > + clk_hw_unregister(&priv->reg[--idx].hw); > > > > The > > > > while (idx--) > > clk_hw_unregister(&priv->reg[idx].hw); > > > > is slightly better to read. > > It's not something I'll insist hard on, but I must admit I disagree. > To me the above looks like cartoon characters running off a cliff and > back. As a middle ground could we maybe do this? > > while (idx) > clk_hw_unregister(&priv->reg[--idx].hw); My point is exactly in having the common pattern for error paths, i.e. while (counter--) ...bla-bla-bla... Your second approach is better, but I think that proposed by me is even better. ... > > > +subsys_initcall(clk_starfive_jh7100_init); > > > > Any explanation why subsys_initcall() is in use? > > TBH I just inherited that from Geert's first mock driver and never > thought to question it. What would be a better alternative to try? At least add a comment to explain the choice. -- With Best Regards, Andy Shevchenko 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85FB9C433F5 for ; Tue, 12 Oct 2021 21:21:58 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 42DE8610CC for ; Tue, 12 Oct 2021 21:21:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 42DE8610CC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org 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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=xI0Dkrxzrw3Vfx+lX3BI+Tpu/ZKneg9UmYtghdu9ASA=; b=sG4GfUVdzhJeui RtgM2BO4UWcr//iSBO25/t8pl2I7/6kAgiUJ6e4abbtQcZWGmQhxLKkTbjdFywM/nb9piP5zeJg/Q +JyC9ovB+zbXorU5U9nPWEuhX3ARH+JFzb3hqDOib9ALn05zugAqQIpPJV5kPe4r655e/gATQRXuo PBGy7r7odki+DNal4dSyEA/HL8n/lBoEMUeojqPV/KhBZSgd++6hKw6qbum5v0Et4OH3xHVH6WZIr ZBQdcUjK0Y4Y4q4Bxe+k+7WsVRkSF3w5ukDtR4eKZuGyqRfvxDCYEXakEXta7dUbpQlVQdxiULdzD fd8pgW9FumSSAa7DBQwg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1maPDK-00E0xW-Ge; Tue, 12 Oct 2021 21:21:34 +0000 Received: from mail-ed1-x52f.google.com ([2a00:1450:4864:20::52f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1maPDH-00E0x8-Ep for linux-riscv@lists.infradead.org; Tue, 12 Oct 2021 21:21:32 +0000 Received: by mail-ed1-x52f.google.com with SMTP id p13so1687007edw.0 for ; Tue, 12 Oct 2021 14:21:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=gOFNBJr+5UGYcXXM52CBBPVCPAsy4cPdDGbGieRT3Pw=; b=TjyrNYrAJjITZgQqDIhNBw1waTJz03vnWMl+Y/NcIBBjvX+/eQQKKsaNJr5LF9vE/m U7HF99K3buQSlliPH7Y57gW8fcassAWYG8KLcminqJ3HpgDIA4QOzM1ttT4Toam8i+zj loUEwm7f4uGNYNP+lEiX76DKTTWoRBqMX6MQtDuOpXW5WdtCCRfhhZbqwC9bVl/Jx137 6zn0fRNrLwrgXBQPfibscSDKJ0TyA2u1+2N4tdOIJTICaaYl9RtiuyGYH01Vxh3gKJJe sbjeY31IlJkRviou4RLSupw9CNHNivQ+REiV0LHNpTHNQiiTBtoh9FtRbcU6L9o6M1md S3NQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=gOFNBJr+5UGYcXXM52CBBPVCPAsy4cPdDGbGieRT3Pw=; b=4zZ/jr596lx9scz1hcv45UBU+Hp+pmIR21GdKKqYFNVRshskJvv7I+86EA0ETCOPQa z7lMGm80s0gkD9SHEAPcGGC4nY5Orf740piOwD/gKedo+nAyiqbefytpfm8jgU7evji5 ALbVE6oWMtZMTbP0Zn+Oz8JC7O7KbF8hxpb4a05M8Z7jQ6hUnBqmimlv+Tnd8Y1aQcur s2m/+ytLCpbnkqOmD4WrTSxAF8Y5i+W6qaxgIDrsll184GhvTDH+6Ustvv+uqDRY2cPD 5x35WOK6mXgmDd9Q+qHd9RFoYGHiDbSYoyWG+OAxvVrTVBCEKouWLop2qzboY0ms5o5r T7QQ== X-Gm-Message-State: AOAM5310fZRjczesxJ1fW5vtert8vQai+/lrneUtDirn2SKOB2ArHpaq FYyMhqPxp6ptOYraFyJr8t9jeaWLiK8HHLr7yLU= X-Google-Smtp-Source: ABdhPJwEOXZ3Sz3Z+75oIHMBoXtzmDYFRay0T/yAdFIEl1UtVqX3R3fR7pveB81mNdmIUdJ/kDbGrP2U5ErDIePd6pc= X-Received: by 2002:a17:906:5a47:: with SMTP id my7mr35357939ejc.128.1634073689974; Tue, 12 Oct 2021 14:21:29 -0700 (PDT) MIME-Version: 1.0 References: <20211012134027.684712-1-kernel@esmil.dk> <20211012134027.684712-7-kernel@esmil.dk> In-Reply-To: From: Andy Shevchenko Date: Wed, 13 Oct 2021 00:20:53 +0300 Message-ID: Subject: Re: [PATCH v1 06/16] clk: starfive: Add JH7100 clock generator driver To: Emil Renner Berthing Cc: linux-riscv , devicetree , linux-clk , "open list:GPIO SUBSYSTEM" , "open list:SERIAL DRIVERS" , Geert Uytterhoeven , Palmer Dabbelt , Paul Walmsley , Rob Herring , Michael Turquette , Stephen Boyd , Thomas Gleixner , Marc Zyngier , Philipp Zabel , Linus Walleij , Greg Kroah-Hartman , Daniel Lezcano , Andy Shevchenko , Jiri Slaby , Maximilian Luz , Sagar Kadam , Drew Fustini , Anup Patel , Atish Patra , Matteo Croce , Linux Kernel Mailing List X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211012_142131_537140_3D4EE551 X-CRM114-Status: GOOD ( 30.27 ) X-BeenThere: linux-riscv@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-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Tue, Oct 12, 2021 at 11:08 PM Emil Renner Berthing wrote: > On Tue, 12 Oct 2021 at 17:40, Andy Shevchenko wrote: > > On Tue, Oct 12, 2021 at 4:42 PM Emil Renner Berthing wrote: ... > > > + value |= readl_relaxed(reg) & ~mask; > > > > value is not masked, is it okay? > > > > Usual pattern for this kind of operations is > > > > value = (current & ~mask) | (value & mask); > > This function is only ever called with constants, already masked > values or the parent number from the clk framework, so it should be > ok. Up to you, but I think it's better to have a usual pattern. > > > + writel_relaxed(value, reg); ... > > > + rate = parent / div; > > > + if (rate < req->min_rate && div > 1) { > > > + div -= 1; > > > + rate = parent / div; > > > + } > > > > Seems like homegrown DIV_ROUND_UP() or so. Who will guarantee that > > decreasing div by 1 will satisfy the conditional again? > > Maths unless I'm mistaken: div = DIV_ROUND_UP(parent, target), so in > rational numbers > div - 1 < parent / target > But the target is clamped by min_rate and max_rate, so > min_rate <= target < parent / (div - 1) = rate > > Sorry, re-using the rate varable for both the target and result is > confusing. I'll fix that. Also needs a comment, I believe. ... > > > +#ifdef CONFIG_DEBUG_FS > > > > Perhaps __maybe_unused? > > I can definitely use __maybe_unused for the function declaration, but > then I'll need a conditional every time clk_ops.debug_init needs to be > initialized to either the function or NULL depending on > CONFIG_DEBUG_FS below. Is that better? Actually, why can't you always initialize the field? Shouldn't CLK core take care about this conditional? > > > +#else > > > +#define jh7100_clk_debug_init NULL > > > +#endif ... > > > + while (idx > 0) > > > + clk_hw_unregister(&priv->reg[--idx].hw); > > > > The > > > > while (idx--) > > clk_hw_unregister(&priv->reg[idx].hw); > > > > is slightly better to read. > > It's not something I'll insist hard on, but I must admit I disagree. > To me the above looks like cartoon characters running off a cliff and > back. As a middle ground could we maybe do this? > > while (idx) > clk_hw_unregister(&priv->reg[--idx].hw); My point is exactly in having the common pattern for error paths, i.e. while (counter--) ...bla-bla-bla... Your second approach is better, but I think that proposed by me is even better. ... > > > +subsys_initcall(clk_starfive_jh7100_init); > > > > Any explanation why subsys_initcall() is in use? > > TBH I just inherited that from Geert's first mock driver and never > thought to question it. What would be a better alternative to try? At least add a comment to explain the choice. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv