On Wed, Feb 10, 2016 at 05:11:25PM +0100, Ard Biesheuvel wrote: > On 10 February 2016 at 16:55, Leif Lindholm <leif.lindh...@linaro.org> 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.
Yeah, that's a good point. Since I knew I was invasively changing the world underneath Haojian's series I was kind of trying to keep the diff minimal, and didn't really look at what the result looked like :) Fixing this for a future PATCH version. Thanks! / Leif > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Leif Lindholm <leif.lindh...@linaro.org> > > --- > > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel