Re: [PATCH v2 2/5] media: venus: add a routine to set venus state

[Vikash Garodia <vgarodia@codeaurora.org>]

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@codeaurora.org> 
> 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