* [lm-sensors] i2ctools: Need capability to write SMBus block command @ 2011-01-25 16:21 Guenter Roeck [not found] ` <20110125162106.GA8024-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2011-01-25 16:21 UTC (permalink / raw) To: lm-sensors Hi, I need the capability to write a SMBus block command with i2ctools. Not an issue writing it myself. Question is if to expand i2cset to include that capability, or to write a new command (i2csetblock ?) to do it. Thoughts ? Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110125162106.GA8024-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>]
* Re: i2ctools: Need capability to write SMBus block command [not found] ` <20110125162106.GA8024-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> @ 2011-01-25 16:43 ` Guenter Roeck 2011-01-25 16:49 ` i2ctools: Need capability to write SMBus block command Jean Delvare 1 sibling, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2011-01-25 16:43 UTC (permalink / raw) To: lm-sensors-GZX6beZjE8VD60Wz+7aTrA; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA On Tue, Jan 25, 2011 at 11:21:06AM -0500, Guenter Roeck wrote: > Hi, > > I need the capability to write a SMBus block command with i2ctools. > > Not an issue writing it myself. Question is if to expand i2cset to include > that capability, or to write a new command (i2csetblock ?) to do it. > > Thoughts ? > Sorry, wrong mailing list. Copying linux-i2c. Thanks, Guenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [lm-sensors] i2ctools: Need capability to write SMBus block @ 2011-01-25 16:43 ` Guenter Roeck 0 siblings, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2011-01-25 16:43 UTC (permalink / raw) To: lm-sensors-GZX6beZjE8VD60Wz+7aTrA; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA On Tue, Jan 25, 2011 at 11:21:06AM -0500, Guenter Roeck wrote: > Hi, > > I need the capability to write a SMBus block command with i2ctools. > > Not an issue writing it myself. Question is if to expand i2cset to include > that capability, or to write a new command (i2csetblock ?) to do it. > > Thoughts ? > Sorry, wrong mailing list. Copying linux-i2c. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: i2ctools: Need capability to write SMBus block command [not found] ` <20110125162106.GA8024-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 2011-01-25 16:43 ` [lm-sensors] i2ctools: Need capability to write SMBus block Guenter Roeck @ 2011-01-25 16:49 ` Jean Delvare [not found] ` <20110125174922.4c802f49-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 1 sibling, 1 reply; 11+ messages in thread From: Jean Delvare @ 2011-01-25 16:49 UTC (permalink / raw) To: Guenter Roeck; +Cc: Linux I2C Hi Guenter, On Tue, 25 Jan 2011 08:21:06 -0800, Guenter Roeck wrote: > I need the capability to write a SMBus block command with i2ctools. > > Not an issue writing it myself. Question is if to expand i2cset to include > that capability, or to write a new command (i2csetblock ?) to do it. > > Thoughts ? This would probably be best discussed on the linux-i2c list than lm-sensors. Redirecting there... I would add it to i2cset. The only problem is that the command line convention of i2cset is a little tricky and thus hard to extend. But I see no reason why allowing multiple VALUEs followed by MODE s (for SMBus block) or i (for I2C block) wouldn't work. I suggest i and s because this is what i2cdump is using. -- Jean Delvare ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110125174922.4c802f49-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>]
* Re: i2ctools: Need capability to write SMBus block command [not found] ` <20110125174922.4c802f49-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> @ 2011-01-25 17:14 ` Guenter Roeck 2011-01-27 16:00 ` [PATCH] i2ctools: Add capability to write " Guenter Roeck 1 sibling, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2011-01-25 17:14 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Tue, Jan 25, 2011 at 11:49:22AM -0500, Jean Delvare wrote: > Hi Guenter, > > On Tue, 25 Jan 2011 08:21:06 -0800, Guenter Roeck wrote: > > I need the capability to write a SMBus block command with i2ctools. > > > > Not an issue writing it myself. Question is if to expand i2cset to include > > that capability, or to write a new command (i2csetblock ?) to do it. > > > > Thoughts ? > > This would probably be best discussed on the linux-i2c list than > lm-sensors. Redirecting there... > Yes, I noticed that after I sent my e-mail. Sorry for that. > I would add it to i2cset. The only problem is that the command line > convention of i2cset is a little tricky and thus hard to extend. But I > see no reason why allowing multiple VALUEs followed by MODE s (for > SMBus block) or i (for I2C block) wouldn't work. I suggest i and s > because this is what i2cdump is using. > Ok, that is pretty much along the line of what I was thinking. I'll implement that and submit it for review. Thanks, Guenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] i2ctools: Add capability to write block command [not found] ` <20110125174922.4c802f49-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 2011-01-25 17:14 ` Guenter Roeck @ 2011-01-27 16:00 ` Guenter Roeck [not found] ` <20110127160058.GA16853-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 1 sibling, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2011-01-27 16:00 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C This patch adds support to write block data to i2cset. I tried to limit the changes as much as possible. Detecting new write modes is a bit tricky since the command supports an undocumented parameter (mask) after the mode. So I decided to handle block data first and bypass the rest of the parameter handling code. Guenter -- Index: tools/i2cset.c =================================================================== --- tools/i2cset.c (revision 5909) +++ tools/i2cset.c (working copy) @@ -35,13 +35,15 @@ static void help(void) { fprintf(stderr, - "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n" + "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n" " I2CBUS is an integer or an I2C bus name\n" " ADDRESS is an integer (0x03 - 0x77)\n" " MODE is one of:\n" " c (byte, no value)\n" " b (byte data, default)\n" " w (word data)\n" + " i (I2C block data)\n" + " s (SMBus block data)\n" " Append p for SMBus PEC\n"); exit(1); } @@ -78,6 +80,19 @@ return -1; } break; + + case I2C_SMBUS_BLOCK_DATA: + if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) { + fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read"); + return -1; + } + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { + fprintf(stderr, MISSING_FUNC_FMT, "I2C block read"); + return -1; + } + break; } if (pec @@ -90,7 +105,7 @@ } static int confirm(const char *filename, int address, int size, int daddress, - int value, int vmask, int pec) + int value, int vmask, unsigned char *block, int len, int pec) { int dont = 0; @@ -109,7 +124,16 @@ "0x%02x, data address\n0x%02x, ", filename, address, daddress); if (size == I2C_SMBUS_BYTE) fprintf(stderr, "no data.\n"); - else + else if (size == I2C_SMBUS_BLOCK_DATA || + size == I2C_SMBUS_I2C_BLOCK_DATA) { + int i; + + fprintf(stderr, "data"); + for (i = 0; i < len; i++) + fprintf(stderr, " 0x%02x", block[i]); + fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA + ? "smbus block" : "i2c block"); + } else fprintf(stderr, "data 0x%02x%s, mode %s.\n", value, vmask ? " (masked)" : "", size == I2C_SMBUS_BYTE_DATA ? "byte" : "word"); @@ -136,6 +160,8 @@ int pec = 0; int flags = 0; int force = 0, yes = 0, version = 0, readback = 0; + unsigned char block[32]; + int len; /* handle (optional) flags first */ while (1+flags < argc && argv[1+flags][0] == '-') { @@ -180,6 +206,30 @@ help(); } + /* check for block data */ + len = 0; + if (argc > flags + 5) { + switch (argv[argc-1][0]) { + case 's': size = I2C_SMBUS_BLOCK_DATA; break; + case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break; + default: + size = 0; + break; + } + if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) { + pec = argv[argc-1][1] == 'p'; + for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) { + value = strtol(argv[flags + len + 4], &end, 0); + if (*end || value < 0 || value > 0xff) { + fprintf(stderr, "Error: Block data value invalid!\n"); + help(); + } + block[len] = value; + } + goto dofile; + } + } + if (argc > flags + 4) { if (!strcmp(argv[flags+4], "c") || !strcmp(argv[flags+4], "cp")) { @@ -236,6 +286,7 @@ help(); } +dofile: file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0); if (file < 0 || check_funcs(file, size, pec) @@ -243,7 +294,7 @@ exit(1); if (!yes && !confirm(filename, address, size, daddress, - value, vmask, pec)) + value, vmask, block, len, pec)) exit(0); if (vmask) { @@ -299,11 +350,18 @@ case I2C_SMBUS_WORD_DATA: res = i2c_smbus_write_word_data(file, daddress, value); break; + case I2C_SMBUS_BLOCK_DATA: + res = i2c_smbus_write_block_data(file, daddress, len, block); + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + res = i2c_smbus_write_i2c_block_data(file, daddress, len, block); + break; default: /* I2C_SMBUS_BYTE_DATA */ res = i2c_smbus_write_byte_data(file, daddress, value); + break; } if (res < 0) { - fprintf(stderr, "Error: Write failed\n"); + perror("Error: Write failed"); close(file); exit(1); } ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110127160058.GA16853-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH] i2ctools: Add capability to write block command [not found] ` <20110127160058.GA16853-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> @ 2011-01-27 16:59 ` Jean Delvare [not found] ` <20110127175908.488edda3-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Jean Delvare @ 2011-01-27 16:59 UTC (permalink / raw) To: Guenter Roeck; +Cc: Linux I2C Hi Guenter, On Thu, 27 Jan 2011 08:00:58 -0800, Guenter Roeck wrote: > This patch adds support to write block data to i2cset. > > I tried to limit the changes as much as possible. Detecting new write modes > is a bit tricky since the command supports an undocumented parameter (mask) > after the mode. This can go away if it bothers you. This was the old way to pass the mask value. I have implemented -m meanwhile, and this is what people should be using by now. I added it 2 years ago, so I think it's acceptable to stop supporting the legacy way. > So I decided to handle block data first and bypass the rest > of the parameter handling code. > > Guenter > > -- > Index: tools/i2cset.c > =================================================================== > --- tools/i2cset.c (revision 5909) > +++ tools/i2cset.c (working copy) > @@ -35,13 +35,15 @@ > static void help(void) > { > fprintf(stderr, > - "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n" > + "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n" > " I2CBUS is an integer or an I2C bus name\n" > " ADDRESS is an integer (0x03 - 0x77)\n" > " MODE is one of:\n" > " c (byte, no value)\n" > " b (byte data, default)\n" > " w (word data)\n" > + " i (I2C block data)\n" > + " s (SMBus block data)\n" > " Append p for SMBus PEC\n"); > exit(1); > } > @@ -78,6 +80,19 @@ > return -1; > } > break; > + > + case I2C_SMBUS_BLOCK_DATA: > + if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) { > + fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read"); > + return -1; > + } > + break; > + case I2C_SMBUS_I2C_BLOCK_DATA: > + if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > + fprintf(stderr, MISSING_FUNC_FMT, "I2C block read"); > + return -1; > + } > + break; Why are you testing READ functionalities when what you want to do is WRITE? > } > > if (pec > @@ -90,7 +105,7 @@ > } > > static int confirm(const char *filename, int address, int size, int daddress, > - int value, int vmask, int pec) > + int value, int vmask, unsigned char *block, int len, int pec) The block pointer could be const. > { > int dont = 0; > > @@ -109,7 +124,16 @@ > "0x%02x, data address\n0x%02x, ", filename, address, daddress); > if (size == I2C_SMBUS_BYTE) > fprintf(stderr, "no data.\n"); > - else > + else if (size == I2C_SMBUS_BLOCK_DATA || > + size == I2C_SMBUS_I2C_BLOCK_DATA) { > + int i; > + > + fprintf(stderr, "data"); > + for (i = 0; i < len; i++) > + fprintf(stderr, " 0x%02x", block[i]); > + fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA > + ? "smbus block" : "i2c block"); > + } else > fprintf(stderr, "data 0x%02x%s, mode %s.\n", value, > vmask ? " (masked)" : "", > size == I2C_SMBUS_BYTE_DATA ? "byte" : "word"); > @@ -136,6 +160,8 @@ > int pec = 0; > int flags = 0; > int force = 0, yes = 0, version = 0, readback = 0; > + unsigned char block[32]; It might make sense to use I2C_SMBUS_BLOCK_MAX instead of hard-coding 32? > + int len; > > /* handle (optional) flags first */ > while (1+flags < argc && argv[1+flags][0] == '-') { > @@ -180,6 +206,30 @@ > help(); > } > > + /* check for block data */ > + len = 0; > + if (argc > flags + 5) { This makes it impossible to write 1-byte blocks, right? This is bad. > + switch (argv[argc-1][0]) { > + case 's': size = I2C_SMBUS_BLOCK_DATA; break; > + case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break; > + default: > + size = 0; > + break; > + } > + if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) { > + pec = argv[argc-1][1] == 'p'; ip isn't a valid mode. PEC is not defined for non-SMBus transactions (and despite its name, I2C_SMBUS_I2C_BLOCK_DATA read and writes are not SMBus transactions. > + for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) { Do you actually need the cast? > + value = strtol(argv[flags + len + 4], &end, 0); > + if (*end || value < 0 || value > 0xff) { > + fprintf(stderr, "Error: Block data value invalid!\n"); > + help(); > + } > + block[len] = value; > + } > + goto dofile; > + } > + } > + > if (argc > flags + 4) { > if (!strcmp(argv[flags+4], "c") > || !strcmp(argv[flags+4], "cp")) { > @@ -236,6 +286,7 @@ > help(); > } > > +dofile: > file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0); > if (file < 0 > || check_funcs(file, size, pec) > @@ -243,7 +294,7 @@ > exit(1); > > if (!yes && !confirm(filename, address, size, daddress, > - value, vmask, pec)) > + value, vmask, block, len, pec)) > exit(0); > > if (vmask) { > @@ -299,11 +350,18 @@ > case I2C_SMBUS_WORD_DATA: > res = i2c_smbus_write_word_data(file, daddress, value); > break; > + case I2C_SMBUS_BLOCK_DATA: > + res = i2c_smbus_write_block_data(file, daddress, len, block); > + break; > + case I2C_SMBUS_I2C_BLOCK_DATA: > + res = i2c_smbus_write_i2c_block_data(file, daddress, len, block); > + break; > default: /* I2C_SMBUS_BYTE_DATA */ > res = i2c_smbus_write_byte_data(file, daddress, value); > + break; > } > if (res < 0) { > - fprintf(stderr, "Error: Write failed\n"); > + perror("Error: Write failed"); Hmm, do i2c_smbus_()* calls actually set errno? I didn't expect them to. Either way, if this change is wanted, it doesn't belong to this patch. > close(file); > exit(1); > } I tested your patch for the I2C block write case, it worked OK. Please also update the manual page. -- Jean Delvare ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110127175908.488edda3-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH] i2ctools: Add capability to write block command [not found] ` <20110127175908.488edda3-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> @ 2011-01-27 18:36 ` Guenter Roeck 2011-01-27 19:58 ` [PATCH v2] " Guenter Roeck 1 sibling, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2011-01-27 18:36 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C On Thu, Jan 27, 2011 at 11:59:08AM -0500, Jean Delvare wrote: > Hi Guenter, > > On Thu, 27 Jan 2011 08:00:58 -0800, Guenter Roeck wrote: > > This patch adds support to write block data to i2cset. > > > > I tried to limit the changes as much as possible. Detecting new write modes > > is a bit tricky since the command supports an undocumented parameter (mask) > > after the mode. > > This can go away if it bothers you. This was the old way to pass the > mask value. I have implemented -m meanwhile, and this is what people > should be using by now. I added it 2 years ago, so I think it's > acceptable to stop supporting the legacy way. > Would be a separate patch. Also, the current code accepts nonsense parameters (such as comments) after the mode parameter. Not sure if I want to change that. > > So I decided to handle block data first and bypass the rest > > of the parameter handling code. > > > > Guenter > > > > -- > > Index: tools/i2cset.c > > =================================================================== > > --- tools/i2cset.c (revision 5909) > > +++ tools/i2cset.c (working copy) > > @@ -35,13 +35,15 @@ > > static void help(void) > > { > > fprintf(stderr, > > - "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n" > > + "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n" > > " I2CBUS is an integer or an I2C bus name\n" > > " ADDRESS is an integer (0x03 - 0x77)\n" > > " MODE is one of:\n" > > " c (byte, no value)\n" > > " b (byte data, default)\n" > > " w (word data)\n" > > + " i (I2C block data)\n" > > + " s (SMBus block data)\n" > > " Append p for SMBus PEC\n"); > > exit(1); > > } > > @@ -78,6 +80,19 @@ > > return -1; > > } > > break; > > + > > + case I2C_SMBUS_BLOCK_DATA: > > + if (!(funcs & I2C_FUNC_SMBUS_READ_BLOCK_DATA)) { > > + fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read"); > > + return -1; > > + } > > + break; > > + case I2C_SMBUS_I2C_BLOCK_DATA: > > + if (!(funcs & I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > > + fprintf(stderr, MISSING_FUNC_FMT, "I2C block read"); > > + return -1; > > + } > > + break; > > Why are you testing READ functionalities when what you want to do is > WRITE? > Typical cut-and-paste error. > > } > > > > if (pec > > @@ -90,7 +105,7 @@ > > } > > > > static int confirm(const char *filename, int address, int size, int daddress, > > - int value, int vmask, int pec) > > + int value, int vmask, unsigned char *block, int len, int pec) > > The block pointer could be const. > Ok. > > { > > int dont = 0; > > > > @@ -109,7 +124,16 @@ > > "0x%02x, data address\n0x%02x, ", filename, address, daddress); > > if (size == I2C_SMBUS_BYTE) > > fprintf(stderr, "no data.\n"); > > - else > > + else if (size == I2C_SMBUS_BLOCK_DATA || > > + size == I2C_SMBUS_I2C_BLOCK_DATA) { > > + int i; > > + > > + fprintf(stderr, "data"); > > + for (i = 0; i < len; i++) > > + fprintf(stderr, " 0x%02x", block[i]); > > + fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA > > + ? "smbus block" : "i2c block"); > > + } else > > fprintf(stderr, "data 0x%02x%s, mode %s.\n", value, > > vmask ? " (masked)" : "", > > size == I2C_SMBUS_BYTE_DATA ? "byte" : "word"); > > @@ -136,6 +160,8 @@ > > int pec = 0; > > int flags = 0; > > int force = 0, yes = 0, version = 0, readback = 0; > > + unsigned char block[32]; > > It might make sense to use I2C_SMBUS_BLOCK_MAX instead of hard-coding > 32? > Ok. > > + int len; > > > > /* handle (optional) flags first */ > > while (1+flags < argc && argv[1+flags][0] == '-') { > > @@ -180,6 +206,30 @@ > > help(); > > } > > > > + /* check for block data */ > > + len = 0; > > + if (argc > flags + 5) { > > This makes it impossible to write 1-byte blocks, right? This is bad. > No, it is the same check used for 'b' and 'w'. I tested it, and it works. > > + switch (argv[argc-1][0]) { > > + case 's': size = I2C_SMBUS_BLOCK_DATA; break; > > + case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break; > > + default: > > + size = 0; > > + break; > > + } > > + if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) { > > + pec = argv[argc-1][1] == 'p'; > > ip isn't a valid mode. PEC is not defined for non-SMBus transactions > (and despite its name, I2C_SMBUS_I2C_BLOCK_DATA read and writes are not > SMBus transactions. > Ok, I added an error check. > > + for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) { > > Do you actually need the cast? > Yes, at least with recent versions of gcc. sizeof() is an unsigned and len is an int, so without the cast gcc warns about a signed-unsigned comparison. I could change len to be unsigned, but then it starts complaining about comparisons of len against other variables, so I'd rather stick with the typecast. > > + value = strtol(argv[flags + len + 4], &end, 0); > > + if (*end || value < 0 || value > 0xff) { > > + fprintf(stderr, "Error: Block data value invalid!\n"); > > + help(); > > + } > > + block[len] = value; > > + } > > + goto dofile; > > + } > > + } > > + > > if (argc > flags + 4) { > > if (!strcmp(argv[flags+4], "c") > > || !strcmp(argv[flags+4], "cp")) { > > @@ -236,6 +286,7 @@ > > help(); > > } > > > > +dofile: > > file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0); > > if (file < 0 > > || check_funcs(file, size, pec) > > @@ -243,7 +294,7 @@ > > exit(1); > > > > if (!yes && !confirm(filename, address, size, daddress, > > - value, vmask, pec)) > > + value, vmask, block, len, pec)) > > exit(0); > > > > if (vmask) { > > @@ -299,11 +350,18 @@ > > case I2C_SMBUS_WORD_DATA: > > res = i2c_smbus_write_word_data(file, daddress, value); > > break; > > + case I2C_SMBUS_BLOCK_DATA: > > + res = i2c_smbus_write_block_data(file, daddress, len, block); > > + break; > > + case I2C_SMBUS_I2C_BLOCK_DATA: > > + res = i2c_smbus_write_i2c_block_data(file, daddress, len, block); > > + break; > > default: /* I2C_SMBUS_BYTE_DATA */ > > res = i2c_smbus_write_byte_data(file, daddress, value); > > + break; > > } > > if (res < 0) { > > - fprintf(stderr, "Error: Write failed\n"); > > + perror("Error: Write failed"); > > Hmm, do i2c_smbus_()* calls actually set errno? I didn't expect them > to. Either way, if this change is wanted, it doesn't belong to this > patch. > Yes, they do. But you are right, this change doesn't belong into this patch. Leftover from testing anyway, so I removed it. > > close(file); > > exit(1); > > } > > I tested your patch for the I2C block write case, it worked OK. > > Please also update the manual page. > Ok, will do. Thanks, Guenter ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] i2ctools: Add capability to write block command [not found] ` <20110127175908.488edda3-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 2011-01-27 18:36 ` Guenter Roeck @ 2011-01-27 19:58 ` Guenter Roeck [not found] ` <20110127195842.GA17775-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 1 sibling, 1 reply; 11+ messages in thread From: Guenter Roeck @ 2011-01-27 19:58 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C Add support to write SMBus and I2C block data to i2cset. Rev 2: Included review feedback -- Index: tools/i2cset.8 =================================================================== --- tools/i2cset.8 (revision 5909) +++ tools/i2cset.8 (working copy) @@ -12,6 +12,7 @@ .I chip-address .I data-address .RI [ value ] +.RI ... .RI [ mode ] .br .B i2cset @@ -62,18 +63,24 @@ integer between 0x00 and 0xFF. .PP The \fIvalue\fR parameter, if specified, is the value to write to that -location on the chip. If this parameter is omited, then a short write is +location on the chip. If this parameter is omitted, then a short write is issued. For most chips, it simply sets an internal pointer to the target location, but doesn't actually write to that location. For a few chips though, in particular simple ones with a single register, this short write -is an actual write. +is an actual write. If the mode parameter is \fBs\fP or \fBi\fP, multiple +values can be specified. .PP -The \fImode\fR parameter, if specified, is one of the letters \fBb\fP or -\fBw\fP, corresponding to a write size of a single byte or a 16-bit word, -respectively. A \fBp\fP can also be appended to the \fImode\fR parameter to -enable PEC. If the \fImode\fR parameter is omitted, i2cset defaults to byte +The \fImode\fR parameter, if specified, is one of the letters \fBb\fP, +\fBw\fP, \fBs\fP, or \fBi\fP, corresponding to a write size of a single byte, +a 16-bit word, a SMBus block write, or an I2C block write, respectively. +For SMBus and I2C block writes, the write size is determined by the number +of \fIvalue\fR parameters. +Except for I2C block writes, a \fBp\fP can also be appended to the \fImode\fR +parameter to enable PEC. +If the \fImode\fR parameter is omitted, i2cset defaults to byte mode without PEC. The \fIvalue\fR provided must be within range for the -specified data type (0x00-0xFF for bytes, 0x0000-0xFFFF for words). +specified data type (0x00-0xFF for byte and block writes, 0x0000-0xFFFF +for words). Another possible mode is \fBc\fP, which doesn't write any value (so-called short write). You usually don't have to specify this mode, as it is the default when no value is provided, unless you also want to enable PEC. Index: tools/i2cset.c =================================================================== --- tools/i2cset.c (revision 5909) +++ tools/i2cset.c (working copy) @@ -35,13 +35,15 @@ static void help(void) { fprintf(stderr, - "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] [MODE]\n" + "Usage: i2cset [-f] [-y] [-m MASK] I2CBUS CHIP-ADDRESS DATA-ADDRESS [VALUE] ... [MODE]\n" " I2CBUS is an integer or an I2C bus name\n" " ADDRESS is an integer (0x03 - 0x77)\n" " MODE is one of:\n" " c (byte, no value)\n" " b (byte data, default)\n" " w (word data)\n" + " i (I2C block data)\n" + " s (SMBus block data)\n" " Append p for SMBus PEC\n"); exit(1); } @@ -78,6 +80,19 @@ return -1; } break; + + case I2C_SMBUS_BLOCK_DATA: + if (!(funcs & I2C_FUNC_SMBUS_WRITE_BLOCK_DATA)) { + fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read"); + return -1; + } + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + if (!(funcs & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) { + fprintf(stderr, MISSING_FUNC_FMT, "I2C block read"); + return -1; + } + break; } if (pec @@ -90,7 +105,8 @@ } static int confirm(const char *filename, int address, int size, int daddress, - int value, int vmask, int pec) + int value, int vmask, const unsigned char *block, int len, + int pec) { int dont = 0; @@ -109,7 +125,16 @@ "0x%02x, data address\n0x%02x, ", filename, address, daddress); if (size == I2C_SMBUS_BYTE) fprintf(stderr, "no data.\n"); - else + else if (size == I2C_SMBUS_BLOCK_DATA || + size == I2C_SMBUS_I2C_BLOCK_DATA) { + int i; + + fprintf(stderr, "data"); + for (i = 0; i < len; i++) + fprintf(stderr, " 0x%02x", block[i]); + fprintf(stderr, ", mode %s.\n", size == I2C_SMBUS_BLOCK_DATA + ? "smbus block" : "i2c block"); + } else fprintf(stderr, "data 0x%02x%s, mode %s.\n", value, vmask ? " (masked)" : "", size == I2C_SMBUS_BYTE_DATA ? "byte" : "word"); @@ -136,6 +161,8 @@ int pec = 0; int flags = 0; int force = 0, yes = 0, version = 0, readback = 0; + unsigned char block[I2C_SMBUS_BLOCK_MAX]; + int len; /* handle (optional) flags first */ while (1+flags < argc && argv[1+flags][0] == '-') { @@ -180,6 +207,34 @@ help(); } + /* check for block data */ + len = 0; + if (argc > flags + 5) { + switch (argv[argc-1][0]) { + case 's': size = I2C_SMBUS_BLOCK_DATA; break; + case 'i': size = I2C_SMBUS_I2C_BLOCK_DATA; break; + default: + size = 0; + break; + } + if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_I2C_BLOCK_DATA) { + pec = argv[argc-1][1] == 'p'; + if (pec && size == I2C_SMBUS_I2C_BLOCK_DATA) { + fprintf(stderr, "Error: PEC not supported for I2C block writes!\n"); + help(); + } + for (len = 0; len < (int)sizeof(block) && len + flags + 5 < argc; len++) { + value = strtol(argv[flags + len + 4], &end, 0); + if (*end || value < 0 || value > 0xff) { + fprintf(stderr, "Error: Block data value invalid!\n"); + help(); + } + block[len] = value; + } + goto dofile; + } + } + if (argc > flags + 4) { if (!strcmp(argv[flags+4], "c") || !strcmp(argv[flags+4], "cp")) { @@ -236,6 +291,7 @@ help(); } +dofile: file = open_i2c_dev(i2cbus, filename, sizeof(filename), 0); if (file < 0 || check_funcs(file, size, pec) @@ -243,7 +299,7 @@ exit(1); if (!yes && !confirm(filename, address, size, daddress, - value, vmask, pec)) + value, vmask, block, len, pec)) exit(0); if (vmask) { @@ -299,8 +355,15 @@ case I2C_SMBUS_WORD_DATA: res = i2c_smbus_write_word_data(file, daddress, value); break; + case I2C_SMBUS_BLOCK_DATA: + res = i2c_smbus_write_block_data(file, daddress, len, block); + break; + case I2C_SMBUS_I2C_BLOCK_DATA: + res = i2c_smbus_write_i2c_block_data(file, daddress, len, block); + break; default: /* I2C_SMBUS_BYTE_DATA */ res = i2c_smbus_write_byte_data(file, daddress, value); + break; } if (res < 0) { fprintf(stderr, "Error: Write failed\n"); Index: CHANGES =================================================================== --- CHANGES (revision 5909) +++ CHANGES (working copy) @@ -3,6 +3,7 @@ SVN i2c-dev.h: Make value arrays const for block write functions + i2cset: Add support for SMBus and I2C block writes 3.0.3 (2010-12-12) Makefile: Let the environment set CC and CFLAGS ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110127195842.GA17775-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>]
* Re: [PATCH v2] i2ctools: Add capability to write block command [not found] ` <20110127195842.GA17775-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> @ 2011-01-29 16:43 ` Jean Delvare [not found] ` <20110129174354.2b146a19-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Jean Delvare @ 2011-01-29 16:43 UTC (permalink / raw) To: Guenter Roeck; +Cc: Linux I2C Hi Guenter, On Thu, 27 Jan 2011 11:58:42 -0800, Guenter Roeck wrote: > Add support to write SMBus and I2C block data to i2cset. > > Rev 2: Included review feedback Two remaining minor issues: > @@ -78,6 +80,19 @@ > return -1; > } > break; > + > + case I2C_SMBUS_BLOCK_DATA: > + if (!(funcs & I2C_FUNC_SMBUS_WRITE_BLOCK_DATA)) { > + fprintf(stderr, MISSING_FUNC_FMT, "SMBus block read"); "SMBus block write" > + return -1; > + } > + break; > + case I2C_SMBUS_I2C_BLOCK_DATA: > + if (!(funcs & I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) { > + fprintf(stderr, MISSING_FUNC_FMT, "I2C block read"); "I2C block write" > + return -1; > + } > + break; > } > > if (pec Other than that, it looks alright, feel free to commit (I've just added you to the list of committers - same credentials as lm-sensors repository.) Next, I think we can remove the old method for setting the mask. Then we'll see if some code cleanup or reorganization makes sense. -- Jean Delvare ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110129174354.2b146a19-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH v2] i2ctools: Add capability to write block command [not found] ` <20110129174354.2b146a19-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> @ 2011-01-29 17:39 ` Guenter Roeck 0 siblings, 0 replies; 11+ messages in thread From: Guenter Roeck @ 2011-01-29 17:39 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C Hi Jean, On Sat, Jan 29, 2011 at 11:43:54AM -0500, Jean Delvare wrote: [ ...] > > Other than that, it looks alright, feel free to commit (I've just added > you to the list of committers - same credentials as lm-sensors > repository.) > Thanks, and done. > Next, I think we can remove the old method for setting the mask. Then > we'll see if some code cleanup or reorganization makes sense. > I'll see what I can do. Guenter ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-01-29 17:39 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-25 16:21 [lm-sensors] i2ctools: Need capability to write SMBus block command Guenter Roeck [not found] ` <20110125162106.GA8024-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 2011-01-25 16:43 ` Guenter Roeck 2011-01-25 16:43 ` [lm-sensors] i2ctools: Need capability to write SMBus block Guenter Roeck 2011-01-25 16:49 ` i2ctools: Need capability to write SMBus block command Jean Delvare [not found] ` <20110125174922.4c802f49-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 2011-01-25 17:14 ` Guenter Roeck 2011-01-27 16:00 ` [PATCH] i2ctools: Add capability to write " Guenter Roeck [not found] ` <20110127160058.GA16853-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 2011-01-27 16:59 ` Jean Delvare [not found] ` <20110127175908.488edda3-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 2011-01-27 18:36 ` Guenter Roeck 2011-01-27 19:58 ` [PATCH v2] " Guenter Roeck [not found] ` <20110127195842.GA17775-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> 2011-01-29 16:43 ` Jean Delvare [not found] ` <20110129174354.2b146a19-R0o5gVi9kd7kN2dkZ6Wm7A@public.gmane.org> 2011-01-29 17:39 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.