smbus read help needed

DISCUSSION Alison Schofield 2 participants 4 messages Aug 3, 2016 – Aug 5, 2016 1 message not in the archive IIO

Share & tools

Alison Schofield

author

2016-08-03 05:05 UTC #0 lore raw

I'm blocked on this smbus read problem.

hdc100x triggered buffer review feedback pointed out that I cannot rely on i2c_master_recv() since this driver currently only requires smbus funcs. That led me to create an alternative path using smbus byte reads like the driver was doing in direct mode.

I found the reads don't work. hdc100x does not expect a stop condition after sending the first byte which is what smbus_read_byte gives you. So, when you do the second read, you are getting the first byte again. Net effect is that of the 14 bits used for the measurement, the 8 most significant bits are correct, the lower 6 are not.

hdc100x only wants this:
S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P

I tried by testing, and by inspection, each flavor of smbus read and none match the pattern that hdc100x wants. (read_byte, read_word_data, read_word_swapped, read_block_data, read_i2c_block_data all fail) The read_byte is actually the only smbus read command the sensor accepts.

Texas Instruments publishes this doc explaining its SMBus (in)compatibility.
http://www.ti.com/lit/an/sloa132/sloa132.pdf
I did get one lead out of it. It suggests using the write block protocol, with the READ bit set. That does look like it could work. I tried using i2c_smbus_xfer() directly, thinking maybe I could fool it, but that doesn't get me down to the low level of control I think I would need to pull this off.

I see flags for i2c_msg that might be helpful...if they worked at the smbus level:
I2C_M_REV_DIR_ADDR reverses r/w bit
I2C_M_NOSTART strips off that beginning segment we don't want

If we could use a NOSTOP flag on the read byte command then i could go back and get the next byte. I don't see such a flag. I don't see any flags, other than for PEC, on smbus.

Also, saw an MDELAY flag that seemed interesting, as if I could program the delay between starting and reading the measurement, so it could all be done in one block data command. Again, not smbus.

I guess these ideas are all breaking the idea of being smbus compliant anyway.

Is this fixable with smbus?
If not, how do you 'graciously' change a drivers requirements?

Thanks,
alisons

Lars-Peter Clausen

2016-08-03 12:13 UTC #1 lore raw

Quoted 41 lines · Alison Schofield

I'm blocked on this smbus read problem.
hdc100x triggered buffer review feedback pointed out that I cannot rely on i2c_master_recv() since this driver currently only requires smbus funcs. That led me to create an alternative path using smbus byte reads like the driver was doing in direct mode.
I found the reads don't work. hdc100x does not expect a stop condition after sending the first byte which is what smbus_read_byte gives you. So, when you do the second read, you are getting the first byte again. Net effect is that of the 14 bits used for the measurement, the 8 most significant bits are correct, the lower 6 are not.
hdc100x only wants this:
S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
I tried by testing, and by inspection, each flavor of smbus read and none match the pattern that hdc100x wants. (read_byte, read_word_data, read_word_swapped, read_block_data, read_i2c_block_data all fail) The read_byte is actually the only smbus read command the sensor accepts.
Texas Instruments publishes this doc explaining its SMBus (in)compatibility.
http://www.ti.com/lit/an/sloa132/sloa132.pdf
I did get one lead out of it. It suggests using the write block protocol, with the READ bit set. That does look like it could work. I tried using i2c_smbus_xfer() directly, thinking maybe I could fool it, but that doesn't get me down to the low level of control I think I would need to pull this off.
I see flags for i2c_msg that might be helpful...if they worked at the smbus level:
I2C_M_REV_DIR_ADDR reverses r/w bit
I2C_M_NOSTART strips off that beginning segment we don't want
If we could use a NOSTOP flag on the read byte command then i could go back and get the next byte. I don't see such a flag. I don't see any flags, other than for PEC, on smbus.
Also, saw an MDELAY flag that seemed interesting, as if I could program the delay between starting and reading the measurement, so it could all be done in one block data command. Again, not smbus.

Quoted 5 lines

I guess these ideas are all breaking the idea of being smbus compliant anyway.
Is this fixable with smbus?
If not, how do you 'graciously' change a drivers requirements?

