Needs an actual commit message. What GPIO controller? If it does not have an explicit name, what family of devices is it in?
On Tue, Sep 15, 2020 at 21:59:00 +0530, Meenakshi Aggarwal wrote: > Signed-off-by: Pramod Kumar <pramod.kuma...@nxp.com> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggar...@nxp.com> Only the poster can sign off that the post is being submitted in accordance with the Developer's Certificate of Origin: https://developercertificate.org/ If the author is different from the poster, that should be described in the patch metadata (i.e. Author: ). I have permitted (although I'm not a fan) Co-authored-by: tags, if that is what this is intended to describe. > --- > Silicon/NXP/Library/GpioLib/GpioLib.inf | 39 +++++ > Silicon/NXP/Include/Library/GpioLib.h | 110 +++++++++++++++ > Silicon/NXP/Library/GpioLib/GpioLib.c | 242 > ++++++++++++++++++++++++++++++++ > 3 files changed, 391 insertions(+) > create mode 100644 Silicon/NXP/Library/GpioLib/GpioLib.inf > create mode 100644 Silicon/NXP/Include/Library/GpioLib.h > create mode 100644 Silicon/NXP/Library/GpioLib/GpioLib.c > > diff --git a/Silicon/NXP/Library/GpioLib/GpioLib.inf > b/Silicon/NXP/Library/GpioLib/GpioLib.inf > new file mode 100644 > index 000000000000..7878d1d03db2 > --- /dev/null > +++ b/Silicon/NXP/Library/GpioLib/GpioLib.inf > @@ -0,0 +1,39 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = GpioLib > + FILE_GUID = addec2b8-d2e0-43c0-a277-41a8d42f3f4f > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = GpioLib > + > +[Sources.common] > + GpioLib.c > + > +[LibraryClasses] > + ArmLib > + BaseMemoryLib > + BaseLib Flip order of above two lines. > + IoAccessLib > + IoLib > + > +[Packages] > + ArmPlatformPkg/ArmPlatformPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + Silicon/NXP/NxpQoriqLs.dec > + > +[Pcd] > + gNxpQoriqLsTokenSpaceGuid.PcdNumGpioController > + gNxpQoriqLsTokenSpaceGuid.PcdGpioModuleBaseAddress > + gNxpQoriqLsTokenSpaceGuid.PcdGpioControllerOffset > + > +[FeaturePcd] > + gNxpQoriqLsTokenSpaceGuid.PcdGpioControllerBigEndian > diff --git a/Silicon/NXP/Include/Library/GpioLib.h > b/Silicon/NXP/Include/Library/GpioLib.h > new file mode 100644 > index 000000000000..5821806226ee > --- /dev/null > +++ b/Silicon/NXP/Include/Library/GpioLib.h > @@ -0,0 +1,110 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef GPIO_H__ > +#define GPIO_H__ > + > +#include <Uefi.h> > + > +/* enum for GPIO number */ > +typedef enum _GPIO_BLOCK { > + GPIO1, > + GPIO2, > + GPIO3, > + GPIO4, > + GPIO_MAX > +} GPIO_BLOCK; > + > +/* enum for GPIO direction */ > +typedef enum _GPIO_DIRECTION { > + INPUT, > + OUTPUT > +} GPIO_DIRECTION; > + > +/* enum for GPIO state */ > +typedef enum _GPIO_STATE { > + LOW, > + HIGH > +} GPIO_VAL; > + > +/** > + SetDir Set GPIO direction as INPUT or OUTPUT > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Dir GPIO Direction as INPUT or OUTPUT > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetDir ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Dir > + ); > + > +/** > + GetDir Retrieve GPIO direction > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO Direction as INPUT or OUTPUT > + **/ > +UINT32 > +GetDir ( > + IN UINT8 Id, > + IN UINT32 Bit > + ); > + > + /** > + GetData Retrieve GPIO Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO value as HIGH or LOW > + **/ > +UINT32 > +GetData ( > + IN UINT8 Id, > + IN UINT32 Bit > + ); > + > +/** > + SetData Set GPIO data Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Data GPIO data value to set > + > + @retval GPIO value as HIGH or LOW > + **/ > +EFI_STATUS > +SetData ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Data > + ); > + > +/** > + SetOpenDrain Set GPIO as Open drain > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] OpenDrain Set as open drain > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetOpenDrain ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN OpenDrain > + ); > + > +#endif > diff --git a/Silicon/NXP/Library/GpioLib/GpioLib.c > b/Silicon/NXP/Library/GpioLib/GpioLib.c > new file mode 100644 > index 000000000000..33cc45c2152b > --- /dev/null > +++ b/Silicon/NXP/Library/GpioLib/GpioLib.c > @@ -0,0 +1,242 @@ > +/** @file > + Which controller is this for. There should be a comment here sufficient for me to figure out where I should start looking for documentation. > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include <Library/GpioLib.h> > +#include <Library/IoAccessLib.h> > +#include <Library/IoLib.h> > +#include <Library/DebugLib.h> > +#include <Library/BaseLib.h> > + > +STATIC MMIO_OPERATIONS *mGpioOps; > + > +/* Structure for GPIO Regsters */ > +typedef struct GpioRegs { > + UINT32 GpDir; > + UINT32 GpOdr; > + UINT32 GpData; > + UINT32 GpIer; > + UINT32 GpImr; > + UINT32 GpIcr; Are the above registers the official names as per the documentation? If so, this is fine even though it violates the CamelCase naming style, but please add a glossary entry to the top-of-file comment block. > +} GPIO_REGS; > + > +/** > + GetBaseAddr GPIO controller Base Address > + > + @param[in] Id GPIO controller number > + > + @retval GPIO controller Base Address, if found > + @retval NULL, if not a valid controller number > + > + **/ > +STATIC > +VOID * > +GetBaseAddr ( > + IN UINT8 Id > + ) > +{ > + > + UINTN GpioBaseAddr; > + UINTN MaxGpioController; > + > + mGpioOps = GetMmioOperations (FeaturePcdGet (PcdGpioControllerBigEndian)); > + > + MaxGpioController = PcdGet32 (PcdNumGpioController); > + > + if (Id < MaxGpioController) { > + GpioBaseAddr = PcdGet64 (PcdGpioModuleBaseAddress) + > + (Id * PcdGet64 (PcdGpioControllerOffset)); > + return (VOID *) GpioBaseAddr; No space after ). > + } > + else { Move to line above. > + DEBUG((DEBUG_ERROR, "Invalid Gpio Controller Id %d, Allowed Ids are > %d-%d", > + Id, GPIO1, MaxGpioController)); > + return NULL; > + } > +} > + > +/** > + GetBitMask: Return Bit Mask > + > + @param[in] Bit Bit to create bitmask > + @retval Bitmask > + > + **/ > + > +STATIC > +UINT32 > +GetBitMask ( > + IN UINT32 Bit > + ) > +{ > + > + if (!FeaturePcdGet (PcdGpioControllerBigEndian)) { > + return (1 << Bit); > + } else { > + return (1 << (31 - Bit)); > + } The above confuses me greatly - endianness affects byte order, not bit order. Is this some compensation for PowerPC numbering bits backwards, and these reused blocks being affected by this when in big-endian mode? Needs a very descriptive comment. The current function comment block contains no information, it just keeps repeating the function name in different word order. > +} > + > + > +/** > + SetDir Set GPIO direction as INPUT or OUTPUT > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Dir GPIO Direction as INPUT or OUTPUT > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetDir ( SetDirection > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Dir > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; The variable should be called something descriptive. You are using it to read a specific value out of a specific register. In this instance, DirectionMask would be a better alternative. Please address accordingly in functions below, with appropriate descriptive names for each location. > + UINT32 Value; > + > + Regs = GetBaseAddr(Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpDir); > + > + if (Dir) { > + mGpioOps->Write32 ((UINTN)&Regs->GpDir, (Value | BitMask)); > + } > + else { > + mGpioOps->Write32 ((UINTN)&Regs->GpDir, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + GetDir Retrieve GPIO direction > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO Direction as INPUT or OUTPUT > + **/ > +UINT32 > +GetDir ( GetDirection > + IN UINT8 Id, > + IN UINT32 Bit > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 Value; > + UINT32 BitMask; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpDir); > + > + return (Value & BitMask); > +} > + > +/** > + GetData Retrieve GPIO Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + > + @retval GPIO value as HIGH or LOW > + **/ > +UINT32 > +GetData ( > + IN UINT8 Id, > + IN UINT32 Bit > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 Value; > + UINT32 BitMask; > + > + Regs = (VOID *)GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Spurious extra blank line. > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpData); > + > + if (Value & BitMask) { > + return 1; > + } else { > + return 0; > + } return (Value & BitMask) == BitMask; ? > +} > + > +/** > + SetData Set GPIO data Value > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] Data GPIO data value to set > + > + @retval GPIO value as HIGH or LOW > + **/ > +EFI_STATUS > +SetData ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN Data > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; > + UINT32 Value; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpData); > + > + if (Data) { > + mGpioOps->Write32 ((UINTN)&Regs->GpData, (Value | BitMask)); > + } else { > + mGpioOps->Write32 ((UINTN)&Regs->GpData, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + SetOpenDrain Set GPIO as Open drain > + > + @param[in] Id GPIO controller number > + @param[in] Bit GPIO number > + @param[in] OpenDrain Set as open drain > + > + @retval EFI_SUCCESS > + **/ > +EFI_STATUS > +SetOpenDrain ( > + IN UINT8 Id, > + IN UINT32 Bit, > + IN BOOLEAN OpenDrain > + ) > +{ > + GPIO_REGS *Regs; > + UINT32 BitMask; > + UINT32 Value; > + > + Regs = GetBaseAddr (Id); > + BitMask = GetBitMask(Bit); Missing space before (. > + > + Value = mGpioOps->Read32 ((UINTN)&Regs->GpOdr); > + if (OpenDrain) { > + mGpioOps->Write32 ((UINTN)&Regs->GpOdr, (Value | BitMask)); > + } > + else { Move to line above. / Leif > + mGpioOps->Write32 ((UINTN)&Regs->GpOdr, (Value & (~BitMask))); > + } > + > + return EFI_SUCCESS; > +} > -- > 1.9.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65621): https://edk2.groups.io/g/devel/message/65621 Mute This Topic: https://groups.io/mt/76861996/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
