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

[Simon Glass]

Hi Heinrich,

On 15 September 2017 at 00:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 08/31/2017 02:52 PM, Simon Glass wrote:
>> On 27 August 2017 at 06:53, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at 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.

>
>>
>>>  {
>>> +       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