I wouldn't worry about this too much. If the part is not SMBUS compliant there is no need to try to access it only with SMBUS methods. A system designer will most likely not connect the part to a SMBUS-only controller.

What's actually quite important though at a software level is that you do your write+read in a single I2C transaction and not split it over multiple calls. Otherwise a device on the same I2C bus could be accessed in between and that would really break things.

If you want to address Jonathan's comments about backwards compatibility, instead of returning with an error simply disable buffer mode if the host controller only has SMBUS capabilities.

Alison Schofield

author

2016-08-03 15:12 UTC #2 lore raw

Quoted 53 lines · Lars-Peter Clausen

Quoted 41 lines · Alison Schofield
I'm blocked on this smbus read problem.
hdc100x triggered buffer review feedback pointed out that I cannot rely on i2c_master_recv() since this driver currently only requires smbus funcs. That led me to create an alternative path using smbus byte reads like the driver was doing in direct mode.
I found the reads don't work. hdc100x does not expect a stop condition after sending the first byte which is what smbus_read_byte gives you. So, when you do the second read, you are getting the first byte again. Net effect is that of the 14 bits used for the measurement, the 8 most significant bits are correct, the lower 6 are not.
hdc100x only wants this:
S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
I tried by testing, and by inspection, each flavor of smbus read and none match the pattern that hdc100x wants. (read_byte, read_word_data, read_word_swapped, read_block_data, read_i2c_block_data all fail) The read_byte is actually the only smbus read command the sensor accepts.
Texas Instruments publishes this doc explaining its SMBus (in)compatibility.
http://www.ti.com/lit/an/sloa132/sloa132.pdf
I did get one lead out of it. It suggests using the write block protocol, with the READ bit set. That does look like it could work. I tried using i2c_smbus_xfer() directly, thinking maybe I could fool it, but that doesn't get me down to the low level of control I think I would need to pull this off.
I see flags for i2c_msg that might be helpful...if they worked at the smbus level:
I2C_M_REV_DIR_ADDR reverses r/w bit
I2C_M_NOSTART strips off that beginning segment we don't want
If we could use a NOSTOP flag on the read byte command then i could go back and get the next byte. I don't see such a flag. I don't see any flags, other than for PEC, on smbus.
Also, saw an MDELAY flag that seemed interesting, as if I could program the delay between starting and reading the measurement, so it could all be done in one block data command. Again, not smbus.
Quoted 5 lines
I guess these ideas are all breaking the idea of being smbus compliant anyway.
Is this fixable with smbus?
If not, how do you 'graciously' change a drivers requirements?
I wouldn't worry about this too much. If the part is not SMBUS compliant there is no need to try to access it only with SMBUS methods. A system designer will most likely not connect the part to a SMBUS-only controller.

Quoted 9 lines

What's actually quite important though at a software level is that you do your write+read in a single I2C transaction and not split it over multiple calls. Otherwise a device on the same I2C bus could be accessed in between and that would really break things.
If you want to address Jonathan's comments about backwards compatibility, instead of returning with an error simply disable buffer mode if the host controller only has SMBUS capabilities.

Thank Lars.
I need to fix the existing direct mode reads first. That is the part that could break existing userspace.

I like what you say about a system designer not connecting the part to a SMBUS-only controller, but I'm guessing this fix is going to break the user who got lucky with SMBUS-only.

I'll work up a patch for fixing the existing reads (with i2c cmds) and get it out for comments.

alisons

A message that is not in the archive (sent off-list or not yet received): [email protected]

Alison Schofield

author

2016-08-05 16:44 UTC #4 lore raw

Quoted 90 lines · Jonathan Cameron

