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
