Re: [PATCH edk2-platforms 3/8] Silicon/NXP/Pcf8563RealTimeClockLib: avoid driver binding protocol

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

On Thu, Jan 25, 2018 at 12:27:31PM +0000, Ard Biesheuvel wrote:
> Instead of registering a notification callback on the driver binding
> protocol, and attempting to connect our I2C master handle each time
> a new driver is registered, switch to the more obvious approach of
> registering a notification callback on the I2C master protocol directly.
> 
> The original code was written under the assumption that it would make
> the RTC available at an earlier time, but given that all handles that
> are created during the execution of a driver entry point are connected
> by DXE core right away (i.e., before StartImage() returns), this is not
> really necessary, and in fact, may result in the driver already having
> been connected by the time we attempt to connect it.
> 
> Note that it is now up to the platform to ensure that ConnectController()
> is called for the handle if DXE core does not call it by itself, or does
> call it but at a time when no I2C master protocol driver is available
> yet.

Presumably the platforms in edk2-platforms using this library already
follow these constraints?

If so:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c   | 31 ++++++++------------
>  Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf |  1 -
>  2 files changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> index 6bc4aef28849..fb58e1feb424 100644
> --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.c
> @@ -41,7 +41,7 @@
>  #define EPOCH_BASE                2000
>  
>  STATIC EFI_HANDLE                 mI2cMasterHandle;
> -STATIC VOID                       *mDriverEventRegistration;
> +STATIC VOID                       *mI2cMasterEventRegistration;
>  STATIC EFI_I2C_MASTER_PROTOCOL    *mI2cMaster;
>  STATIC EFI_EVENT                  mRtcVirtualAddrChangeEvent;
>  
> @@ -263,12 +263,12 @@ LibSetWakeupTime (
>  
>  STATIC
>  VOID
> -DriverRegistrationEvent (
> +I2cMasterRegistrationEvent (
>    IN  EFI_EVENT       Event,
>    IN  VOID            *Context
>    )
>  {
> -  EFI_HANDLE                Handle[2];
> +  EFI_HANDLE                Handle;
>    UINTN                     BufferSize;
>    EFI_STATUS                Status;
>    EFI_I2C_MASTER_PROTOCOL   *I2cMaster;
> @@ -280,10 +280,10 @@ DriverRegistrationEvent (
>    do {
>      BufferSize = sizeof (EFI_HANDLE);
>      Status = gBS->LocateHandle (ByRegisterNotify,
> -                                &gEfiDriverBindingProtocolGuid,
> -                                mDriverEventRegistration,
> +                                &gEfiI2cMasterProtocolGuid,
> +                                mI2cMasterEventRegistration,
>                                  &BufferSize,
> -                                Handle);
> +                                &Handle);
>      if (EFI_ERROR (Status)) {
>        if (Status != EFI_NOT_FOUND) {
>          DEBUG ((DEBUG_WARN, "%a: gBS->LocateHandle () returned %r\n",
> @@ -292,12 +292,7 @@ DriverRegistrationEvent (
>        break;
>      }
>  
> -    //
> -    // Check if we can connect our handle to this driver.
> -    //
> -    Handle[1] = NULL;
> -    Status = gBS->ConnectController (mI2cMasterHandle, Handle, NULL, FALSE);
> -    if (EFI_ERROR (Status)) {
> +    if (Handle != mI2cMasterHandle) {
>        continue;
>      }
>  
> @@ -378,16 +373,16 @@ LibRtcInitialize (
>    ASSERT_EFI_ERROR (Status);
>  
>    //
> -  // Register a protocol registration notification callback on the driver
> -  // binding protocol so we can attempt to connect our I2C master to it
> -  // as soon as it appears.
> +  // Register a protocol registration notification callback on the I2C master
> +  // protocol. This will notify us even if the protocol instance we are looking
> +  // for has already been installed.
>    //
>    EfiCreateProtocolNotifyEvent (
> -    &gEfiDriverBindingProtocolGuid,
> +    &gEfiI2cMasterProtocolGuid,
>      TPL_CALLBACK,
> -    DriverRegistrationEvent,
> +    I2cMasterRegistrationEvent,
>      NULL,
> -    &mDriverEventRegistration);
> +    &mI2cMasterEventRegistration);
>  
>    //
>    // Register for the virtual address change event
> diff --git a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf
> index 1a9a6f6c9cf3..e232902c6b5d 100644
> --- a/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf
> +++ b/Silicon/NXP/Library/Pcf8563RealTimeClockLib/Pcf8563RealTimeClockLib.inf
> @@ -40,7 +40,6 @@ [Guids]
>    gEfiEventVirtualAddressChangeGuid
>  
>  [Protocols]
> -  gEfiDriverBindingProtocolGuid                   ## CONSUMES
>    gEfiI2cMasterProtocolGuid                       ## CONSUMES
>    gPcf8563RealTimeClockLibI2cMasterProtocolGuid   ## CONSUMES
>  
> -- 
> 2.11.0
>