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
| ||
|
Message-id: <5614F036.1060805@samsung.com> Date: Wed, 07 Oct 2015 19:13:10 +0900 From: Chanwoo Choi <cw00.choi@...sung.com> To: Roger Quadros <rogerq@...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 Roger, On 2015년 10월 07일 17:52, Roger Quadros wrote: > 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? Yes. When SDP is attached or detached, extcon device will update the state of both EXTCON_USB and EXTCON_CHG_USB_SDP. And when CDP is attached or detached, extcon device will update the state of both EXTCON_USB and EXTCON_CHG_USB_CDP. > > OK in that case having EXTCON_USB as well as EXTCON_CHG_USB_SDP and CDP > make sense. OK. Thanks for your review and discussion. Thanks, Chanwoo Choi -- 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