On Friday 28 June 2013 09:13:38 Stephen Warren wrote:

On 06/27/2013 12:26 PM, Marc Dietrich wrote:

quoted

Hi,

(CC'ing linux-tegra)

quoted

I'm puzzling for some time now to find a binding for a mfd device. So
let's
start with something I think I have learned already, let's describe the
hardware.

Here the problem starts, because the nvec (nvidia [protocol] compliant
embedded controller) isn't a well defined piece of silicon at all, but
more a protocol which specifies how to communicate with certain devices
(keyboard, mouse, gpio, system state, battery, oem specific hw ...). Even
worse, the number of "attached" devices may vary for each implementation
and there is no way to sanely probe which which devices exist or not.

For example, the "hardware" in my case is an ENE KB926 keyboard controller
with an additional ps/2 port (for the touchpad) and a number of gpio's for
connecting leds, spi bus, a charger chip and some other stuff. Most gpio's
cannot be controlled directly, but only through certain ec commands. The
i2c protocol used for communication with the host cpu is handled by a
good old 8051 type integrated in the keyboard controller.

Other vendors may implement this in totally different hardware, keeping
only the protocol. So I think this is a case where the "describe the
hardware" principle just fails.

I think "describe the HW" should work quite well. However, it's a matter
of interpretation what that means:-) I take it to mean describing the
SW-visible aspects of that HW, which in this case is the protocol used
to talk to that HW.

quoted

So I thought ok - forget this principle for now and describe it from a
higher> 
protocol point of view. We currently have something like this:
        nvec@7000c500 {
        
                compatible = "nvidia,nvec";
                reg = <0x7000c500 0x100>;
                interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
                #address-cells = <1>;
                #size-cells = <0>;
                clock-frequency = <80000>;
                request-gpios = <&gpio TEGRA_GPIO(V, 2) GPIO_ACTIVE_HIGH>;
                slave-addr = <138>;
                clocks = <&tegra_car TEGRA20_CLK_I2C3>,
                
                         <&tegra_car TEGRA20_CLK_PLL_P_OUT3>;
                
                clock-names = "div-clk", "fast-clk";
        
        };

I would eventually like to do the following:

* Have a node that represents the I2C slave controller HW module in Tegra.

* Have a node (or nodes) (perhaps a child node of that) which represents
the protocol that "binds" to the slave controller. This would have its
own compatible value that describes which protocol to instantiate.

This will separate the driver for the I2C slave HW from the driver for
the protocol, so that you could even run the same protocol with a
different I2C slave device if needed.

I agree with you that the slave driver should be separated from the protocol 
(which itself can also be separated into e.g. smbus and nvec layers). But 
that's more like a long term goal which we cannot handle so quickly. For this 
to work, you probably also need a kernel api for i2c slaves which many people 
failed to introduce in the past already. The NVIDIA downstream kernel has 
something like this already (are there any clients?), but unfortunately no 
smbus protocol support yet.

So keeping this in mind, the binding above already looks ok, even if it is 
named "nvec" instead of e.g. "i2c-slave" for now.

quoted

which describes how the host cpu should setup the i2c communication. The
devices are currently hard coded in the driver, but I like to make them
more> 
flexible through a device tree binding, eg. something like this:
		nvec {
		
				...
				<properties from above>
				...
				
				keyboard {
				
						<add some key bindings here>
				
				};
				
				ps2 {
				
						packet-size = 6; /* for the ac100 tp */
				
				};

... (more child nodes)

quoted

		};

One question is now how to instantiate the mfd children from this. I see
three> 
different methods used in different drivers:
		a) just add a "compatible =" property to the child nodes and a
		
			"simple-bus" property to the the driver

(or instead of simple-bus, have the top-level node's probe() do the
following at the end:

        of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

That's what sounds/soc/tegra/tegra30_ahub.c does.

"simple-bus" and maybe also of_platform_populate are only favored if there is 
a real bus, AFAIU, which isn't the here (and maybe also not for other mfds).

quoted

		b) search for sub-nodes and instantiate the children "manually" by
		
			"guessing" some driver name from the node name and use
			platform_add_device
		
		c) use an mfd_cell struct hard coded in the driver and "enable" 

only

quoted

		
			the children found in the sub-node list and use 

mfd_add_devices.

quoted

All these methods above have their advantages and disadvantages. But lets
ignore them for now and ask what's the preferred way to do this?

I'm not sure if there is a preferred way as a global rule.

I prefer either:

1) The existing method of entirely hard-coding everything in the
top-level device driver, since it is a driver for the particular HW
setup, it knows exactly what is there, and so it's fine to hard-code
everything. The disadvantage here is that any configuration for all the
child nodes gets merged together into the one top-level node though,
which could get messy.

also this method is hard to expand to other devices which may only implement a 
subset of the functions (or needs different platform data).

2) Your option a. I believe if we're going to break down the MFD device
into chunks, the way we do it should mirror existing DT techniques as
closely as possible, and using nodes with compatible values to represent
the child devices seems most consistent with existing techniques.

Thinking about what you said at the beginning, I think it's best not to use 
any sub-nodes at all, but give all children a node of their own (standalone 
driver) with compatible properties and a link property to the device 
responsible for the communications (nvec or i2c-slave in our case), similar to 
the backlight property for panels:

root {
....
	nvec: nvec {
		compatible = "nvidia,nvec";
		...
	};

	keyboard {
		compatible = "nvidia,nvec-keyboard";
		nvec = <&nvec>;
	};
	....
};

I'm not sure if we can still use the function exports from the nvec or if we 
need to add some "ops" to the nvec struct. Maybe the latter would be the 
better solution. I'm also wondering if we can guaranty that the nvec is loaded 
before any children and that the children are unloaded before the nvec core 
driver.

With the above method, we should have no problems to parse the platform data, 
as each "child" needs to provide its own dt parser function.

Marc

quoted

Another question is how the children will get their platform data. I guess
they will just walk the device tree of their parent node and search for
it,
but maybe there is a better way?

In option (a), there will be a separate struct device for each child
node, instantiated directly from the child DT node. Hence, the driver
(chosen from the compatible value just like any other) would simply
parse dev.of_node just like any other driver. The only difference would
be that the driver would have to go to the parent node, and search for a
driver for it, and ask that driver for any "supporting resources", such
as a device handle that allowed you to all APIs to send/receive bytes
over the protocol. Perhaps the parent device might just pre-create
regmaps for all the child devices if that abstraction works. That would
save the child devices even having to look for the parent 
device.