[U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction
Heiko Schocher
hs at denx.de
Fri Feb 6 07:54:18 CET 2015
Hello Simon, Lubomir,
Am 03.02.2015 01:59, schrieb Simon Glass:
> Hi,
>
> On 30 January 2015 at 10:56, Lubomir Popov <lpopov at mm-sol.com> 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 <l-popov at ti.com>
>> ---
>> 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 <sjg at chromium.org>
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 <length> 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
More information about the U-Boot
mailing list