On 04/06/16 18:14, Ard Biesheuvel wrote:
> This introduces the FdtClientProtocol, which will be used to expose the
> device tree provided by the host to other DXE drivers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtPkg.dec               |  3 +
>  ArmVirtPkg/Include/Protocol/FdtClient.h | 89 ++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
> index b6ff63677837..fa908253b320 100644
> --- a/ArmVirtPkg/ArmVirtPkg.dec
> +++ b/ArmVirtPkg/ArmVirtPkg.dec
> @@ -34,6 +34,9 @@ [Guids.common]
>    gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 
> 0x36, 0x5B, 0x80, 0x63, 0x66 } }
>    gEarlyPL011BaseAddressGuid       = { 0xB199DEA9, 0xFD5C, 0x4A84, { 0x80, 
> 0x82, 0x2F, 0x41, 0x70, 0x78, 0x03, 0x05 } }
>  
> +[Protocols]
> +  gFdtClientProtocolGuid = { 0xE11FACA0, 0x4710, 0x4C8E, { 0xA7, 0xA2, 0x01, 
> 0xBA, 0xA2, 0x59, 0x1B, 0x4C } }
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule]
>    #
>    # This is the physical address where the device tree is expected to be 
> stored
> diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h 
> b/ArmVirtPkg/Include/Protocol/FdtClient.h
> new file mode 100644
> index 000000000000..b7cf8191b5ab
> --- /dev/null
> +++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
> @@ -0,0 +1,89 @@
> +/** @file

(1) Please steal the disclaimer from the beginning of
"OvmfPkg/Include/Protocol/VirtioDevice.h", and tack it here.

> +
> +  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#ifndef __FDT_CLIENT_PROTOCOL_H__
> +#define __FDT_CLIENT_PROTOCOL_H__
> +

(2) Usually the word PROTOCOL is not added to the include guard. (I'm
sure you can find some counter-examples, but still :))

(3) Please add a macro here that is suitable for initializing GUID
objects with static storage duration.

(An "extern EFI_GUID ..." at the end of the file may not be necessary,
if you are against it -- we seem to have discussed that BaseTools
generates that declaration. If you don't feel strongly about it, I'd
prefer to stick with the current tradition. I don't insist.)

Regarding the rest of the patch, it looks sane, but of course all such
interfaces emerge from the actual callers' needs, so I will try to look
at the protocol's implementation, and how it is used. Until then, the
rest looks okay to me.

Thanks!
Laszlo

> +//
> +// Protocol interface structure
> +//
> +typedef struct _FDT_CLIENT_PROTOCOL FDT_CLIENT_PROTOCOL;
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_GET_NODE_PROPERTY) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  INT32                   Node,
> +  IN  CONST CHAR8             *PropertyName,
> +  OUT CONST VOID              **Prop,
> +  OUT UINTN                   *PropSize OPTIONAL
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_SET_NODE_PROPERTY) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  INT32                   Node,
> +  IN  CONST CHAR8             *PropertyName,
> +  IN  CONST VOID              *Prop,
> +  IN  UINTN                   PropSize
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_FIND_COMPATIBLE_NODE) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  CONST CHAR8             *CompatibleString,
> +  OUT INT32                   *Node
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_FIND_COMPATIBLE_NODE_PROPERTY) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  CONST CHAR8             *CompatibleString,
> +  IN  CONST CHAR8             *PropertyName,
> +  OUT CONST VOID              **Prop,
> +  OUT UINTN                   *PropSize OPTIONAL
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_FIND_COMPATIBLE_NODE_REG) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  CONST CHAR8             *CompatibleString,
> +  OUT CONST VOID              **Reg,
> +  OUT UINTN                   *RegElemSize,
> +  OUT UINTN                   *RegSize
> +  );
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *FDT_CLIENT_GET_CHOSEN_NODE) (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  OUT INT32                   *Node
> +  );
> +
> +struct _FDT_CLIENT_PROTOCOL {
> +  FDT_CLIENT_GET_NODE_PROPERTY             GetNodeProperty;
> +  FDT_CLIENT_SET_NODE_PROPERTY             SetNodeProperty;
> +
> +  FDT_CLIENT_FIND_COMPATIBLE_NODE          FindCompatibleNode;
> +  FDT_CLIENT_FIND_COMPATIBLE_NODE_PROPERTY FindCompatibleNodeProperty;
> +  FDT_CLIENT_FIND_COMPATIBLE_NODE_REG      FindCompatibleNodeReg;
> +
> +  FDT_CLIENT_GET_CHOSEN_NODE               GetChosenNode;
> +};
> +
> +#endif
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to