Hi Jon,
On 30 May 2014 08:49, Jon Loeliger loeliger@gmail.com wrote:
Folks,
I'd like to discuss the new Driver Model's parsing of the DTS file for the purposes of instancing and binding devices as I was not able to get the existing code to work anything like I was expecting.
The current code only finds and binds the top-level nodes of the DTS file. This leads to a bind function one-level above where it should be for each of the actual device nodes and an extra "parent" node that is empty. For example, the gpio_exynos_gpio_bind() function here:
http://git.denx.de/?p=u-boot/u-boot-x86.git;a=blob;f=drivers/gpio/s5p_gpio.c...
Looks like this:
386 /** 387 * We have a top-level GPIO device with no actual GPIOs. It has a child 388 * device for each Exynos GPIO bank. 389 */ 390 static int gpio_exynos_bind(struct device *parent) 391 { 392 struct exynos_gpio_platdata *plat = parent->platdata; 393 struct s5p_gpio_bank *bank; 394 const void *blob = gd->fdt_blob; 395 int node; 396 397 /* If this is a child device, there is nothing to do here */ 398 if (plat) 399 return 0; 400 401 bank = (struct s5p_gpio_bank *)fdtdec_get_addr(gd->fdt_blob, 402 parent->of_offset, "reg"); 403 for (node = fdt_first_subnode(blob, parent->of_offset); node > 0; 404 node = fdt_next_subnode(blob, node)) { 405 struct exynos_gpio_platdata *plat; 406 struct device *dev; 407 int ret; 408 409 plat = calloc(1, sizeof(*plat)); 410 if (!plat) 411 return -ENOMEM; 412 plat->bank = bank; 413 plat->port_name = fdt_get_name(blob, node, NULL); 414 415 ret = device_bind(parent, parent->driver, 416 plat->port_name, plat, -1, &dev); 417 if (ret) 418 return ret; 419 dev->of_offset = parent->of_offset; 420 bank++; 421 } 422 423 return 0; 424 }
Why is this function being called once at the parent node, which then iterates over each device, instantiates and binds it? Why isn't this function instead called once for each individual device as matched from the DTS? Where did the compatible matching and check take place in this implementation?
Driver model works by looking up compatible strings in the top-level nodes and binding a driver for each one it finds.
The exynos pinctrl device tree binding does not have a compatible string in each of its banks. In fact it just has a single compatible string at the level above the banks. So there seems to be no alternative but to iterate through the banks binding devices as we go.
The node looks something like this:
pinctrl@13410000 { compatible = "samsung,exynos5420-pinctrl"; reg = <0x13410000 0x0000011f>; interrupts = <0x00000000 0x67706330 0x000004a6>; gpc0 { gpio-controller; #gpio-cells = <0x00000002>; interrupt-controller; #interrupt-cells = <0x00000002>; }; gpc1 { gpio-controller; #gpio-cells = <0x00000002>; interrupt-controller; #interrupt-cells = <0x00000002>; };
So we get a bind call on the pinctrl node and then bind each of the banks.
Instead, I think it should be a recursive structure essentially identical in structure to the Linux of_platform_populate() function. There should be a compatible matching step, and then the call to bind the specific instance.
Am I missing something here? Or is this code that just needs to be developed further still?
Certainly this could be done, and it's a small change but this code doesn't exist yet. I deliberately left this out of the implementation until we have I2C implemented, or something similar. Then it will be clearer what is needed here.
My concern is partly that access to the device may be mediated through the parent and thus not discoverable by the child. As an example, the Chrome OS EC driver can attached to I2C, SPI or LPC. The connection needs to be made at the parent level, which provides a 'communications' layer for the generic driver.
So in short I think we need to address these things and make the decisions as we go. For the same reason I didn't implement driver model for SPL or pre-relocation (although I have a series out for the latter now).
A lot of things will be clearer to me when I2C is ported over.
Regards, Simon