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.
We need I2C_MST_ADDR to be programmed even in a single write message.
If we move it inside "if (Read)" branch then a single write message will not get it.
How does the below look?
for (i = 0; i < num; i++) {
write msg[i].addr to I2C_MST_ADDR;
if (Read) {
read (without stop);
} else {
start;
addr;
write;
}
}
stop;
Thanks
Ajay
--
nvpublic
--
> Cheers,
> Peter
>
> >
> > Thanks
> > Ajay
> >
> > --
> > nvpublic
> > --
> >>
> >> Cheers,
> >> Peter
