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 it obvious (at least to me) their relationship.
Simplify by folding one function into the other.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/soc/qcom/rpmh-rsc.c | 39 ++++++++++++++++---------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 0a409988d103..099603bf14bf 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -549,27 +549,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
return 0;
}
-static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
-{
- struct tcs_group *tcs;
- int tcs_id = 0, cmd_id = 0;
- unsigned long flags;
- int ret;
-
- 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);
- if (!ret)
- __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
- spin_unlock_irqrestore(&tcs->lock, flags);
-
- return ret;
-}
-
/**
* rpmh_rsc_write_ctrl_data: Write request to the controller
*
@@ -580,6 +559,11 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
*/
int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
{
+ struct tcs_group *tcs;
+ int tcs_id = 0, cmd_id = 0;
+ unsigned long flags;
+ int ret;
+
if (!msg || !msg->cmds || !msg->num_cmds ||
msg->num_cmds > MAX_RPMH_PAYLOAD) {
pr_err("Payload error\n");
@@ -590,7 +574,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);
+ if (!ret)
+ __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
+ spin_unlock_irqrestore(&tcs->lock, flags);
+
+ return ret;
}
static int rpmh_probe_tcs_config(struct platform_device *pdev,
--
2.25.1.481.gfbce0eb801-goog
Hi,
On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> 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 it obvious (at least to me) their relationship.
>
> Simplify by folding one function into the other.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/soc/qcom/rpmh-rsc.c | 39 ++++++++++++++++---------------------
> 1 file changed, 17 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 0a409988d103..099603bf14bf 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -549,27 +549,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
> return 0;
> }
>
> -static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
> -{
> - struct tcs_group *tcs;
> - int tcs_id = 0, cmd_id = 0;
> - unsigned long flags;
> - int ret;
> -
> - 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);
> - if (!ret)
> - __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> - spin_unlock_irqrestore(&tcs->lock, flags);
> -
> - return ret;
> -}
> -
> /**
> * rpmh_rsc_write_ctrl_data: Write request to the controller
> *
> @@ -580,6 +559,11 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg)
> */
> int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
> {
> + struct tcs_group *tcs;
> + int tcs_id = 0, cmd_id = 0;
> + unsigned long flags;
> + int ret;
> +
> if (!msg || !msg->cmds || !msg->num_cmds ||
> msg->num_cmds > MAX_RPMH_PAYLOAD) {
> pr_err("Payload error\n");
> @@ -590,7 +574,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);
> + if (!ret)
> + __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
> + spin_unlock_irqrestore(&tcs->lock, flags);
> +
> + return ret;
> }
>
> static int rpmh_probe_tcs_config(struct platform_device *pdev,
i am ok with this change.
Reviewed-by: Maulik Shah <[email protected]>
Thanks,
Maulik
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation