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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, May 11, 2025 at 07:09:26PM +0530, 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.

...

> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox/riscv-rpmi-message.h>

Just to point out again that the above misses a lot of headers definitions
and/or APIs this driver uses. Follow IWYU principle.

...

> +#define GET_RATE_U64(hi_u32, lo_u32)	((u64)(hi_u32) << 32 | (lo_u32))

Hmm... Perhaps add this kind of macro to wordpart.h ? IIRC not only this driver
uses something like this.

...

> +enum rpmi_clk_type {
> +	RPMI_CLK_DISCRETE = 0,
> +	RPMI_CLK_LINEAR = 1,

> +	RPMI_CLK_TYPE_MAX_IDX,

No comma for the terminator. Please, clean all these cases.

> +};

...

> +union rpmi_clk_rates {
> +	u64 discrete[RPMI_CLK_DISCRETE_MAX_NUM_RATES];
> +	struct {
> +		u64 min;
> +		u64 max;
> +		u64 step;
> +	} linear;

Have you looked at the linear ranges library we have in the kernel? Can you
utilise it here?

> +};

...

> +struct rpmi_clk {
> +	struct rpmi_clk_context *context;
> +	u32 id;
> +	u32 num_rates;
> +	u32 transition_latency;
> +	enum rpmi_clk_type type;
> +	union rpmi_clk_rates *rates;
> +	char name[RPMI_CLK_NAME_LEN];
> +	struct clk_hw hw;

Just a reminder to use `pahole` to check that your data layout is optimised for
memory consumption.

> +};

...

> +struct rpmi_get_supp_rates_rx {
> +	u32 status;
> +	u32 flags;
> +	u32 remaining;
> +	u32 returned;
> +	u32 rates[];
> +};

Is it ABI? (I mean if this is interface with some kind of FW)
If so, Use proper endianess aware types. Same Q for all data
types defined in this driver.

...

> +			for (j = 0; j < rx->returned; j++) {
> +				if (rateidx >= (clk_rate_idx + rx->returned))

Too many parentheses.

> +					break;
> +				rpmi_clk->rates->discrete[rateidx++] =
> +					GET_RATE_U64(rate_discrete[j].hi,
> +						     rate_discrete[j].lo);
> +			}
> +		}

...

> +	devm_kfree(context->dev, rx);

Why?! This is a red flag to point that here is misunderstanding or abuse of
managed resources approach. Either use __Free() from cleanup.h or don't call
devm_kfree(). The latter must have a very good justification to explain why.

> +	return 0;

(this is even not an error path, where it might have a little argument for)

...

> +	/* Keep the requested rate if the clock format
> +	 * is of discrete type. Let the platform which
> +	 * is actually controlling the clock handle that.
> +	 */

/*
 * Use proper style for the multi-line comments. You can
 * refer to this comment as an example.
 */

...

> +out:

Redundant label. Note, the labels are recommended to be named after the flow
they will run if goto. This one can be named as out_literally_with_return_0,
which makes it obvious how useless it is.

> +	return 0;

...

> +	rates = devm_kzalloc(dev, sizeof(union rpmi_clk_rates), GFP_KERNEL);

sizeof(*...)

> +	if (!rates)
> +		return ERR_PTR(-ENOMEM);
> +
> +	rpmi_clk = devm_kzalloc(dev, sizeof(struct rpmi_clk), GFP_KERNEL);

Ditto.

> +	if (!rpmi_clk)
> +		return ERR_PTR(-ENOMEM);

...

> +	ret = rpmi_clk_get_supported_rates(clkid, rpmi_clk);
> +	if (ret)
> +		return dev_err_ptr_probe(dev, ret,
> +			"Get supported rates failed for clk-%u, %d\n", clkid, ret);

Indentation issues. Repetitive ret in the message. Please, get familiar with
the format dev_err_probe() uses.

...

> +	int ret, num_clocks, i;

Why is 'i' signed?

...

> +	/* Allocate RPMI clock context */
> +	context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);

Ha-ha, you have even inconsistent style in the same file! So, go through the
whole series and make sure that the style used in each file is consistent.

> +	if (!context)
> +		return -ENOMEM;

...

> +	/* Validate RPMI specification version */
> +	rpmi_mbox_init_get_attribute(&msg, RPMI_MBOX_ATTR_SPEC_VERSION);
> +	ret = rpmi_mbox_send_message(context->chan, &msg);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to get spec version\n");
> +		goto fail_free_channel;

This is simply wrong. You should not do goto before any devm_*() calls.
The error path and ->remove(), if present) is broken. Fix it accordingly.

Here should be

		return dev_err_probe(...);

it's your homework to understand how to achieve that. Plenty of the examples in
the kernel.

> +	}

...

> +enum rpmi_clock_service_id {
> +	RPMI_CLK_SRV_ENABLE_NOTIFICATION = 0x01,
> +	RPMI_CLK_SRV_GET_NUM_CLOCKS = 0x02,
> +	RPMI_CLK_SRV_GET_ATTRIBUTES = 0x03,
> +	RPMI_CLK_SRV_GET_SUPPORTED_RATES = 0x04,
> +	RPMI_CLK_SRV_SET_CONFIG = 0x05,
> +	RPMI_CLK_SRV_GET_CONFIG = 0x06,
> +	RPMI_CLK_SRV_SET_RATE = 0x07,
> +	RPMI_CLK_SRV_GET_RATE = 0x08,

> +	RPMI_CLK_SRV_ID_MAX_COUNT,

No comma in the terminator line.

> +};

-- 
With Best Regards,
Andy Shevchenko