Hi Jordan, Tomasz,
Thanks for your valuable review comments.
On 2018-06-04 18:24, Tomasz Figa wrote:
Hi Jordan, Vikash,
On Sat, Jun 2, 2018 at 6:21 AM Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
wrote:
On Sat, Jun 02, 2018 at 01:56:05AM +0530, Vikash Garodia wrote:
[snip]
> +int venus_set_hw_state(enum tzbsp_video_state state, struct venus_core *core)
> +{
> + int ret;
> + struct device *dev = core->dev;
If you get rid of the log message as you should, you don't need this.
Would prefer to keep the log for cases when enum is expanded while the
switch does
not handle it.
> + void __iomem *reg_base = core->base;
> +
> + switch (state) {
> + case TZBSP_VIDEO_SUSPEND:
> + if (qcom_scm_is_available())
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_SUSPEND, 0);
> + else
> + writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
You can just use core->base here and not bother making a local
variable for it.
Ok.
> + break;
> + case TZBSP_VIDEO_RESUME:
> + if (qcom_scm_is_available())
> + ret = qcom_scm_set_remote_state(TZBSP_VIDEO_RESUME, 0);
> + else
> + venus_reset_hw(core);
> + break;
> + default:
> + dev_err(dev, "invalid state\n");
state is a enum - you are highly unlikely to be calling it in your own
code with
a random value. It is smart to have the default, but you don't need
the log
message - that is just wasted space in the binary.
Incase enum is expanded while the switch does not handle it, default
will be useful.
> + break;
> + }
There are three paths in the switch statement that could end up with
'ret' being
uninitialized here. Set it to 0 when you declare it.
Does this actually compile? The compiler should detect that ret is
used uninitialized. Setting it to 0 at declaration time actually
prevents compiler from doing that and makes it impossible to catch
cases when the ret should actually be non-zero, e.g. the invalid enum
value case.
It does compile while it gave me failure while doing the functional
validation.
I have fixed it in next series of patch.
Given that this function is supposed to substitute existing calls into
qcom_scm_set_remote_state(), why not just do something like this:
if (qcom_scm_is_available())
return qcom_scm_set_remote_state(state, 0);
switch (state) {
case TZBSP_VIDEO_SUSPEND:
writel_relaxed(1, reg_base + WRAPPER_A9SS_SW_RESET);
break;
case TZBSP_VIDEO_RESUME:
venus_reset_hw(core);
break;
}
return 0;
This will not work as driver will write on the register irrespective of
scm
availability.
Best regards,
Tomasz
