| Message ID | 20200311161104.RFT.v2.3.Ie88ce5ccfc0c6055903ccca5286ae28ed3b85ed3@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, Tested-by: Maulik Shah <mkshah@codeaurora.org> Thanks, Maulik On 3/12/2020 4:43 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 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> > --- > > Changes in v2: None > > 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 02c8e0ffbbe4..799847b08038 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -550,27 +550,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 > * > @@ -581,6 +560,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"); > @@ -591,7 +575,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,
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 02c8e0ffbbe4..799847b08038 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -550,27 +550,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 * @@ -581,6 +560,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"); @@ -591,7 +575,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,