On 10 February 2016 at 16:55, Leif Lindholm <[email protected]> wrote: > For whatever reason, every single operation on the PL061 looked for an > "Initialized" flag, and manually called the initialization function if > not set. Move this to a single call on protocol installation. >
I think it is not entirely unreasonable to only install the protocol in the constructor, and not interface with the actual hardware unless it is actually used. However, in this case, the only initialization that takes places is confirming that we are talking to a supported revision of the PL061, which is something that /does/ make sense, since the protocol should not be installed in the first place if that is not the case. That does mean that it would be better to remove PL061Initialize() entirely, and call PL061Identify() from the constructor instead. > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Leif Lindholm <[email protected]> > --- > ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c | 39 > ++++--------------------- > 1 file changed, 6 insertions(+), 33 deletions(-) > > diff --git a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c > b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c > index 492cd16..7fa9ab6 100644 > --- a/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c > +++ b/ArmPlatformPkg/Drivers/PL061GpioDxe/PL061Gpio.c > @@ -28,7 +28,6 @@ > #include <Protocol/EmbeddedGpio.h> > #include <Drivers/PL061Gpio.h> > > -BOOLEAN mPL061Initialized = FALSE; > > /** > Function implementations > @@ -79,8 +78,6 @@ PL061Initialize ( > // // Ensure interrupts are disabled > //} > > - mPL061Initialized = TRUE; > - > EXIT: > return Status; > } > @@ -110,30 +107,19 @@ Get ( > OUT UINTN *Value > ) > { > - EFI_STATUS Status = EFI_SUCCESS; > - > if ( (Value == NULL) > || (Gpio > LAST_GPIO_PIN)) > { > return EFI_INVALID_PARAMETER; > } > > - // Initialize the hardware if not already done > - if (!mPL061Initialized) { > - Status = PL061Initialize(); > - if (EFI_ERROR(Status)) { > - goto EXIT; > - } > - } > - > if (MmioRead8 (PL061_GPIO_DATA_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) { > *Value = 1; > } else { > *Value = 0; > } > > - EXIT: > - return Status; > + return EFI_SUCCESS; > } > > /** > @@ -170,14 +156,6 @@ Set ( > goto EXIT; > } > > - // Initialize the hardware if not already done > - if (!mPL061Initialized) { > - Status = PL061Initialize(); > - if (EFI_ERROR(Status)) { > - goto EXIT; > - } > - } > - > switch (Mode) > { > case GPIO_MODE_INPUT: > @@ -234,22 +212,12 @@ GetMode ( > OUT EMBEDDED_GPIO_MODE *Mode > ) > { > - EFI_STATUS Status; > - > // Check for errors > if ( (Mode == NULL) > || (Gpio > LAST_GPIO_PIN)) { > return EFI_INVALID_PARAMETER; > } > > - // Initialize the hardware if not already done > - if (!mPL061Initialized) { > - Status = PL061Initialize(); > - if (EFI_ERROR(Status)) { > - return Status; > - } > - } > - > // Check if it is input or output > if (MmioRead8 (PL061_GPIO_DIR_REG) & GPIO_PIN_MASK_HIGH_8BIT(Gpio)) { > // Pin set to output > @@ -330,6 +298,11 @@ PL061InstallProtocol ( > // > ASSERT_PROTOCOL_ALREADY_INSTALLED (NULL, &gEmbeddedGpioProtocolGuid); > > + Status = PL061Initialize(); > + if (EFI_ERROR(Status)) { > + return EFI_DEVICE_ERROR; > + } > + > // Install the Embedded GPIO Protocol onto a new handle > Handle = NULL; > Status = gBS->InstallMultipleProtocolInterfaces( > -- > 2.1.4 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
