[PATCH 1/2] serial/imx: get rid of the uses of cpu_is_mx1()

[Uwe Kleine-König]

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:
> > On Mon, Jul 04, 2011 at 10:19:25AM +0800, Shawn Guo wrote:
> > > > > +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/  |