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

[Peter Rosin <peda@axentia.se>]

On 2018-09-10 23:53, Ajay Gupta wrote:
> 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))

What do you mean by "in little endian as per macro"? Should not the non-offset
0xf0 byte of CCGX_RAB_UCSI_DATA_BLOCK be in the other byte of rab compared to
e.g. the 0x06 byte of CCGX_RAB_INTR_REG?

I assumed *all* CCGX_RAB_... defines to be in cpu-native endian. Are they not?

In case they are, I think that no further conversion is needed. You feed rab
to ccg_read/ccg_write as cpu-native, and ccg_read/ccg_write then converts to
little-endian (with put_unaligned_le16).

And it's actually crucial that rab is cpu-native when ccg_read performs
arithmetic on it, otherwise the result can turn out to be garbage...

Cheers,
Peter