linux-kernel - Re: [PATCH v10 10/24] clk: Add clock driver for the RISC-V RPMI clock service group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <823e11f3-ba2b-f0ec-8bb9-0785c89e8234@kernel.org>
Date: Thu, 25 Sep 2025 20:06:59 -0600 (MDT)
From: Paul Walmsley <pjw@...nel.org>
To: Rahul Pathak <rpathak@...tanamicro.com>
cc: Anup Patel <apatel@...tanamicro.com>, 
    Michael Turquette <mturquette@...libre.com>, 
    Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>, 
    Krzysztof Kozlowski <krzk+dt@...nel.org>, 
    Conor Dooley <conor+dt@...nel.org>, Jassi Brar <jassisinghbrar@...il.com>, 
    Thomas Gleixner <tglx@...utronix.de>, 
    "Rafael J . Wysocki" <rafael@...nel.org>, 
    Mika Westerberg <mika.westerberg@...ux.intel.com>, 
    Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
    Linus Walleij <linus.walleij@...aro.org>, 
    Bartosz Golaszewski <brgl@...ev.pl>, 
    Uwe Kleine-König <ukleinek@...nel.org>, 
    devicetree@...r.kernel.org, Andrew Jones <ajones@...tanamicro.com>, 
    Alexandre Ghiti <alex@...ti.fr>, Atish Patra <atish.patra@...ux.dev>, 
    Leyfoon Tan <leyfoon.tan@...rfivetech.com>, 
    Anup Patel <anup@...infault.org>, linux-kernel@...r.kernel.org, 
    Samuel Holland <samuel.holland@...ive.com>, linux-acpi@...r.kernel.org, 
    Palmer Dabbelt <palmer@...belt.com>, 
    Paul Walmsley <paul.walmsley@...ive.com>, linux-riscv@...ts.infradead.org, 
    Len Brown <lenb@...nel.org>, linux-clk@...r.kernel.org
Subject: Re: [PATCH v10 10/24] clk: Add clock driver for the RISC-V RPMI
 clock service group

Hi Rahul,

On Mon, 18 Aug 2025, Anup Patel wrote:

> From: Rahul Pathak <rpathak@...tanamicro.com>
> 
> 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 <sboyd@...nel.org>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Co-developed-by: Anup Patel <apatel@...tanamicro.com>
> Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> Signed-off-by: Rahul Pathak <rpathak@...tanamicro.com>

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 <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/riscv-rpmi-message.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/wordpart.h>
> +
> +#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