Re: i2c_burst_write API


Erwan Gouriou
 



On 3 April 2017 at 11:18, Piotr Mienkowski <piotr.mienkowski@...> wrote:
On 03.04.2017 10:23, Erwan Gouriou wrote:
On 2 April 2017 at 23:52, Piotr Mienkowski <piotr.mienkowski@...> wrote:
The proper way to write data to an i2c device with a one byte internal
address (on a bus level) is as follows:

S Addr Wr [A] subAddr [A] Data [A] Data [A] ... [A] Data [A] P
 
Where:

S     (1 bit) : Start bit
P     (1 bit) : Stop bit
Rd/Wr (1 bit) : Read/Write bit. Rd equals 1, Wr equals 0.
A (1 bit) : Acknowledge bit.
Addr  (7 bits): I2C 7 bit device address.
subAddr  (8 bits): Internal address
Data  (8 bits): A plain data byte.

Agreed.
 
Which is what your sensor is expecting. If i2c_burst_write as
implemented by i2c_stm32lx.c driver generates

S Addr Wr [A] subAddr [A] S Addr Wr [A] Data [A] Data [A] ... [A] Data
[A] P

that's wrong. I don't have an STM32 MCU device to try it out but if I
look at the driver's code it seems to confuse I2C_MSG_STOP and
I2C_MSG_RESTART flags. The I2C_MSG_STOP flag is missing completely from
the source code and the decision whether to generate the stop bit is
based on I2C_MSG_RESTART flag. That feels wrong and would result exactly
in the kind of behavior you described.

Problem is actually a bit different.
Generated message is as follows (2 separated messages actually)
S Addr Wr [A] subAddr [A] P
S Addr Wr [A] Data [A] Data [A] ... [A] Data [A] P
Thanks for the correction. Yes, this behavior makes more sense but it is still an invalid implementation on part of the i2c_stm32lx.c driver.

Invalid and not working, it has to be fixed. My issue  is which part
For instance, for burst_read operation, same 2 messages sequence is
generated (second message being a read instruction)
S Addr Wr [A] subAddr [A] P
S Addr Rr [A] Data [A] Data [A] ... [A] Data [A] P
This sequence is correctly treated y sensor, even if the use of RESTART had
been a nicer of doing it (and with better performance on a loaded I2C bus).
This would be also an invalid behavior and not following the specification. In case of read operation from an internal address the driver must use RESTART.

Agreed. Driver should be fixed to generated RESTART instead of STOP/START.
If we agree the brust_write should generated the correct message you described,
I'm not sure this is up to driver to modify the 2 messages so we get the correct
single message.
Yes, I believe we should fix the driver code. The main purpose of i2c_transfer() and all subsequent functions using it, like i2c_burst_write, is to allow generation of these combined messages. Otherwise the i2c_read, i2c_write functions would fully suffice.

Sorry if I missed an element, but I'm not sure to get your point here.
As you described, for burst write operation, only one message has to be sent,
S Addr Wr [A] subAddr [A] Data [A] Data [A] ... [A] Data [A] P
(unlike for burst read where 2 messages are expected).

If we keep the burst write API as is, we define an incorrect sequence of 2 messages:
    msg[0].buf = &start_addr;
    msg[0].len = 1;
    msg[0].flags = I2C_MSG_WRITE;

    msg[1].buf = buf;
    msg[1].len = num_bytes;
    msg[1].flags = I2C_MSG_WRITE | I2C_MSG_STOP;

and then expect the driver to generate a correct single message.
I mean this is doable, but adds unneeded complexity and would require to
explicit the contact in the API.

Erwan

--
Piotr

Join devel@lists.zephyrproject.org to automatically receive all group messages.