From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D34E317C220; Fri, 26 Sep 2025 02:07:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758852426; cv=none; b=okcJ7vGG7XXbyJWhxLaaJO6Oop31eMem69GzqEQGydoLnvhRUOtVs6DSB5vd9NTTXZeftUuM1RdhYDfjWtxByk9rknHXL1+w2D9Gtu7/BnKna1ZbHFfPmprQWixPDEA1TTrLIj/IcxEGXa22GUBl04ajJoAa9qOzrHuzIUM1dBc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758852426; c=relaxed/simple; bh=2YJDK99s0lMvRJGOHiSJOwxLZMxkVYoczRMWoF8baEc=; h=Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=STfXkZzVDrm6MgdI4mZI+i6kiCunjC+s6XRDHu0RUdMVsXQA61pCHiqHBbR3e1dlmaLaDuRNtELzZ146qhvCei/N/WiGBm96Uyzc5p+Mz9rkk6KYcEpX2+np6PXxBUoU0+Se4iIj1eB1BFpa/p2Hx07zDCvLJrSMEelGfIL5nn8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=VktWaqHH; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VktWaqHH" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C5F46C4CEF0; Fri, 26 Sep 2025 02:07:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1758852425; bh=2YJDK99s0lMvRJGOHiSJOwxLZMxkVYoczRMWoF8baEc=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=VktWaqHHYYs86nihTQyoku8q1eGcuuMXhC4UHSJV+//JfmezDua+vtqRiiRV6Mqc4 UHjry0UpMkwXw8qX58EgbafwWPFKgTAzhUCud8zlCN6SfxxfJCOJxc9JHMLOTdzL/e 9ckLgp9O+6+k+9MZtD+pSI+jncUUpcSL6mODwS8KzWOsGmIiPSKMNJdsnfkbc2Pg3Y NeIzOpY7exAmHlyolxKFPx64f5zFpZ5Qm0wJG6djlhqA48FAa95rNVkYXNWnOODn+d v8ePPOjLn7H/ZlOT6xmIvuj5uAVxs9RVu7hjFgqORzi1kAW19WbmeAGcSvhgrYIMe8 wqIOHczzBbfQQ== Date: Thu, 25 Sep 2025 20:06:59 -0600 (MDT) From: Paul Walmsley To: Rahul Pathak cc: Anup Patel , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jassi Brar , Thomas Gleixner , "Rafael J . Wysocki" , Mika Westerberg , Andy Shevchenko , Linus Walleij , Bartosz Golaszewski , =?ISO-8859-15?Q?Uwe_Kleine-K=F6nig?= , devicetree@vger.kernel.org, Andrew Jones , Alexandre Ghiti , Atish Patra , Leyfoon Tan , Anup Patel , linux-kernel@vger.kernel.org, Samuel Holland , linux-acpi@vger.kernel.org, Palmer Dabbelt , Paul Walmsley , linux-riscv@lists.infradead.org, Len Brown , linux-clk@vger.kernel.org Subject: Re: [PATCH v10 10/24] clk: Add clock driver for the RISC-V RPMI clock service group In-Reply-To: <20250818040920.272664-11-apatel@ventanamicro.com> Message-ID: <823e11f3-ba2b-f0ec-8bb9-0785c89e8234@kernel.org> References: <20250818040920.272664-1-apatel@ventanamicro.com> <20250818040920.272664-11-apatel@ventanamicro.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Hi Rahul, On Mon, 18 Aug 2025, Anup Patel wrote: > From: Rahul Pathak > > The RPMI specification defines a clock service group which can be > accessed via SBI MPXY extension or dedicated S-mode RPMI transport. > > Add mailbox client based clock driver for the RISC-V RPMI clock > service group. > > Reviewed-by: Stephen Boyd > Reviewed-by: Andy Shevchenko > Co-developed-by: Anup Patel > Signed-off-by: Anup Patel > Signed-off-by: Rahul Pathak a few minor comments: > diff --git a/drivers/clk/clk-rpmi.c b/drivers/clk/clk-rpmi.c > new file mode 100644 > index 000000000000..7a0a62456314 > --- /dev/null > +++ b/drivers/clk/clk-rpmi.c > @@ -0,0 +1,616 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * RISC-V MPXY Based Clock Driver > + * > + * Copyright (C) 2025 Ventana Micro Systems Ltd. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RPMI_CLK_DISCRETE_MAX_NUM_RATES 16 > +#define RPMI_CLK_NAME_LEN 16 > + > +#define to_rpmi_clk(clk) container_of(clk, struct rpmi_clk, hw) > + > +#define rpmi_clkrate_u64(__hi, __lo) (((u64)(__hi) << 32) | (u32)(__lo)) I'd prefer to see code like this implemented as static inline functions, rather than macros. > +static int rpmi_clk_get_attrs(u32 clkid, struct rpmi_clk *rpmi_clk) > +{ [ ... ] > + > + format = le32_to_cpu(resp->flags) & 3U; And similarly, it's best to pull these kinds of magic numbers up into appropriately-named macros, to help reviewers understand your intention. Since we're pretty close to the merge window opening, and the changes are minor, I've gone ahead and just made these two changes in the patch, and queued it for v6.18 (hopefully). But maybe you can keep them in mind for next time. thanks, - Paul