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

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

On Thu, Jun 26, 2025 at 12:32:14PM +0530, Rahul Pathak wrote:
> On Mon, Jun 23, 2025 at 2:36 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Jun 18, 2025 at 05:43:44PM +0530, Anup Patel wrote:

...

> > > +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_range.h? Why can it not be (re-)used here?
> 
> I did the first time only when you commented. And i dont see any
> benefit in that.
> linear_range has slightly different way to access any value using `sel`.
> Here this union represents how RPMI protocol represents the rates and
> reusing linear_range will only introduce conversion to and fro.

Summarize this in the comment on top of the struct definition so one will not
attempt to convert the driver in the future.

> > > +};

...

> > > +static u32 rpmi_clk_get_num_clocks(struct rpmi_clk_context *context)
> > > +{
> > > +     struct rpmi_get_num_clocks_rx rx;
> > > +     struct rpmi_mbox_message msg;
> > > +     int ret;
> > > +
> > > +     rpmi_mbox_init_send_with_response(&msg, RPMI_CLK_SRV_GET_NUM_CLOCKS,
> > > +                                       NULL, 0, &rx, sizeof(rx));
> >
> > ...here
> >
> > > +     ret = rpmi_mbox_send_message(context->chan, &msg);
> > > +
> >
> > This blank line should be rather ^^^
> 
> Sure, I will update.
> 
> >
> > > +     if (ret || rx.status)
> > > +             return 0;
> >
> > Why rx.status can't be checked before calling to a sending message?
> > Sounds like the rpmi_mbox_init_send_with_response() links rx to msg somehow.
> > If this is the case, use msg here, otherwise move the check to be in the
> > correct place.
> 
> Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> the rx to msg. It's a very simple function which only performs assignments.
> 
> Using msg instead of rx directly will require additional typecasting
> which will only clutter
> I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> is used.

This is besides harder-to-read code is kinda of layering violation.
If you afraid of a casting, add a helper to check for the status error.
Comment won't help much as making code better to begin with.

> > Seems the same question to the all similar checks in the code.
> >
> > > +     return le32_to_cpu(rx.num_clocks);
> > > +}

...

> > > +     struct rpmi_clk_context *context = rpmi_clk->context;
> > > +     struct rpmi_clk_rate_discrete *rate_discrete;
> > > +     struct rpmi_clk_rate_linear *rate_linear;

> > > +     struct rpmi_get_supp_rates_rx *rx __free(kfree) = NULL;

This should be assigned where it's used. NULL assignment is not encouraged.

> > > +     struct rpmi_get_supp_rates_tx tx;
> > > +     struct rpmi_mbox_message msg;
> >
> > > +     size_t clk_rate_idx = 0;
> >
> > This kind of assignments is hard to maintain and it's mistake prone in case
> > some additional code is injected in the future that might reuse it.
> >
> I dont understand what is the problem with this assignment. If any
> code added in the future reuse it then it has to make sure that
> clk_rate_idx has the correct initial value before any further references.

Yes, it will add an additional churn and require more brain activity to
maintain such a code. It's a general recommendation to assign when it's
actually needed (there are, of course, exceptions for some small functions,
but this one is not).

> > > +     int ret, rateidx, j;

...

> > > +     context->chan = mbox_request_channel(&context->client, 0);
> > > +     if (IS_ERR(context->chan))
> > > +             return PTR_ERR(context->chan);
> >
> > Here is an incorrect order of the freeing resources. Besides that, wrapping
> > the mbox_free_channel() into managed resources reduces this code by more
> > than 10 LoCs! At bare minimum it will fix the bug,
> 
> Understood. So we can use devm_add_action_or_reset to link a release function
> with the context->chan. Is this what you are suggesting? This will also make
> the .remove callback redundant which can be removed.

Yes.

-- 
With Best Regards,
Andy Shevchenko