On Tue, 21 Jul 2009 11:38:31 +0200
Stefan Roese <s...@denx.de> wrote:
> > might want to add a doc/README.vme8349 at some point.
>
> Currently not planned. I have to admit that I usually don't add such a
> readme.
> It's not an evaluation board after all.
your call - the board will be easier to maintain provided the intended
memory map, etc.
> > > diff --git a/board/esd/vme8349/caddy.h b/board/esd/vme8349/caddy.h
> > >
> > > +typedef enum {
> > > + CADDY_CMD_IO_READ_8,
> > > + CADDY_CMD_IO_READ_16,
> > > + CADDY_CMD_IO_READ_32,
> > > + CADDY_CMD_IO_WRITE_8,
> > > + CADDY_CMD_IO_WRITE_16,
> > > + CADDY_CMD_IO_WRITE_32,
> > > + CADDY_CMD_CONFIG_READ_8,
> > > + CADDY_CMD_CONFIG_READ_16,
> > > + CADDY_CMD_CONFIG_READ_32,
> > > + CADDY_CMD_CONFIG_WRITE_8,
> > > + CADDY_CMD_CONFIG_WRITE_16,
> > > + CADDY_CMD_CONFIG_WRITE_32,
> > > +} CADDY_CMDS;
> > > +
> > > +
> > > +typedef struct {
> > > + uint32_t cmd;
> > > + uint32_t issue;
> > > + uint32_t addr;
> > > + uint32_t par[5];
> > > +} CADDY_CMD;
> > > +
> > > +typedef struct {
> > > + uint32_t answer;
> > > + uint32_t issue;
> > > + uint32_t status;
> > > + uint32_t par[5];
> > > +} CADDY_ANSWER;
> > > +
> > > +typedef struct {
> > > + uint8_t magic[16];
> > > + uint32_t cmd_in;
> > > + uint32_t cmd_out;
> > > + uint32_t heartbeat;
> > > + uint32_t reserved1;
> > > + CADDY_CMD cmd[CMD_SIZE];
> > > + uint32_t answer_in;
> > > + uint32_t answer_out;
> > > + uint32_t reserved2;
> > > + uint32_t reserved3;
> > > + CADDY_ANSWER answer[CMD_SIZE];
> > > +} CADDY_INTERFACE;
> >
> > please remove all typedefs (see CodingStyle Ch. 5). Use 'struct xxx'
> > in each instance instead.
>
> We really would like to keep these typedefs. The reason for this is that
> multiple customers already are using this header for their work. And
> maintaining multiple versions of this file doesn't sound like a good idea.
eh? It's a straight violation of CodingStyle and makes the code
hard to read; STUFF_IN_CAPS to me read as defines, and anyway,
typedefs, assuming CodingStyle liked them, would be appended with _t.
But these need to be defined as 'struct <name>', and used in such a way.
Can't they write a header wrapper for "their work"? Can you make them
realize they won't need to be wasting time on such effort if they
submit the remainder of their code upstream?
> > > diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c
> > > index c20b981..393e44d 100644
> > > --- a/drivers/pci/pci_auto.c
> > > +++ b/drivers/pci/pci_auto.c
> > > @@ -403,6 +403,7 @@ int pciauto_config_device(struct pci_controller
> > > *hose, pci_dev_t dev) PCI_DEV(dev));
> > > break;
> > > #endif
> > > +#ifndef CONFIG_VME8349
> > > #ifdef CONFIG_MPC834X
> >
> > #if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
> >
> > I don't know - this might need to be changed to ifdef
> > CONFIG_MPC8349EMDS...
>
> Should I change this to CONFIG_MPC8349EMDS? Or use
>
> #if defined(CONFIG_MPC834x) && !defined(CONFIG_VME8349)
>
> for now?
hmm...based on commit 6902df56a0b493f369153b09d11afcd74a580561 "Add PCI
support for the TQM834x board.", it should really be ifdef
CONFIG_TQM834x...but what does the VME8349 do? does it want to define
CONFIG_PCIAUTO_SKIP_HOST_BRIDGE instead?
Kim
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
