On 2018-09-10 22:04, Ajay Gupta wrote:
> Hi Peter,
>
>>>>>>>>> + 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.
>>>>>> That would require to create new buffer and copy data for each
>>>>>> write request from UCSI core driver for sending UCSI command. This
>>>>>> doesn't look proper way of doing it.
>>>>>
>>>>> Well, that's the way master_xfer works, so you will just have to
>>>>> live with it if you do not want a stop in the middle.
>>>>
>>>> Bzzt, s/stop/restart/
>>> Obviously, a stop in middle will not work. For any read or write
>>> request,
>>
>> Yes, with "s/stop/restart/", I meant "replace stop with restart", and not "stop
>> followed by restart". That's pretty standard notation, but sorry if that was not
>> clear.
>>
>>> 2 byte rab write has to go first. We anyway have to separate rab part
>>> in read transactions so write should be okay to have separate rab
>>> write message.
>>
>> That is not at all certain. The two below transfers need not mean the same
>> thing at all.
>>
>> S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P S Addr Wr [A] rab1 [A] rab2 [A] S
>> Addr Wr [A] Data [A] ... P
>>
>> In fact, I strongly suspect that for the latter case, the device will interpret the
>> first two Data bytes as rab and that you therefore have to make sure that the
>> first transfer (w/o restart) is what you generate in ccg_write.
>>
>>> All the client driver of this needs to create messages in pairs where
>>> first one will always be a rab write and second one either read or write.
>>> I can add a check for this condition in master_xfer if needed.
>>>
>>> Do you still see any issue with this?
>>
>> Yes, this is very much an issue. master_xfer has to start each message with a
>> start for the first message, and a restart for subsequent messages. And after
>> the final message there should be a stop. All starts/restarts should be
>> followed by the address and the read/write bit. Then data in the given
>> direction. The master_xfer function should never do any device specific
>> checks for device specific conditions. It simply should not care about which
>> client driver is currently using it, and instead just stick to implementing the
>> master_xfer api so that all client drivers know what to expect. No cludges.
>
> I didn't get how read will happen. Read will also require a 2 byte rab write and
> followed by read.
>
> Are you saying to have two messages for reads [rab + read]
> and combined (rab and write) single message for write?
> Then for read: rab is written first followed by reads and then actual explicit stop?
>
> for (i = 0; i < num; i++)
> if (Read) {
> read (without stop);
> } else {
> start;
> addr;
> write;
> }
> }
> stop;
Yes, exactly like that (just missing a { for the for loop). If you can do reads
without an implicit stop, that's excellent news!
My guess is that with the above, you can actually program the I2C_MST_ADDR
register inside the "if (Read)" branch and then remove the
I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.
Cheers,
Peter
>
> Thanks
> Ajay
>
> --
> nvpublic
> --
>>
>> Cheers,
>> Peter
