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

[Masahiro Yamada]

Hi Simon,


I have some comments on sandbox I2C driver.


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