[U-Boot] want to clarify a couple things about vendor common/ directories

[U-Boot] want to clarify a couple things about vendor common/ directories

[Robert P. J. Day]

  currently refactoring a pile of (closely-related) board support code
and can clearly stuff a lot of duplicate code and header files into a
vendor-specific "common/" directory so just want to make sure i'm
doing this properly.

  first, i can see from the top-level Makefile that a common board
directory is identified specifically by containing a Makefile:

HAVE_VENDOR_COMMON_LIB = $(if $(wildcard
  $(srctree)/board/$(VENDOR)/common/Makefile),y,n)

but not necessarily a Kconfig file.

  (in fact, i can see that of the several vendors that have common/
directories, only ti/common/ has a Kconfig file, so i'm concluding
that a common/ directory containing a Kconfig file is more the
exception rather than the norm. ti/common/ seems like a special case,
in that it contains just some board_detect code, and its Kconfig would
be explicitly sourced by the subset of ti boards for which it's
relevant, so that makes sense. but, as i mentioned, that's the only
example i see.)

  next, when including common header files, it seems clear that board
files need to refer explicitly to that sibling directory, as in:

  #include "../common/qixis.h"

i suppose it might have been possible for the build process to add the
common directory to the include search path for header files, but it's
clear that wasn't done so common header file inclusion *needs* that
"../common/whatever.h" form, correct?

  finally, in terms of pulling in common source files, i'm just going
to be appalled by the occasional form of this:

  amcc/bubinga/flash.c:#include "../common/flash.c"
  amcc/walnut/flash.c:#include "../common/flash.c"
  amcc/bamboo/flash.c:#include "../common/flash.c"
  amcc/luan/flash.c:#include "../common/flash.c"

or is textual inclusion of source files from a common directory
acceptable practice? i normally really dislike this, but is doing that
in this specific context in u-boot considered acceptable?

  in any event, the regular way appears to be having a Makefile
controlling what vendor-common code gets compiled, and Kconfig files
elsewhere allowing the selection of config options to drive that
process, is that about right?

  am i missing anything regarding proper common/ vendor usage?

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

[U-Boot] want to clarify a couple things about vendor common/ directories

[Wolfgang Denk]

Dear Robert,

In message <alpine.LFD.2.20.1604130835360.4548@localhost.localdomain> you wrote:
> 
>   (in fact, i can see that of the several vendors that have common/
> directories, only ti/common/ has a Kconfig file, so i'm concluding
> that a common/ directory containing a Kconfig file is more the
> exception rather than the norm. ti/common/ seems like a special case,
> in that it contains just some board_detect code, and its Kconfig would
> be explicitly sourced by the subset of ti boards for which it's
> relevant, so that makes sense. but, as i mentioned, that's the only
> example i see.)

Kconfig stuff is still relatively new, and not many vendors update
their code on a regular base, unless pressed into it ...

> i suppose it might have been possible for the build process to add the
> common directory to the include search path for header files, but it's

I think we tried this (many, many years ago), and it caused all kinds
of problems; the vendor specific code is often... umm... vendor
specific.

> clear that wasn't done so common header file inclusion *needs* that
> "../common/whatever.h" form, correct?

As is, yes.

>   finally, in terms of pulling in common source files, i'm just going
> to be appalled by the occasional form of this:
> 
>   amcc/bubinga/flash.c:#include "../common/flash.c"
>   amcc/walnut/flash.c:#include "../common/flash.c"
>   amcc/bamboo/flash.c:#include "../common/flash.c"
>   amcc/luan/flash.c:#include "../common/flash.c"

I share your dislike...

> or is textual inclusion of source files from a common directory
> acceptable practice? i normally really dislike this, but is doing that
> in this specific context in u-boot considered acceptable?

This is very, very old code. It would not be accepted these days.
And if you look closer, the code is totally redundant, as the standard
CFI driver would probably work on most of these boards - if not
everywhere.

I would not be surprised to see these boards on the remove list in a
not too far future...

>   in any event, the regular way appears to be having a Makefile
> controlling what vendor-common code gets compiled, and Kconfig files
> elsewhere allowing the selection of config options to drive that
> process, is that about right?
> 
>   am i missing anything regarding proper common/ vendor usage?

Kconfig usage is expected to grow...


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The years of peak mental activity are undoubtedly between the ages of
four and eighteen. At four we know all the questions, at eighteen all
the answers.

[U-Boot] want to clarify a couple things about vendor common/ directories

[Robert P. J. Day]

On Wed, 13 Apr 2016, Wolfgang Denk wrote:

> Dear Robert,
>
> In message <alpine.LFD.2.20.1604130835360.4548@localhost.localdomain> you wrote:
> >
> >   (in fact, i can see that of the several vendors that have
> > common/ directories, only ti/common/ has a Kconfig file, so i'm
> > concluding that a common/ directory containing a Kconfig file is
> > more the exception rather than the norm. ti/common/ seems like a
> > special case, in that it contains just some board_detect code, and
> > its Kconfig would be explicitly sourced by the subset of ti boards
> > for which it's relevant, so that makes sense. but, as i mentioned,
> > that's the only example i see.)
>
> Kconfig stuff is still relatively new, and not many vendors update
> their code on a regular base, unless pressed into it ...

  actually, the point i was trying to make (badly) is that almost all
Kconfig files exist *outside* of vendor common/ directories, which
seems to make sense, as selection is tied more to boards, while the
common/ directories are treated simply as the source of code that is
being selected. so it makes sense (at least to me) that vendor/ common
directories will contain a Makefile and piles of selectable common
code, but not a Kconfig file.

  i was only noting that there is a single example -- board/ti/common/
-- that contains a Kconfig file, but that seems like a trivial case.

> > i suppose it might have been possible for the build process to add
> > the common directory to the include search path for header files,
>
> I think we tried this (many, many years ago), and it caused all
> kinds of problems; the vendor specific code is often... umm...
> vendor specific.

  it took only a few more minutes to realize that adding that
directory to the search path would be a really bad idea.

> >   finally, in terms of pulling in common source files, i'm just
> > going to be appalled by the occasional form of this:
> >
> >   amcc/bubinga/flash.c:#include "../common/flash.c"
> >   amcc/walnut/flash.c:#include "../common/flash.c"
> >   amcc/bamboo/flash.c:#include "../common/flash.c"
> >   amcc/luan/flash.c:#include "../common/flash.c"
>
> I share your dislike...
>
> > or is textual inclusion of source files from a common directory
> > acceptable practice? i normally really dislike this, but is doing
> > that in this specific context in u-boot considered acceptable?
>
> This is very, very old code. It would not be accepted these days.
> And if you look closer, the code is totally redundant, as the
> standard CFI driver would probably work on most of these boards - if
> not everywhere.

  good. i'm bringing a pile of legacy u-boot code up to date and some
of it does this, so i was wondering if this was some approved coding
style for u-boot. i'm relieved that it isn't, so i can refactor the
code and get rid of that.

  onward ...

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================