Re: [PATCH] at91: Support for at91rm9200: core chip & board support

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 09:17 Tue 10 May     , Sascha Hauer wrote:
> On Mon, May 09, 2011 at 06:53:59PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 17:36 Mon 09 May     , Sascha Hauer wrote:
> > > On Mon, May 09, 2011 at 04:48:38PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 16:25 Mon 09 May     , Sascha Hauer wrote:
> > > > > > +
> > > > > > +static struct device_d sdram_dev = {
> > > > > > +	.id = -1,
> > > > > > +	.name = "mem",
> > > > > > +	.map_base = AT91_CHIPSELECT_1,
> > > > > > +	.platform_data = &ram_pdata,
> > > > > > +};
> > > > > > +
> > > > > > +void at91_add_device_sdram(u32 size)
> > > > > > +{
> > > > > > +	sdram_dev.size = size;
> > > > > > +	register_device(&sdram_dev);
> > > > > > +	armlinux_add_dram(&sdram_dev);
> > > > > > +}
> > > > > 
> > > > > We already have this function in the tree four times and there is
> > > > > nothing at91 specific in it. Please stop duplicating it.
> > > > yes but the structure is local and can not be shared between SOC
> > > 
> > > Just move both the function and the structure to a common place.
> > > Arguably this is not even at91 specific. It should be usable by other
> > > architectures aswell (this would need dynamic allocation of the data
> > > structure and id counting).
> > I'm not really a fan of dynamic resources allocation but this is not the scope
> > of this patch
> > This shoulb be done in a second time
> > > 
> > > > > > +
> > > > > > +void __init at91_add_device_eth(struct at91_ether_platform_data *data)
> > > > > > +{
> > > > > > +	if (!data)
> > > > > > +		return;
> > > > > 
> > > > > Why this check here? I'd rather see a crash when someone calls this
> > > > > function without data than just nothing happening.
> > > > i prefer to keep the code running and do not register the ethernet device
> > > 
> > > It does not make sense. No board calls this function without valid data,
> > > because it's not working.
> > bug hanging is a bad habit
> > print something ok but not hanging
> > > 
> > > > > 
> > > > > > +
> > > > > > +void __init at91_register_uart(unsigned id, unsigned pins)
> > > > > > +{
> > > > > > +	switch (id) {
> > > > > 
> > > > > This id dispatching does not make much sense. You should export
> > > > > the functions for the individual uarts instead. This makes this funcion
> > > > > disappear completely and gives the linker a chance to throw away the
> > > > > code for unused uarts.
> > > > It's the same API as in the kernel I do want to keep then sync
> > > > I do not want to have to maintain 2 implemetations for few bytes
> > > 
> > > Honestly this can't be the excuse for everything. Then go out and fix
> > > the kernel aswell. Arm folks have great interest in shrinking the code
> > > footprint lately.
> > sorry here I do not think of any improvment even in the kernel to have one
> > function per uart resources it will not reduce the footprint so much but just
> > increase the number of API.
> 
> I don't buy this. Where's the problem of having three functions instead
> of one when the end result is shorter and easier to read?
no asin the kernel we are going to have one fuction for ALL soc
with on implemetation so not 6 functions for 6 uart as differet soc have
different number of uart

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox