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

[Paul Walmsley <pjw@kernel.org>]

Hi Rahul,

On Mon, 18 Aug 2025, Anup Patel wrote:

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