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
