Re: [PATCH v2 09/13] ArmPlatformPkg: PCD to swap red/blue format for HDLCD

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 December 2017 at 18:34,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak at arm.com>
>
> This change adds a new PCD PcdArmHdlcdSwapBlueRedSelect
> to swap values for HDLCD RED_SELECT and BLUE_SELECT registers
> on platforms where blue and red hardware lines are swapped.
>
> If set to TRUE in the platform dsc, HDLCD library will swap the values
> while setting RED_SELECT and BLUE_SELECT registers. The default
> value of the PCD is FALSE.
>
> NOTE: The motive for this is that a discrepancy in the Red/Blue lines
> exists between some VersatileExpress platforms.  Rather than have
> divergent code, this build switch allows a simple, pragmatic solution.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  ArmPlatformPkg/ArmPlatformPkg.dec      | 3 +++
>  ArmPlatformPkg/Library/HdLcd/HdLcd.inf | 2 ++
>  ArmPlatformPkg/Library/HdLcd/HdLcd.c   | 4 ++++
>  3 files changed, 9 insertions(+)
>
> diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec
> index e412414e0ba6506c7158e69bac04469e45601736..9a61e4a511024c80e787500de5038779363f0d95 100644
> --- a/ArmPlatformPkg/ArmPlatformPkg.dec
> +++ b/ArmPlatformPkg/ArmPlatformPkg.dec
> @@ -104,6 +104,9 @@ [PcdsFixedAtBuild.common]
>    # Default is set to UEFI console font format PixelBlueGreenRedReserved8BitPerColor
>    gArmPlatformTokenSpaceGuid.PcdGopPixelFormat|0x00000001|UINT32|0x00000040
>
> +  ## If set, this will swap settings for HDLCD RED_SELECT and BLUE_SELECT registers
> +  gArmPlatformTokenSpaceGuid.PcdArmHdLcdSwapBlueRedSelect|FALSE|BOOLEAN|0x00000045
> +
>  [PcdsFixedAtBuild.common,PcdsDynamic.common]
>    ## PL031 RealTimeClock
>    gArmPlatformTokenSpaceGuid.PcdPL031RtcBase|0x0|UINT32|0x00000024
> diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.inf b/ArmPlatformPkg/Library/HdLcd/HdLcd.inf
> index 67aad05d210b95b2d23b8e52e4392685efcf3795..0f440d1ef730159a8be3780667700979b607a1e2 100644
> --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.inf
> +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.inf
> @@ -40,3 +40,5 @@ [LibraryClasses]
>
>  [FixedPcd]
>    gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase
> +  gArmPlatformTokenSpaceGuid.PcdArmHdLcdSwapBlueRedSelect
> +
> diff --git a/ArmPlatformPkg/Library/HdLcd/HdLcd.c b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> index 72cd5fa33b2553195638c595e72843a56b2e267c..4f802977e8b55ed83364d3ec8fa46082e128c76b 100644
> --- a/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> +++ b/ArmPlatformPkg/Library/HdLcd/HdLcd.c
> @@ -98,7 +98,11 @@ LcdSetMode (
>      return Status;
>    }
>
> +#if (!FixedPcdGetBool (PcdArmHdLcdSwapBlueRedSelect))
>    if (ModeInfo.PixelFormat == PixelBlueGreenRedReserved8BitPerColor) {
> +#else
> +  if (ModeInfo.PixelFormat == PixelRedGreenBlueReserved8BitPerColor) {
> +#endif

Please don't nest CPP and C conditionals like this. It is difficult to
follow, and results in poor build time coverage (the non-taken branch
at the CPP level is never seen by the compiler)

I suggest you use a variable for the RHS of the inner if, or find some
other way to get rid of the #if/#else/#endif

Thanks,
Ard.


>      MmioWrite32 (HDLCD_REG_RED_SELECT,  (8 << 8) | 16);
>      MmioWrite32 (HDLCD_REG_BLUE_SELECT, (8 << 8) | 0);
>    } else {
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>