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