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@xxxxxxxxxxxx> > Reviewed-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx> > Tested-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx> > --- Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > 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.
