| Message ID | 20200311161104.RFT.v2.7.I8e187cdfb7a31f5bb7724f1f937f2862ee464a35@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, On 3/12/2020 4:43 AM, Douglas Anderson wrote: > tcs_write() is documented to only be useful for writing > RPMH_ACTIVE_ONLY_STATE. Let's be loud if someone messes up. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: None > > drivers/soc/qcom/rpmh-rsc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index 93f5d1fb71ca..ba489d18c20e 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -573,6 +573,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg) > unsigned long flags; > int ret; > > + /* > + * It only makes sense to grab a whole TCS for ourselves if we're > + * triggering right away, which we only do for ACTIVE_ONLY. > + */ > + WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE); > + Unnecessary check, we will never hit this warning. Lets not add such check. Saying that you can modify this change to drop below check from rpmh_rsc_write_ctrl_data() as that never hit. /* Data sent to this API will not be sent immediately */ if (msg->state == RPMH_ACTIVE_ONLY_STATE) return -EINVAL; we always call rpmh_rsc_write_ctrl_data() for RPMH_SLEEP_STATE and RPMH_WAKE_ONLY_STATE. Thanks, Maulik > /* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */ > tcs = get_tcs_for_msg(drv, msg); > if (IS_ERR(tcs))
Hi, On Wed, Apr 1, 2020 at 5:40 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > Hi, > > On 3/12/2020 4:43 AM, Douglas Anderson wrote: > > tcs_write() is documented to only be useful for writing > > RPMH_ACTIVE_ONLY_STATE. Let's be loud if someone messes up. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v2: None > > > > drivers/soc/qcom/rpmh-rsc.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > > index 93f5d1fb71ca..ba489d18c20e 100644 > > --- a/drivers/soc/qcom/rpmh-rsc.c > > +++ b/drivers/soc/qcom/rpmh-rsc.c > > @@ -573,6 +573,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg) > > unsigned long flags; > > int ret; > > > > + /* > > + * It only makes sense to grab a whole TCS for ourselves if we're > > + * triggering right away, which we only do for ACTIVE_ONLY. > > + */ > > + WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE); > > + > > Unnecessary check, we will never hit this warning. Lets not add such check. That's fine. I can drop it, especially now that comments explain that this is only for ACTIVE_ONLY. Personally I like having extra assertion failures like this that indicate a serious internal logic error in the code, but I won't push strongly for it. > Saying that you can modify this change to drop below check from > rpmh_rsc_write_ctrl_data() as that never hit. > > /* Data sent to this API will not be sent immediately */ > if (msg->state == RPMH_ACTIVE_ONLY_STATE) > return -EINVAL; > > we always call rpmh_rsc_write_ctrl_data() for RPMH_SLEEP_STATE and > RPMH_WAKE_ONLY_STATE. Sure. My preference would have been to change it to a WARN_ON() too (because it signifies an internal error within the RPMH driver, not an external error that a client of RPMH could trigger), but I can just drop it entirely. -Doug
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 93f5d1fb71ca..ba489d18c20e 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -573,6 +573,12 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg) unsigned long flags; int ret; + /* + * It only makes sense to grab a whole TCS for ourselves if we're + * triggering right away, which we only do for ACTIVE_ONLY. + */ + WARN_ON(msg->state != RPMH_ACTIVE_ONLY_STATE); + /* TODO: get_tcs_for_msg() can return -EAGAIN and nobody handles */ tcs = get_tcs_for_msg(drv, msg); if (IS_ERR(tcs))
tcs_write() is documented to only be useful for writing RPMH_ACTIVE_ONLY_STATE. Let's be loud if someone messes up. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: None drivers/soc/qcom/rpmh-rsc.c | 6 ++++++ 1 file changed, 6 insertions(+)