[U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController

[Simon Glass <sjg@chromium.org>]

Hi Rob,

On 20 September 2017 at 08:23, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Sep 20, 2017 at 9:49 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Heinrich,
>>
>> On 15 September 2017 at 00:35, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>> On 08/31/2017 02:52 PM, Simon Glass wrote:
>>>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>  lib/efi_loader/efi_boottime.c | 77 ++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>>> index 1069da7d79..c5a17b6252 100644
>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>> @@ -1052,9 +1052,84 @@ static efi_status_t EFIAPI efi_disconnect_controller(void *controller_handle,
>>>>>                                                      void *driver_image_handle,
>>>>>                                                      void *child_handle)
>>>>
>>>> Can you add a function comment?
>>>
>>> Hello Simon,
>>>
>>> the API functions (here DisconnectController) are documented in the UEFI
>>> spec. Is your idea that we should duplicate part of this information for
>>> all API functions? Or do you miss a comment on implementation details?
>>
>> I think the code in U-Boot should stand alone, so arguments should be
>> described here along with a one-line function description. The args
>> are all void which is not good, but makes it even more important to
>> document.
>
> couple notes, fwiw..
>
> 1) As someone else implementing parts of UEFI interface, I'd find link
> to section in spec (or perhaps to http://wiki.phoenix.com/) to be more
> useful than re-writing the spec in our own words (and possibly getting
> it wrong)

The problem is that there are 3 void pointers and I have no idea what
they really are and what to pass. We have to have some coding
standards in U-Boot. I am not looking for a detailed description of
the purpose of the function, but one line and a description of the
arguments is the minimum we should have for exported functions.

I think it is a great idea to link to the spec as well, particularly
if it can be a URL.

>
> 2) Leif introduced (iirc, in the stub HII or unicode patch)
> efi_handle_t, and efi_string_t (and maybe we should add efi_char_t)..
> which we should probably use more extensively.  Although I haven't
> wanted to go on a major housecleaning while we still have so many
> patches pending on list..  but would be a nice cleanup to make at some
> point.

Sounds good to me. No hurry, but it's nice to know that this is
heading in the right direction. The EFI API is really awful IMO. The
obfuscation is so painful.

Regards,
Simon