On Fri, May 11, 2012 at 05:44:19PM +0200, Cousson, Benoit wrote:
On 5/11/2012 3:08 PM, Mark Brown wrote:
This binding doesn't do anything to move towards that goal given that the only information it includes about the contents of the chip is the name.
But it does not have to expose everything. And what will be your definition of everything in that case?
A useful binding that abstracted things sufficiently to allow multiple devices would be one which contained some information about the contents of the chip.
Writing the name out in separate CODEC and vibra nodes really isn't going to accomplish much to promote reuse that can't trivially be achieved by parsing the name in the MFD driver.
Maybe, but... so what? This is not because you can do it with MFD that you cannot make it more flexible with DT.
The issue is the tastefulness. Just looking at the binding it's immediately clear that the only thing that the extra levels of indirection are adding is the removal of the mfd_add_cells() call from the MFD driver which isn't particularly helping anything and is basically Linux specific.
Preferring to hard code that in MFD is similar to keeping all the static C board files we are all trying to remove. It is possible, sure, but there are now better way to describe HW modules than hard-coding that in a C file.
Sure, but this binding doesn't do anything like that. If it said things like "there's a PDM speaker driver with register map X at address Y" then you'd have a block you could easily pick up and reuse when defining another device but it doesn't do that at all.
Moreover, there is not an unique way to describe the HW. So for sure we will cheat a little bit and make some assumption about what the SW will really use. But otherwise you will end up putting the full RTL inside the DT node.
This isn't a "cheat a little bit" type thing - it's really not talking about blocks that are likely to reappear elsewhere in exactly the same form on a device that's different enough that you'd be able to add some useful reuse via the device tree.
With CODECs the IP blocks you see are generally things like audio interfaces, mixers, PLLs, amplifiers and so on. New devices that are meaningfully different to software will generally be some new combination of that sort of stuff rather than a straight clone of the chip top level which is what this binding is talking about.
It is very similar to the discussion we had for the clock tree. For sure we can describe the full clock tree in DT, but that's huge and useless. So we are taking some assumption and expose only the ones that have to be exposed because dependent of the board or needed by the devices.
The difference I'm seeing here is that the split in the device tree here is at such that it's the equivalent of having a single device tree node for the entire clock tree of a given OMAP model - it's just completely redundant, it's not adding any additional information beyond the top level information about which chip this is.
It is up to the driver owner to assess the level of information he'd like to expose to DT based on the number of chip variants that already exist.
This code handles exactly one chip and looking at it it's *extremely* hard to see it as a useful abstraction for multiple chips. There's also the fact that with things like this where the chip has to be hooked up to other pieces of the system it makes much more of a difference than it might otherwise since it's much more externally visible. In this particular case for example we might want to split the clocking out differently as the generic clock API comes along, and there's extcon too. All of which should be entirely Linux internal things.
Both formats are valid. It is just a matter of flexibility / personal choice / mood of the day.
The important point is that this is not a black or white kind of decision, we can be in the grey area and use a little bit of both approach.
If I could see how this would allow reuse I'd probably agree with you but I really can't - it's immediately obvious looking at what's there that all that's happening is that a bit of the Linux internals that's not particularly great as a generic abstraction dropped straight into the device tree.
Plus there's the fact that from both a device tree and a code point of view it's utterly trivial to not land ourselves with this in the device tree, it's just registering two devices and using a different of_node for reading properties in code and removing a layer of indentation in the DT.