Re: [PATCH 5/5] ArmPkg: Tidy up GenericWatchdogDxe.c

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Sep 11, 2017 at 04:23:35PM +0100, evan.lloyd@arm.com wrote:
> From: Evan Lloyd <evan.lloyd@arm.com>
> 
> This cosmetic change has no functional content.
> It adjusts comment oddities, etc, noticed during previous work.
> Specific changes are:
>     Re-order #includes
>     Use ns consistently (always "100ns" not sometimes "100 nS")

Yes, in general I prefer fewer Siemens in my code.

>     Reflow overlength comments
>     Change multiline comments to C style
>     Adjust indent for overlength code lines.
>     Replace explicit test and assert with ASSERT_EFI_ERROR.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> Signed-off-by: Alexei Fedorov <alexei.fedorov@arm.com>

No objections to any of the changes, but I would be really happy if
this was reordered such that this comes before any patches with
functional changes.
That way git blame/praise output is more helpful.

/
    Leif

> ---
>  ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c | 127 ++++++++++----------
>  1 file changed, 61 insertions(+), 66 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> index 69844db2e11f51907e6c8bff5c67d27ceb498150..7c4c9ecd4e12d433e222d7d08adf20bda1ff9842 100644
> --- a/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> +++ b/ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.c
> @@ -29,16 +29,16 @@
>  
>  #include "GenericWatchdog.h"
>  
> -// The number of 100ns periods (the unit of time passed to these functions)
> -// in a second
> +/* The number of 100ns periods (the unit of time passed to these functions)
> +   in a second */
>  #define TIME_UNITS_PER_SECOND 10000000
>  
> -// Tick frequency of the generic timer that is the basis of the generic watchdog
> +// Tick frequency of the generic timer basis of the generic watchdog.
>  UINTN mTimerFrequencyHz = 0;
>  
> -// In cases where the compare register was set manually, information about
> -// how long the watchdog was asked to wait cannot be retrieved from hardware.
> -// It is therefore stored here. 0 means the timer is not running.
> +/* In cases where the compare register was set manually, information about
> +   how long the watchdog was asked to wait cannot be retrieved from hardware.
> +   It is therefore stored here. 0 means the timer is not running. */
>  UINT64 mNumTimerTicks = 0;
>  
>  EFI_HARDWARE_INTERRUPT2_PROTOCOL *mInterruptProtocol;
> @@ -75,8 +75,7 @@ WatchdogDisable (
>    return MmioWrite32 (GENERIC_WDOG_CONTROL_STATUS_REG, GENERIC_WDOG_DISABLED);
>  }
>  
> -/**
> -    On exiting boot services we must make sure the Watchdog Timer
> +/** On exiting boot services we must make sure the Watchdog Timer
>      is stopped.
>  **/
>  VOID
> @@ -90,9 +89,8 @@ WatchdogExitBootServicesEvent (
>    mNumTimerTicks = 0;
>  }
>  
> -/*
> -  This function is called when the watchdog's first signal (WS0) goes high.
> -  It uses the ResetSystem Runtime Service to reset the board.
> +/* This function is called when the watchdog's first signal (WS0) goes high.
> +   It uses the ResetSystem Runtime Service to reset the board.
>  */
>  VOID
>  EFIAPI
> @@ -101,7 +99,7 @@ WatchdogInterruptHandler (
>    IN  EFI_SYSTEM_CONTEXT          SystemContext
>    )
>  {
> -  STATIC CONST CHAR16      ResetString[] = L"The generic watchdog timer ran out.";
> +  STATIC CONST CHAR16 ResetString[]= L"The generic watchdog timer ran out.";
>  
>    WatchdogDisable ();
>  
> @@ -126,10 +124,10 @@ WatchdogInterruptHandler (
>    then the new handler is registered and EFI_SUCCESS is returned.
>    If NotifyFunction is NULL, and a handler is already registered,
>    then that handler is unregistered.
> -  If an attempt is made to register a handler when a handler is already registered,
> -  then EFI_ALREADY_STARTED is returned.
> -  If an attempt is made to unregister a handler when a handler is not registered,
> -  then EFI_INVALID_PARAMETER is returned.
> +  If an attempt is made to register a handler when a handler is already
> +  registered, then EFI_ALREADY_STARTED is returned.
> +  If an attempt is made to unregister a handler when a handler is not
> +  registered, then EFI_INVALID_PARAMETER is returned.
>  
>    @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
>    @param  NotifyFunction   The function to call when a timer interrupt fires.
> @@ -139,11 +137,7 @@ WatchdogInterruptHandler (
>                             information is used to signal timer based events.
>                             NULL will unregister the handler.
>  
> -  @retval EFI_SUCCESS           The watchdog timer handler was registered.
> -  @retval EFI_ALREADY_STARTED   NotifyFunction is not NULL, and a handler is already
> -                                registered.
> -  @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler was not
> -                                previously registered.
> +  @retval EFI_UNSUPPORTED       The code does not support NotifyFunction.
>  
>  **/
>  EFI_STATUS
> @@ -160,18 +154,18 @@ WatchdogRegisterHandler (
>  
>  /**
>    This function sets the amount of time to wait before firing the watchdog
> -  timer to TimerPeriod 100 nS units.  If TimerPeriod is 0, then the watchdog
> +  timer to TimerPeriod 100ns units.  If TimerPeriod is 0, then the watchdog
>    timer is disabled.
>  
>    @param  This             The EFI_WATCHDOG_TIMER_ARCH_PROTOCOL instance.
> -  @param  TimerPeriod      The amount of time in 100 nS units to wait before the watchdog
> -                           timer is fired. If TimerPeriod is zero, then the watchdog
> -                           timer is disabled.
> +  @param  TimerPeriod      The amount of time in 100ns units to wait before
> +                           the watchdog timer is fired. If TimerPeriod is zero,
> +                           then the watchdog timer is disabled.
>  
> -  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire in Time
> -                                100 nS units.
> -  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due to a device
> -                                error.
> +  @retval EFI_SUCCESS           The watchdog timer has been programmed to fire
> +                                in Time  100ns units.
> +  @retval EFI_DEVICE_ERROR      A watchdog timer could not be programmed due
> +                                to a device error.
>  
>  **/
>  EFI_STATUS
> @@ -184,7 +178,7 @@ WatchdogSetTimerPeriod (
>    UINTN       SystemCount;
>    EFI_STATUS  Status;
>  
> -  // if TimerPerdiod is 0, this is a request to stop the watchdog.
> +  // if TimerPeriod is 0, this is a request to stop the watchdog.
>    if (TimerPeriod == 0) {
>      mNumTimerTicks = 0;
>      return WatchdogDisable ();
> @@ -193,17 +187,16 @@ WatchdogSetTimerPeriod (
>    // Work out how many timer ticks will equate to TimerPeriod
>    mNumTimerTicks = (mTimerFrequencyHz * TimerPeriod) / TIME_UNITS_PER_SECOND;
>  
> -
> -  // If the number of required ticks is greater than the max number the
> -  // watchdog's offset register (WOR) can hold, we need to manually compute and
> -  // set the compare register (WCV)
> +  /* If the number of required ticks is greater than the max the watchdog's
> +     offset register (WOR) can hold, we need to manually compute and set
> +     the compare register (WCV) */
>  
>    if (mNumTimerTicks > MAX_UINT32) {
>  
> -    // We need to enable the watchdog *before* writing to the compare register,
> -    // because enabling the watchdog causes an "explicit refresh", which
> -    // clobbers the compare register (WCV). In order to make sure this doesn't
> -    // trigger an interrupt, set the offset to max.
> +    /* We need to enable the watchdog *before* writing to the compare register,
> +       because enabling the watchdog causes an "explicit refresh", which
> +       clobbers the compare register (WCV). In order to make sure this doesn't
> +       trigger an interrupt, set the offset to max. */
>  
>      Status = WatchdogWriteOffsetRegister (MAX_UINT32);
>      if (EFI_ERROR (Status)) {
> @@ -221,14 +214,14 @@ WatchdogSetTimerPeriod (
>  }
>  
>  /**
> -  This function retrieves the period of timer interrupts in 100 ns units,
> +  This function retrieves the period of timer interrupts in 100ns units,
>    returns that value in TimerPeriod, and returns EFI_SUCCESS.  If TimerPeriod
>    is NULL, then EFI_INVALID_PARAMETER is returned.  If a TimerPeriod of 0 is
>    returned, then the timer is currently disabled.
>  
>    @param  This             The EFI_TIMER_ARCH_PROTOCOL instance.
> -  @param  TimerPeriod      A pointer to the timer period to retrieve in 100
> -                           ns units. If 0 is returned, then the timer is
> +  @param  TimerPeriod      A pointer to the timer period to retrieve in
> +                           100ns units. If 0 is returned, then the timer is
>                             currently disabled.
>  
>  
> @@ -275,19 +268,19 @@ WatchdogGetTimerPeriod (
>          this function will not have any chance of executing.
>  
>    @param SetTimerPeriod
> -  Sets the period of the timer interrupt in 100 nS units.
> +  Sets the period of the timer interrupt in 100ns units.
>    This function is optional, and may return EFI_UNSUPPORTED.
>    If this function is supported, then the timer period will
>    be rounded up to the nearest supported timer period.
>  
>    @param GetTimerPeriod
> -  Retrieves the period of the timer interrupt in 100 nS units.
> +  Retrieves the period of the timer interrupt in 100ns units.
>  
>  **/
>  EFI_WATCHDOG_TIMER_ARCH_PROTOCOL    gWatchdogTimer = {
> -  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER) WatchdogRegisterHandler,
> -  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD) WatchdogSetTimerPeriod,
> -  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD) WatchdogGetTimerPeriod
> +  (EFI_WATCHDOG_TIMER_REGISTER_HANDLER)WatchdogRegisterHandler,
> +  (EFI_WATCHDOG_TIMER_SET_TIMER_PERIOD)WatchdogSetTimerPeriod,
> +  (EFI_WATCHDOG_TIMER_GET_TIMER_PERIOD)WatchdogGetTimerPeriod
>  };
>  
>  EFI_EVENT                           EfiExitBootServicesEvent = (EFI_EVENT)NULL;
> @@ -303,9 +296,9 @@ GenericWatchdogEntry (
>    EFI_HANDLE                      Handle;
>  
>  
> -  // Make sure the Watchdog Timer Architectural Protocol has not been installed
> -  // in the system yet.
> -  // This will avoid conflicts with the universal watchdog
> +  /* Make sure the Watchdog Timer Architectural Protocol has not been installed
> +     in the system yet.
> +     This will avoid conflicts with the universal watchdog */
>  
>    ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEfiWatchdogTimerArchProtocolGuid);
>  
> @@ -314,8 +307,11 @@ GenericWatchdogEntry (
>  
>    // Register for an ExitBootServicesEvent
>    Status = gBS->CreateEvent (
> -                  EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_NOTIFY,
> -                  WatchdogExitBootServicesEvent, NULL, &EfiExitBootServicesEvent
> +                  EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                  TPL_NOTIFY,
> +                  WatchdogExitBootServicesEvent,
> +                  NULL,
> +                  &EfiExitBootServicesEvent
>                    );
>    if (!EFI_ERROR (Status)) {
>      // Install interrupt handler
> @@ -326,32 +322,31 @@ GenericWatchdogEntry (
>                      );
>      if (!EFI_ERROR (Status)) {
>        Status = mInterruptProtocol->RegisterInterruptSource (
> -                                    mInterruptProtocol,
> -                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> -                                    WatchdogInterruptHandler
> -                                    );
> +                 mInterruptProtocol,
> +                 FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> +                 WatchdogInterruptHandler
> +                 );
>        if (!EFI_ERROR (Status)) {
>          Status = mInterruptProtocol->SetTriggerType (
> -                                    mInterruptProtocol,
> -                                    FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> -                                    EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING);
> +                   mInterruptProtocol,
> +                   FixedPcdGet32 (PcdGenericWatchdogEl2IntrNum),
> +                   EFI_HARDWARE_INTERRUPT2_TRIGGER_EDGE_RISING
> +                   );
>          if (!EFI_ERROR (Status)) {
>            // Install the Timer Architectural Protocol onto a new handle
>            Handle = NULL;
>            Status = gBS->InstallMultipleProtocolInterfaces (
> -                          &Handle,
> -                          &gEfiWatchdogTimerArchProtocolGuid, &gWatchdogTimer,
> -                          NULL
> -                          );
> +                     &Handle,
> +                     &gEfiWatchdogTimerArchProtocolGuid,
> +                     &gWatchdogTimer,
> +                     NULL
> +                     );
>          }
>        }
>      }
>    }
>  
> -  if (EFI_ERROR (Status)) {
> -    // The watchdog failed to initialize
> -    ASSERT (FALSE);
> -  }
> +  ASSERT_EFI_ERROR (Status);
>  
>    mNumTimerTicks = 0;
>    WatchdogDisable ();
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>