On 2018-09-10 20:51, Ajay Gupta wrote:
>>>>> +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.
Cheers,
Peter
