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

From: Roger Quadros
Date: Wed Oct 07 2015 - 03:51:02 EST

Next message: Christoph Hellwig: "Re: [PATCH v8 00/41] Richacls"
Previous message: Gabriel Fernandez: "Re: [PATCH v3 2/4] drivers: clk: st: PLL rate change implementation for DVFS"
In reply to: Chanwoo Choi: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Next in thread: Chanwoo Choi: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

>
>>
>>> break;
>>> default:
>>> dev_warn(info->dev,

<snip>

cheers,
-roger
--
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: Christoph Hellwig: "Re: [PATCH v8 00/41] Richacls"
Previous message: Gabriel Fernandez: "Re: [PATCH v3 2/4] drivers: clk: st: PLL rate change implementation for DVFS"
In reply to: Chanwoo Choi: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Next in thread: Chanwoo Choi: "Re: [PATCH v3] extcon: Modify the id and name of external connector"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]