Re: [platforms: PATCH v2 06/12] Marvell/Drivers: MvBoardDesc: Extend protocol with GPIO support

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

On Tue, Jan 15, 2019 at 11:05:12AM +0100, Marcin Wojtas wrote:
> > > Anyway, I tried to play with the MV_BOARD_GPIO_DESCRIPTION to be the
> > > global variable, but was not convinced by the outcome. My biggest
> > > objection to the global variable and checking whether it's NULL in the
> > > consumer driver is following - until now all users of the
> > > BoardDescProtocol 'know' and call only the relevant protocol routine,
> > > everything what happens underneath (PCDs, variable names, etc) is
> > > transparent.
> > >
> > > The consumers should be responsible for avoiding the memory leaks and
> > > this was an improvement of v2. Both MvGpioDxe and MvPca9xxxDxe have
> > > now fixed error path an properly take care of freeing the memory after
> > > using protocol.
> >
> > But the call sites don't keep track of freeing it when error handling.
> >
> > Which I think serves as a clear demonstrator of how magically
> > allocating buffers makes for difficult code to keep correct. (Which is
> > why the UEFI intefaces all require you to allocate a buffer and then
> > pass that and a size as parameters.)
> >
> > So, since we are dealing with data that isn't changing, I prefer the
> > original design - just not the original implemenation.
> >
> > So how about sticking with that, but moving the STATIC struct global
> > in that source file (but keeping it STATIC)?
> >
> 
> Good, keeping the global variable inside MvBoardDescDxe and checking
> it there is a clean and easy solution (consumers won't have to bother
> about the stuff additional to calling the protocol and error handling
> will be simpler).
> 
> How about on top of the file I add a section for global varibles (IMO
> it's worth to modify other interfaces to that scheme later) and call
> it:
> 
> STATIC MV_BOARD_GPIO_DESCRIPTION gGpioDescription;
> ?

Static, so 'm', not 'g', but yeah.

Regards,

Leif