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

Reply via email to