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

[Rob Clark <robdclark@gmail.com>]

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)

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.

BR,
-R


>>
>>>
>>>>  {
>>>> +       struct efi_driver_binding_protocol *binding_protocol;
>>>> +       efi_handle_t child_handle_buffer;
>>>> +       unsigned long driver_count;
>>>> +       efi_handle_t *driver_handle_buffer;
>>>> +       size_t i;
>>>> +       UINTN number_of_children;
>>>
>>> Can we somehow use a lower-case type for this? Otherwise U-Boot will
>>> start to look like EFI and I will have to start taking
>>> antidepressants.
>>>
>>
>> In different places the EFI API requires a bitness dependent unsigned
>> integer.
>>
>> Shall we globally rename UINTN to uintn?
>> This is my patch to blame:
>> 503f2695548 (efi_loader: correct size for tpl level)
>
> What does bitness-dependent mean? Do you mean 32-bit?  It looks like
> this is just passed to a function, so could be uint or instead?
>
> If it is 32-bit then uint32_t should do.
>
> Regards,
> Simon