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

[Marcin Wojtas <mw@semihalf.com>]

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.

Thanks,
Marcin