Hi Simon,
2015-01-22 1:12 GMT+09:00 Simon Glass <s...@chromium.org>: > Hi Bin, > > On 21 January 2015 at 03:45, Masahiro Yamada <yamad...@jp.panasonic.com> > wrote: >> Hi Simon, >> >> >> >> On Mon, 19 Jan 2015 20:12:30 -0700 >> Simon Glass <s...@chromium.org> wrote: >> >> >>> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c >>> index 1e500fb..7c3ad00 100644 >>> --- a/common/cmd_i2c.c >>> +++ b/common/cmd_i2c.c >>> @@ -168,7 +168,7 @@ static int i2c_get_cur_bus_chip(uint chip_addr, struct >>> udevice **devp) >>> if (ret) >>> return ret; >>> >>> - return i2c_get_chip(bus, chip_addr, devp); >>> + return i2c_get_chip(bus, chip_addr, 1, devp); >>> } >> >> The i2c command calls >> [1] i2c_get_cur_bus_chip() = set the offset len to 1 >> [2] i2c_set_chip_offset_len = change the offset >> >> >> Now we can do [1] and [2] at the same time, right? >> >> >> If we set the offset address when we get the device, >> we won't need i2c_set_chip_offset_len(), I think. >> >> The offset_len for each device does not change. >> Chainging it on the way makes no sense. > > Except that I inserted this patch into the series I sent a week ago, > which I want to apply today/tomorrow. So I wanted to limit the scope > of the change. I was not able to convince myself that there would be > no side-effects with the cmd_i2c.c change. But if you have had a look > too then perhaps we are OK. > > I'll send a follow-up patch to clean this up (still will be in this > merge window). Sorry, I noticed a side-effect. If you mistype the offset length on the first run of the I2C command, a generic chip is created and bound with the wrong offset length. You cannot correct this because on the next run, i2c_get_chip() find the chip from the bound-devices list. We still need i2c_set_chip_offset_len to set the offset length explicitly. I take back my former comment. Sorry. >> >> >> >> >>> } >>> >>> -int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice >>> **devp) >>> +int i2c_get_chip(struct udevice *bus, uint chip_addr, uint offset_len, >>> + struct udevice **devp) >> >> >> If the device tree for the child device is not found >> (i.e. i2c_bind_driver() is called), the new generic chip >> will be given with the offset_len. >> >> On the other hand, if the device tree is found, >> offset_len is default to 1 because >> i2c_chip_ofdata_to_platdata() always set chip->offset_len to 1. >> >> It is a pity. >> >> I wonder if it would not be possible to >> get the default offset_len from the device tree node of the child device. >> >> >> For example, the EEPROM on my board expects chip->offset_len == 2. >> >> It would be nice if we could have the offset property for the EEPROM device >> node. >> >> I dug into Documentation/devicetree/bindings/, but I could not find the one. > > I have a local patch which adds a 'u-boot,i2c-offset-length' property, > but in the case of cros_ec I just changed the value in the probe > function. So it wasn't essential. > > I would prefer to add device tree support though. Since it seems you > agree I will go ahead with this. Any preference on property naming? I checked Documentation/devicetree/bindings/eeprom.txt, but nothing is mentioned about the offset length. So, I am OK with your idea. -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot