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:14:16AM +0100, Marcin Wojtas wrote:
> wt., 15 sty 2019 o 11:12 Leif Lindholm <leif.lindholm@linaro.org> napisał(a):
> >
> > 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.
> 
> Effectively it will be global to other files :) But I will change to
> 'm' prefix of course.

Global is about visibility, not accessibility. This will not be
globally visible.

/
    Leif