| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 7 Oct 2015 11:52:16 +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
On 07/10/15 11:30, Chanwoo Choi wrote:
> 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@...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?
>
> 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.
So USB data detection and Charger detection are separate extcon devices right?
OK in that case having EXTCON_USB as well as EXTCON_CHG_USB_SDP and CDP
make sense.
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