From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Date: Wed, 20 Sep 2017 10:23:23 -0400 Subject: [U-Boot] [PATCH 16/23] efi_loader: implement DisconnectController In-Reply-To: References: <20170826225110.7381-1-xypron.glpk@gmx.de> <20170826225328.7550-1-xypron.glpk@gmx.de> <20170826225328.7550-7-xypron.glpk@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Sep 20, 2017 at 9:49 AM, Simon Glass wrote: > Hi Heinrich, > > On 15 September 2017 at 00:35, Heinrich Schuchardt wrote: >> On 08/31/2017 02:52 PM, Simon Glass wrote: >>> On 27 August 2017 at 06:53, Heinrich Schuchardt wrote: >>>> Signed-off-by: Heinrich Schuchardt >>>> --- >>>> 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