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;
Thanks
Ajay
--
nvpublic
--
>
> Cheers,
> Peter
