Hi Peter,
> >>>>>>>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
> >>>>>>>> + unsigned char buf1[USBC_MSG_OUT_SIZE];
> >>>>>>>> + unsigned char buf2[USBC_CONTROL_SIZE];
> >>>>>>>> + int status;
> >>>>>>>> + u16 rab;
> >>>>>>>> +
> >>>>>>>> + memcpy(buf1, (u8 *)(uc->ppm.data) +
> >> USBC_MSG_OUT_OFFSET,
> >>>>>>> sizeof(buf1));
> >>>>>>>> + memcpy(buf2, (u8 *)(uc->ppm.data) +
> >> USBC_CONTROL_OFFSET,
> >>>>>>>> +sizeof(buf2));
> >>>>>>>
> >>>>>>> Hmm, now that I see what this function does, instead of just
> >>>>>>> seeing a bunch of magic numbers, I wonder why you make copies
> >>>>>>> instead of feeding the correct section of the ppm.data buffer
> >>>>>>> directly to ccg_write, like you do below for recv?
> >>>>>> Ok, will fix.
> >>>>>
> >>>>> Hmm, now that I see this again, it makes me wonder why you
> >>>>> complained about copying the buffer to fix the misunderstanding of
> >>>>> the i2c_transfer interface, when you already copy the buffer in
> >>>>> the first
> >> place?
> >>>> Copy is indeed not needed. I will fix it in next version.
> >>>> We will have to do copy in ccg_write()if we try to combine two
> >>>> write i2c_msg into one and I want to rather stay with two i2c_msg
> >>>> to avoid
> >> copy.
> >>>> Also master_xfer() will become tricky since rab write for read alsp
> >>>> has to go
> >>> first.
> >>>
> >>> You are stuck with the construction of the extended buffer. See my
> >>> mail in the
> >>> 1/2 thread.
> >>>
> >>>>>>>> + rab =
> >> CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
> >>>>>>>> + status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
> >>>>>>> USBC_VERSION_OFFSET,
> >>>>>>>> + USBC_VERSION_SIZE);
> >>>>>>>
> >>>>>>> E.g.
> >>>>>>> rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct
> ucsi_data,
> >>>>>>> version));
> >>>>>>> status = ccg_read(uc, rab, (u8 *)&uc->ppm.data->version,
> >>>>>>> sizeof(uc->ppm.data->version));
> >>>>>>>
> >>>>>>> Hmm, but this highlights that you are not doing any endian
> >>>>>>> conversion of the fields in that struct as you read/write it.
> >>>>>>
> >>>>>>> Do you need to in case you have an endian mismatch?
> >>>>>> Looks like don't need it. I have tested it and it works as is.
> >>>>>
> >>>>> Yeah, but have you tested the driver on a machine with the other
> >>>>> byte-
> >> sex?
> >>>> No, I think better to convert to desired endian.
> >>>
> >>> The device has a specific endianess. The host cpu has a specific
> >>> endianess.
> >>> You transfer data byte-by-byte to/from a struct that appears to have
> >>> multi- byte integers, e.g. the 16-bit version. You do not do any
> >>> conversion that I see and you report that it works. So, there are
> >>> two cases. Either
> >>>
> >>> 1. your host cpu and the device has the same endianess, and it all just
> >>> works by accident
> >>>
> >>> or
> >>>
> >>> 2. whatever is consuming the ppm data does the endian conversion for
> you
> >>> on "the other side", and it all just works by design.
> >>>
> >>> I have no idea which it is since I know nothing about whatever
> >>> handles the ppm data on the other side of that ucsi_register_ppm call. So,
> I asked.
> >> UCSI specification requires the ppm data to be in little-endian format.
> >>
> >> Below is from the UCSI specification.
> >> "All multiple byte fields in this specification are interpreted as
> >> and moved over the bus in little-endian order, i.e., LSB to MSB unless
> otherwise specified"
> >
> > Do we still need any conversion here? The ppm data is now directly fed
> > for read and write both and rab should be in little endian as per macro.
> > #define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
>
> What do you mean by "in little endian as per macro"?
> Should not the non-offset 0xf0 byte of CCGX_RAB_UCSI_DATA_BLOCK
> be in the other byte of rab
Shouldn't it be always in bits D[8:15] of rab so that it gets written to
buf[1] (2nd byte) in ccg_read/write() ?
> compared to e.g. the 0x06 byte of CCGX_RAB_INTR_REG?
>
> I assumed *all* CCGX_RAB_... defines to be in cpu-native endian.
> Are they not?
How to know/confirm this?
Thanks
Ajay
--nvpublic
> In case they are, I think that no further conversion is needed. You feed rab
> to
> ccg_read/ccg_write as cpu-native, and ccg_read/ccg_write then converts to
> little-endian (with put_unaligned_le16).
>
> And it's actually crucial that rab is cpu-native when ccg_read performs
> arithmetic on it, otherwise the result can turn out to be garbage...
>
> Cheers,
> Peter
