Re: [v5 PATCH 1/5] extcon: Add Type-C and DP support

[Chris Zhong <zyw@rock-chips.com>]

Hi Chanwoo Choi

On 07/14/2016 08:49 AM, Chanwoo Choi wrote:
> Hi Chris,
>
> On 2016년 07월 13일 11:54, Chris Zhong wrote:
>> Hi Chanwoo Choi
>>
>> On 07/13/2016 10:05 AM, Chanwoo Choi wrote:
>>> Hi Chris,
>>>
>>> On 2016년 07월 13일 10:39, Chris Zhong wrote:
>>>> Hi Chanwoo Choi
>>>>
>>>>
>>>> On 07/13/2016 09:11 AM, Chanwoo Choi wrote:
>>>>> Hi Chris,
>>>>>
>>>>> I'm now developing the extcon property on extcon-test branch.
>>>>> But, it has not been completed.
>>>>>
>>>>> On next version, I'll remove the notification about extcon property
>>>>> and only support the following two functions.
>>>>> - extcon_set_cable_property()
>>>>> - extcon_get_cable_property()
>>>>>
>>>>> Because the number of properties would be risen and the all properties
>>>>> depend on the specific external connector(e.g., EXTCON_PROP_USB_VBUS
>>>>> depend on the EXTCON_TYPE_USB type). When the specific external connector
>>>>> is detached, extcon framework should make the property state as default state.
>>>> Yes, I think getting the notification from cable state is enough, actually I am using it like you said.
>>> OK.
>>>
>>>>> It may send the too many notification for extcon property.
>>>>> For example, Assume that EXTCON_TYPE_USB has the over 20 properties,
>>>>> when EXTCON_USB or EXTCON_USB_HOST is detached, extcon should send
>>>>> the notification for the over 20 properties and one more notificaiton
>>>>> for state of external connector.
>>>>>
>>>>> So, I'll send the RFC patchset without the notification of proerty.
>>>>>
>>>>> Lastly,
>>>>> I have a comment on below.
>>>>>
>>>>> Thanks,
>>>>> Chanwoo Choi
>>>>>
>>>>> On 2016년 07월 13일 00:09, Chris Zhong wrote:
>>>>>> Add EXTCON_DISP_DP for the Display external connector. For Type-C
>>>>>> connector the DisplayPort can work as an Alternate Mode(VESA DisplayPort
>>>>>> Alt Mode on USB Type-C Standard). The Type-C support both normal and
>>>>>> flipped orientation, so add a property to extcon.
>>>>>>
>>>>>> Signe-off-by: Chris Zhong <zyw@rock-chips.com>
>>>>>>
>>>>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v5:
>>>>>> - support get property
>>>>>>
>>>>>> Changes in v4: None
>>>>>> Changes in v3: None
>>>>>> Changes in v2: None
>>>>>> Changes in v1: None
>>>>>>
>>>>>>     drivers/extcon/extcon.c | 28 ++++++++++++++++++++++++++++
>>>>>>     include/linux/extcon.h  | 13 +++++++++++++
>>>>>>     2 files changed, 41 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>>>> index a1117db..2591b28 100644
>>>>>> --- a/drivers/extcon/extcon.c
>>>>>> +++ b/drivers/extcon/extcon.c
>>>>>> @@ -157,6 +157,11 @@ struct __extcon_info {
>>>>>>             .id = EXTCON_DISP_VGA,
>>>>>>             .name = "VGA",
>>>>>>         },
>>>>>> +    [EXTCON_DISP_DP] = {
>>>>>> +        .type = EXTCON_TYPE_DISP,
>>>>>> +        .id = EXTCON_DISP_DP,
>>>>>> +        .name = "DP",
>>>>>> +    },
>>>>>>           /* Miscellaneous external connector */
>>>>>>         [EXTCON_DOCK] = {
>>>>>> @@ -270,6 +275,7 @@ static bool is_extcon_property_supported(unsigned int id,
>>>>>>             switch (prop) {
>>>>>>             case EXTCON_PROP_USB_ID:
>>>>>>             case EXTCON_PROP_USB_VBUS:
>>>>>> +        case EXTCON_PROP_TYPEC_POLARITY:
>>>>>>                 return true;
>>>>>>             default:
>>>>>>                 break;
>>>>>> @@ -286,6 +292,8 @@ static bool is_extcon_property_supported(unsigned int id,
>>>>>>             }
>>>>>>         case EXTCON_TYPE_DISP:
>>>>>>             switch (prop) {
>>>>>> +        case EXTCON_PROP_TYPEC_POLARITY:
>>>>> Should EXTCON_PROP_TYPEC_POLARITY property add to both EXTCON_TYPE_USB and EXTCON_TYP_DISP?
>>>>> EXTCON_PROP_TYPEC_POLARITY is the property of USB C-type?
>>>> it is for USB Type-C, But at Display Port alt mode, both EXTCON_USB and EXTCON_USB_HOST may be detached. Does it support set the property to a detached cable, if so, I think move this case to EXTCON_USB is fine.
>>> One external connector can set the state of one more external connector
>>> if the one connector support the various functions.
>>> For example, EXTCON_USB and EXTCON_CHG_USB_SDP
>>> The existing extcon driver[1](e.g., max14577/max77693 etc.) set the state of both EXTCON_USB and EXTCON_CHG_USB_SDP connector at the same time
>>> when usb cable is attached. Because in this case, the usb connector uses as both power supply(EXTCON_CHG_USB_SDP) and data transfer(EXTCON_USB).
>>> [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next&id=8b45b6a0741678902810d7be95e635c210fbb198
>>>
>>> So, DP Alt mode uses the USB Type-C. So, When USB C-type connector is attached for DP Alt mode,
>>> Maybe, you can set the following two state of connector and one property:
>>> - extcon_set_cable_state(edev, [EXTCON_USB or EXTCON_USB_HOST], 1);
>>> - extcon_set_cable_state(edev, EXTCON_DISP_DP, 1);
>>> - extcon_set_cable_state(edev, [EXTCON_USB or EXTCON_USB_HOST], EXTCON_PROP_TYPEC_POLARITY, 0 or 1);
>>>
>>> Thanks,
>>> Chanwoo Choi
>> There are 4 modes for Type-C DP alt mode:
>> 1) USB host only  :
>>
>> extcon_set_cable_state(edev, EXTCON_USB_HOST, 1);
>> extcon_set_cable_state(edev, EXTCON_USB, 0);
>> extcon_set_cable_state(edev, EXTCON_DISP_DP, 0);
>>
>> 2) USB device only
>>
>> extcon_set_cable_state(edev, EXTCON_USB_HOST, 0);
>> extcon_set_cable_state(edev, EXTCON_USB, 1);
>> extcon_set_cable_state(edev, EXTCON_DISP_DP, 0);
>>
>> 3) DP only
>>
>> extcon_set_cable_state(edev, EXTCON_USB_HOST, 0);
>> extcon_set_cable_state(edev, EXTCON_USB, 0);
>> extcon_set_cable_state(edev, EXTCON_DISP_DP, 1);
>>
>> 4) USB + DP
>>
>> extcon_set_cable_state(edev, EXTCON_USB_HOST, 1);
>> extcon_set_cable_state(edev, EXTCON_USB, 0);
>> extcon_set_cable_state(edev, EXTCON_DISP_DP, 1);
>>
>>
>> for 3rd mode: DP only, there is only EXTCON_DISP_DP is attached, the EXTCON_USB_HOST
>> and EXTCON_USB are detached, Can I set the property into these 2 detached cable?
>> or just call extcon_set_cable_state(edev, EXTCON_DISP_DP, EXTCON_PROP_TYPEC_POLARITY, 0 or 1);
> I'm thinking to solve this issue. In result, we can add one more type to specific connector.
> For EXTCON_DISP_DP, we can add the two type as following. And EXTCON_PROP_TYPEC_POLARITY property
> should be added to EXTCON_TYPE_USB.
>
> +	[EXTCON_DISP_DP] = {
> +		.type = EXTCON_TYPE_DISP | EXTCON_TYPE_USB,
> +		.id = EXTCON_DISP_DP,
> +		.name = "DP",
> +	},
>
> So, as you mentioned, EXTCON_DISP_DP can set the EXTCON_PROP_TYPEC_POLARITY property as following:
> - extcon_set_cable_state(edev, EXTCON_DISP_DP, EXTCON_PROP_TYPEC_POLARITY, 0 or 1);
>
> I'll again developing the extcon property.
>
> Thanks,
> Chanwoo Choi
>
Nice idea, But I am thinking about is it compatible with real 
DisplayPort without Type-C.
Maybe we should add a new cable: EXTCON_DISP_DP_ALT

+	[EXTCON_DISP_DP] = {
+		.type = EXTCON_TYPE_DISP,
+		.id = EXTCON_DISP_DP,
+		.name = "DP",
+	},

+	[EXTCON_DISP_DP_ALT] = {
+		.type = EXTCON_TYPE_DISP | EXTCON_TYPE_USB,
+		.id = EXTCON_DISP_DP,
+		.name = "DP ALT",
+	},



>
>