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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Jun 03, 2008 at 10:29:03AM -0500, Menon, Nishanth wrote:
> 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..

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
> 

-- 
Pengutronix e.K. - Linux Solutions for Science and Industry
-----------------------------------------------------------
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite -> http://www.pengutronix.de/impressum/ <-