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/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
