[U-Boot] [PATCH 15/23] efi_loader: implement ConnectController

[Heinrich Schuchardt]

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>
>> ---
>>  include/efi_api.h             |  22 ++++++++
>>  include/efi_loader.h          |   1 +
>>  lib/efi_loader/efi_boottime.c | 119 +++++++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 141 insertions(+), 1 deletion(-)
>>
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Please see below.
> 
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> index 8efc8dfab8..b2838125d7 100644
>> --- a/include/efi_api.h
>> +++ b/include/efi_api.h
>> @@ -609,4 +609,26 @@ struct efi_pxe {
>>         struct efi_pxe_mode *mode;
>>  };
>>
>> +#define EFI_DRIVER_BINDING_PROTOCOL_GUID \
>> +       EFI_GUID(0x18a031ab, 0xb443, 0x4d1a,\
>> +                0xa5, 0xc0, 0x0c, 0x09, 0x26, 0x1e, 0x9f, 0x71)
>> +struct efi_driver_binding_protocol {
>> +       efi_status_t (EFIAPI * supported)(
> 
> I think the * should be up against 'supported'. Similarly below.

This is what checkpatch wants to see. Your suggestion leads to

ERROR: need consistent spacing around '*' (ctx:WxV)
#25: FILE: include/efi_api.h:616:
+       efi_status_t (EFIAPI *supported)(

So I prefer not change this before checkpatch.pl is not giving a
different result.

> 
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       struct efi_device_path *remaining_device_path);
>> +       efi_status_t (EFIAPI * start)(
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       struct efi_device_path *remaining_device_path);
>> +       efi_status_t (EFIAPI * stop)(
>> +                       struct efi_driver_binding_protocol *this,
>> +                       efi_handle_t controller_handle,
>> +                       UINTN number_of_children,
>> +                       efi_handle_t child_handle_buffer);
> 
> These should have function comments.
> 
>> +       u32 version;
>> +       efi_handle_t image_handle;
>> +       efi_handle_t driver_binding_handle;
>> +};
>> +
>>  #endif
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 9c68246c7c..f9f33e1d01 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -74,6 +74,7 @@ extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>
>>  extern const efi_guid_t efi_guid_console_control;
>>  extern const efi_guid_t efi_guid_device_path;
>> +extern const efi_guid_t efi_guid_driver_binding_protocol;
> 
> comment for this?

The GUIDs are defined in the UEFI spec.
So maybe we should just put a comment above all of these.

Regards

Heinrich

> 
>>  extern const efi_guid_t efi_guid_loaded_image;
>>  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 5a73ea5cd0..1069da7d79 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -18,6 +18,14 @@
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> +static efi_status_t EFIAPI efi_locate_protocol(const efi_guid_t *protocol,
>> +                                              void *registration,
>> +                                              void **protocol_interface);
>> +static efi_status_t EFIAPI efi_locate_handle_buffer(
>> +                       enum efi_locate_search_type search_type,
>> +                       const efi_guid_t *protocol, void *search_key,
>> +                       unsigned long *no_handles, efi_handle_t **buffer);
>> +
> 
> Is this a forward decl? Can you reorder (in a separate patch) to avoid it?
>