Date:	Wed, 7 Oct 2015 11:23:51 +0300
From:	Roger Quadros <rogerq@...com>
To:	Chanwoo Choi <cw00.choi@...sung.com>,
	<linux-kernel@...r.kernel.org>
CC:	<k.kozlowski@...sung.com>, <ckeepax@...nsource.wolfsonmicro.com>,
	<gregkh@...uxfoundation.org>, <ramakrishna.pallala@...el.com>,
	<patches@...nsource.wolfsonmicro.com>, <myungjoo.ham@...sung.com>
Subject: Re: [PATCH v3] extcon: Modify the id and name of external connector

Chanwoo,

On 07/10/15 11:09, Chanwoo Choi wrote:
> Roger,
> 
> On 2015년 10월 07일 16:59, Chanwoo Choi wrote:
>> On 2015년 10월 07일 16:50, Roger Quadros wrote:
>>> On 07/10/15 10:42, Chanwoo Choi wrote:
>>>> On 2015년 10월 07일 16:33, Roger Quadros wrote:
>>>>> On 07/10/15 03:48, 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_DCP_FAST|
>>>>>> EXTCON_SLOW_CHARGER     | EXTCON_CHG_USB_DCP_SLOW|
>>>>>
>>>>> I still have some concerns regarding the name DCP_FAST and DCP_SLOW.
>>>>> If I see the charger IC drivers that use FAST & SLOW, they either call it
>>>>> APPLE_500MA/1A or SPECIAL_500MA/1A.
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> They are probably not compliant with USB DCP specification and hence
>>>>> the special name.
>>>>>
>>>>> So, should we call them EXTCON_CHG_USB_SPECIAL_FAST/SLOW instead?
>>>>
>>>> I don't prefer to use "SPECIAL" adjective for connector name.
>>>> I prefer following name at the first patch.
>>>> - EXTCON_CHG_USB_FAST
>>>> - EXTCON_CHG_USB_SLOW
>>>
>>> OK, that is fine with me.
>>>>
>>>>>
>>>>>> --------------------------------------------------
>>>>>> 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_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@...sung.com>
>>>>>> [ckeepax: For the Arizona changes]
>>>>>> Acked-by: Charles Keepax <ckeepax@...nsource.wolfsonmicro.com>
>>>>>> ---
>>>>>> 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          | 60 ++++++++++++++++++++-------------------
>>>>>>  include/linux/extcon.h           | 61 +++++++++++++++++++++++-----------------
>>>>>>  10 files changed, 137 insertions(+), 119 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>>>>>> index a1ab0a56b798..e4890dd4fefd 100644
>>>>>> --- a/drivers/extcon/extcon-arizona.c
>>>>>> +++ b/drivers/extcon/extcon-arizona.c
>>>>>> @@ -137,9 +137,9 @@ static const int arizona_micd_levels[] = {
>>>>>>  
>>>>>>  static const unsigned int arizona_cable[] = {
>>>>>>  	EXTCON_MECHANICAL,
>>>>>> -	EXTCON_MICROPHONE,
>>>>>> -	EXTCON_HEADPHONE,
>>>>>> -	EXTCON_LINE_OUT,
>>>>>> +	EXTCON_JACK_MICROPHONE,
>>>>>> +	EXTCON_JACK_HEADPHONE,
>>>>>> +	EXTCON_JACK_LINE_OUT,
>>>>>>  	EXTCON_NONE,
>>>>>>  };
>>>>>>  
>>>>>> @@ -600,7 +600,7 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>>>>>>  	struct arizona_extcon_info *info = data;
>>>>>>  	struct arizona *arizona = info->arizona;
>>>>>>  	int id_gpio = arizona->pdata.hpdet_id_gpio;
>>>>>> -	unsigned int report = EXTCON_HEADPHONE;
>>>>>> +	unsigned int report = EXTCON_JACK_HEADPHONE;
>>>>>>  	int ret, reading;
>>>>>>  	bool mic = false;
>>>>>>  
>>>>>> @@ -645,9 +645,9 @@ static irqreturn_t arizona_hpdet_irq(int irq, void *data)
>>>>>>  
>>>>>>  	/* Report high impedence cables as line outputs */
>>>>>>  	if (reading >= 5000)
>>>>>> -		report = EXTCON_LINE_OUT;
>>>>>> +		report = EXTCON_JACK_LINE_OUT;
>>>>>>  	else
>>>>>> -		report = EXTCON_HEADPHONE;
>>>>>> +		report = EXTCON_JACK_HEADPHONE;
>>>>>>  
>>>>>>  	ret = extcon_set_cable_state_(info->edev, report, true);
>>>>>>  	if (ret != 0)
>>>>>> @@ -732,7 +732,7 @@ err:
>>>>>>  			   ARIZONA_ACCDET_MODE_MASK, ARIZONA_ACCDET_MODE_MIC);
>>>>>>  
>>>>>>  	/* Just report headphone */
>>>>>> -	ret = extcon_set_cable_state_(info->edev, EXTCON_HEADPHONE, true);
>>>>>> +	ret = extcon_set_cable_state_(info->edev, EXTCON_JACK_HEADPHONE, true);
>>>>>>  	if (ret != 0)
>>>>>>  		dev_err(arizona->dev, "Failed to report headphone: %d\n", ret);
>>>>>>  
>>>>>> @@ -789,7 +789,7 @@ err:
>>>>>>  			   ARIZONA_ACCDET_MODE_MASK, ARIZONA_ACCDET_MODE_MIC);
>>>>>>  
>>>>>>  	/* Just report headphone */
>>>>>> -	ret = extcon_set_cable_state_(info->edev, EXTCON_HEADPHONE, true);
>>>>>> +	ret = extcon_set_cable_state_(info->edev, EXTCON_JACK_HEADPHONE, true);
>>>>>>  	if (ret != 0)
>>>>>>  		dev_err(arizona->dev, "Failed to report headphone: %d\n", ret);
>>>>>>  
>>>>>> @@ -915,7 +915,7 @@ static void arizona_micd_detect(struct work_struct *work)
>>>>>>  		arizona_identify_headphone(info);
>>>>>>  
>>>>>>  		ret = extcon_set_cable_state_(info->edev,
>>>>>> -					      EXTCON_MICROPHONE, true);
>>>>>> +					      EXTCON_JACK_MICROPHONE, true);
>>>>>>  		if (ret != 0)
>>>>>>  			dev_err(arizona->dev, "Headset report failed: %d\n",
>>>>>>  				ret);
>>>>>> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
>>>>>> index 9668d6a94e38..14e748051288 100644
>>>>>> --- a/drivers/extcon/extcon-axp288.c
>>>>>> +++ b/drivers/extcon/extcon-axp288.c
>>>>>> @@ -102,9 +102,9 @@ enum axp288_extcon_irq {
>>>>>>  };
>>>>>>  
>>>>>>  static const unsigned int axp288_extcon_cables[] = {
>>>>>> -	EXTCON_SLOW_CHARGER,
>>>>>> -	EXTCON_CHARGE_DOWNSTREAM,
>>>>>> -	EXTCON_FAST_CHARGER,
>>>>>> +	EXTCON_CHG_USB_DCP_SLOW,
>>>>>> +	EXTCON_CHG_USB_CDP,
>>>>>> +	EXTCON_CHG_USB_DCP_FAST,
>>>>>>  	EXTCON_NONE,
>>>>>>  };
>>>>>>  
>>>>>> @@ -192,18 +192,18 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>>>>>>  		dev_dbg(info->dev, "sdp cable is connecetd\n");
>>>>>
>>>>> as per comment this seems to be SDP.
>>>>
>>>> This patch modifies the only connector name.
>>>> If some comment should be fixed, we need to implement on separate patch.
>>>
>>> Agreed.
>>>>
>>>>>
>>>>>>  		notify_otg = true;
>>>>>>  		notify_charger = true;
>>>>>> -		cable = EXTCON_SLOW_CHARGER;
>>>>>> +		cable = EXTCON_CHG_USB_DCP_SLOW;
>>>>>
>>>>> So shouldn't this be cable = EXTCON_USB?
>>>>
>>>> No, EXTCON_CHG_USB_*_SLOW is right.
>>>>
>>>>>
>>>>>>  		break;
>>>>>>  	case DET_STAT_CDP:
>>>>>>  		dev_dbg(info->dev, "cdp cable is connecetd\n");
>>>>>>  		notify_otg = true;
>>>>>>  		notify_charger = true;
>>>>>> -		cable = EXTCON_CHARGE_DOWNSTREAM;
>>>>>> +		cable = EXTCON_CHG_USB_CDP;
>>>>>>  		break;
>>>>>>  	case DET_STAT_DCP:
>>>>>>  		dev_dbg(info->dev, "dcp cable is connecetd\n");
>>>>>
>>>>> This looks like standard DCP.
>>>>
>>>> ditto. Need the separate patch to fix it.
>>>
>>> Agreed.
>>>
>>>>
>>>>>
>>>>>>  		notify_charger = true;
>>>>>> -		cable = EXTCON_FAST_CHARGER;
>>>>>> +		cable = EXTCON_CHG_USB_DCP_FAST;
>>>>>
>>>>> Shouldn't this be cable = EXTCON_CHG_USB_DCP?
>>>>
>>>> No, EXTCON_CHG_USB_*_FAST is right.
>>>
>>> Why? The case is DET_STAT_DCP.
>>
>> You're right. I'll fix it as following:
>>
>> case DET_STAT_SDP -> EXTCON_USB 
>> case DET_STAT_CDP -> EXTCON_CHG_USB_CDP
>> case DET_STAT_DCP -> EXTCON_CHG_USB_DCP
> 
> I think that we need to consider about SDP and CDP port again
> because SDP/CDP support the both data transmission and charging at the same time.
> 
> Case 1. if SDP is attached
> Following connector type mean the each role of SDP.
> - EXTCON_USB for data transmission
> - EXTCON_CHG_USB_SDP for charging
> 
> Case 2. if CDP is attached
> ditto.
> - EXTCON_USB for data transmission
> - EXTCON_CHG_USB_CDP for charging
> 

The only issue is that charging and data transmission happen
simultaneously. Can the framework allow both states to be
active simultaneously?

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists