On Wed, Feb 10, 2016 at 05:11:25PM +0100, Ard Biesheuvel wrote:
> 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.
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 <[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
