On 2019-09-26 1:06 PM, Marco Felsch wrote:
> On 19-09-26 08:03, Anson Huang wrote:
>>> On 19-09-25 18:07, Anson Huang wrote:
>>>> The SCU firmware does NOT always have return value stored in message
>>>> header's function element even the API has response data, those
>>>> special APIs are defined as void function in SCU firmware, so they
>>>> should be treated as return success always.
>>>>
>>>> +static const struct imx_sc_rpc_msg whitelist[] = {
>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
>>>> + { .svc = IMX_SC_RPC_SVC_MISC, .func =
>>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
>>>
>>> Is this going to be extended in the near future? I see some upcoming
>>> problems here if someone uses a different scu-fw<->kernel combination as
>>> nxp would suggest.
>>
>> Could be, but I checked the current APIs, ONLY these 2 will be used in Linux
>> kernel, so
>> I ONLY add these 2 APIs for now.
>
> Okay.
>
>> However, after rethink, maybe we should add another imx_sc_rpc API for those
>> special
>> APIs? To avoid checking it for all the APIs called which may impact some
>> performance.
>> Still under discussion, if you have better idea, please advise, thanks!
My suggestion is to refactor the code and add a new API for the this "no
error value" convention. Internally they can call a common function with
flags.
> Adding a special api shouldn't be the right fix. Imagine if someone (not
> a nxp-developer) wants to add a new driver. How could he be expected to
> know which api he should use. The better abbroach would be to fix the
> scu-fw instead of adding quirks..
Right now developers who want to make SCFW calls in upstream need to
define the message struct in their driver based on protocol
documentation. This includes:
* Binary layout of the message (a packed struct)
* If the message has a response (already a bool flag)
* If an error code is returned (this patch adds support for it)
Since callers are already exposed to the binary protocol exposing them
to minor quirks of the calling convention also seems reasonable. Having
the low-level IPC code peek at message IDs seems like a hack; this
belong at a slightly higher level.
In older internal trees we use a very large amount of generated wrapper
functions. This hides calling convention details from callers but is
extremely ugly and verbose.
--
Regards,
Leonard
