Re: [PATCH v4] extcon: Modify the id and name of external connector

[Chanwoo Choi <cw00.choi@samsung.com>]

Roger,

On 2015년 10월 15일 16:16, Roger Quadros wrote:
> Chanwoo,
> 
> On 15/10/15 05:22, Chanwoo Choi wrote:
>> Hi Roger,
>>
>> On 2015년 10월 14일 16:13, Roger Quadros wrote:
>>> Chanwoo,
>>>
>>> On 08/10/15 12:24, Chanwoo Choi wrote:
>>>> This patch modifies the id and name of external connector with the
>>>> additional prefix to clarify both attribute and meaning of external
>>>> connector as following:
>>>> - EXTCON_CHG_* mean the charger connector.
>>>> - EXTCON_JACK_* mean the jack connector.
>>>> - EXTCON_DISP_* mean the display port connector.
>>>>
>>>> Following table show the new name of external connector with old name:
>>>> --------------------------------------------------
>>>> Old extcon name         | New extcon name        |
>>>> --------------------------------------------------
>>>> EXTCON_TA               | EXTCON_CHG_USB_DCP     |
>>>> EXTCON_CHARGE_DOWNSTREAM| EXTCON_CHG_USB_CDP     |
>>>> EXTCON_FAST_CHARGER     | EXTCON_CHG_USB_FAST    |
>>>> EXTCON_SLOW_CHARGER     | EXTCON_CHG_USB_SLOW    |
>>>> --------------------------------------------------
>>>> EXTCON_MICROPHONE       | EXTCON_JACK_MICROPHONE |
>>>> EXTCON_HEADPHONE        | EXTCON_JACK_HEADPHONE  |
>>>> EXTCON_LINE_IN          | EXTCON_JACK_LINE_IN    |
>>>> EXTCON_LINE_OUT         | EXTCON_JACK_LINE_OUT   |
>>>> EXTCON_VIDEO_IN         | EXTCON_JACK_VIDEO_IN   |
>>>> EXTCON_VIDEO_OUT        | EXTCON_JACK_VIDEO_OUT  |
>>>> EXTCON_SPDIF_IN         | EXTCON_JACK_SPDIF_IN   |
>>>> EXTCON_SPDIF_OUT        | EXTCON_JACK_SPDIF_OUT  |
>>>> --------------------------------------------------
>>>> EXTCON_HMDI             | EXTCON_DISP_HDMI       |
>>>> EXTCON_MHL              | EXTCON_DISP_MHL        |
>>>> EXTCON_DVI              | EXTCON_DISP_DVI        |
>>>> EXTCON_VGA              | EXTCON_DISP_VGA        |
>>>> --------------------------------------------------
>>>>
>>>> And, when altering the name of USB charger connector, EXTCON refers to the
>>>> "Battery Charging v1.2 Spec and Adopters Agreement"[1] to use the standard
>>>> name of USB charging port as following. Following name of USB charging port
>>>> are already used in power_supply subsystem. We chan check it on patch[2].
>>>> - EXTCON_CHG_USB_SDP	/* Standard Downstream Port */
>>>> - EXTCON_CHG_USB_DCP	/* Dedicated Charging Port */
>>>> - EXTCON_CHG_USB_CDP	/* Charging Downstream Port */
>>>> - EXTCON_CHG_USB_ACA	/* Accessory Charger Adapter */
>>>>
>>>> [1] www.usb.org/developers/docs/devclass_docs/BCv1.2_070312.zip
>>>> [2] commit 85efc8a18ced ("power_supply: Add types for USB chargers")
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> [ckeepax: For the Arizona changes]
>>>> Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>> ---
>>>> Changes from v3:
>>>> (https://lkml.org/lkml/2015/10/6/984)
>>>> - Modify the name of fast/slow charger connector as following:
>>>> : EXTCON_CHG_USB_DCP_FAST -> EXTCON_CHG_USB_FAST
>>>> : EXTCON_CHG_USB_DCP_SLOW -> EXTCON_CHG_USB_SLOW
>>>> - Add EXTCON_CHG_USB_SDP to mean the charging connector of SDP (Standard
>>>>   Downstream Port)
>>>>
>>>> Changes from v2:
>>>> (https://lkml.org/lkml/2015/10/6/239)
>>>> - Remove the EXTCON_CHG_USB type to remove the possible confusion according to
>>>>   Roger's comment and drop patch2 about EXTCON_CHG_USB.
>>>> - Fix the warning issue provided by scripts/checkpatch.pl
>>>>
>>>> Changes from v1:
>>>> (https://lkml.org/lkml/2015/10/3/304)
>>>> - Add acked tag by Charles Keepax for arizona changes
>>>> - Modify the name of USB charger connector as following:
>>>>  : EXTCON_CHG_USB_FAST -> EXTCON_CHG_USB_DCP_FAST
>>>>  : EXTCON_CHG_USB_SLOW -> EXTCON_CHG_USB_DCP_SLOW
>>>> - Add the missing EXTCON_CHG_USB_ACA charger connector
>>>> - Add one more patch to support the EXTCON_CHG_USB when SDP port is
>>>>   connected or not
>>>>
>>>>  drivers/extcon/extcon-arizona.c  | 18 ++++++------
>>>>  drivers/extcon/extcon-axp288.c   | 12 ++++----
>>>>  drivers/extcon/extcon-max14577.c | 17 +++++------
>>>>  drivers/extcon/extcon-max77693.c | 32 +++++++++++----------
>>>>  drivers/extcon/extcon-max77843.c | 27 +++++++++--------
>>>>  drivers/extcon/extcon-max8997.c  | 21 +++++++-------
>>>>  drivers/extcon/extcon-rt8973a.c  |  4 +--
>>>>  drivers/extcon/extcon-sm5502.c   |  4 +--
>>>>  drivers/extcon/extcon.c          | 61 ++++++++++++++++++++-------------------
>>>>  include/linux/extcon.h           | 62 +++++++++++++++++++++++-----------------
>>>>  10 files changed, 139 insertions(+), 119 deletions(-)
>>>>
> 
> <snip>
> 
>>>> diff --git a/drivers/extcon/extcon-rt8973a.c b/drivers/extcon/extcon-rt8973a.c
>>>> index 1bc3737ea01c..36bf1d63791c 100644
>>>> --- a/drivers/extcon/extcon-rt8973a.c
>>>> +++ b/drivers/extcon/extcon-rt8973a.c
>>>> @@ -93,7 +93,7 @@ static struct reg_data rt8973a_reg_data[] = {
>>>>  static const unsigned int rt8973a_extcon_cable[] = {
>>>>  	EXTCON_USB,
>>>>  	EXTCON_USB_HOST,
>>>> -	EXTCON_TA,
>>>> +	EXTCON_CHG_USB_DCP,
>>>>  	EXTCON_JIG,
>>>>  	EXTCON_NONE,
>>>>  };
>>>> @@ -333,7 +333,7 @@ static int rt8973a_muic_cable_handler(struct rt8973a_muic_info *info,
>>>>  		con_sw = DM_DP_SWITCH_USB;
>>>>  		break;
>>>>  	case RT8973A_MUIC_ADC_TA:
>>>> -		id = EXTCON_TA;
>>>> +		id = EXTCON_CHG_USB_DCP;
>>>>  		con_sw = DM_DP_SWITCH_OPEN;
>>>>  		break;
>>>>  	case RT8973A_MUIC_ADC_FACTORY_MODE_BOOT_OFF_USB:
>>>> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
>>>> index 2945091bfd0e..7aac3cc7efd7 100644
>>>> --- a/drivers/extcon/extcon-sm5502.c
>>>> +++ b/drivers/extcon/extcon-sm5502.c
>>>> @@ -95,7 +95,7 @@ static struct reg_data sm5502_reg_data[] = {
>>>>  static const unsigned int sm5502_extcon_cable[] = {
>>>>  	EXTCON_USB,
>>>>  	EXTCON_USB_HOST,
>>>> -	EXTCON_TA,
>>>> +	EXTCON_CHG_USB_DCP,
>>>>  	EXTCON_NONE,
>>>>  };
>>>>  
>>>> @@ -389,7 +389,7 @@ static int sm5502_muic_cable_handler(struct sm5502_muic_info *info,
>>>>  		vbus_sw	= VBUSIN_SWITCH_VBUSOUT_WITH_USB;
>>>>  		break;
>>>>  	case SM5502_MUIC_ADC_OPEN_TA:
>>>> -		id	= EXTCON_TA;
>>>> +		id	= EXTCON_CHG_USB_DCP;
>>>>  		con_sw	= DM_DP_SWITCH_OPEN;
>>>>  		vbus_sw	= VBUSIN_SWITCH_VBUSOUT;
>>>>  		break;
>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>> index 8dd0af1d50bc..f345d492d4a1 100644
>>>> --- a/drivers/extcon/extcon.c
>>>> +++ b/drivers/extcon/extcon.c
>>>> @@ -39,37 +39,40 @@
>>>>  #define CABLE_NAME_MAX		30
>>>>  
>>>>  static const char *extcon_name[] =  {
>>>> -	[EXTCON_NONE]		= "NONE",
>>>> +	[EXTCON_NONE]			= "EXTCON_NONE",
>>>>  
>>>>  	/* USB external connector */
>>>> -	[EXTCON_USB]		= "USB",
>>>> -	[EXTCON_USB_HOST]	= "USB-HOST",
>>>> -
>>>> -	/* Charger external connector */
>>>> -	[EXTCON_TA]		= "TA",
>>>> -	[EXTCON_FAST_CHARGER]	= "FAST-CHARGER",
>>>> -	[EXTCON_SLOW_CHARGER]	= "SLOW-CHARGER",
>>>> -	[EXTCON_CHARGE_DOWNSTREAM] = "CHARGE-DOWNSTREAM",
>>>> -
>>>> -	/* Audio/Video external connector */
>>>> -	[EXTCON_LINE_IN]	= "LINE-IN",
>>>> -	[EXTCON_LINE_OUT]	= "LINE-OUT",
>>>> -	[EXTCON_MICROPHONE]	= "MICROPHONE",
>>>> -	[EXTCON_HEADPHONE]	= "HEADPHONE",
>>>> -
>>>> -	[EXTCON_HDMI]		= "HDMI",
>>>> -	[EXTCON_MHL]		= "MHL",
>>>> -	[EXTCON_DVI]		= "DVI",
>>>> -	[EXTCON_VGA]		= "VGA",
>>>> -	[EXTCON_SPDIF_IN]	= "SPDIF-IN",
>>>> -	[EXTCON_SPDIF_OUT]	= "SPDIF-OUT",
>>>> -	[EXTCON_VIDEO_IN]	= "VIDEO-IN",
>>>> -	[EXTCON_VIDEO_OUT]	= "VIDEO-OUT",
>>>> -
>>>> -	/* Etc external connector */
>>>> -	[EXTCON_DOCK]		= "DOCK",
>>>> -	[EXTCON_JIG]		= "JIG",
>>>> -	[EXTCON_MECHANICAL]	= "MECHANICAL",
>>>> +	[EXTCON_USB]			= "EXTCON_USB",
>>>
>>> Should the name string be "USB-PERIPHERAL"?
>>
>> I think 'PERIPHERAL' is not necessary. The extcon name is
>> used for only end user using the platform developer.
>> 'PERIPHERAL' might cause the confusion to end user.
>>
> OK.
>>>
>>>> +	[EXTCON_USB_HOST]		= "EXTCON_USB_HOST",
>>>
>>> Why prefix EXTCON and change hyphen to underscore?
>>> Wasn't the original version i.e. "USB-HOST" better?
>>
>> Agreee.
>>
>>>
>>> Why the change in the name strings? Who is the end user of the name string?
>>> If the end use is just for information to a human user then the human readable
>>> format makes more sense. i.e. "MHL" or "MICROPHONE" makes more sense than
>>> "EXTCON_DISP_MHL" or "EXTCON_JACK_MICROPHONE"
>>
>> Your comment make sense. I'll not modify the name of external connector.
>> I'll use the existing name.
>>
> 
> OK. Sorry for not pointing this out earlier.

No problem.

Thanks for your review.

Regards,
Chanwoo Choi