On 2015-11-09 22:13:18, Ni, Ruiyu wrote:
> How about define:
> typedef enum {
> ImageFormatUnknown,
> ImageFormatBmp,
> ...
> ImageFormatGif
> } IMAGE_FORMAT
> ?
>
> So that extending the image format doesn't have binary compatibility
> issue.
Ok. That works too.
I did have one other question about the names. Do you think they
should be PlatformLogoFormat* rather than ImageFormat*?
My suggestion would have been:
PlatformLogoFormatUnknown,
PlatformLogoFormatImageBmp,
PlatformLogoFormatImageJpeg,
PlatformLogoFormatImageTiff,
PlatformLogoFormatImageGif,
Because these look close to the mime-types. image/jpeg, etc...
https://www.iana.org/assignments/media-types/media-types.xhtml#image
I don't have a big concern about this, so I'll leave that decision up
to you.
> Edkii prefix was added to indicate that this protocol is not defined
> by UEFI spec but a EDKII implementation specific one. We had
> EdkiiVariableLock protocol defined in
> MdeModulePkg/Include/Protocol/VariableLock.h. I agree to remove
> Edkii prefix from the commit message, #ifdef macro, but will keep
> the prefix in the protocol name, enum name.
Ok.
Reviewed-by: Jordan Justen <[email protected]>
>
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Tuesday, November 10, 2015 11:14 AM
> To: Ni, Ruiyu <[email protected]>; [email protected]
> Cc: Ni, Ruiyu <[email protected]>; Tian, Feng <[email protected]>
> Subject: Re: [edk2] [Patch V2 1/5] MdeModulePkg: Add EdkiiPlatformLogo
> protocol definition.
>
> In the subject, can you change EdkiiPlatformLogo => PlatformLogo?
>
> On 2015-11-08 21:23:40, Ruiyu Ni wrote:
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ruiyu Ni <[email protected]>
> > Cc: Feng Tian <[email protected]>
> > ---
> > MdeModulePkg/Include/Protocol/PlatformLogo.h | 86
> > ++++++++++++++++++++++++++++
> > MdeModulePkg/MdeModulePkg.dec | 3 +
> > 2 files changed, 89 insertions(+)
> > create mode 100644 MdeModulePkg/Include/Protocol/PlatformLogo.h
> >
> > diff --git a/MdeModulePkg/Include/Protocol/PlatformLogo.h
> > b/MdeModulePkg/Include/Protocol/PlatformLogo.h
> > new file mode 100644
> > index 0000000..aa8b398
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/PlatformLogo.h
> > @@ -0,0 +1,86 @@
> > +/** @file
> > + The Platform Logo Protocol defines the interface to get the Platform logo
> > + image with the display attribute.
> > +
> > +Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> > +This program and the accompanying materials are licensed and made
> > available under
> > +the terms and conditions of the BSD License that 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 __EDKII_PLATFORM_LOGO__
> > +#define __EDKII_PLATFORM_LOGO__
>
> Can you remove EDKII from the various places in the patch? It seems
> better as 'platform logo'.
>
> > +
> > +//
> > +// GUID for EDKII Platform Logo Protocol
> > +//
> > +#define EDKII_PLATFORM_LOGO_PROTOCOL_GUID \
> > + { 0x9b517978, 0xeba1, 0x44e7, { 0xba, 0x65, 0x7c, 0x2c, 0xd0, 0x8b,
> > 0xf8, 0xe9 } }
> > +
> > +typedef struct _EDKII_PLATFORM_LOGO_PROTOCOL EDKII_PLATFORM_LOGO_PROTOCOL;
> > +
> > +typedef enum {
> > + ImageFormatBmp,
> > + ImageFormatJpeg,
> > + ImageFormatTiff,
> > + ImageFormatGif,
> > + ImageFormatUnknown
>
> Can we set ImageFormatUnknown = MAX_INT32? (So, we can add other
> formats in the middle.)
>
> > +} IMAGE_FORMAT;
> > +
> > +typedef enum {
> > + EdkiiPlatformLogoDisplayAttributeLeftTop,
> > + EdkiiPlatformLogoDisplayAttributeCenterTop,
> > + EdkiiPlatformLogoDisplayAttributeRightTop,
> > + EdkiiPlatformLogoDisplayAttributeCenterRight,
> > + EdkiiPlatformLogoDisplayAttributeRightBottom,
> > + EdkiiPlatformLogoDisplayAttributeCenterBottom,
> > + EdkiiPlatformLogoDisplayAttributeLeftBottom,
> > + EdkiiPlatformLogoDisplayAttributeCenterLeft,
> > + EdkiiPlatformLogoDisplayAttributeCenter
> > +} EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE;
> > +
> > +/**
> > +
> > + Load a platform logo image and return its data and attributes.
> > +
> > + @param This The pointer to this protocol instance.
> > + @param Instance The visible image instance is found.
> > + @param Format The format of the image. Examples: BMP, JPEG.
> > + @param ImageData The image data for the badge file. Currently
> > only
> > + supports the .bmp file format.
> > + @param ImageSize The size of the image returned.
> > + @param Attribute The display attributes of the image returned.
> > + @param CoordinateX The X coordinate of the image.
> > + @param CoordinateY The Y coordinate of the image.
> > +
> > + @retval EFI_SUCCESS The image was fetched successfully.
> > + @retval EFI_NOT_FOUND The specified image could not be found.
> > +
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *EDKII_PLATFORM_LOGO_GET_IMAGE)(
> > + IN EDKII_PLATFORM_LOGO_PROTOCOL *This,
> > + IN OUT UINT32 *Instance,
> > + OUT IMAGE_FORMAT *Format,
> > + OUT UINT8 **ImageData,
> > + OUT UINTN *ImageSize,
> > + OUT EDKII_PLATFORM_LOGO_DISPLAY_ATTRIBUTE *Attribute,
> > + OUT UINTN *CoordinateX,
> > + OUT UINTN *CoordinateY
> > +);
> > +
> > +
> > +struct _EDKII_PLATFORM_LOGO_PROTOCOL {
> > + EDKII_PLATFORM_LOGO_GET_IMAGE GetImage;
> > +};
> > +
> > +
> > +extern EFI_GUID gEdkiiPlatformLogoProtocolGuid;
> > +
> > +#endif
> > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> > index 3dfcd6a..08148e3 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -449,6 +449,9 @@
> > ## Include/Protocol/SmmReadyToBoot.h
> > gEdkiiSmmReadyToBootProtocolGuid = { 0x6e057ecf, 0xfa99, 0x4f39, { 0x95,
> > 0xbc, 0x59, 0xf9, 0x92, 0x1d, 0x17, 0xe4 } }
> >
> > + ## Include/Protocol/PlatformLogo.h
> > + gEdkiiPlatformLogoProtocolGuid = { 0x9b517978, 0xeba1, 0x44e7, { 0xba,
> > 0x65, 0x7c, 0x2c, 0xd0, 0x8b, 0xf8, 0xe9 } }
> > +
> > #
> > # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> > # 0x80000001 | Invalid value provided.
> > --
> > 1.9.5.msysgit.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > [email protected]
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
