* [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
* 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
* 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
* 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
* 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
* 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
* 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.