Jordan, The enum was named IMAGE_FORAMT because it is also used by ImageDecoderLib. The ImageDecoderLib can decode any image including platform logo. So I prefer to use IMAGE_FORMAT instead of PlatformLogoFormat*.
Regards, Ray -----Original Message----- From: Justen, Jordan L Sent: Tuesday, November 10, 2015 3:42 PM To: Ni, Ruiyu <[email protected]>; [email protected] Cc: Tian, Feng <[email protected]> Subject: RE: [edk2] [Patch V2 1/5] MdeModulePkg: Add EdkiiPlatformLogo protocol definition. 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