Quoted 87 lines · Alison Schofield
Quoted 4 lines · Lars-Peter Clausen
Quoted 3 lines · Alison Schofield
I'm blocked on this smbus read problem.
hdc100x triggered buffer review feedback pointed out that I cannot
rely
Quoted 1 line
Quoted 1 line
on i2c_master_recv() since this driver currently only requires
smbus funcs.
Quoted 1 line
Quoted 1 line
That led me to create an alternative path using smbus byte reads
like the
Quoted 3 lines
Quoted 3 lines
driver was doing in direct mode.
I found the reads don't work. hdc100x does not expect a stop
condition
Quoted 1 line
Quoted 1 line
after sending the first byte which is what smbus_read_byte gives
you. So,
Quoted 1 line
Quoted 1 line
when you do the second read, you are getting the first byte again.
Net
Quoted 7 lines
Quoted 7 lines
effect is that of the 14 bits used for the measurement, the 8 most significant bits are correct, the lower 6 are not.
hdc100x only wants this:
S Addr Rd [A] [Data] A [Data] A ... A [Data] NA P
I tried by testing, and by inspection, each flavor of smbus read
and none
Quoted 3 lines
Quoted 3 lines
match the pattern that hdc100x wants. (read_byte, read_word_data, read_word_swapped, read_block_data, read_i2c_block_data all fail) The read_byte is actually the only smbus read command the sensor
accepts.
Quoted 2 lines
Quoted 2 lines
Texas Instruments publishes this doc explaining its SMBus
(in)compatibility.
Quoted 2 lines
Quoted 2 lines
http://www.ti.com/lit/an/sloa132/sloa132.pdf
I did get one lead out of it. It suggests using the write block
protocol,
Quoted 1 line
Quoted 1 line
with the READ bit set. That does look like it could work. I tried
using
Quoted 1 line
Quoted 1 line
i2c_smbus_xfer() directly, thinking maybe I could fool it, but that
doesn't
Quoted 1 line
Quoted 1 line
get me down to the low level of control I think I would need to
pull this off.
Quoted 2 lines
Quoted 2 lines
I see flags for i2c_msg that might be helpful...if they worked at
the
Quoted 6 lines
Quoted 6 lines
smbus level:
I2C_M_REV_DIR_ADDR reverses r/w bit
I2C_M_NOSTART strips off that beginning segment we don't want
If we could use a NOSTOP flag on the read byte command then i could go back and get the next byte. I don't see such a flag. I don't
see
Quoted 3 lines
Quoted 3 lines
any flags, other than for PEC, on smbus.
Also, saw an MDELAY flag that seemed interesting, as if I could
program
Quoted 1 line
Quoted 1 line
the delay between starting and reading the measurement, so it could
all
Quoted 3 lines
Quoted 3 lines
be done in one block data command. Again, not smbus.
I guess these ideas are all breaking the idea of being smbus
compliant
Quoted 7 lines
Quoted 4 lines
anyway.
Is this fixable with smbus?
If not, how do you 'graciously' change a drivers requirements?
I wouldn't worry about this too much. If the part is not SMBUS
compliant
Quoted 1 line
there is no need to try to access it only with SMBUS methods. A
system
Quoted 1 line
designer will most likely not connect the part to a SMBUS-only
controller.
Quoted 2 lines
What's actually quite important though at a software level is that
you do
Quoted 1 line
your write+read in a single I2C transaction and not split it over
multiple
Quoted 1 line
calls. Otherwise a device on the same I2C bus could be accessed in
between
Quoted 3 lines
and that would really break things.
If you want to address Jonathan's comments about backwards
compatibility,
Quoted 1 line
instead of returning with an error simply disable buffer mode if the
host

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
controller only has SMBUS capabilities.
Thank Lars.
I need to fix the existing direct mode reads first. That is the part that could break existing userspace.
If it is broken then fix it as you need to.
I hadn't registered existing use doesn't work!

Not to beat a dead horse, but I didn't know either. I had just chosen the i2c way for efficiency. It wasn't until you suggested I work up an smbus alternative that I found the bug.

Quoted 5 lines

Quoted 4 lines
I like what you say about a system designer not connecting the part to a
SMBUS-only controller, but I'm guessing this fix is going to break the user who got lucky with SMBUS-only.
How could they get lucky if it needs fixing?

Excuse my poor choice of words. Perhaps lucky in an ignorance-is-bliss sort of way. They are getting measurements and may be immune to the level of inaccuracy.

Working up a v2 of the patch w Peter's comments.

Quoted 12 lines

Quoted 9 lines
I'll work up a patch for fixing the existing reads (with i2c cmds) and get it out for comments.
alisons
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.