[U-Boot-Users] [Patch 09/17] U-Boot-V2:Serial: Add support for NS16550Driver.`

[Menon, Nishanth]

Sascha,
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Tuesday, June 03, 2008 3:09 AM
> To: Menon, Nishanth
> Cc: u-boot-users at lists.sourceforge.net; Laurent Desnogues; dirk.behme at googlemail.com;
> philip.balister at 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..
a) code size increases
b) un-necessary overhead in performance.
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.

> > +	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