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

[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(-)

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.

>
> 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