[U-Boot] [PATCH V3] cmd_i2c: Provide option for bulk 'i2c write' in one transaction

[Heiko Schocher <hs@denx.de>]

Hello Simon, Lubomir,

Am 03.02.2015 01:59, schrieb Simon Glass:
> Hi,
>
> On 30 January 2015 at 10:56, Lubomir Popov <lpopov@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@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@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