Re: [PATCH] Make ATNGW100 serial ports configurable

[Anders Blomdell <anders.blomdell@control.lth.se>]

Haavard Skinnemoen wrote:
> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>> Haavard Skinnemoen wrote:
>>> Anders Blomdell <anders.blomdell@control.lth.se> wrote:
>>>> Make configuration of ATNGW100 serial ports selectable in kernel configuration.
>>>>
>>>> Signed-off-by: Anders Blomdell <anders.blomdell@control.lth.se>
>>> 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.
Does that mean that 'at32_add_device_usart(...);' is not needed?

> 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?
You could compile your board specific inits outside the kernel tree, making it
much easier to follow kernel versions (not everyone gets their board specific
code into the kernel tree :-).

> 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.
I don't understand the above paragraph, could you please elaborate?


-- 
Anders Blomdell                  Email: anders.blomdell@control.lth.se
Department of Automatic Control
Lund University                  Phone:    +46 46 222 4625
P.O. Box 118                     Fax:      +46 46 138118
SE-221 00 Lund, Sweden