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

[Simon Glass]

Hi Lubomir,

On 26 November 2014 at 05:47, 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>

Reviewed-by: Simon Glass

(see possible nit below)

> ---
> Changes in V2:
> The option to use bulk transfer vs re-addressing is implemented as a run-time
> command argument and shall better fit into the new I2C DM (thanks to Simon for
> the hint). V1 used conditional compilation through a board header definition.
>
>  common/cmd_i2c.c |   35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
> index c266b88..5cb7dca 100644
> --- a/common/cmd_i2c.c
> +++ b/common/cmd_i2c.c
> @@ -270,7 +270,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[
>         uint    devaddr, alen, length;
>         u_char  *memaddr;
>
> -       if (argc != 5)
> +       if ((argc < 5) || (argc > 6))
>                 return cmd_usage(cmdtp);
>
>         /*
> @@ -293,20 +293,34 @@ 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);
> -
> -       while (length-- > 0) {
> -               if (i2c_write(chip, devaddr++, alen, memaddr++, 1) != 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.
> +                */
> +               if (i2c_write(chip, devaddr, alen, memaddr, length) != 0)
>                         return i2c_report_err(-1, I2C_ERR_WRITE);
> -               }
> +       } else {
> +               /*
> +                * Repeated addressing - perform <length> separate
> +                * write transactions of one byte each
> +                */
> +               while (length-- > 0) {
> +                       if (i2c_write(chip, devaddr++, alen, memaddr++, 1) != 0)
> +                               return i2c_report_err(-1, I2C_ERR_WRITE);
>  /*
>   * No write delay with FRAM devices.
>   */

Should this be indented?

>  #if !defined(CONFIG_SYS_I2C_FRAM)
> -               udelay(11000);
> +                       udelay(11000);
>  #endif
> +               }
>         }
>         return 0;
>  }
> @@ -1559,7 +1573,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, "", ""),
>         U_BOOT_CMD_MKENT(reset, 0, 1, do_i2c_reset, "", ""),
>  #if defined(CONFIG_CMD_SDRAM)
>         U_BOOT_CMD_MKENT(sdram, 1, 1, do_sdram, "", ""),
> @@ -1623,7 +1637,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"
>         "i2c reset - re-init the I2C Controller\n"
>  #if defined(CONFIG_CMD_SDRAM)
>         "i2c sdram chip - print SDRAM configuration information\n"
> @@ -1632,7 +1647,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