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

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 26 April 2007, Dale Farnsworth wrote:
> On Thu, Apr 26, 2007 at 01:24:19PM +0200, Arnd Bergmann wrote:
> > 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.
> 
> Hmm, I wouldn't call that a driver, I'd call it OF interface
> glue used to register the driver. 

I mean driver in the sense that it registers a struct device_driver, not
that it does anything particularly interesting with the hardware

> I guess I could put the 
> platform_device_register call for the mpsc driver in that file.
> But I think that will increase, rather than reduce complexity.

Right, I wasn't thinking of that. Instead I meant you should call the
mpsc_drv_probe() function (or some variation of that) directly from
of_platform_serial_probe(), the way that I call serial8250_register_port.

> > > > > +???????????pdev = platform_device_register_simple(MPSC_CTLR_NAME, i, r, 5);
> > > > > +???????????if (IS_ERR(pdev)) {
> > > > > +???????????????????err = PTR_ERR(pdev);
> > > > > +???????????????????goto ret_node_put;
> > > > > +???????????}
> 
> Can you coerce your mailer to stop munging tabs?

sorry about that, it always happens when I copy something over from my vim.

> > 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?
> 
> The sdma and brg are not simply properties of the serial device, these
> nodes represent hardware modules that may be shared by multiple drivers.

Ok, this looks like a mess that is rather hard to clean up and now might
not be the time to start working on that. If these are shared by multiple
high-level drivers, it sounds like there ought to be a driver for each of
them that also does the necessary locking to serialize register accesses.

	Arnd <><