Subject: Re: [PATCH 7/13] powerpc: Add arch/powerpc mv64x60 MPSC platform data setup

From: Arnd Bergmann <[email-masked]>
Date: Thu, 26 Apr 2007 13:24:19 +0200

On Thursday 26 April 2007, Dale Farnsworth wrote:
> > This looks wrong to me. See drivers/serial/of_serial.c to find how we do it for
> > 8250 compatible serial ports. You should probably just add your serial port
> > stuff in there as well, instead of doing your own scanning of the device tree.
> 
> Unfortunately, this hardware is very much non-8250 compatible.

That shouldn't matter much. The driver is not 8250 specific by itself, it's just
that right now it doesn't know about any other chips.

If you find that there is more code that needs to be added than what is already
in there, you could also create a new one based on of_serial for your port.

I can understand why you want to keep using the platform_device here, and in the
other places, but if you do then please use platform_device_register from an
of_platform_driver probe function instead of platform_device_register_simple
so you can set the parent to the of_device.

> > > +           pdev = platform_device_register_simple(MPSC_CTLR_NAME, i, r, 5);
> > > +           if (IS_ERR(pdev)) {
> > > +                   err = PTR_ERR(pdev);
> > > +                   goto ret_node_put;
> > > +           }
> > 
> > Now this really needs some explanation.
> > 
> > Why the heck do you have a platform device that gets its resources from
> > nonstandard properties of a serial port?
> 
> There is an existing mpsc driver usable on both MIPS and powerpc platforms
> that requires these non-standard properties.

Ok, I see where some of the limitations come from. However, instead of
introducing the new "sdma" and "brg" properties, why not just add the
register ranges to the "reg" property, like other drivers do?

	Arnd <><