From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 06 Feb 2015 07:54:18 +0100 Subject: [U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction In-Reply-To: References: Message-ID: <54D4651A.4080105@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, Lubomir, Am 03.02.2015 01:59, schrieb Simon Glass: > Hi, > > On 30 January 2015 at 10:56, Lubomir Popov wrote: >> I2C chips do exist that require a write of some multi-byte data to occur in >> a single bus transaction (aka atomic transfer), otherwise either the write >> does not come into effect at all, or normal operation of internal circuitry >> cannot be guaranteed. The current implementation of the 'i2c write' command >> (transfer of multiple bytes from a memory buffer) in fact performs a separate >> transaction for each byte to be written and thus cannot support such types of >> I2C slave devices. >> >> This patch provides an alternative by allowing 'i2c write' to execute the >> write transfer of the given number of bytes in a single bus transaction if >> the '-s' option is specified as a final command argument. Else the current >> re-addressing method is used. >> >> Signed-off-by: Lubomir Popov >> --- >> Changes in V3: >> Rebased on current master. >> Changes in V2: >> The option to use bulk transfer vs re-addressing is implemented as a run-time >> command argument. V1 used conditional compilation through a board header >> definition. >> >> common/cmd_i2c.c | 39 ++++++++++++++++++++++++++++++--------- >> 1 file changed, 30 insertions(+), 9 deletions(-) I should try to apply a patch before saying I tend to accept a patch ;-) This patch fails again (Sorry Lubomir) ... because in the meantime this patch from Simon is in mainline: commit f9a4c2da72d04e13b05deecb800f232d2948eb85 Author: Simon Glass Date: Mon Jan 12 18:02:07 2015 -0700 dm: i2c: Rename driver model I2C functions to permit compatibility Which introduced dm_i2c_write() ... > What platform are you testing on? > > It seems like you could implement this using driver model - just set > or clear the DM_I2C_CHIP_WR_ADDRESS flag. > > That would solve the problem of existing platforms, since they could > be tested when converted to driver model. > > So what do you think about adjusting this patch to move the '#ifdef > CONFIG_DM_I2C' outside the while loop, and set the flag instead? > Although then your feature would only be available for driver model. Thinking about this, wouldn;t it be better to add this patch to this patch? diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c index a1a269f..df18b3f 100644 --- a/common/cmd_i2c.c +++ b/common/cmd_i2c.c @@ -342,6 +342,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ int ret; #ifdef CONFIG_DM_I2C struct udevice *dev; + struct dm_i2c_chip *i2c_chip; #endif if ((argc < 5) || (argc > 6)) @@ -377,6 +378,9 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ ret = i2c_set_chip_offset_len(dev, alen); if (ret) return i2c_report_err(ret, I2C_ERR_WRITE); + i2c_chip = dev_get_parent_platdata(dev); + if (!i2c_chip) + return i2c_report_err(ret, I2C_ERR_WRITE); #endif if (argc == 6 && !strcmp(argv[5], "-s")) { @@ -387,7 +391,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ * into account if linking commands. */ #ifdef CONFIG_DM_I2C - ret = i2c_write(dev, devaddr, memaddr, length); + i2c_chip &= ~DM_I2C_CHIP_WR_ADDRESS; + ret = dm_i2c_write(dev, devaddr, memaddr, length); #else ret = i2c_write(chip, devaddr, alen, memaddr, length); #endif @@ -400,7 +405,8 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ */ while (length-- > 0) { #ifdef CONFIG_DM_I2C - ret = i2c_write(dev, devaddr++, memaddr++, 1); + i2c_chip |= DM_I2C_CHIP_WR_ADDRESS; + ret = dm_i2c_write(dev, devaddr++, memaddr++, 1); #else ret = i2c_write(chip, devaddr++, alen, memaddr++, 1); #endif @Simon: Do I have to check if dev_get_parent_platdata() returns a pointer? bye, Heiko >> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c >> index 22db1bb..8d4f5f6 100644 >> --- a/common/cmd_i2c.c >> +++ b/common/cmd_i2c.c >> @@ -344,7 +344,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ >> struct udevice *dev; >> #endif >> >> - if (argc != 5) >> + if ((argc < 5) || (argc > 6)) >> return cmd_usage(cmdtp); >> >> /* >> @@ -367,7 +367,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ >> return cmd_usage(cmdtp); >> >> /* >> - * Length is the number of objects, not number of bytes. >> + * Length is the number of bytes. >> */ >> length = simple_strtoul(argv[4], NULL, 16); >> >> @@ -379,20 +379,40 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ >> return i2c_report_err(ret, I2C_ERR_WRITE); >> #endif >> >> - while (length-- > 0) { >> + if (argc == 6 && !strcmp(argv[5], "-s")) { >> + /* >> + * Write all bytes in a single I2C transaction. If the target >> + * device is an EEPROM, it is your responsibility to not cross >> + * a page boundary. No write delay upon completion, take this >> + * into account if linking commands. >> + */ >> #ifdef CONFIG_DM_I2C >> - ret = i2c_write(dev, devaddr++, memaddr++, 1); >> + ret = i2c_write(dev, devaddr, memaddr, length); >> #else >> - ret = i2c_write(chip, devaddr++, alen, memaddr++, 1); >> + ret = i2c_write(chip, devaddr, alen, memaddr, length); >> #endif >> if (ret) >> return i2c_report_err(ret, I2C_ERR_WRITE); >> + } else { >> + /* >> + * Repeated addressing - perform separate >> + * write transactions of one byte each >> + */ >> + while (length-- > 0) { >> +#ifdef CONFIG_DM_I2C >> + ret = i2c_write(dev, devaddr++, memaddr++, 1); >> +#else >> + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1); >> +#endif >> + if (ret) >> + return i2c_report_err(ret, I2C_ERR_WRITE); >> /* >> * No write delay with FRAM devices. >> */ >> #if !defined(CONFIG_SYS_I2C_FRAM) >> - udelay(11000); >> + udelay(11000); >> #endif >> + } >> } >> return 0; >> } >> @@ -1823,7 +1843,7 @@ static cmd_tbl_t cmd_i2c_sub[] = { >> U_BOOT_CMD_MKENT(nm, 2, 1, do_i2c_nm, "", ""), >> U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""), >> U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""), >> - U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""), >> + U_BOOT_CMD_MKENT(write, 6, 0, do_i2c_write, "", ""), >> #ifdef CONFIG_DM_I2C >> U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""), >> #endif >> @@ -1890,7 +1910,8 @@ static char i2c_help_text[] = >> "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n" >> "i2c probe [address] - test for and show device(s) on the I2C bus\n" >> "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n" >> - "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n" >> + "i2c write memaddress chip address[.0, .1, .2] length [-s] - write memory\n" >> + " to I2C; the -s option selects bulk write in a single transaction\n" >> #ifdef CONFIG_DM_I2C >> "i2c flags chip [flags] - set or get chip flags\n" >> #endif >> @@ -1902,7 +1923,7 @@ static char i2c_help_text[] = >> #endif >> >> U_BOOT_CMD( >> - i2c, 6, 1, do_i2c, >> + i2c, 7, 1, do_i2c, >> "I2C sub-system", >> i2c_help_text >> ); >> -- >> 1.7.9.5 >> > > Regards, > Simon > -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany