Re: [U-Boot] [PATCH v5 04/11] dm: i2c: Add a sandbox I2C driver

Hi Simon,

I have some comments on sandbox I2C driver.

On Fri, 5 Dec 2014 08:32:07 -0700 Simon Glass sjg@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