On 2018-09-08 21:17, Peter Rosin wrote:
>> +
>> + if (msgs[i].flags & I2C_M_RD) {
>> + /* gpu_i2c_read has implicit start and stop */
>> + status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>> + if (status < 0)
>> + return status;
>> + } else {
>> + /* start on first write message */
>> + if (i == 0) {
>
> This "if (i == 0)" test is completely bogus to me. I fail to see why the
> meat of the block should not happen for both writes in a double-write
> transfer.
>
> If the second message is a write, you do not issue any start nor do you
> write out the address for the second message. You want to generate the
> following for a transfer consisting of 2 write-messages:
>
> S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
> =============
>
> (where "..." denotes further optional "Data [A]" instances)
>
> As is, the stuff underlined by equal signs are not generated, at least as
> I read the code.
>
> This is what I meant in my comment around this area for the v9 patch.
Oh, I just realized, this probably means that the ccg_write function in
patch 2/2 asks for the wrong thing. If this code actually works, the client
driver should probably ask for a single-message transfer consisting of the
2-byte rab concatenated with the data buffer. And that actually makes sense,
there is no reason to split the two (dependent) parts of the write into
separate messages.
Otherwise, fixing the bug here will break your client driver, but two bugs
that just happen to cancel each other out are not what we want...
All assuming my hunch is correct, of course.
Cheers,
Peter
