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: <5614D697.5090304@ti.com> 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