From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Wed, 20 Sep 2017 07:49:47 -0600
Subject: [U-Boot] [PATCH 15/23] efi_loader: implement ConnectController
In-Reply-To: <15a8b074-4287-e97a-d977-835b3da93445@gmx.de>
References: <20170826225110.7381-1-xypron.glpk@gmx.de>
 <20170826225328.7550-1-xypron.glpk@gmx.de>
 <20170826225328.7550-6-xypron.glpk@gmx.de>
 <CAPnjgZ3+HZupf6AsJkRu+DWKv+Yv8+f-JDbt4VurMW9_v676FQ@mail.gmail.com>
 <15a8b074-4287-e97a-d977-835b3da93445@gmx.de>
Message-ID: <CAPnjgZ3GyBNSvci2E1xD44DWprZV_Xf2OpJco1iq1+G-dHjfRg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Heinrich,

On 15 September 2017 at 00:48, 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>
>>> ---
>>>  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@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.

OK I see. I suppose the EFIAPI is confusing it.

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

I know what a GUID is (or can look up efi_guid_t to find out) but not
what these variables are for.

Regards,
Simon