Looks good, but I'd prefer to get rid of the `void *` casting.

I'm a bit concerned by one of the calls to amd_late_run_ap_task().
I would expect agesa_RunFcnOnAllAps() to have its `Func` parameter
set to `AGESA_RUNFUNC_ON_ALL_APS` but callout_ap_entry() explicitly
checks for `AGESA_RUNFUNC_ONAP`. (beside my usual concern if it's
possible to comply with the GPL when distributing this in compiled
form along with a calling blob) With the limited scope of the global
`agesadata` there, one consumer two providers, I don't think the
`Func` field is useful at all.

Patch set 5:Code-Review +2

View Change

3 comments:

• File src/soc/amd/common/block/pi/agesawrapper.c:

  • Patch Set #5, Line 45: StdHeader

    I would prefer not to add any more CamelCase.

  • Patch Set #5, Line 62: AGESA_STATUS amd_late_run_ap_task(AP_EXE_PARAMS *ApExeParams)

    So this seems to be the exception why you set `StdHeader->Func` in
    module_dispatch(). This wasn't easy to grasp. Maybe make this more
    explicit:

        static AGESA_STATUS amd_dispatch(void *Params)
        {
                MODULE_ENTRY dispatcher = agesa_get_dispatcher();
                if (!dispatcher)
                        return AGESA_UNSUPPORTED;
                return dispatcher(Params);
        }

        AGESA_STATUS amd_late_run_ap_task(AP_EXE_PARAMS *ApExeParams)
        {
                ApExeParams->StdHeader.Func = AMD_LATE_RUN_AP_TASK;
                return amd_dispatch(ApExeParams);
        }

    If I didn't miss anything, it should do the same with the excep-
    tion that `Func` gets also set when agesa_get_dispatcher() fails.
    But that was the old behavior anyway. It also avoids the implicit
    cast from `void *`.

  • Patch Set #5, Line 65: StdHeader

    Simply `&ApExeParams->StdHeader` would avoid the casting.

To view, visit change 31485. To unsubscribe, or for help writing mail filters, visit settings.