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

[Ajay Gupta <ajayg@nvidia.com>]

Hi Peter,

> >>> +	memcpy(buf1, ((const void *)uc->ppm.data) + 0x20, sizeof(buf1));
> >>> +	memcpy(buf2, ((const void *)uc->ppm.data) + 0x8, sizeof(buf2));
> >>> +
> >>> +	status = ccg_write(uc, *(u16 *)buf, buf1, sizeof(buf1));
> >>
> >> This seems to be endian-dependent. May I suggest that you do as
> >> suggested above for ccg_read, and then somthing like
> >>
> >> #define CCGX_I2C_RAB_USCI_DATA_BLOCK(xxx) (0xf000 | ((xxx) &
> <mask>))
> >>
> >> where you of course use an appropriate value for <mask> (perhaps
> >> 0xff, or 0xfff, what do I know) and a better name for the field than
> >> xxx (perhaps len, what do I know), and then finally do
> >>
> >> 	status = ccg_write(uc, CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20), ...
> >>
> >> Also, the 0x20 and 0x8 are repeated and are some magic numbers that
> >> really should be given a name or some explanation. They appear to be
> >> data lengths, but again, what do I know?
> > I will check on this.
> 
> From the below reference, it's
> 
> 0x8 is USBC_CONTROL with USBC_CONTROL_SIZE 0x8 (64/8)
> 0x20 is USBC_MESSAGE_OUT with USBC_MESSAGE_OUT_SIZE 0x10 (128/8)
> 
> You could do
> #define USBC_MESSAGE_OUT CCGX_I2C_RAB_USCI_DATA_BLOCK(0x20)
> #define USBC_MESSAGE_OUT_SIZE (128/8)
> etc, so that it becomes
> 
> unsigned char buf1[USBC_MESSAGE_OUT_SIZE]; ...
> status = ccg_write(uc, USBC_MESSAGE_OUT, buf1, sizeof(buf1));
> 
> Which is a whole lot more readable IMHO.
Sure.
 
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	return ccg_write(uc, *(u16 *)(buf + 2), buf2, sizeof(buf2)); }
> >>> +
> >>> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
> >>> +	u8 *ppm = (u8 *)uc->ppm.data;
> >>> +	int status;
> >>> +	unsigned char buf[6] = {
> >>> +		0x0, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
> >>> +		0x4, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
> >>> +		0x10, CCGX_I2C_RAB_UCSI_DATA_BLOCK >> 8,
> >>> +	};
> >>> +
> >>> +	status = ccg_read(uc, *(u16 *)buf, ppm, 0x2);
> >>
> >> There are plenty magic numbers, but this call does not follow the pattern.
> >> Should perhaps buf[0] be 0x2, or should perhaps the last 0x2 argument
> >> be 0x0? All other ...DATA_BLOCK calls seem to have the len in the
> >> other byte of the rab argument. Why does this call not follow the pattern?
> > We are reading message IN data from Type-C controller in response to a
> > UCSI command. You can find details at
> >
> https://www.intel.com/content/dam/www/public/us/en/documents/technica
> l
> > -specifications/usb-type-c-ucsi-spec.pdf
> 
> So, according to table 3-1,
> 0x0 is UCSI_VERSION with UCSI_VERSION_SIZE 0x2 (16/8)
> 0x4 is USBC_CCI (connector change indication) with USBC_CCI_SIZE 0x4 (32/8)
> 0x10 is USBC_MESSAGE_IN with USBC_MESSAGE_IN_SIZE 0x10 (128/8)
> 
> The pattern for 0x4 and 0x10 was a accidental, but again, *please* use defines
> for all these magic numbers.
Will fix in next version.

Thanks
Ajay

--
nvpublic