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