This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: various submissions


Thomas Koeller wrote:
Hi,

here's some stuff I made, submitted for itegration into ecos cvs.
There's the modular AT91 HAL, consisting of hal_at91.epk and
[snip]

Thanks for the contrib. Sorry it's taken so long to get round to reviewing, but the larger a patch is, the less easy it is to set aside time...

Anyway, okay, I've been reviewing this, and there are a fair few issues that we need to work out before I can put it in. The best approach to take is to comment on bits of this, rather than all at once, or having many overlapping threads. I'll start with io_flash.diff as that has a knock-on effect.

- Why is flash_block_query a separate extern? Can't it just be a field in the struct flash_info? BTW, not something I would intend you to deal with definitely, but the situation in the flash drivers is already a bit naff in terms of namespace pollution (the flash drivers were unfortunately only written in the context of redboot), and we don't want to make the issue worse.

- Call it personal taste, but I'm not fond of the interface. In particular, the old function flash_get_block_info() is still left behind. I don't think a function that will effectively be "broken" for certain flash parts should be left. That function is only used in a very small number of places (and it's not an official API function, so backward compatibility should not be a problem). Only redboot and devs/flash/synth/current/tests/flash1.c use it. I think flash_get_block_info should have its arguments tweaked to be the new interface.

In any case, instead of the (computationally expensive) iterator concept, it would be much simpler for the driver to fill in a static table of regions and their size in the struct flash_info. Most times the table will be pretty small - most commonly just 2. So e.g. the driver would have:

// io_flash.h contains:
struct flash_block_table_entry {
void *offset_addr; // offset of this address to the start of the device
unsigned long size;
unsigned long block_size;
// potential for expansion in future...
};

// underlying flash driver contains:

static const struct flash_block_table_entry block_table[3] = {
{0x800000, 0x10000, 0x2000},
{0x810000, 0x1f0000, 0x10000},
{0x0, 0x0, 0x0} // end of table
}

// flash_hwr_init() contains:
flash_info.block_table = block_table;

size == 0 indicates the last table entry.

Otherwise we go through a lot of effort to support something that is actually 99% of the time quite simple.

- I see no reason for flash_get_block_for_index() or flash_iterate_blocks() to be externs. statics should suffice.

- There are plenty of drivers we have that will never need variable block sizes, but there's a non-trivial amount of code now permanently included to support it. Instead I think this support should be controlled by a CDL interface. The underlying flash driver would implement the interface and io/flash would react accordingly. Obviously something more intelligent can be done than just #ifdeffing every line you changed in your patch :-). Something like CYGINT_IO_FLASH_VARIABLE_BLOCK_SIZE.

If you like I'm prepared to implement this in io/flash for you. If you can then make the changes to your flash driver and test it (since I can't - no at91!).

Oh and BTW, the copyright assignment you have signed permits you to make further contributions if you wish without a new assignment. Have a read of http://sources.redhat.com/ecos/assign.html

I'd appreciate other feedback on what I suggest above. (Hi Gary!).

Jifl
--
eCosCentric http://www.eCosCentric.com/ <info@eCosCentric.com>
--[ "You can complain because roses have thorns, or you ]--
--[ can rejoice because thorns have roses." -Lincoln ]-- Opinions==mine


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]