smbus read help needed

smbus read help needed

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

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  

Re: smbus read help needed

[Lars-Peter Clausen]

On 08/03/2016 06:57 AM, Alison Schofield wrote:
> 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?


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.

Re: smbus read help needed

[Alison Schofield]

On Wed, Aug 03, 2016 at 02:13:17PM +0200, Lars-Peter Clausen wrote:
> On 08/03/2016 06:57 AM, Alison Schofield wrote:
> > 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?
> 
> 
> 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.
>

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

Re: smbus read help needed

[Alison Schofield]

On Fri, Aug 05, 2016 at 03:08:43PM +0530, Jonathan Cameron wrote:
> 
> 
> On 3 August 2016 20:42:30 GMT+05:30, Alison Schofield <amsfield22@gmail.com> wrote:
> >On Wed, Aug 03, 2016 at 02:13:17PM +0200, Lars-Peter Clausen wrote:
> >> On 08/03/2016 06:57 AM, Alison Schofield wrote:
> >> > 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?
> >> 
> >> 
> >> 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.
> >>
> >
> >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.

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

> >
> >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 majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.