Re: [platforms: PATCH 02/12] Marvell/Library: ArmadaBoardDescLib: Add GPIO information

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

On Wed, Nov 14, 2018 at 01:12:22AM +0000, Leif Lindholm wrote:
> On Sat, Oct 20, 2018 at 03:57:31AM +0200, Marcin Wojtas wrote:
> > This patch extends library with GPIO devices per-board
> > description. Both embedded SoC controllers and
> > I2C IO expanders are supported. Add a helper routine
> > for obtaining information about the latter.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h | 23 ++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > index ee8e06e..109164c 100644
> > --- a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > +++ b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h
> > @@ -25,6 +25,29 @@ typedef struct {
> >  } MV_BOARD_COMPHY_DESC;
> >  
> >  //
> > +// GPIO devices per-board description
> > +//
> > +typedef struct {
> > +  UINTN ChipId;
> > +  UINTN I2cAddress;
> > +  UINTN I2cBus;
> > +} MV_I2C_IO_EXPANDER_DESC;
> > +
> > +typedef struct {
> > +  MV_SOC_GPIO_DESC        *SoC;
> > +  UINTN                    GpioDevCount;
> > +  MV_I2C_IO_EXPANDER_DESC *I2cIoExpanderDesc;
> > +  UINTN                    I2cIoExpanderCount;
> > +} MV_BOARD_GPIO_DESC;
> > +
> > +EFI_STATUS
> > +EFIAPI
> > +ArmadaBoardDescGpioGet (
> > +  IN OUT MV_I2C_IO_EXPANDER_DESC **I2cIoExpanderDesc,
> > +  IN OUT UINTN                    *I2cIoExpanderCount
> 
> Please expand all DESC/Desc to descriptor/description/whatever as
> appropriate.
> With that, Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Actually, I spotted something in 3-4/12 on finally getting back to this
set. The function is called ArmadaBoardDescGpioGet, but the IN/OUT
parameters talk abour I2c.

This seems like either a lack of abstraction (which given this is a
generic library class seems suboptimal).

Could that too be addressed in v2? (I'll mention it here instead of in
the users.)

Regards,

Leif

> > +  );
> > +
> > +//
> >  // I2C devices per-board description
> >  //
> >  typedef struct {
> > -- 
> > 2.7.4
> >