On Tue, Jun 03, 2008 at 10:29:03AM -0500, Menon, Nishanth wrote:
Sascha,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Tuesday, June 03, 2008 3:09 AM To: Menon, Nishanth Cc: u-boot-users@lists.sourceforge.net; Laurent Desnogues; dirk.behme@googlemail.com; philip.balister@gmail.com; Gopinath, Thara; Kamat, Nishant; Syed Mohammed, Khasim Subject: Re: [Patch 09/17] U-Boot-V2:Serial: Add support for NS16550Driver.`
+config DRIVER_SERIAL_NS16550_REG_SIZE_8_BITS_PAD_TO_64
- bool "8 bit register Padded to 64 bit"
- help
Say Y here if you are using a 8 bit register padded to 64 bits for NS16550+endchoice
Please have a look at drivers/net/dm9000.c for an example on how to pass data between the board and a driver via platform_data. The kconfig approach for this has several disadvantages. The user is presented a couple of options which he better does not change because it would break the driver. Another problem is that you cannot connect two ns16550 with different register widths on one board (though that's an unlikely case).
My problem is not sending data size APIs. I do not want to maintain multiple different structures variable if I can avoid at compile time. I do pass the clock and other variant params through platform_data. If I don't use the #defines, I need to do something like the following: Pass reg_type thru platform data, then for each set of access, do: switch (reg_type) { case REG_SIZE_8_BITS_PAD_TO_64: com_port_8_pad64->thr = something; break; case REG_SIZE_8_BITS_PAD_TO_32: com_port_8_pad32->thr = something; break; etc..
What I meant is that you should put the access functions itself into platform_data, not a description of their register size / padding. That way each board can decide which funky access functions it needs, be it that the 16550 is connected via SPI, memory io or whatever.
a) code size increases
Yes, by a few bytes
b) un-necessary overhead in performance.
I bet it's hardly measurable
c) resultant code is going to look dirty. NS16550_t com_port as in the current implementation is easy to read and looks neat. I believe the Kconfig is a neater approach. If I plan on doing a #define, it will still be need a higher level definition to decide on this. The next alternative will be to: #define THR1 1 (for all platforms)
And each platform code implement: unsigned long reg_read(unsigned reg) { readb(silicon_offset_of_reg); } And pass these access functions over platform_data. I really don't like the overhead in doing this.
+/* Forward declaration */ +static unsigned int ns16550_calc_divisor(struct console_device *cdev,
unsigned int baudrate);+static void ns16550_serial_init_port(struct console_device *cdev); +static void ns16550_putc(struct console_device *cdev, char c); +static int ns16550_getc(struct console_device *cdev); +static int ns16550_tstc(struct console_device *cdev); +static int ns16550_setbaudrate(struct console_device *cdev, int baud_rate); +static int ns16550_probe(struct device_d *dev); +static int ns16550_serial_init(void);
Please reorder the functions to get rid of these declarations. It helps reading the code when you know that a functions definition is before its usage.
It is a matter of a programmer preference I believe. Folks who do a topdown reading prefer to have the high level callers on tops and dig to the lower levels. Others do a bottom up reading. Yeah probably doing a bottom to top approach could be cleaner in this case. Will fix.
Yes it is. Most kernel programmers (and I think most U-Boot hackers are kernel programmers ;) are used to read bottom to top.
- cdev->f_caps = plat->f_caps;
Ok, you know about platform_data ;). I suggest that you put the register access functions there.
I really would not like to do it as explained above. The code will end up unreadable at the end of it.
Regards, Nishanth Menon