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

From: Chanwoo Choi
Date: Wed Oct 07 2015 - 04:30:15 EST

Next message: Michael Kerrisk (man-pages): "Re: Next round: revised futex(2) man page for review"
Previous message: Marc Gonzalez: "Re: [PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal"
In reply to: Roger Quadros: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Next in thread: Roger Quadros: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Roger,

On 2015ë 10ì 07ì 17:23, Roger Quadros wrote:
> 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@xxxxxxxxxxx>
>>>>>>> [ckeepax: For the Arizona changes]
>>>>>>> Acked-by: Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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?

Yes. Usually, EXTCON_USB is used for USB and PHY framework
and EXTCON_CHG_USB_SDP/EXTCON_CHG_USB_CDP will be used for power-supply framework.

For example,
In case of samsung mobile and wearable product,
when SDP is attached to device, the device can transfer the data
and charge the battery through same USB connector at the same time.

Thanks,
Chanwoo Choi

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

Next message: Michael Kerrisk (man-pages): "Re: Next round: revised futex(2) man page for review"
Previous message: Marc Gonzalez: "Re: [PATCH v1] clocksource: Sigma Designs Tango 27 MHz xtal"
In reply to: Roger Quadros: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Next in thread: Roger Quadros: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]