RE: [PATCH v10 2/2] usb: typec: ucsi: add support for Cypress CCGx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux