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

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

[Ajay Gupta]

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
Shouldn't it be always in bits D[8:15] of rab so that it gets written to 
buf[1] (2nd byte) in ccg_read/write() ?

> 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?
How to know/confirm this?

Thanks
Ajay

--nvpublic
> 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

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

[Ajay Gupta]

Hi Peter

> On Sep 10, 2018, at 11:29 PM, Peter Rosin <peda@axentia.se> wrote:
> 
>> On 2018-09-11 06:30, 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"
> 
> Taking another peek into the UCSI spec, and I do not find any mention of
> any rab. So, I think the rab is out of scope for that spec. I.e. that the
> rab falls into this bucket:
> 
>    This specification does not define the method to use
>    (PCIe/ACPI/I2C/etc.) in order to interface with the PPM.
>    It is left to individual system manufacturers to determine 
>    what bus/protocol they use to expose the PPM.
> 
> What abbreviation is rab anyway? Register Address Block?
Yes
> 
>>>> 
>>>> 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
>> Shouldn't it be always in bits D[8:15] of rab so that it gets written to 
>> buf[1] (2nd byte) in ccg_read/write() ?
>> 
>>> 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?
>> How to know/confirm this?
> 
> I don't know what you want to have on the wire, but for the code as
> written, a rab that is CCGX_RAB_UCSI_DATA_BLOCK(0x10) gets the bytes
> 0x10,0xf0 and for a rab that is CCGX_RAB_INTR_REG you get 0x06,0x00
I guess you meant
0x10 at lower byte D0:7 and
0xf0 at upper byte D8:15
Similarly,
0x06 at D0:7
0x00 at D8:15
I don’t see any issue here. Consider a 16 bit offset where CCGX_RAB_INTR_REG is at 0x0006 and other one at 0xf010
> 
> 
> What makes me a little worried is that the offset of the data-block
> rab appears where the significant bits of the other rabs are.
> However, that might not be a problem at all since the second byte
> of the data-block rab (0xf0) appears to be distinct from all other
> rabs. It just looks like the data-block rab might have been
> byte-swapped when comparing it to all the other rabs you have defined.
> 
> If you can run the code and see that it works, then obviously you
> are getting the byte-ordering of the rabs correct. No?
Yes, they work as is on multiple systems.

Thanks
Ajay

—nvpublic
> 
> Cheers,
> Peter

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

[Peter Rosin]

On 2018-09-11 06:30, 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"

Taking another peek into the UCSI spec, and I do not find any mention of
any rab. So, I think the rab is out of scope for that spec. I.e. that the
rab falls into this bucket:

	This specification does not define the method to use
	(PCIe/ACPI/I2C/etc.) in order to interface with the PPM.
	It is left to individual system manufacturers to determine 
	what bus/protocol they use to expose the PPM.

What abbreviation is rab anyway? Register Address Block?

>>>
>>> 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
> Shouldn't it be always in bits D[8:15] of rab so that it gets written to 
> buf[1] (2nd byte) in ccg_read/write() ?
> 
>> 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?
> How to know/confirm this?

I don't know what you want to have on the wire, but for the code as
written, a rab that is CCGX_RAB_UCSI_DATA_BLOCK(0x10) gets the bytes
0x10,0xf0 and for a rab that is CCGX_RAB_INTR_REG you get 0x06,0x00

What makes me a little worried is that the offset of the data-block
rab appears where the significant bits of the other rabs are.
However, that might not be a problem at all since the second byte
of the data-block rab (0xf0) appears to be distinct from all other
rabs. It just looks like the data-block rab might have been
byte-swapped when comparing it to all the other rabs you have defined.

If you can run the code and see that it works, then obviously you
are getting the byte-ordering of the rabs correct. No?

Cheers,
Peter

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

[Peter Rosin]

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

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

[Ajay Gupta]

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

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

[Ajay Gupta]

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

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

[Peter Rosin]

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

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

[Ajay Gupta]

Hi Peter,

> >>> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32
> >>> +len) {
> >>> +	struct i2c_client *client = uc->client;
> >>> +	unsigned char buf[2];
> >>> +	struct i2c_msg msgs[] = {
> >>> +		{
> >>> +			.addr	= client->addr,
> >>> +			.flags  = 0x0,
> >>> +			.len	= 0x2,
> >>
> >> sizeof(buf)?
> > ok
> >>
> >>> +			.buf	= buf,
> >>> +		},
> >>> +		{
> >>> +			.addr	= client->addr,
> >>> +			.flags  = I2C_M_RD,
> >>> +			.buf	= data,
> >>> +		},
> >>> +	};
> >>> +	u32 rlen, rem_len = len;
> >>> +	int status;
> >>> +
> >>
> >> If your target I2C adapter had supported larger reads, this would
> >> have been a single xfer instead of the loop, correct? I think this
> >> deserves a comment, and perhaps e.g. the eeprom drivers should be
> >> examined to see how they handle deficient I2C adapters (there is a
> >> module_param named io_limit in the at24 driver). Because it is a
> >> little bit sad to penalise all users just because you have an adapter with
> limitations.
> >> Or is this driver tied to that adapter?
> >> Anyway, I'm satisfied with a comment, as I don't care all that much.
> > I am thinking of moving the loop to the i2c driver.
> 
> No, that will not work. The i2c driver cannot know how the splitting works,
> since how to split transfers will typically be different for different drivers
> needing large reads.
> 
> You will have to do the split here in this driver.
Yes, I realized it later.
> 
> >>> +	while (rem_len > 0) {
> >>> +		msgs[1].buf = &data[len - rem_len];
> >>> +		rlen = min_t(u16, rem_len, 4);
> >>> +		msgs[1].len = rlen;
> >>> +		put_unaligned_le16(rab, buf);
> >>> +		status = i2c_transfer(client->adapter, msgs,
> >> ARRAY_SIZE(msgs));
> >>> +		if (status < 0) {
> >>> +			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> >>> +			return status;
> >>> +		}
> >>> +		rab += rlen;
> >>> +		rem_len -= rlen;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32
> >>> +len) {
> >>> +	struct i2c_client *client = uc->client;
> >>> +	unsigned char buf[2];
> >>> +	struct i2c_msg msgs[] = {
> >>> +		{
> >>> +			.addr	= client->addr,
> >>> +			.flags  = 0x0,
> >>> +			.len	= 0x2,
> >>
> >> sizeof(buf)?
> > ok
> >>
> >>> +			.buf	= buf,
> >>> +		},
> >>> +		{
> >>> +			.addr	= client->addr,
> >>> +			.flags  = 0x0,
> >>> +			.buf	= data,
> >>> +			.len	= len,
> >>> +		},
> >>> +	};
> >>> +	int status;
> >>> +
> >>> +	put_unaligned_le16(rab, buf);
> >>> +	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> >>> +	if (status < 0) {
> >>> +		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> >>> +		return status;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
> >>> +	struct device *dev = uc->dev;
> >>> +	unsigned int count = 10;
> >>> +	u8 data[64];
> >>> +	int status;
> >>> +
> >>> +	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
> >> sizeof(data));
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	dev_dbg(dev, "Silicon id %2ph", data +
> >> CCGX_I2C_RAB_READ_SILICON_ID);
> >>> +	dev_dbg(dev, "FW1 version %8ph\n", data +
> >> CCGX_I2C_RAB_FW1_VERSION);
> >>> +	dev_dbg(dev, "FW2 version %8ph\n", data +
> >> CCGX_I2C_RAB_FW2_VERSION);
> >>> +
> >>> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
> >>> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
> >>> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	/*
> >>> +	 * Flush CCGx RESPONSE queue by acking interrupts
> >>> +	 * - above ucsi control register write will push response
> >>> +	 * which must be flushed
> >>> +	 * - affects f/w update which reads response register
> >>> +	 */
> >>> +	data[0] = 0xff;
> >>> +	do {
> >>> +		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> >>> +		if (status < 0)
> >>> +			return status;
> >>> +
> >>> +		usleep_range(10000, 11000);
> >>> +
> >>> +		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> >>> +		if (status < 0)
> >>> +			return status;
> >>> +	} while ((data[0] != 0x00) && count--);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +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.
 
> >>> +
> >>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
> >>> +	status = ccg_write(uc, rab, buf1, sizeof(buf1));
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
> >>> +	return ccg_write(uc, rab, buf2, sizeof(buf2)); }
> >>> +
> >>> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
> >>> +	u8 *ppm = (u8 *)uc->ppm.data;
> >>> +	int status;
> >>> +	u16 rab;
> >>> +
> >>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
> >>> +	status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
> >>> +	return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET,
> >>> +USBC_MSG_IN_SIZE); }
> >>> +
> >>> +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
> >>> +	int status;
> >>> +	unsigned char buf[1] = {0x0};
> >>
> >> The initializer can be dropped.
> > ok
> >>
> >>> +
> >>> +	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
> >>
> >> sizeof(buf)?
> > ok
> >>
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
> >>
> >> sizeof(buf)?
> > ok
> >>
> >>> +}
> >>> +
> >>> +static int ucsi_ccg_sync(struct ucsi_ppm *ppm) {
> >>> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> >>> +	int status;
> >>> +
> >>> +	status = ucsi_ccg_recv_data(uc);
> >>> +	if (status < 0)
> >>> +		return status;
> >>> +
> >>> +	/* ack interrupt to allow next command to run */
> >>> +	return ucsi_ccg_ack_interrupt(uc); }
> >>> +
> >>> +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
> >>> +*ctrl) {
> >>> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> >>> +
> >>> +	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> >>> +	return ucsi_ccg_send_data(uc);
> >>> +}
> >>> +
> >>> +static irqreturn_t ccg_irq_handler(int irq, void *data) {
> >>> +	struct ucsi_ccg *uc = data;
> >>> +
> >>> +	ucsi_notify(uc->ucsi);
> >>> +
> >>> +	return IRQ_HANDLED;
> >>> +}
> >>> +
> >>> +static int ucsi_ccg_probe(struct i2c_client *client,
> >>> +			  const struct i2c_device_id *id) {
> >>> +	struct device *dev = &client->dev;
> >>> +	struct ucsi_ccg *uc;
> >>> +	int status;
> >>> +	u16 rab;
> >>> +
> >>> +	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
> >>> +	if (!uc)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),
> >> GFP_KERNEL);
> >>> +	if (!uc->ppm.data)
> >>> +		return -ENOMEM;
> >>
> >> Wait a minute, ppm.data is allocated as a struct? And it's __packed!
> >> So, it's clearly intended to match something real. I didn't notice
> >> that before, but that means that all the new offsets and sizes
> >> defined in v10 are available with
> >> offsetof() and sizeof() which would be much neater than a bunch of defines.
> >> Sorry for not catching this earlier!
> >>
> >> See below for an example.
> > Sure.
> >>
> >>> +
> >>> +	uc->ppm.cmd = ucsi_ccg_cmd;
> >>> +	uc->ppm.sync = ucsi_ccg_sync;
> >>> +	uc->dev = dev;
> >>> +	uc->client = client;
> >>> +
> >>> +	/* reset ccg device and initialize ucsi */
> >>> +	status = ucsi_ccg_init(uc);
> >>> +	if (status < 0) {
> >>> +		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
> >>> +		return status;
> >>> +	}
> >>> +
> >>> +	uc->irq = client->irq;
> >>> +
> >>> +	status = devm_request_threaded_irq(dev, uc->irq, NULL,
> >> ccg_irq_handler,
> >>> +					   IRQF_ONESHOT |
> >> IRQF_TRIGGER_HIGH,
> >>> +					   dev_name(dev), uc);
> >>> +	if (status < 0) {
> >>> +		dev_err(uc->dev, "request_threaded_irq failed - %d\n",
> >> status);
> >>> +		return status;
> >>> +	}
> >>> +
> >>> +	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
> >>> +	if (IS_ERR(uc->ucsi)) {
> >>> +		dev_err(uc->dev, "ucsi_register_ppm failed\n");
> >>> +		return PTR_ERR(uc->ucsi);
> >>> +	}
> >>> +
> >>> +	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.
 
Thanks
Ajay
--
nvpublic
--
> Cheers,
> Peter
> 
> > Thanks
> > Ajay
> >
> > --
> > nvpublic
> > --
> >>
> >> Cheers,
> >> Peter
> >>
> >>> +	if (status < 0) {
> >>> +		ucsi_unregister_ppm(uc->ucsi);
> >>> +		return status;
> >>> +	}
> >>> +
> >>> +	i2c_set_clientdata(client, uc);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int ucsi_ccg_remove(struct i2c_client *client) {
> >>> +	struct ucsi_ccg *uc = i2c_get_clientdata(client);
> >>> +
> >>> +	ucsi_unregister_ppm(uc->ucsi);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> >>> +	{"ccgx-ucsi", 0},
> >>> +	{}
> >>> +};
> >>> +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
> >>> +
> >>> +static struct i2c_driver ucsi_ccg_driver = {
> >>> +	.driver = {
> >>> +		.name = "ucsi_ccg",
> >>> +	},
> >>> +	.probe = ucsi_ccg_probe,
> >>> +	.remove = ucsi_ccg_remove,
> >>> +	.id_table = ucsi_ccg_device_id,
> >>> +};
> >>> +
> >>> +module_i2c_driver(ucsi_ccg_driver);
> >>> +
> >>> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> >>> +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C
> >>> +controller"); MODULE_LICENSE("GPL v2");
> >>>
> >

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

[Peter Rosin]

On 2018-09-10 19:32, Ajay Gupta wrote:
>>> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> +	struct i2c_client *client = uc->client;
>>> +	unsigned char buf[2];
>>> +	struct i2c_msg msgs[] = {
>>> +		{
>>> +			.addr	= client->addr,
>>> +			.flags  = 0x0,
>>> +			.len	= 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> +			.buf	= buf,
>>> +		},
>>> +		{
>>> +			.addr	= client->addr,
>>> +			.flags  = I2C_M_RD,
>>> +			.buf	= data,
>>> +		},
>>> +	};
>>> +	u32 rlen, rem_len = len;
>>> +	int status;
>>> +
>>
>> If your target I2C adapter had supported larger reads, this would have been a
>> single xfer instead of the loop, correct? I think this deserves a comment, and
>> perhaps e.g. the eeprom drivers should be examined to see how they handle
>> deficient I2C adapters (there is a module_param named io_limit in the at24
>> driver). Because it is a little bit sad to penalise all users just because you have
>> an adapter with limitations.
>> Or is this driver tied to that adapter?
>> Anyway, I'm satisfied with a comment, as I don't care all that much.
> I am thinking of moving the loop to the i2c driver. 

No, that will not work. The i2c driver cannot know how the splitting works,
since how to split transfers will typically be different for different drivers
needing large reads.

You will have to do the split here in this driver.

>>> +	while (rem_len > 0) {
>>> +		msgs[1].buf = &data[len - rem_len];
>>> +		rlen = min_t(u16, rem_len, 4);
>>> +		msgs[1].len = rlen;
>>> +		put_unaligned_le16(rab, buf);
>>> +		status = i2c_transfer(client->adapter, msgs,
>> ARRAY_SIZE(msgs));
>>> +		if (status < 0) {
>>> +			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> +			return status;
>>> +		}
>>> +		rab += rlen;
>>> +		rem_len -= rlen;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
>>> +{
>>> +	struct i2c_client *client = uc->client;
>>> +	unsigned char buf[2];
>>> +	struct i2c_msg msgs[] = {
>>> +		{
>>> +			.addr	= client->addr,
>>> +			.flags  = 0x0,
>>> +			.len	= 0x2,
>>
>> sizeof(buf)?
> ok
>>
>>> +			.buf	= buf,
>>> +		},
>>> +		{
>>> +			.addr	= client->addr,
>>> +			.flags  = 0x0,
>>> +			.buf	= data,
>>> +			.len	= len,
>>> +		},
>>> +	};
>>> +	int status;
>>> +
>>> +	put_unaligned_le16(rab, buf);
>>> +	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
>>> +	if (status < 0) {
>>> +		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
>>> +		return status;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
>>> +	struct device *dev = uc->dev;
>>> +	unsigned int count = 10;
>>> +	u8 data[64];
>>> +	int status;
>>> +
>>> +	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
>> sizeof(data));
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	dev_dbg(dev, "Silicon id %2ph", data +
>> CCGX_I2C_RAB_READ_SILICON_ID);
>>> +	dev_dbg(dev, "FW1 version %8ph\n", data +
>> CCGX_I2C_RAB_FW1_VERSION);
>>> +	dev_dbg(dev, "FW2 version %8ph\n", data +
>> CCGX_I2C_RAB_FW2_VERSION);
>>> +
>>> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
>>> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
>>> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	/*
>>> +	 * Flush CCGx RESPONSE queue by acking interrupts
>>> +	 * - above ucsi control register write will push response
>>> +	 * which must be flushed
>>> +	 * - affects f/w update which reads response register
>>> +	 */
>>> +	data[0] = 0xff;
>>> +	do {
>>> +		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> +		if (status < 0)
>>> +			return status;
>>> +
>>> +		usleep_range(10000, 11000);
>>> +
>>> +		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
>>> +		if (status < 0)
>>> +			return status;
>>> +	} while ((data[0] != 0x00) && count--);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +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?

>>> +
>>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
>>> +	status = ccg_write(uc, rab, buf1, sizeof(buf1));
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
>>> +	return ccg_write(uc, rab, buf2, sizeof(buf2)); }
>>> +
>>> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
>>> +	u8 *ppm = (u8 *)uc->ppm.data;
>>> +	int status;
>>> +	u16 rab;
>>> +
>>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
>>> +	status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
>>> +	return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET,
>>> +USBC_MSG_IN_SIZE); }
>>> +
>>> +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
>>> +	int status;
>>> +	unsigned char buf[1] = {0x0};
>>
>> The initializer can be dropped.
> ok
>>
>>> +
>>> +	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
>>
>> sizeof(buf)?
> ok
>>
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
>>
>> sizeof(buf)?
> ok
>>
>>> +}
>>> +
>>> +static int ucsi_ccg_sync(struct ucsi_ppm *ppm) {
>>> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
>>> +	int status;
>>> +
>>> +	status = ucsi_ccg_recv_data(uc);
>>> +	if (status < 0)
>>> +		return status;
>>> +
>>> +	/* ack interrupt to allow next command to run */
>>> +	return ucsi_ccg_ack_interrupt(uc);
>>> +}
>>> +
>>> +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
>>> +*ctrl) {
>>> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
>>> +
>>> +	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
>>> +	return ucsi_ccg_send_data(uc);
>>> +}
>>> +
>>> +static irqreturn_t ccg_irq_handler(int irq, void *data) {
>>> +	struct ucsi_ccg *uc = data;
>>> +
>>> +	ucsi_notify(uc->ucsi);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ucsi_ccg_probe(struct i2c_client *client,
>>> +			  const struct i2c_device_id *id)
>>> +{
>>> +	struct device *dev = &client->dev;
>>> +	struct ucsi_ccg *uc;
>>> +	int status;
>>> +	u16 rab;
>>> +
>>> +	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
>>> +	if (!uc)
>>> +		return -ENOMEM;
>>> +
>>> +	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),
>> GFP_KERNEL);
>>> +	if (!uc->ppm.data)
>>> +		return -ENOMEM;
>>
>> Wait a minute, ppm.data is allocated as a struct? And it's __packed! So, it's
>> clearly intended to match something real. I didn't notice that before, but that
>> means that all the new offsets and sizes defined in v10 are available with
>> offsetof() and sizeof() which would be much neater than a bunch of defines.
>> Sorry for not catching this earlier!
>>
>> See below for an example.
> Sure.
>>
>>> +
>>> +	uc->ppm.cmd = ucsi_ccg_cmd;
>>> +	uc->ppm.sync = ucsi_ccg_sync;
>>> +	uc->dev = dev;
>>> +	uc->client = client;
>>> +
>>> +	/* reset ccg device and initialize ucsi */
>>> +	status = ucsi_ccg_init(uc);
>>> +	if (status < 0) {
>>> +		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
>>> +		return status;
>>> +	}
>>> +
>>> +	uc->irq = client->irq;
>>> +
>>> +	status = devm_request_threaded_irq(dev, uc->irq, NULL,
>> ccg_irq_handler,
>>> +					   IRQF_ONESHOT |
>> IRQF_TRIGGER_HIGH,
>>> +					   dev_name(dev), uc);
>>> +	if (status < 0) {
>>> +		dev_err(uc->dev, "request_threaded_irq failed - %d\n",
>> status);
>>> +		return status;
>>> +	}
>>> +
>>> +	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
>>> +	if (IS_ERR(uc->ucsi)) {
>>> +		dev_err(uc->dev, "ucsi_register_ppm failed\n");
>>> +		return PTR_ERR(uc->ucsi);
>>> +	}
>>> +
>>> +	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?

Cheers,
Peter

> Thanks
> Ajay
> 
> --
> nvpublic
> --
>>
>> Cheers,
>> Peter
>>
>>> +	if (status < 0) {
>>> +		ucsi_unregister_ppm(uc->ucsi);
>>> +		return status;
>>> +	}
>>> +
>>> +	i2c_set_clientdata(client, uc);
>>> +	return 0;
>>> +}
>>> +
>>> +static int ucsi_ccg_remove(struct i2c_client *client) {
>>> +	struct ucsi_ccg *uc = i2c_get_clientdata(client);
>>> +
>>> +	ucsi_unregister_ppm(uc->ucsi);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id ucsi_ccg_device_id[] = {
>>> +	{"ccgx-ucsi", 0},
>>> +	{}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
>>> +
>>> +static struct i2c_driver ucsi_ccg_driver = {
>>> +	.driver = {
>>> +		.name = "ucsi_ccg",
>>> +	},
>>> +	.probe = ucsi_ccg_probe,
>>> +	.remove = ucsi_ccg_remove,
>>> +	.id_table = ucsi_ccg_device_id,
>>> +};
>>> +
>>> +module_i2c_driver(ucsi_ccg_driver);
>>> +
>>> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
>>> +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
>>> +MODULE_LICENSE("GPL v2");
>>>
>

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

[Ajay Gupta]

Hi Peter,

> > Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller over I2C
> > interface.
> >
> > This UCSI I2C driver uses I2C bus driver interface for communicating
> > with Type-C controller.
> >
> > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> > Changes from v1 -> v2
> > 	Fixed identation in drivers/usb/typec/ucsi/Kconfig Changes from v2 ->
> > v3
> > 	Fixed most of comments from Heikki
> > 	Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> > Changes from v3 -> v4
> > 	Fixed comments from Andy
> > Changes from v4 -> v5
> > 	Fixed comments from Andy
> > Changes from v5 -> v6
> > 	Fixed review comments from Greg
> > Changes from v6 -> v7
> > 	None
> > Changes from v7 -> v8
> > 	Fixed review comments from Peter
> > 	- Removed empty STOP message
> > 	- Using stack memory for i2c_transfer() Changes from v8 -> v9
> > 	None
> > Changes from v9 -> v10
> > 	Fixed review comments from Peter
> > 	- Use UCSI macros
> > 	- Cleanups
> >
> >  drivers/usb/typec/ucsi/Kconfig    |  10 ++
> >  drivers/usb/typec/ucsi/Makefile   |   2 +
> >  drivers/usb/typec/ucsi/ucsi_ccg.c | 324
> > ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 336 insertions(+)
> >  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> >
> > diff --git a/drivers/usb/typec/ucsi/Kconfig
> > b/drivers/usb/typec/ucsi/Kconfig index e36d6c7..7811888 100644
> > --- a/drivers/usb/typec/ucsi/Kconfig
> > +++ b/drivers/usb/typec/ucsi/Kconfig
> > @@ -23,6 +23,16 @@ config TYPEC_UCSI
> >
> >  if TYPEC_UCSI
> >
> > +config UCSI_CCG
> > +	tristate "UCSI Interface Driver for Cypress CCGx"
> > +	depends on I2C
> > +	help
> > +	  This driver enables UCSI support on platforms that expose a
> > +	  Cypress CCGx Type-C controller over I2C interface.
> > +
> > +	  To compile the driver as a module, choose M here: the module will
> be
> > +	  called ucsi_ccg.
> > +
> >  config UCSI_ACPI
> >  	tristate "UCSI ACPI Interface Driver"
> >  	depends on ACPI
> > diff --git a/drivers/usb/typec/ucsi/Makefile
> > b/drivers/usb/typec/ucsi/Makefile index 7afbea5..2f4900b 100644
> > --- a/drivers/usb/typec/ucsi/Makefile
> > +++ b/drivers/usb/typec/ucsi/Makefile
> > @@ -8,3 +8,5 @@ typec_ucsi-y			:= ucsi.o
> >  typec_ucsi-$(CONFIG_TRACING)	+= trace.o
> >
> >  obj-$(CONFIG_UCSI_ACPI)		+= ucsi_acpi.o
> > +
> > +obj-$(CONFIG_UCSI_CCG)		+= ucsi_ccg.o
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > new file mode 100644
> > index 0000000..c346e6a
> > --- /dev/null
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -0,0 +1,324 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * UCSI driver for Cypress CCGx Type-C controller
> > + *
> > + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> > + * Author: Ajay Gupta <ajayg@nvidia.com>
> > + *
> > + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> > + */
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <asm/unaligned.h>
> > +#include "ucsi.h"
> > +
> > +struct ucsi_ccg {
> > +	struct device *dev;
> > +	struct ucsi *ucsi;
> > +	struct ucsi_ppm ppm;
> > +	struct i2c_client *client;
> > +	int irq;
> > +};
> > +
> > +#define CCGX_I2C_RAB_DEVICE_MODE			0x00
> > +#define CCGX_I2C_RAB_READ_SILICON_ID			0x2
> > +#define CCGX_I2C_RAB_INTR_REG				0x06
> > +#define CCGX_I2C_RAB_FW1_VERSION			0x28
> > +#define CCGX_I2C_RAB_FW2_VERSION			0x20
> > +#define CCGX_I2C_RAB_UCSI_CONTROL			0x39
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_START			BIT(0)
> > +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP			BIT(1)
> > +#define CCGX_I2C_RAB_RESPONSE_REG			0x7E
> > +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) &
> 0xff))
> > +
> > +#define USBC_VERSION_OFFSET	(0x0)
> > +#define USBC_VERSION_SIZE	(2)
> > +#define USBC_CCI_OFFSET		(0x4)
> > +#define USBC_CCI_SIZE		(4)
> > +#define USBC_CONTROL_OFFSET	(0x8)
> > +#define USBC_CONTROL_SIZE	(8)
> > +#define USBC_MSG_IN_OFFSET	(0x10)
> > +#define USBC_MSG_IN_SIZE	(16)
> > +#define USBC_MSG_OUT_OFFSET	(0x20)
> > +#define USBC_MSG_OUT_SIZE	(16)
> > +
> > +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> > +{
> > +	struct i2c_client *client = uc->client;
> > +	unsigned char buf[2];
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr	= client->addr,
> > +			.flags  = 0x0,
> > +			.len	= 0x2,
> 
> sizeof(buf)?
ok
> 
> > +			.buf	= buf,
> > +		},
> > +		{
> > +			.addr	= client->addr,
> > +			.flags  = I2C_M_RD,
> > +			.buf	= data,
> > +		},
> > +	};
> > +	u32 rlen, rem_len = len;
> > +	int status;
> > +
> 
> If your target I2C adapter had supported larger reads, this would have been a
> single xfer instead of the loop, correct? I think this deserves a comment, and
> perhaps e.g. the eeprom drivers should be examined to see how they handle
> deficient I2C adapters (there is a module_param named io_limit in the at24
> driver). Because it is a little bit sad to penalise all users just because you have
> an adapter with limitations.
> Or is this driver tied to that adapter?
> Anyway, I'm satisfied with a comment, as I don't care all that much.
I am thinking of moving the loop to the i2c driver. 
 
> > +	while (rem_len > 0) {
> > +		msgs[1].buf = &data[len - rem_len];
> > +		rlen = min_t(u16, rem_len, 4);
> > +		msgs[1].len = rlen;
> > +		put_unaligned_le16(rab, buf);
> > +		status = i2c_transfer(client->adapter, msgs,
> ARRAY_SIZE(msgs));
> > +		if (status < 0) {
> > +			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > +			return status;
> > +		}
> > +		rab += rlen;
> > +		rem_len -= rlen;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> > +{
> > +	struct i2c_client *client = uc->client;
> > +	unsigned char buf[2];
> > +	struct i2c_msg msgs[] = {
> > +		{
> > +			.addr	= client->addr,
> > +			.flags  = 0x0,
> > +			.len	= 0x2,
> 
> sizeof(buf)?
ok
> 
> > +			.buf	= buf,
> > +		},
> > +		{
> > +			.addr	= client->addr,
> > +			.flags  = 0x0,
> > +			.buf	= data,
> > +			.len	= len,
> > +		},
> > +	};
> > +	int status;
> > +
> > +	put_unaligned_le16(rab, buf);
> > +	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (status < 0) {
> > +		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> > +		return status;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ucsi_ccg_init(struct ucsi_ccg *uc) {
> > +	struct device *dev = uc->dev;
> > +	unsigned int count = 10;
> > +	u8 data[64];
> > +	int status;
> > +
> > +	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data,
> sizeof(data));
> > +	if (status < 0)
> > +		return status;
> > +
> > +	dev_dbg(dev, "Silicon id %2ph", data +
> CCGX_I2C_RAB_READ_SILICON_ID);
> > +	dev_dbg(dev, "FW1 version %8ph\n", data +
> CCGX_I2C_RAB_FW1_VERSION);
> > +	dev_dbg(dev, "FW2 version %8ph\n", data +
> CCGX_I2C_RAB_FW2_VERSION);
> > +
> > +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
> > +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
> > +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	/*
> > +	 * Flush CCGx RESPONSE queue by acking interrupts
> > +	 * - above ucsi control register write will push response
> > +	 * which must be flushed
> > +	 * - affects f/w update which reads response register
> > +	 */
> > +	data[0] = 0xff;
> > +	do {
> > +		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> > +		if (status < 0)
> > +			return status;
> > +
> > +		usleep_range(10000, 11000);
> > +
> > +		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> > +		if (status < 0)
> > +			return status;
> > +	} while ((data[0] != 0x00) && count--);
> > +
> > +	return 0;
> > +}
> > +
> > +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.
> 
> > +
> > +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
> > +	status = ccg_write(uc, rab, buf1, sizeof(buf1));
> > +	if (status < 0)
> > +		return status;
> > +
> > +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
> > +	return ccg_write(uc, rab, buf2, sizeof(buf2)); }
> > +
> > +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc) {
> > +	u8 *ppm = (u8 *)uc->ppm.data;
> > +	int status;
> > +	u16 rab;
> > +
> > +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
> > +	status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
> > +	return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET,
> > +USBC_MSG_IN_SIZE); }
> > +
> > +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc) {
> > +	int status;
> > +	unsigned char buf[1] = {0x0};
> 
> The initializer can be dropped.
ok
> 
> > +
> > +	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
> 
> sizeof(buf)?
ok
> 
> > +	if (status < 0)
> > +		return status;
> > +
> > +	return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
> 
> sizeof(buf)?
ok
> 
> > +}
> > +
> > +static int ucsi_ccg_sync(struct ucsi_ppm *ppm) {
> > +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> > +	int status;
> > +
> > +	status = ucsi_ccg_recv_data(uc);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	/* ack interrupt to allow next command to run */
> > +	return ucsi_ccg_ack_interrupt(uc);
> > +}
> > +
> > +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
> > +*ctrl) {
> > +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> > +
> > +	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> > +	return ucsi_ccg_send_data(uc);
> > +}
> > +
> > +static irqreturn_t ccg_irq_handler(int irq, void *data) {
> > +	struct ucsi_ccg *uc = data;
> > +
> > +	ucsi_notify(uc->ucsi);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int ucsi_ccg_probe(struct i2c_client *client,
> > +			  const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct ucsi_ccg *uc;
> > +	int status;
> > +	u16 rab;
> > +
> > +	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
> > +	if (!uc)
> > +		return -ENOMEM;
> > +
> > +	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data),
> GFP_KERNEL);
> > +	if (!uc->ppm.data)
> > +		return -ENOMEM;
> 
> Wait a minute, ppm.data is allocated as a struct? And it's __packed! So, it's
> clearly intended to match something real. I didn't notice that before, but that
> means that all the new offsets and sizes defined in v10 are available with
> offsetof() and sizeof() which would be much neater than a bunch of defines.
> Sorry for not catching this earlier!
> 
> See below for an example.
Sure.
> 
> > +
> > +	uc->ppm.cmd = ucsi_ccg_cmd;
> > +	uc->ppm.sync = ucsi_ccg_sync;
> > +	uc->dev = dev;
> > +	uc->client = client;
> > +
> > +	/* reset ccg device and initialize ucsi */
> > +	status = ucsi_ccg_init(uc);
> > +	if (status < 0) {
> > +		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
> > +		return status;
> > +	}
> > +
> > +	uc->irq = client->irq;
> > +
> > +	status = devm_request_threaded_irq(dev, uc->irq, NULL,
> ccg_irq_handler,
> > +					   IRQF_ONESHOT |
> IRQF_TRIGGER_HIGH,
> > +					   dev_name(dev), uc);
> > +	if (status < 0) {
> > +		dev_err(uc->dev, "request_threaded_irq failed - %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
> > +	if (IS_ERR(uc->ucsi)) {
> > +		dev_err(uc->dev, "ucsi_register_ppm failed\n");
> > +		return PTR_ERR(uc->ucsi);
> > +	}
> > +
> > +	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.

Thanks
Ajay

--
nvpublic
--
> 
> Cheers,
> Peter
> 
> > +	if (status < 0) {
> > +		ucsi_unregister_ppm(uc->ucsi);
> > +		return status;
> > +	}
> > +
> > +	i2c_set_clientdata(client, uc);
> > +	return 0;
> > +}
> > +
> > +static int ucsi_ccg_remove(struct i2c_client *client) {
> > +	struct ucsi_ccg *uc = i2c_get_clientdata(client);
> > +
> > +	ucsi_unregister_ppm(uc->ucsi);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> > +	{"ccgx-ucsi", 0},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
> > +
> > +static struct i2c_driver ucsi_ccg_driver = {
> > +	.driver = {
> > +		.name = "ucsi_ccg",
> > +	},
> > +	.probe = ucsi_ccg_probe,
> > +	.remove = ucsi_ccg_remove,
> > +	.id_table = ucsi_ccg_device_id,
> > +};
> > +
> > +module_i2c_driver(ucsi_ccg_driver);
> > +
> > +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> > +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
> > +MODULE_LICENSE("GPL v2");
> >

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

[Peter Rosin]

On 2018-09-08 02:09, Ajay Gupta wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
> 
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes from v1 -> v2
> 	Fixed identation in drivers/usb/typec/ucsi/Kconfig
> Changes from v2 -> v3
> 	Fixed most of comments from Heikki
> 	Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
> Changes from v3 -> v4
> 	Fixed comments from Andy
> Changes from v4 -> v5
> 	Fixed comments from Andy
> Changes from v5 -> v6
> 	Fixed review comments from Greg 
> Changes from v6 -> v7
> 	None
> Changes from v7 -> v8
> 	Fixed review comments from Peter 
> 	- Removed empty STOP message
> 	- Using stack memory for i2c_transfer()
> Changes from v8 -> v9
> 	None
> Changes from v9 -> v10
> 	Fixed review comments from Peter 
> 	- Use UCSI macros
> 	- Cleanups
> 
>  drivers/usb/typec/ucsi/Kconfig    |  10 ++
>  drivers/usb/typec/ucsi/Makefile   |   2 +
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 324 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
> 
> diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
> index e36d6c7..7811888 100644
> --- a/drivers/usb/typec/ucsi/Kconfig
> +++ b/drivers/usb/typec/ucsi/Kconfig
> @@ -23,6 +23,16 @@ config TYPEC_UCSI
>  
>  if TYPEC_UCSI
>  
> +config UCSI_CCG
> +	tristate "UCSI Interface Driver for Cypress CCGx"
> +	depends on I2C
> +	help
> +	  This driver enables UCSI support on platforms that expose a
> +	  Cypress CCGx Type-C controller over I2C interface.
> +
> +	  To compile the driver as a module, choose M here: the module will be
> +	  called ucsi_ccg.
> +
>  config UCSI_ACPI
>  	tristate "UCSI ACPI Interface Driver"
>  	depends on ACPI
> diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
> index 7afbea5..2f4900b 100644
> --- a/drivers/usb/typec/ucsi/Makefile
> +++ b/drivers/usb/typec/ucsi/Makefile
> @@ -8,3 +8,5 @@ typec_ucsi-y			:= ucsi.o
>  typec_ucsi-$(CONFIG_TRACING)	+= trace.o
>  
>  obj-$(CONFIG_UCSI_ACPI)		+= ucsi_acpi.o
> +
> +obj-$(CONFIG_UCSI_CCG)		+= ucsi_ccg.o
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> new file mode 100644
> index 0000000..c346e6a
> --- /dev/null
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * UCSI driver for Cypress CCGx Type-C controller
> + *
> + * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta <ajayg@nvidia.com>
> + *
> + * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
> + */
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/unaligned.h>
> +#include "ucsi.h"
> +
> +struct ucsi_ccg {
> +	struct device *dev;
> +	struct ucsi *ucsi;
> +	struct ucsi_ppm ppm;
> +	struct i2c_client *client;
> +	int irq;
> +};
> +
> +#define CCGX_I2C_RAB_DEVICE_MODE			0x00
> +#define CCGX_I2C_RAB_READ_SILICON_ID			0x2
> +#define CCGX_I2C_RAB_INTR_REG				0x06
> +#define CCGX_I2C_RAB_FW1_VERSION			0x28
> +#define CCGX_I2C_RAB_FW2_VERSION			0x20
> +#define CCGX_I2C_RAB_UCSI_CONTROL			0x39
> +#define CCGX_I2C_RAB_UCSI_CONTROL_START			BIT(0)
> +#define CCGX_I2C_RAB_UCSI_CONTROL_STOP			BIT(1)
> +#define CCGX_I2C_RAB_RESPONSE_REG			0x7E
> +#define CCGX_I2C_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) & 0xff))
> +
> +#define USBC_VERSION_OFFSET	(0x0)
> +#define USBC_VERSION_SIZE	(2)
> +#define USBC_CCI_OFFSET		(0x4)
> +#define USBC_CCI_SIZE		(4)
> +#define USBC_CONTROL_OFFSET	(0x8)
> +#define USBC_CONTROL_SIZE	(8)
> +#define USBC_MSG_IN_OFFSET	(0x10)
> +#define USBC_MSG_IN_SIZE	(16)
> +#define USBC_MSG_OUT_OFFSET	(0x20)
> +#define USBC_MSG_OUT_SIZE	(16)
> +
> +static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> +	struct i2c_client *client = uc->client;
> +	unsigned char buf[2];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.len	= 0x2,

sizeof(buf)?

> +			.buf	= buf,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags  = I2C_M_RD,
> +			.buf	= data,
> +		},
> +	};
> +	u32 rlen, rem_len = len;
> +	int status;
> +

If your target I2C adapter had supported larger reads, this would have
been a single xfer instead of the loop, correct? I think this deserves
a comment, and perhaps e.g. the eeprom drivers should be examined to
see how they handle deficient I2C adapters (there is a module_param
named io_limit in the at24 driver). Because it is a little bit sad to
penalise all users just because you have an adapter with limitations.
Or is this driver tied to that adapter?

Anyway, I'm satisfied with a comment, as I don't care all that much.

> +	while (rem_len > 0) {
> +		msgs[1].buf = &data[len - rem_len];
> +		rlen = min_t(u16, rem_len, 4);
> +		msgs[1].len = rlen;
> +		put_unaligned_le16(rab, buf);
> +		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +		if (status < 0) {
> +			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> +			return status;
> +		}
> +		rab += rlen;
> +		rem_len -= rlen;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
> +{
> +	struct i2c_client *client = uc->client;
> +	unsigned char buf[2];
> +	struct i2c_msg msgs[] = {
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.len	= 0x2,

sizeof(buf)?

> +			.buf	= buf,
> +		},
> +		{
> +			.addr	= client->addr,
> +			.flags  = 0x0,
> +			.buf	= data,
> +			.len	= len,
> +		},
> +	};
> +	int status;
> +
> +	put_unaligned_le16(rab, buf);
> +	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (status < 0) {
> +		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ucsi_ccg_init(struct ucsi_ccg *uc)
> +{
> +	struct device *dev = uc->dev;
> +	unsigned int count = 10;
> +	u8 data[64];
> +	int status;
> +
> +	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data));
> +	if (status < 0)
> +		return status;
> +
> +	dev_dbg(dev, "Silicon id %2ph", data + CCGX_I2C_RAB_READ_SILICON_ID);
> +	dev_dbg(dev, "FW1 version %8ph\n", data + CCGX_I2C_RAB_FW1_VERSION);
> +	dev_dbg(dev, "FW2 version %8ph\n", data + CCGX_I2C_RAB_FW2_VERSION);
> +
> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> +	if (status < 0)
> +		return status;
> +
> +	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
> +	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
> +	if (status < 0)
> +		return status;
> +
> +	/*
> +	 * Flush CCGx RESPONSE queue by acking interrupts
> +	 * - above ucsi control register write will push response
> +	 * which must be flushed
> +	 * - affects f/w update which reads response register
> +	 */
> +	data[0] = 0xff;
> +	do {
> +		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> +		if (status < 0)
> +			return status;
> +
> +		usleep_range(10000, 11000);
> +
> +		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
> +		if (status < 0)
> +			return status;
> +	} while ((data[0] != 0x00) && count--);
> +
> +	return 0;
> +}
> +
> +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?

> +
> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
> +	status = ccg_write(uc, rab, buf1, sizeof(buf1));
> +	if (status < 0)
> +		return status;
> +
> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
> +	return ccg_write(uc, rab, buf2, sizeof(buf2));
> +}
> +
> +static int ucsi_ccg_recv_data(struct ucsi_ccg *uc)
> +{
> +	u8 *ppm = (u8 *)uc->ppm.data;
> +	int status;
> +	u16 rab;
> +
> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
> +	status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
> +	if (status < 0)
> +		return status;
> +
> +	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
> +	return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET, USBC_MSG_IN_SIZE);
> +}
> +
> +static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc)
> +{
> +	int status;
> +	unsigned char buf[1] = {0x0};

The initializer can be dropped.

> +
> +	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);

sizeof(buf)?

> +	if (status < 0)
> +		return status;
> +
> +	return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);

sizeof(buf)?

> +}
> +
> +static int ucsi_ccg_sync(struct ucsi_ppm *ppm)
> +{
> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> +	int status;
> +
> +	status = ucsi_ccg_recv_data(uc);
> +	if (status < 0)
> +		return status;
> +
> +	/* ack interrupt to allow next command to run */
> +	return ucsi_ccg_ack_interrupt(uc);
> +}
> +
> +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
> +{
> +	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> +
> +	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> +	return ucsi_ccg_send_data(uc);
> +}
> +
> +static irqreturn_t ccg_irq_handler(int irq, void *data)
> +{
> +	struct ucsi_ccg *uc = data;
> +
> +	ucsi_notify(uc->ucsi);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ucsi_ccg_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct ucsi_ccg *uc;
> +	int status;
> +	u16 rab;
> +
> +	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
> +	if (!uc)
> +		return -ENOMEM;
> +
> +	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data), GFP_KERNEL);
> +	if (!uc->ppm.data)
> +		return -ENOMEM;

Wait a minute, ppm.data is allocated as a struct? And it's __packed! So, it's
clearly intended to match something real. I didn't notice that before,
but that means that all the new offsets and sizes defined in v10 are available
with offsetof() and sizeof() which would be much neater than a bunch of
defines. Sorry for not catching this earlier!

See below for an example.

> +
> +	uc->ppm.cmd = ucsi_ccg_cmd;
> +	uc->ppm.sync = ucsi_ccg_sync;
> +	uc->dev = dev;
> +	uc->client = client;
> +
> +	/* reset ccg device and initialize ucsi */
> +	status = ucsi_ccg_init(uc);
> +	if (status < 0) {
> +		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
> +		return status;
> +	}
> +
> +	uc->irq = client->irq;
> +
> +	status = devm_request_threaded_irq(dev, uc->irq, NULL, ccg_irq_handler,
> +					   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +					   dev_name(dev), uc);
> +	if (status < 0) {
> +		dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
> +		return status;
> +	}
> +
> +	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
> +	if (IS_ERR(uc->ucsi)) {
> +		dev_err(uc->dev, "ucsi_register_ppm failed\n");
> +		return PTR_ERR(uc->ucsi);
> +	}
> +
> +	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?

Cheers,
Peter

> +	if (status < 0) {
> +		ucsi_unregister_ppm(uc->ucsi);
> +		return status;
> +	}
> +
> +	i2c_set_clientdata(client, uc);
> +	return 0;
> +}
> +
> +static int ucsi_ccg_remove(struct i2c_client *client)
> +{
> +	struct ucsi_ccg *uc = i2c_get_clientdata(client);
> +
> +	ucsi_unregister_ppm(uc->ucsi);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> +	{"ccgx-ucsi", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
> +
> +static struct i2c_driver ucsi_ccg_driver = {
> +	.driver = {
> +		.name = "ucsi_ccg",
> +	},
> +	.probe = ucsi_ccg_probe,
> +	.remove = ucsi_ccg_remove,
> +	.id_table = ucsi_ccg_device_id,
> +};
> +
> +module_i2c_driver(ucsi_ccg_driver);
> +
> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
> +MODULE_LICENSE("GPL v2");
>

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

[Ajay Gupta]

Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
over I2C interface.

This UCSI I2C driver uses I2C bus driver interface for communicating
with Type-C controller.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
Changes from v1 -> v2
	Fixed identation in drivers/usb/typec/ucsi/Kconfig
Changes from v2 -> v3
	Fixed most of comments from Heikki
	Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
Changes from v3 -> v4
	Fixed comments from Andy
Changes from v4 -> v5
	Fixed comments from Andy
Changes from v5 -> v6
	Fixed review comments from Greg 
Changes from v6 -> v7
	None
Changes from v7 -> v8
	Fixed review comments from Peter 
	- Removed empty STOP message
	- Using stack memory for i2c_transfer()
Changes from v8 -> v9
	None
Changes from v9 -> v10
	Fixed review comments from Peter 
	- Use UCSI macros
	- Cleanups

 drivers/usb/typec/ucsi/Kconfig    |  10 ++
 drivers/usb/typec/ucsi/Makefile   |   2 +
 drivers/usb/typec/ucsi/ucsi_ccg.c | 324 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c

diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index e36d6c7..7811888 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -23,6 +23,16 @@ config TYPEC_UCSI
 
 if TYPEC_UCSI
 
+config UCSI_CCG
+	tristate "UCSI Interface Driver for Cypress CCGx"
+	depends on I2C
+	help
+	  This driver enables UCSI support on platforms that expose a
+	  Cypress CCGx Type-C controller over I2C interface.
+
+	  To compile the driver as a module, choose M here: the module will be
+	  called ucsi_ccg.
+
 config UCSI_ACPI
 	tristate "UCSI ACPI Interface Driver"
 	depends on ACPI
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 7afbea5..2f4900b 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -8,3 +8,5 @@ typec_ucsi-y			:= ucsi.o
 typec_ucsi-$(CONFIG_TRACING)	+= trace.o
 
 obj-$(CONFIG_UCSI_ACPI)		+= ucsi_acpi.o
+
+obj-$(CONFIG_UCSI_CCG)		+= ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
new file mode 100644
index 0000000..c346e6a
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for Cypress CCGx Type-C controller
+ *
+ * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta <ajayg@nvidia.com>
+ *
+ * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
+ */
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include <asm/unaligned.h>
+#include "ucsi.h"
+
+struct ucsi_ccg {
+	struct device *dev;
+	struct ucsi *ucsi;
+	struct ucsi_ppm ppm;
+	struct i2c_client *client;
+	int irq;
+};
+
+#define CCGX_I2C_RAB_DEVICE_MODE			0x00
+#define CCGX_I2C_RAB_READ_SILICON_ID			0x2
+#define CCGX_I2C_RAB_INTR_REG				0x06
+#define CCGX_I2C_RAB_FW1_VERSION			0x28
+#define CCGX_I2C_RAB_FW2_VERSION			0x20
+#define CCGX_I2C_RAB_UCSI_CONTROL			0x39
+#define CCGX_I2C_RAB_UCSI_CONTROL_START			BIT(0)
+#define CCGX_I2C_RAB_UCSI_CONTROL_STOP			BIT(1)
+#define CCGX_I2C_RAB_RESPONSE_REG			0x7E
+#define CCGX_I2C_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) & 0xff))
+
+#define USBC_VERSION_OFFSET	(0x0)
+#define USBC_VERSION_SIZE	(2)
+#define USBC_CCI_OFFSET		(0x4)
+#define USBC_CCI_SIZE		(4)
+#define USBC_CONTROL_OFFSET	(0x8)
+#define USBC_CONTROL_SIZE	(8)
+#define USBC_MSG_IN_OFFSET	(0x10)
+#define USBC_MSG_IN_SIZE	(16)
+#define USBC_MSG_OUT_OFFSET	(0x20)
+#define USBC_MSG_OUT_SIZE	(16)
+
+static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+	struct i2c_client *client = uc->client;
+	unsigned char buf[2];
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.len	= 0x2,
+			.buf	= buf,
+		},
+		{
+			.addr	= client->addr,
+			.flags  = I2C_M_RD,
+			.buf	= data,
+		},
+	};
+	u32 rlen, rem_len = len;
+	int status;
+
+	while (rem_len > 0) {
+		msgs[1].buf = &data[len - rem_len];
+		rlen = min_t(u16, rem_len, 4);
+		msgs[1].len = rlen;
+		put_unaligned_le16(rab, buf);
+		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+		if (status < 0) {
+			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+			return status;
+		}
+		rab += rlen;
+		rem_len -= rlen;
+	}
+
+	return 0;
+}
+
+static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+	struct i2c_client *client = uc->client;
+	unsigned char buf[2];
+	struct i2c_msg msgs[] = {
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.len	= 0x2,
+			.buf	= buf,
+		},
+		{
+			.addr	= client->addr,
+			.flags  = 0x0,
+			.buf	= data,
+			.len	= len,
+		},
+	};
+	int status;
+
+	put_unaligned_le16(rab, buf);
+	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (status < 0) {
+		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+		return status;
+	}
+
+	return 0;
+}
+
+static int ucsi_ccg_init(struct ucsi_ccg *uc)
+{
+	struct device *dev = uc->dev;
+	unsigned int count = 10;
+	u8 data[64];
+	int status;
+
+	status = ccg_read(uc, CCGX_I2C_RAB_DEVICE_MODE, data, sizeof(data));
+	if (status < 0)
+		return status;
+
+	dev_dbg(dev, "Silicon id %2ph", data + CCGX_I2C_RAB_READ_SILICON_ID);
+	dev_dbg(dev, "FW1 version %8ph\n", data + CCGX_I2C_RAB_FW1_VERSION);
+	dev_dbg(dev, "FW2 version %8ph\n", data + CCGX_I2C_RAB_FW2_VERSION);
+
+	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_STOP;
+	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+	if (status < 0)
+		return status;
+
+	data[0] = CCGX_I2C_RAB_UCSI_CONTROL_START;
+	status = ccg_write(uc, CCGX_I2C_RAB_UCSI_CONTROL, data, 0x1);
+	if (status < 0)
+		return status;
+
+	/*
+	 * Flush CCGx RESPONSE queue by acking interrupts
+	 * - above ucsi control register write will push response
+	 * which must be flushed
+	 * - affects f/w update which reads response register
+	 */
+	data[0] = 0xff;
+	do {
+		status = ccg_write(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+		if (status < 0)
+			return status;
+
+		usleep_range(10000, 11000);
+
+		status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, data, 0x1);
+		if (status < 0)
+			return status;
+	} while ((data[0] != 0x00) && count--);
+
+	return 0;
+}
+
+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));
+
+	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_OUT_OFFSET);
+	status = ccg_write(uc, rab, buf1, sizeof(buf1));
+	if (status < 0)
+		return status;
+
+	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CONTROL_OFFSET);
+	return ccg_write(uc, rab, buf2, sizeof(buf2));
+}
+
+static int ucsi_ccg_recv_data(struct ucsi_ccg *uc)
+{
+	u8 *ppm = (u8 *)uc->ppm.data;
+	int status;
+	u16 rab;
+
+	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_CCI_OFFSET);
+	status = ccg_read(uc, rab, ppm + USBC_CCI_OFFSET, USBC_CCI_SIZE);
+	if (status < 0)
+		return status;
+
+	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_MSG_IN_OFFSET);
+	return ccg_read(uc, rab, ppm + USBC_MSG_IN_OFFSET, USBC_MSG_IN_SIZE);
+}
+
+static int ucsi_ccg_ack_interrupt(struct ucsi_ccg *uc)
+{
+	int status;
+	unsigned char buf[1] = {0x0};
+
+	status = ccg_read(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
+	if (status < 0)
+		return status;
+
+	return ccg_write(uc, CCGX_I2C_RAB_INTR_REG, buf, 0x1);
+}
+
+static int ucsi_ccg_sync(struct ucsi_ppm *ppm)
+{
+	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
+	int status;
+
+	status = ucsi_ccg_recv_data(uc);
+	if (status < 0)
+		return status;
+
+	/* ack interrupt to allow next command to run */
+	return ucsi_ccg_ack_interrupt(uc);
+}
+
+static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
+{
+	struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
+
+	ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
+	return ucsi_ccg_send_data(uc);
+}
+
+static irqreturn_t ccg_irq_handler(int irq, void *data)
+{
+	struct ucsi_ccg *uc = data;
+
+	ucsi_notify(uc->ucsi);
+
+	return IRQ_HANDLED;
+}
+
+static int ucsi_ccg_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct ucsi_ccg *uc;
+	int status;
+	u16 rab;
+
+	uc = devm_kzalloc(dev, sizeof(*uc), GFP_KERNEL);
+	if (!uc)
+		return -ENOMEM;
+
+	uc->ppm.data = devm_kzalloc(dev, sizeof(struct ucsi_data), GFP_KERNEL);
+	if (!uc->ppm.data)
+		return -ENOMEM;
+
+	uc->ppm.cmd = ucsi_ccg_cmd;
+	uc->ppm.sync = ucsi_ccg_sync;
+	uc->dev = dev;
+	uc->client = client;
+
+	/* reset ccg device and initialize ucsi */
+	status = ucsi_ccg_init(uc);
+	if (status < 0) {
+		dev_err(uc->dev, "ucsi_ccg_init failed - %d\n", status);
+		return status;
+	}
+
+	uc->irq = client->irq;
+
+	status = devm_request_threaded_irq(dev, uc->irq, NULL, ccg_irq_handler,
+					   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+					   dev_name(dev), uc);
+	if (status < 0) {
+		dev_err(uc->dev, "request_threaded_irq failed - %d\n", status);
+		return status;
+	}
+
+	uc->ucsi = ucsi_register_ppm(dev, &uc->ppm);
+	if (IS_ERR(uc->ucsi)) {
+		dev_err(uc->dev, "ucsi_register_ppm failed\n");
+		return PTR_ERR(uc->ucsi);
+	}
+
+	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);
+	if (status < 0) {
+		ucsi_unregister_ppm(uc->ucsi);
+		return status;
+	}
+
+	i2c_set_clientdata(client, uc);
+	return 0;
+}
+
+static int ucsi_ccg_remove(struct i2c_client *client)
+{
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+
+	ucsi_unregister_ppm(uc->ucsi);
+
+	return 0;
+}
+
+static const struct i2c_device_id ucsi_ccg_device_id[] = {
+	{"ccgx-ucsi", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
+
+static struct i2c_driver ucsi_ccg_driver = {
+	.driver = {
+		.name = "ucsi_ccg",
+	},
+	.probe = ucsi_ccg_probe,
+	.remove = ucsi_ccg_remove,
+	.id_table = ucsi_ccg_device_id,
+};
+
+module_i2c_driver(ucsi_ccg_driver);
+
+MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
+MODULE_DESCRIPTION("UCSI driver for Cypress CCGx Type-C controller");
+MODULE_LICENSE("GPL v2");