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