lkml.org 
[lkml]   [2020]   [Apr]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
From
Date
Quoting Douglas Anderson (2020-04-13 10:04:08)
> I was trying to write documentation for the functions in rpmh-rsc and
> I got to tcs_ctrl_write(). The documentation for the function would
> have been: "This is the core of rpmh_rsc_write_ctrl_data(); all the
> caller does is error-check and then call this".
>
> Having the error checks in a separate function doesn't help for
> anything since:
> - There are no other callers that need to bypass the error checks.
> - It's less documenting. When I read tcs_ctrl_write() I kept
> wondering if I need to handle cases other than ACTIVE_ONLY or cases
> with more commands than could fit in a TCS. This is obvious when
> the error checks and code are together.
> - The function just isn't that long, so there's no problem
> understanding the combined function.
>
> Things were even more confusing because the two functions names didn't
> make obvious (at least to me) their relationship.
>
> Simplify by folding one function into the other.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
> Tested-by: Maulik Shah <mkshah@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 439a0eadabf1..d9177324c6a2 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -628,7 +612,18 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
> if (msg->state == RPMH_ACTIVE_ONLY_STATE)
> return -EINVAL;
>
> - return tcs_ctrl_write(drv, msg);
> + tcs = get_tcs_for_msg(drv, msg);
> + if (IS_ERR(tcs))
> + return PTR_ERR(tcs);
> +
> + spin_lock_irqsave(&tcs->lock, flags);
> + /* find the TCS id and the command in the TCS to write to */
> + ret = find_slots(tcs, msg, &tcs_id, &cmd_id);

It may be better to inline find_slots() too. It's only used here and
that tcs_id = 0, cmd_id = 0 line at the top of this function is
annoying.

\
 
 \ /
  Last update: 2020-04-13 23:37    [W:0.117 / U:0.328 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site