Re: [PATCH] Make ATNGW100 serial ports configurable

[Haavard Skinnemoen]

Anders Blomdell <anders.blomdell@xxxxxxxxxxxxxx> wrote:
> Haavard Skinnemoen wrote:
> > Anders Blomdell <anders.blomdell@xxxxxxxxxxxxxx> wrote:
> >> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
> >>
> >> Signed-off-by: Anders Blomdell <anders.blomdell@xxxxxxxxxxxxxx>
> > 
> > Gak. If we're going down this path, why not add #ifdefs for every other
> > conceivable hardware mod as well and turn the ATNGW100 board code into
> > an even worse mess than the ATSTK1000 board code?
> OK, good point. The reason I sent in the patch is that people on AVRFreaks had
> asked how to do it, and putting it in the configuration would (theoretically)
> make it possible to catch pin conflicts.

Yes, I do realize people want this kind of thing. But I also think
there's more than one way to do it, and while a Kconfig option might
make things easier while trying it out, I think it will cause
maintenance pains in the long run.

> > The ATNGW100 has one serial port on board, so IMO the standard board
> > code should only initialize that one port.
> Which was my intention, unless SERIAL_ATMEL_USARTn was selected.

Yes, but it's not sensible to even offer the option of other ports when
the board just doesn't have them.

> > However, it might be sensible to add some sort of interface for
> > expansion board code. For example something like this:
> > 
> > #ifdef CONFIG_ATNGW100_EXPANSION
> > 	atngw100_setup_expansion_board();
> > #endif
> Since that file has to reside inside the kernel tree (?), one could almost as
> easily replace the entire setup.c.

True...but it would take quite a bit of duplication, and you'll have to
sync up whenever the "main" setup.c changes.

> Shouldn't this be something like:
> 
> void __init setup_board(void)
> {
> ...
> #ifdef CONFIG_ATNGW100_EXPANSION
>  	atngw100_setup_expansion_board();
> #endif
> }
> 
> static int __init atngw100_init(void)
> {
> ...
> #ifdef CONFIG_ATNGW100_EXPANSION
>  	atngw100_init_expansion_board();
> #endif
> }

Well, you could do it like that, but you could also just add another
postcore_initcall. The only hook which is really needed is the "setup"
part, and only if you have additional serial ports.

In fact, it's probably better to just add a weak definition for it
since not all expansion boards will need to override it.

> > This will allow people with hardware mods to add all the extra devices
> > they need, including serial ports, by simply adding another file with
> > expansion board code. People who aren't afraid to solder stuff on their
> > boards shouldn't be afraid of writing some board code too, right?
> > 
> > What do you think?
> An even nicer way of handling it (provided that initialization does not need to
> take place during boot), might be to do EXPORT_SYMBOL() on
> at32_add_device_usart, at32_map_usart, etc and then write a loadable module that
> handles the initialization.

Hmm...why would that be nicer exactly?

What _I_ think would be a nicer way to do it is to implement support
for flattened device trees and get rid of the board code entirely. Or
almost entirely; it depends on how complete we can make the device tree.

Haavard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/