Hi Simon,
I have some comments on sandbox I2C driver. On Fri, 5 Dec 2014 08:32:07 -0700 Simon Glass <s...@chromium.org> wrote: > + > +static int get_emul(struct udevice *bus, uint chip_addr, struct udevice > **devp, > + struct dm_i2c_ops **opsp) > +{ > + const void *blob = gd->fdt_blob; > + struct dm_i2c_chip *priv; > + struct udevice *dev; > + int ret; > + > + *devp = NULL; > + *opsp = NULL; > + ret = i2c_get_chip(bus, chip_addr, &dev); > + if (ret) > + return ret; This implementation is not efficient. You invokes "i2c_get_chip(bus, chip_addr, &dev)" twice: in get_emul() and in sandbox_i2c_xfer(). You have already calculated "struct udevice *chip" in sandbox_i2c_xfer(). Pass it to get_emul(). The prototype of this function will be: static int get_emul(struct udevice *chip, struct udevice **emul, struct dm_i2c_ops **opsp) > + priv = dev_get_parentdata(dev); > + if (!priv->emul) { > + int node; > + > + debug("Scanning i2c bus '%s' for devices\n", dev->name); This debug comment is weird because dev->name is not a name of a bus, but a name of a chip. Perhaps the message should be: debug("Scanning chip '%s' for a emul device\n", dev->name); > + for (node = fdt_first_subnode(blob, dev->of_offset); > + node >= 0; > + node = fdt_next_subnode(blob, node)) { > + int ret; > + > + ret = lists_bind_fdt(dev, blob, node, &priv->emul); > + if (ret) > + return ret; > + debug("Found emul '%s' for i2c device '%s'\n", > + priv->emul->name, dev->name); > + break; > + } > + } > + > + if (!priv->emul) > + return -ENODEV; > + ret = device_probe(priv->emul); > + if (ret) > + return ret; > + *devp = priv->emul; > + *opsp = i2c_get_ops(priv->emul); Can we make this part simpler?? if (!priv->emul) { ret = dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false) if (ret) return ret; ret = device_get_child(dev, 0, &priv->emul); if (ret) return ret; } *devp = priv->emul; *opsp = i2c_get_ops(priv->emul); return 0; } I have not tested, though... > + return 0; > +} > + > +static int sandbox_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, > + int nmsgs) > +{ > + struct dm_i2c_bus *i2c = bus->uclass_priv; > + struct dm_i2c_ops *ops; > + struct udevice *emul, *dev; > + bool is_read; > + int ret; > + > + /* Special test code to return success but with no emulation */ > + if (msg->addr == SANDBOX_I2C_TEST_ADDR) > + return 0; > + > + ret = get_emul(bus, msg->addr, &emul, &ops); > + if (ret) > + return ret; > + > + /* For testing, require an offset length of 1 */ > + ret = i2c_get_chip(bus, msg->addr, &dev); > + if (ret) > + return ret; Swap the call order. As mentioned above, you can save the second call of i2c_get_chip(). ret = i2c_get_chip(bus, msg->addr, &dev); if (ret) return ret; ret = get_emul(dev, &emul, &ops); if (ret) return ret; > + > +static const struct udevice_id sandbox_i2c_ids[] = { > + { .compatible = "sandbox,i2c" }, > + { } > +}; > + > +U_BOOT_DRIVER(i2c_sandbox) = { > + .name = "i2c_sandbox", > + .id = UCLASS_I2C, > + .of_match = sandbox_i2c_ids, > + .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip), > + .child_pre_probe = sandbox_i2c_child_pre_probe, > + .ops = &sandbox_i2c_ops, > +}; It looks odd to add .emul member to the common part "struct dm_i2c_chip". .emul is Sandbox-specific. I think one possible solution is to define a local structure "struct sandbox_i2c_chip" and override .per_child_auto_alloc_size. Like this: struct sandbox_i2c_chip { struct dm_i2c_chip chip; struct udevice *emul; } [snip] U_BOOT_DRIVER(i2c_sandbox) = { .name = "i2c_sandbox", .id = UCLASS_I2C, .of_match = sandbox_i2c_ids, .per_child_auto_alloc_size = sizeof(struct sandbox_i2c_chip), /* override */ .child_pre_probe = sandbox_i2c_child_pre_probe, .ops = &sandbox_i2c_ops, }; Another solution is, as I might have mentioned before, that both udevice and uclass have .parent_priv. struct dm_i2c_chip goes to uclass and driver-specific member (in this case, "emul") goes to udevice. Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot