> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 12 October 2017 20:02
> To: Evan Lloyd <evan.ll...@arm.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy
> LcdGraphicsOutputDxe code: Added comments
>
> Given that all changes to the first file _remove_ comments, it may be better
> with a subject line saying "updating comments".
>
> On Tue, Sep 26, 2017 at 09:15:12PM +0100, evan.ll...@arm.com wrote:
> > From: Girish Pathak <girish.pat...@arm.com>
> >
> > There is no functional modification in this change As preparation for
> > a Change (Rejig of LcdGraphicsOutPutDxe), some comments are modified
> > and a few new comments are added.
> > This is to prevent mixing formatting changes with functional changes.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak <girish.pat...@arm.com>
> > Signed-off-by: Evan Lloyd <evan.ll...@arm.com>
> > ---
...
> >
> > -
> > +/** Platform related initialization function.
> > + *
> > + * @param IN Handle Handle to the LCD device instance.
> > + *
> > + * @retval EFI_SUCCESS Platform initialization success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
>
> So ... 6.8 lists
> /**
> text
> **/
> as the
>
> The format
> /**
> * text
> **/
> is mentioned as "also legal because doxygen ignores the leading *".
>
> The format
> /**
> *
> **/
> is never mentioned, although I guess "also legal" because * ignored.
>
> However, a quick skim in MdePkg suggests the former is the generally used
> variant. Can you please update to that format throughout (drop the leading
> '*' on lines not starting or ending the comment block)?
[[Evan Lloyd]] I'm not sure if Outlook has mangled something, or I'm being
obtuse,
but I'm not sure I follow the distinction you are making there.
However, if your objection is to the leading '*' then we can remove it.
By the way - shouldn't it be:
/** Brief description
Details
**/ (see Horor vacuii)
I actually think the CCS is woefully inconsistent in its example comment style,
and that although leading '*'s are acceptable to Doxygen, it would be better to
stick to one style (that of the file header comment, without leading '*'s)
throughout.
>
> No other comments (other than having these prototype documentations
> are a great improvement).
>
> /
> Leif
>
> > EFI_STATUS
> > LcdPlatformInitializeDisplay (
> > IN EFI_HANDLE Handle
> > );
> >
> > +/** Reserve VRAM memory in DRAM for the frame buffer
> > + * (unless it is reserved already).
> > + *
> > + * The allocated address can be used to set the frame buffer.
> > + * @param OUT VramBaseAddress A pointer to the frame buffer
> address.
> > + * @param OUT VramSize A pointer to the size of the frame
> > + * buffer in bytes
> > + *
> > + * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetVram (
> > OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress,
> > OUT UINTN* VramSize
> > );
> >
> > +/** Return total number of modes.
> > + *
> > + * @retval UINT32 Mode Number.
> > +**/
> > UINT32
> > LcdPlatformGetMaxMode (
> > VOID
> > );
> >
> > +/** Set the requested display mode.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @retval EFI_SUCCESS Set mode success.
> > + * @retval EFI_INVALID_PARAMTER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformSetMode (
> > IN UINT32 ModeNumber
> > );
> >
> > +/** Return information for the requested mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Info Pointer for returned mode information
> > + * (on success).
> > + *
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformQueryMode (
> > IN UINT32 ModeNumber,
> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info
> > );
> >
> > +/** Returns the display timing information for the requested mode
> number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT HRes Pointer to horizontal resolution.
> > + * @param OUT HSync Pointer to horizontal sync width.
> > + * @param OUT HBackPorch Pointer to horizontal back porch.
> > + * @param OUT HFrontPorch Pointer to horizontal front porch.
> > + * @param OUT VRes Pointer to vertical resolution.
> > + * @param OUT VSync Pointer to vertical sync width.
> > + * @param OUT VBackPorch Pointer to vertical back porch.
> > + * @param OUT VFrontPorch Pointer to vertical front porch.
> > +
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetTimings (
> > IN UINT32 ModeNumber,
> > @@ -212,6 +244,14 @@ LcdPlatformGetTimings (
> > OUT UINT32* VFrontPorch
> > );
> >
> > +/** Return bits per pixel information for a mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Bpp Pointer to value Bytes Per Pixel.
> > + *
> > + * @retval EFI_SUCCESS The requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetBpp (
> > IN UINT32 ModeNumber,
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c index
> >
> 2041de5f63c72de6f0ce4047420c282507a1d04a..cfe3259d3c737de240350
> e8c3eab
> > 867b80c40948 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> A
> > +++ rmVExpress.c
> > @@ -44,7 +44,8 @@ typedef struct {
> > UINT32 VFrontPorch;
> > } LCD_RESOLUTION;
> >
> > -
> > +/** The display modes supported by the platform.
> > +**/
> > LCD_RESOLUTION mResolutions[] = {
> > { // Mode 0 : VGA : 640 x 480 x 24 bpp
> > VGA, VGA_H_RES_PIXELS, VGA_V_RES_PIXELS,
> LCD_BITS_PER_PIXEL_24,
> > @@ -94,6 +95,11 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = {
> > NULL
> > };
> >
> > +/** HDLCD Platform specific initialization function.
> > + *
> > + * @retval EFI_SUCCESS Plaform library initialization success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformInitializeDisplay (
> > IN EFI_HANDLE Handle
> > @@ -124,6 +130,18 @@ LcdPlatformInitializeDisplay (
> > return Status;
> > }
> >
> > +/** Reserve VRAM memory in DRAM for the frame buffer
> > + * (unless it is reserved already).
> > + *
> > + * The allocated address can be used to set the frame buffer.
> > + *
> > + * @param OUT VramBaseAddress A pointer to the frame buffer
> address.
> > + * @param OUT VramSize A pointer to the size of the frame
> > + * buffer in bytes
> > + *
> > + * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetVram (
> > OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress, @@ -170,6 +188,13
> @@
> > LcdPlatformGetVram (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Return total number of modes supported.
> > + *
> > + * Note: Valid mode numbers are 0 to MaxMode - 1
> > + * See Section 11.9 of the UEFI Specification 2.6 Errata A (Jan
> > +2017)
> > + *
> > + * @retval UINT32 Mode Number.
> > +**/
> > UINT32
> > LcdPlatformGetMaxMode(VOID)
> > {
> > @@ -178,6 +203,10 @@ LcdPlatformGetMaxMode(VOID)
> > return (sizeof (mResolutions) / sizeof (LCD_RESOLUTION)); }
> >
> > +/** Set the requested display mode.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > +**/
> > EFI_STATUS
> > LcdPlatformSetMode (
> > IN UINT32 ModeNumber
> > @@ -227,6 +256,15 @@ LcdPlatformSetMode (
> > return Status;
> > }
> >
> > +/** Return information for the requested mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Info Pointer for returned mode information
> > + * (on success).
> > + *
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformQueryMode (
> > IN UINT32 ModeNumber,
> > @@ -267,6 +305,21 @@ LcdPlatformQueryMode (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Returns the display timing information for the requested mode
> number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT HRes Pointer to horizontal resolution.
> > + * @param OUT HSync Pointer to horizontal sync width.
> > + * @param OUT HBackPorch Pointer to horizontal back porch.
> > + * @param OUT HFrontPorch Pointer to horizontal front porch.
> > + * @param OUT VRes Pointer to vertical resolution.
> > + * @param OUT VSync Pointer to vertical sync width.
> > + * @param OUT VBackPorch Pointer to vertical back porch.
> > + * @param OUT VFrontPorch Pointer to vertical front porch.
> > + *
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetTimings (
> > IN UINT32 ModeNumber,
> > @@ -296,6 +349,14 @@ LcdPlatformGetTimings (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Return bits per pixel for a mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Bpp Pointer to value Bits Per Pixel.
> > + *
> > + * @retval EFI_SUCCESS The requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetBpp (
> > IN UINT32 ModeNumber,
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c index
> >
> 8d046816454f642bced00e29c4e02093b74afd24..84880e5fd1dfe6f824b27
> e53926f
> > 9bb32ff6cdf7 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> > +++ 11LcdArmVExpress.c
> > @@ -41,7 +41,8 @@ typedef struct {
> > UINT32 VFrontPorch;
> > } LCD_RESOLUTION;
> >
> > -
> > +/** The display modes supported by the platform.
> > +**/
> > LCD_RESOLUTION mResolutions[] = {
> > { // Mode 0 : VGA : 640 x 480 x 24 bpp
> > VGA, VGA_H_RES_PIXELS, VGA_V_RES_PIXELS,
> LCD_BITS_PER_PIXEL_24,
> > @@ -151,7 +152,11 @@ EFI_EDID_ACTIVE_PROTOCOL mEdidActive = {
> > NULL
> > };
> >
> > -
> > +/** PL111 Platform specific initialization function.
> > + *
> > + * @retval EFI_SUCCESS Plaform library initialization success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformInitializeDisplay (
> > IN EFI_HANDLE Handle
> > @@ -176,6 +181,18 @@ LcdPlatformInitializeDisplay (
> > return Status;
> > }
> >
> > +/** Reserve VRAM memory in DRAM for the frame buffer
> > + * (unless it is reserved already).
> > + *
> > + * The allocated address can be used to set the frame buffer.
> > + *
> > + * @param OUT VramBaseAddress A pointer to the frame buffer
> address.
> > + * @param OUT VramSize A pointer to the size of the frame
> > + * buffer in bytes
> > + *
> > + * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetVram (
> > OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress, @@ -232,6 +249,13
> @@
> > LcdPlatformGetVram (
> > return Status;
> > }
> >
> > +/** Return total number of modes supported.
> > + *
> > + * Note: Valid mode numbers are 0 to MaxMode - 1
> > + * See Section 11.9 of the UEFI Specification 2.6 Errata A (Jan
> > +2017)
> > + *
> > + * @retval UINT32 Mode Number.
> > +**/
> > UINT32
> > LcdPlatformGetMaxMode(VOID)
> > {
> > @@ -249,6 +273,14 @@ LcdPlatformGetMaxMode(VOID)
> > return (PcdGet32 (PcdPL111LcdMaxMode)); }
> >
> > +/** Set the requested display mode.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + *
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > + * @retval EFI_UNSUPPORTED PLL111 configuration not supported.
> > + * @retval !(EFI_SUCCESS) Other errors.
> > +**/
> > EFI_STATUS
> > LcdPlatformSetMode (
> > IN UINT32 ModeNumber
> > @@ -320,6 +352,15 @@ LcdPlatformSetMode (
> > return Status;
> > }
> >
> > +/** Return information for the requested mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Info Pointer for returned mode information
> > + * (on success).
> > + *
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformQueryMode (
> > IN UINT32 ModeNumber,
> > @@ -360,6 +401,21 @@ LcdPlatformQueryMode (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Returns the display timing information for the requested mode
> number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT HRes Pointer to horizontal resolution.
> > + * @param OUT HSync Pointer to horizontal sync width.
> > + * @param OUT HBackPorch Pointer to horizontal back porch.
> > + * @param OUT HFrontPorch Pointer to horizontal front porch.
> > + * @param OUT VRes Pointer to vertical resolution.
> > + * @param OUT VSync Pointer to vertical sync width.
> > + * @param OUT VBackPorch Pointer to vertical back porch.
> > + * @param OUT VFrontPorch Pointer to vertical front porch.
> > + *
> > + * @retval EFI_SUCCESS Success if the requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetTimings (
> > IN UINT32 ModeNumber,
> > @@ -389,6 +445,14 @@ LcdPlatformGetTimings (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Return bits per pixel for a mode number.
> > + *
> > + * @param IN ModeNumber Mode Number.
> > + * @param OUT Bpp Pointer to value Bits Per Pixel.
> > + *
> > + * @retval EFI_SUCCESS The requested mode is found.
> > + * @retval EFI_INVALID_PARAMETER Requested mode not found.
> > +**/
> > EFI_STATUS
> > LcdPlatformGetBpp (
> > IN UINT32 ModeNumber,
> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > index
> >
> eb0b6fb3fbbc1cb605469433f6c6dcb85bac668c..744dd3d556b5071defc6b
> cad5a9a
> > 30881bcb4b6f 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > @@ -29,6 +29,12 @@
> > *
> >
> >
> **********************************************************
> ************
> > /
> >
> > +/** Initialize display.
> > + *
> > + * @param VramBaseAddress Address of the frame buffer.
> > + *
> > + * @retval EFI_SUCCESS Display initialization success.
> > +**/
> > EFI_STATUS
> > LcdInitialize (
> > IN EFI_PHYSICAL_ADDRESS VramBaseAddress
> > @@ -67,6 +73,12 @@ LcdInitialize (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Set requested mode of the display.
> > + *
> > + * @param ModeNumber Display mode number.
> > + * @retval EFI_SUCCESS Display set mode success.
> > + * @retval EFI_DEVICE_ERROR If mode not found/supported.
> > +**/
> > EFI_STATUS
> > LcdSetMode (
> > IN UINT32 ModeNumber
> > @@ -136,6 +148,8 @@ LcdSetMode (
> > return EFI_SUCCESS;
> > }
> >
> > +/** De-initializes the display.
> > +**/
> > VOID
> > LcdShutdown (
> > VOID
> > @@ -145,6 +159,12 @@ LcdShutdown (
> > MmioWrite32 (HDLCD_REG_COMMAND, HDLCD_DISABLE); }
> >
> > +/** Check for presence of HDLCD.
> > + *
> > + * @retval EFI_SUCCESS Platform implements HDLCD.
> > + * @retval EFI_NOT_FOUND HDLCD display controller not
> > + * found.
> > +**/
> > EFI_STATUS
> > LcdIdentify (
> > VOID
> > diff --git
> >
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> >
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> > index
> >
> 2dd8f39873f77b1c211bff407cabe90c1795b121..c40c8e0fa6f4b5f7798aeb
> 3c8bf3
> > f261f14cb67b 100644
> > ---
> >
> a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.
> c
> > +++
> b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe
> > +++ .c
> > @@ -357,8 +357,8 @@ LcdGraphicsSetMode (
> > goto EXIT;
> > }
> >
> > - // The UEFI spec requires that we now clear the visible portions of
> > the
> > - // output display to black.
> > + /* The UEFI spec requires that we now clear the visible portions of the
> > + * output display to black. */
> >
> > // Set the fill colour to black
> > SetMem (&FillColour, sizeof (EFI_GRAPHICS_OUTPUT_BLT_PIXEL), 0x0);
> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > index
> >
> 0b0c4204fbc44bc9e90dce3d7b410ce167d9f40c..f8a3c1f8266c0a11f111c3
> 747688
> > defc0d49877c 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > @@ -26,6 +26,12 @@
> > *
> >
> >
> **********************************************************
> ************
> > /
> >
> > +/** Check for presence of PL111.
> > + *
> > + * @retval EFI_SUCCESS Platform implements PL111.
> > + * @retval EFI_NOT_FOUND PL111 display controller not
> > + * found.
> > +**/
> > EFI_STATUS
> > LcdIdentify (
> > VOID
> > @@ -48,6 +54,12 @@ LcdIdentify (
> > return EFI_NOT_FOUND;
> > }
> >
> > +/** Initialize display.
> > + *
> > + * @param VramBaseAddress Address of the frame buffer.
> > + *
> > + * @retval EFI_SUCCESS Display initialization success.
> > +**/
> > EFI_STATUS
> > LcdInitialize (
> > IN EFI_PHYSICAL_ADDRESS VramBaseAddress
> > @@ -63,6 +75,12 @@ LcdInitialize (
> > return EFI_SUCCESS;
> > }
> >
> > +/** Set requested mode of the display.
> > + *
> > + * @param ModeNumber Display mode number.
> > + * @retval EFI_SUCCESS Display set mode success.
> > + * @retval EFI_DEVICE_ERROR If mode not found/supported.
> > +**/
> > EFI_STATUS
> > LcdSetMode (
> > IN UINT32 ModeNumber
> > @@ -123,7 +141,7 @@ LcdSetMode (
> >
> > // PL111_REG_LCD_CONTROL
> > LcdControl = PL111_CTRL_LCD_EN | PL111_CTRL_LCD_BPP (LcdBpp)
> > - | PL111_CTRL_LCD_TFT | PL111_CTRL_BGR;
> > + | PL111_CTRL_LCD_TFT | PL111_CTRL_BGR;
> >
> > MmioWrite32 (PL111_REG_LCD_CONTROL, LcdControl);
> >
> > @@ -134,6 +152,8 @@ LcdSetMode (
> > return EFI_SUCCESS;
> > }
> >
> > +/** De-initializes the display.
> > +*/
> > VOID
> > LcdShutdown (
> > VOID
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
