Hello Shawn,

On Mon, Jul 04, 2011 at 04:43:25PM +0800, Shawn Guo wrote:

On Mon, Jul 04, 2011 at 09:33:20AM +0200, Uwe Kleine-K?nig wrote:

quoted

On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:

quoted

quoted

quoted

+static struct platform_device_id imx_uart_devtype[] = {
+	{
+		.name = "imx1-uart",
+		.driver_data = IMX1_UART,
+	}, {
+		.name = "imx-uart",
+		.driver_data = IMX2_UART,

It feels strange to introduce IMX2 today meaning
i.MX{21,25,27,31,35,50,51,53}. I didn't check the changes in detail, but
if the only relevant change is that the UTS register is at a different

No, it's not the only relevant change.  The patch also changes a couple of
'if (!cpu_is_imx1())' to 'if (IS_IMX2_UART())'.

The 'IMX2' in 'IMX2_UART' does not necessarily mean i.MX{21,25,27,
31,35,50,51,53}.  It actually means i.MX{21,25,27,31,35,50,51,53} all
have the same UART block which was firstly introduced on IMX2.

This is ugly. IMHO something like imx_uart_v2 would be much better.
For a driver that supports all i.MX generations

	if (IS_IMX2_UART())

is simply misleading. Even if you had documented at the definition
of imx_uart_type (and/or IS_IMX2_UART) that IMX2_UART means everything
currently known but MX1, just looking at the if condition yields wrong
expectations.

	if (cpu_is_imx1())

is much better (in this aspect). (At least until Freescale comes up with
a new SOC in the mx1 family that includes a completely new UART block
:-)

With

	if (somehow_get_current_driver_data() & SOME_FLAG)
		do_the(things, that, "are only needed with SOME_FLAG");

you can have both, even local understandable code and no dependency on
the cpu_is_... stuff.

It's actually a change respecting the existing code, which defines
MX2_ONEMS for all imx except imx1.

IMHO that's a bad excuse. Take it the other way 'round: You can do
better than the existing code (and even fix it on the go if you're
motivated).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |