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

Andy Shevchenko
2025-05-12T07:08:07Z

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