Hi Heiko,
> Hello Simon, Lubomir,
>
> Am 03.02.2015 01:59, schrieb Simon Glass:
>> Hi,
>>
>> On 30 January 2015 at 10:56, Lubomir Popov <lpo...@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-po...@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 <s...@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
>
This looks OK to me; however, since I don't have any DM-enabled
board to test upon, nor currently have any time to test whatever
in general (really sorry), I'm leaving this in your hands, guys.
BR,
Lubo
> @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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
