[PATCH 9/13] powerpc: Add arch/powerpc mv64x60 I2C platform data setup

Dale Farnsworth dale at farnsworth.org
Sat Apr 28 09:50:41 EST 2007


On Thu, Apr 26, 2007 at 05:04:06PM +0200, Arnd Bergmann wrote:
> Maybe you still haven't understood the difference of an of_platform_driver
> compared to the platform_driver glue which you are adding here.
> 
> I don't want you to move the glue code into the device driver -- I really
> think the glue code should not be there in the first place.
> 
> As you probably understand, the Linux driver model represents every piece
> of hardware as a 'struct device' which can be embedded in things like
> of_device, pci_device or platform_device. Then there are 'struct
> device_driver's than handle all devices of a given bus_type/device_id
> combination.
> 
> With the of device tree, you automatically get an of_device for everything
> that is connected to an internal (soc, plb, ssb, ...) bus on the chip
> or on the board. According to the driver model, they should be driven
> by an of_platform_driver.

Well, since our platform hasn't called of_platform_bus_probe() with
a device id for the mv64x60 device node, we don't create an of_device
for each of its sub-devices.  Instead, we scan separately and create
platform_devices that the existing drivers require, and for the
benefit of those who join this discussion late, which we must
maintain for use by MIPS platforms.

> What your glue code does is to find a backdoor into the device tree
> (through of_find_compatible_node) and create a second struct device
> for the same hardware, in an unrelated location in the linux device tree.
> This is very confusing if you look at sysfs, e.g. trying to find out
> which driver is attached to a given of_device.

No, we don't create the first struct device you mentioned, so we avoid
the duplication and the confusion.

> It also makes you lose the ability to autoload the driver module,
> because autoloading is not supported for a platform_driver (there
> is no MODULE_DEVICE_TABLE()).
> 
> As you made clear, we will need the platform_driver for the forseeable
> future, but I really think that we also need an of_platform_driver
> to drive them on powerpc instead of adding another pile of junk like
> fsl_soc.c.

Heh, I was going to ask if you made these points when fsl_soc.c went in.  :)

> I can see multiple ways for you to get there:

I find each of these methods lacking.

> 1. have a driver that binds to all of_devices supported by mv64x60
> and then creates the platform_device for them the way you do in your
> glue, but without adding code that manually iterates through
> the device tree. Make the platform_device a child of the of_device.
> This approach is the closest to what you have right now and would
> at least get the sysfs representation right, but not allow module
> autoloading and it still duplicates all the devices.

Like you, I dislike the resulting dev duplication.  Also, having
the platform_device be a child of a superfluous of_device seems weird.

> 2. remove the dependencies on platform_device data structures from
> the current driver code, and add them to a separate file, so you
> can link the module either with the platform_driver or with the
> of_platform_driver, as I suggested in a previous mail.
> I think this would be the best solution.

This is a very OF-centric proposal.  IMHO it's a mistake for drivers
to call of_get_property().  There should be a firmware-independent
way of passing parameters to drivers.  I'm not a big fan of the
details of platform_device, but at least it's firmware independent.
I think it's a layering violation even when ppc-only drivers
query for parameters via of_get_property().

> 3. Have a small of_device_driver part that gets added to each
> of the device drivers, and that adds the platform_device internally.
> This would be like 1., but also allow autoloading.

Again, I'd like to keep arch-specific code out of drivers, as much
as possible.

Since you now recognize our need for platform_devices, I'd like to
understand your primary objection to our not creating of_devices.
Is it because it violates the assumption that of_devices be created
for all devices on the platform?  Is that a hard requirement?

BTW Arnd, thank you for taking the time to comment on this code.

-Dale



More information about the Linuxppc-dev mailing list