Hi Masahiro,
On 10 December 2014 at 06:19, Masahiro Yamada <yamad...@jp.panasonic.com> wrote:
> 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)
>
>
OK, will adjust.
>
>
>
>
>> + 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...
>
>
Yes, I was wanting to find the first *emulator*, but we can just say
it is an error to have anything other than a single subnode (which
must be an emulator) within the device. Seems quite reasonable and the
code is simpler. This is just for testing after all.
>
>
>
>
>> + 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;
>
OK
>
>
>
>
>> +
>> +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.
I think this way is better. So far as possible I'm trying to avoid
'extending' structures as it makes things harder to debug. I plan to
get to this later as mentioned.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
