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

[Simon Glass]

Hi Masahiro,

On 10 December 2014 at 06:19, Masahiro Yamada <yamada.m at 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 <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)
>
>

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