[PATCH v3 0/6] extcon: Add the support for extcon type and property

[PATCH v3 0/6] extcon: Add the support for extcon type and property

[Chanwoo Choi]

This patch-set add the support the extcon type, extcon property
and the synchronization functions.

The each external connector has the common characters. So, the external
connectors are able to gather in the specific type. And the each external
connectors has the specific H/W desigin to support the multiple features
throught h/w lines. There are the requirement to express the each h/w
character of each external connector. Lastly, when the state and property
are changed, the extcon notify the extcon client driver of the changed
information. To support the notification on extcon provider drivers,
this patches support the three sync functions.

Changes from v2:
- Fix minor coding style issue.
- Add Tested-by tag of Guenter Roeck for all patches.
- Add Reviewed-by tag of Guenter Roeck for all patches

Changes from v1:
- Expand the size (+1) of the property array for each extcon type.
- Use the memset() to initialize the property when connector is detached.
- Wrap the data of struct extcon_dev in the lock mechanism.
- Don't send the notification if connector state is not changed in
  extcon_set_state_sync()
- Fix the minor issue.
- Add Tested-by tag of Chris Zhong for these patches.
- Add Signed-off tag of Myungjoo Ham for patch1.
- Add Reviewed-by tag of Guenter Roeck for patch6.

Depends on:
This patch depend on the extcon git repository[2]. (branch : extcon-next)

[1] https://en.wikipedia.org/wiki/DisplayPort
[2] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/

[Detailed description of these patches]
1. Add the extcon type to group the each external connector.
There are five categories unitl now as following:
- EXTCON_TYPE_USB  : USB connector
- EXTCON_TYPE_CHG  : Charger connector
- EXTCON_TYPE_JACK : Jack connector
- EXTCON_TYPE_DISP : Display connector
- EXTCON_TYPE_MISC : Miscellaneous connector

2. Add the extcon property to support the multiple characteristic
for the specific H/W design.
- EXTCON_PROP_USB_[property name]
- EXTCON_PROP_CHG_[property name]
- EXTCON_PROP_JACK_[property name]
- EXTCON_PROP_DISP_[property name]
e.g., EXTCON_PROP_USB_ID and EXTCON_PROP_USB_VBUS
The list of the new extcon APIs for the property as following:
- int extcon_get_property(struct extcon_dev *edev,
			unsigned int id, unsigned int prop,
			union extcon_property_value *prop_val)
- int extcon_set_property(struct extcon_dev *edev,
			unsigned int id, unsigned int prop,
			union extcon_property_value prop_val)
- int extcon_get_property_capability(struct extcon_dev *edev,
			unsigned int id, unsigned int prop);
- int extcon_set_property_capability(struct extcon_dev *edev,
			unsigned int id, unsigned int prop);

3. Add the sync functions to synchronize the data of each external connector
between an extcon provider driver and the extcon client drivers.
The list of the new extcon sync APIs as following:
- extcon_sync() : Send the notification for each external connector to
		synchronize the information between and extcon provider driver
		and the extcon client drivers.
- extcon_set_state_sync() : Set the state of external connector with noti.
- extcon_set_property_sync() : Set the property of external connector with noti.

4. Add the new external connector definition. The EXTCON_DISP_DP
means the Display external connector[1].
The list of new external connector as following:
- EXTCON_DISP_DP
The list of new property of USB connector as following:
- EXTCON_PROP_USB_TYPEC_POLARITY

5. Rename the renames the existing extcon_get/set_cable_state_()
to maintain the function naming pattern like as extcon APIs for property.
- extcon_set_cable_state_() -> extcon_set_state()
- extcon_get_cable_state_() -> extcon_get_state()

For example,
case 1, change the state of external connector and synchronized the data.
	extcon_set_state_sync(edev, EXTCON_USB, 1);
case 2, change both the state and property of external connector
	and synchronized the data.
	extcon_set_state(edev, EXTCON_USB, 1);
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
	extcon_sync(edev, EXTCON_USB);
case 3, change the property of external connector and synchronized the data.
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
	extcon_sync(edev, EXTCON_USB);
case 4, change the property of external connector and synchronized the data.
	extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);

Chanwoo Choi (5):
  extcon: Add the extcon_type to gather each connector into five category
  extcon: Add the support for extcon property according to extcon type
  extcon: Add the support for the capability of each property
  extcon: Rename the extcon_set/get_state() to maintain the function  naming pattern
  extcon: Add the synchronization extcon APIs to support the notification

Chris Zhong (1):
  extcon: Add EXTCON_DISP_DP and the property for USB Type-C

 drivers/extcon/extcon.c | 730 ++++++++++++++++++++++++++++++++++++++++--------
 include/linux/extcon.h  | 176 +++++++++++-
 2 files changed, 786 insertions(+), 120 deletions(-)

-- 
1.9.1

[PATCH v3 1/6] extcon: Add the extcon_type to gather each connector into five category

[Chanwoo Choi]

This patch adds the new extcon type to group the each connecotr
into following five category. This type would be used to handle
the connectors as a group unit instead of a connector unit.
- EXTCON_TYPE_USB  : USB connector
- EXTCON_TYPE_CHG  : Charger connector
- EXTCON_TYPE_JACK : Jack connector
- EXTCON_TYPE_DISP : Display connector
- EXTCON_TYPE_MISC : Miscellaneous connector

Also, each external connector is possible to belong to one more extcon type.
In caes of EXTCON_CHG_USB_SDP, it have the EXTCON_TYPE_CHG and EXTCON_TYPE_USB.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 159 +++++++++++++++++++++++++++++++++++++++---------
 include/linux/extcon.h  |   9 +++
 2 files changed, 139 insertions(+), 29 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 9a266e5c7e10..129afc87313e 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -38,43 +38,144 @@
 #define SUPPORTED_CABLE_MAX	32
 #define CABLE_NAME_MAX		30
 
-static const char *extcon_name[] =  {
-	[EXTCON_NONE]			= "NONE",
+struct __extcon_info {
+	unsigned int type;
+	unsigned int id;
+	const char *name;
+
+} extcon_info[] = {
+	[EXTCON_NONE] = {
+		.type = EXTCON_TYPE_MISC,
+		.id = EXTCON_NONE,
+		.name = "NONE",
+	},
 
 	/* USB external connector */
-	[EXTCON_USB]			= "USB",
-	[EXTCON_USB_HOST]		= "USB-HOST",
+	[EXTCON_USB] = {
+		.type = EXTCON_TYPE_USB,
+		.id = EXTCON_USB,
+		.name = "USB",
+	},
+	[EXTCON_USB_HOST] = {
+		.type = EXTCON_TYPE_USB,
+		.id = EXTCON_USB,
+		.name = "USB_HOST",
+	},
 
 	/* Charging external connector */
-	[EXTCON_CHG_USB_SDP]		= "SDP",
-	[EXTCON_CHG_USB_DCP]		= "DCP",
-	[EXTCON_CHG_USB_CDP]		= "CDP",
-	[EXTCON_CHG_USB_ACA]		= "ACA",
-	[EXTCON_CHG_USB_FAST]		= "FAST-CHARGER",
-	[EXTCON_CHG_USB_SLOW]		= "SLOW-CHARGER",
+	[EXTCON_CHG_USB_SDP] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_SDP,
+		.name = "SDP",
+	},
+	[EXTCON_CHG_USB_DCP] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_DCP,
+		.name = "DCP",
+	},
+	[EXTCON_CHG_USB_CDP] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_CDP,
+		.name = "CDP",
+	},
+	[EXTCON_CHG_USB_ACA] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_ACA,
+		.name = "ACA",
+	},
+	[EXTCON_CHG_USB_FAST] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_FAST,
+		.name = "FAST-CHARGER",
+	},
+	[EXTCON_CHG_USB_SLOW] = {
+		.type = EXTCON_TYPE_CHG | EXTCON_TYPE_USB,
+		.id = EXTCON_CHG_USB_SLOW,
+		.name = "SLOW-CHARGER",
+	},
 
 	/* Jack external connector */
-	[EXTCON_JACK_MICROPHONE]	= "MICROPHONE",
-	[EXTCON_JACK_HEADPHONE]		= "HEADPHONE",
-	[EXTCON_JACK_LINE_IN]		= "LINE-IN",
-	[EXTCON_JACK_LINE_OUT]		= "LINE-OUT",
-	[EXTCON_JACK_VIDEO_IN]		= "VIDEO-IN",
-	[EXTCON_JACK_VIDEO_OUT]		= "VIDEO-OUT",
-	[EXTCON_JACK_SPDIF_IN]		= "SPDIF-IN",
-	[EXTCON_JACK_SPDIF_OUT]		= "SPDIF-OUT",
+	[EXTCON_JACK_MICROPHONE] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_MICROPHONE,
+		.name = "MICROPHONE",
+	},
+	[EXTCON_JACK_HEADPHONE] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_HEADPHONE,
+		.name = "HEADPHONE",
+	},
+	[EXTCON_JACK_LINE_IN] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_LINE_IN,
+		.name = "LINE-IN",
+	},
+	[EXTCON_JACK_LINE_OUT] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_LINE_OUT,
+		.name = "LINE-OUT",
+	},
+	[EXTCON_JACK_VIDEO_IN] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_VIDEO_IN,
+		.name = "VIDEO-IN",
+	},
+	[EXTCON_JACK_VIDEO_OUT] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_VIDEO_OUT,
+		.name = "VIDEO-OUT",
+	},
+	[EXTCON_JACK_SPDIF_IN] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_SPDIF_IN,
+		.name = "SPDIF-IN",
+	},
+	[EXTCON_JACK_SPDIF_OUT] = {
+		.type = EXTCON_TYPE_JACK,
+		.id = EXTCON_JACK_SPDIF_OUT,
+		.name = "SPDIF-OUT",
+	},
 
 	/* Display external connector */
-	[EXTCON_DISP_HDMI]		= "HDMI",
-	[EXTCON_DISP_MHL]		= "MHL",
-	[EXTCON_DISP_DVI]		= "DVI",
-	[EXTCON_DISP_VGA]		= "VGA",
+	[EXTCON_DISP_HDMI] = {
+		.type = EXTCON_TYPE_DISP,
+		.id = EXTCON_DISP_HDMI,
+		.name = "HDMI",
+	},
+	[EXTCON_DISP_MHL] = {
+		.type = EXTCON_TYPE_DISP,
+		.id = EXTCON_DISP_MHL,
+		.name = "MHL",
+	},
+	[EXTCON_DISP_DVI] = {
+		.type = EXTCON_TYPE_DISP,
+		.id = EXTCON_DISP_DVI,
+		.name = "DVI",
+	},
+	[EXTCON_DISP_VGA] = {
+		.type = EXTCON_TYPE_DISP,
+		.id = EXTCON_DISP_VGA,
+		.name = "VGA",
+	},
 
 	/* Miscellaneous external connector */
-	[EXTCON_DOCK]			= "DOCK",
-	[EXTCON_JIG]			= "JIG",
-	[EXTCON_MECHANICAL]		= "MECHANICAL",
-
-	NULL,
+	[EXTCON_DOCK] = {
+		.type = EXTCON_TYPE_MISC,
+		.id = EXTCON_DOCK,
+		.name = "DOCK",
+	},
+	[EXTCON_JIG] = {
+		.type = EXTCON_TYPE_MISC,
+		.id = EXTCON_JIG,
+		.name = "JIG",
+	},
+	[EXTCON_MECHANICAL] = {
+		.type = EXTCON_TYPE_MISC,
+		.id = EXTCON_MECHANICAL,
+		.name = "MECHANICAL",
+	},
+
+	{ /* sentinel */ }
 };
 
 /**
@@ -168,7 +269,7 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 
 	for (i = 0; i < edev->max_supported; i++) {
 		count += sprintf(buf + count, "%s=%d\n",
-				extcon_name[edev->supported_cable[i]],
+				extcon_info[edev->supported_cable[i]].name,
 				 !!(edev->state & (1 << i)));
 	}
 
@@ -193,7 +294,7 @@ static ssize_t cable_name_show(struct device *dev,
 	int i = cable->cable_index;
 
 	return sprintf(buf, "%s\n",
-			extcon_name[cable->edev->supported_cable[i]]);
+			extcon_info[cable->edev->supported_cable[i]].name);
 }
 
 static ssize_t cable_state_show(struct device *dev,
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 667b1d35af12..46d802892c82 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -29,6 +29,15 @@
 #include <linux/device.h>
 
 /*
+ * Define the type of supported external connectors
+ */
+#define EXTCON_TYPE_USB		BIT(0)	/* USB connector */
+#define EXTCON_TYPE_CHG		BIT(1)	/* Charger connector */
+#define EXTCON_TYPE_JACK	BIT(2)	/* Jack connector */
+#define EXTCON_TYPE_DISP	BIT(3)	/* Display connector */
+#define EXTCON_TYPE_MISC	BIT(4)	/* Miscellaneous connector */
+
+/*
  * Define the unique id of supported external connectors
  */
 #define EXTCON_NONE		0
-- 
1.9.1

[PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi]

This patch support the extcon property for the external connector
because each external connector might have the property according to
the H/W design and the specific characteristics.

- EXTCON_PROP_USB_[property name]
- EXTCON_PROP_CHG_[property name]
- EXTCON_PROP_JACK_[property name]
- EXTCON_PROP_DISP_[property name]

Add the new extcon APIs to get/set the property value as following:
- int extcon_get_property(struct extcon_dev *edev, unsigned int id,
			unsigned int prop,
			union extcon_property_value *prop_val)
- int extcon_set_property(struct extcon_dev *edev, unsigned int id,
			unsigned int prop,
			union extcon_property_value prop_val)

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h  |  86 +++++++++++++++++++++
 2 files changed, 286 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 129afc87313e..bb6e99fe84c8 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -196,6 +196,11 @@ struct extcon_cable {
 	struct device_attribute attr_state;
 
 	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
+
+	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
+	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
+	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
+	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
 };
 
 static struct class *extcon_class;
@@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
 	return -EINVAL;
 }
 
+static int get_extcon_type(unsigned int prop)
+{
+	switch (prop) {
+	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
+		return EXTCON_TYPE_USB;
+	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
+		return EXTCON_TYPE_CHG;
+	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
+		return EXTCON_TYPE_JACK;
+	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
+		return EXTCON_TYPE_DISP;
+	default:
+		return -EINVAL;
+	}
+}
+
+static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
+{
+	return !!(edev->state & BIT(index));
+}
+
 static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
 {
 	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
@@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
 	return false;
 }
 
+static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
+{
+	int type;
+
+	/* Check whether the property is supported or not. */
+	type = get_extcon_type(prop);
+	if (type < 0)
+		return false;
+
+	/* Check whether a specific extcon id supports the property or not. */
+	return !!(extcon_info[id].type & type);
+}
+
+static void init_property(struct extcon_dev *edev, unsigned int id, int index)
+{
+	unsigned int type = extcon_info[id].type;
+	struct extcon_cable *cable = &edev->cables[index];
+
+	if (EXTCON_TYPE_USB & type)
+		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
+	if (EXTCON_TYPE_CHG & type)
+		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
+	if (EXTCON_TYPE_JACK & type)
+		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
+	if (EXTCON_TYPE_DISP & type)
+		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
+}
+
 static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
@@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
 	if (edev->max_supported && edev->max_supported <= index)
 		return -EINVAL;
 
-	return !!(edev->state & (1 << index));
+	return is_extcon_attached(edev, index);
 }
 EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
 
@@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
 	if (edev->max_supported && edev->max_supported <= index)
 		return -EINVAL;
 
+	/*
+	 * Initialize the value of extcon property before setting
+	 * the detached state for an external connector.
+	 */
+	if (!cable_state)
+		init_property(edev, id, index);
+
 	state = cable_state ? (1 << index) : 0;
 	return extcon_update_state(edev, 1 << index, state);
 }
 EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
 
 /**
+ * extcon_get_property() - Get the property value of a specific cable.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector
+ *			in extcon enumeration.
+ * @prop:		the property id among enum extcon_property.
+ * @prop_val:		the pointer which store the value of property.
+ *
+ * When getting the property value of external connector, the external connector
+ * should be attached. If detached state, function just return 0 without
+ * property value. Also, the each property should be included in the list of
+ * supported properties according to the type of external connectors.
+ *
+ * Returns 0 if success or error number if fail
+ */
+int extcon_get_property(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value *prop_val)
+{
+	struct extcon_cable *cable;
+	unsigned long flags;
+	int index, ret = 0;
+
+	*prop_val = (union extcon_property_value)(0);
+
+	if (!edev)
+		return -EINVAL;
+
+	/* Check whether the property is supported or not */
+	if (!is_extcon_property_supported(id, prop))
+		return -EINVAL;
+
+	/* Find the cable index of external connector by using id */
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	spin_lock_irqsave(&edev->lock, flags);
+
+	/*
+	 * Check whether the external connector is attached.
+	 * If external connector is detached, the user can not
+	 * get the property value.
+	 */
+	if (!is_extcon_attached(edev, index)) {
+		spin_unlock_irqrestore(&edev->lock, flags);
+		return 0;
+	}
+
+	cable = &edev->cables[index];
+
+	/* Get the property value according to extcon type */
+	switch (prop) {
+	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
+		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
+		break;
+	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
+		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
+		break;
+	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
+		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
+		break;
+	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
+		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_get_property);
+
+/**
+ * extcon_set_property() - Set the property value of a specific cable.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector
+ *			in extcon enumeration.
+ * @prop:		the property id among enum extcon_property.
+ * @prop_val:		the pointer including the new value of property.
+ *
+ * The each property should be included in the list of supported properties
+ * according to the type of external connectors.
+ *
+ * Returns 0 if success or error number if fail
+ */
+int extcon_set_property(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value prop_val)
+{
+	struct extcon_cable *cable;
+	unsigned long flags;
+	int index, ret = 0;
+
+	if (!edev)
+		return -EINVAL;
+
+	/* Check whether the property is supported or not */
+	if (!is_extcon_property_supported(id, prop))
+		return -EINVAL;
+
+	/* Find the cable index of external connector by using id */
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	spin_lock_irqsave(&edev->lock, flags);
+
+	cable = &edev->cables[index];
+
+	/* Set the property value according to extcon type */
+	switch (prop) {
+	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
+		cable->usb_propval[prop - EXTCON_PROP_USB_MIN] = prop_val;
+		break;
+	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
+		cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] = prop_val;
+		break;
+	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
+		cable->jack_propval[prop - EXTCON_PROP_JACK_MIN] = prop_val;
+		break;
+	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
+		cable->disp_propval[prop - EXTCON_PROP_DISP_MIN] = prop_val;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_set_property);
+
+/**
  * extcon_get_extcon_dev() - Get the extcon device instance from the name
  * @extcon_name:	The extcon name provided with extcon_dev_register()
  */
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 46d802892c82..fe583fca7732 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -77,6 +77,68 @@
 
 #define EXTCON_NUM		63
 
+/*
+ * Define the property of supported external connectors.
+ *
+ * When adding the new extcon property, they *must* have
+ * the type/value/default information. Also, you *have to*
+ * modify the EXTCON_PROP_[type]_START/END definitions
+ * which mean the range of the supported properties
+ * for each extcon type.
+ *
+ * The naming style of property
+ * : EXTCON_PROP_[type]_[property name]
+ *
+ * EXTCON_PROP_USB_[property name]	: USB property
+ * EXTCON_PROP_CHG_[property name]	: Charger property
+ * EXTCON_PROP_JACK_[property name]	: Jack property
+ * EXTCON_PROP_DISP_[property name]	: Display property
+ */
+
+/*
+ * Properties of EXTCON_TYPE_USB.
+ *
+ * - EXTCON_PROP_USB_ID
+ * @type:	integer (intval)
+ * @value:	0 (low) or 1 (high)
+ * @default:	0 (low)
+ * - EXTCON_PROP_USB_VBUS
+ * @type:	integer (intval)
+ * @value:	0 (low) or 1 (high)
+ * @default:	0 (low)
+ */
+#define EXTCON_PROP_USB_ID		0
+#define EXTCON_PROP_USB_VBUS		1
+
+#define EXTCON_PROP_USB_MIN		0
+#define EXTCON_PROP_USB_MAX		1
+#define EXTCON_PROP_USB_CNT	(EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1)
+
+/* Properties of EXTCON_TYPE_CHG. */
+#define EXTCON_PROP_CHG_MIN		50
+#define EXTCON_PROP_CHG_MAX		50
+#define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1)
+
+/* Properties of EXTCON_TYPE_JACK. */
+#define EXTCON_PROP_JACK_MIN		100
+#define EXTCON_PROP_JACK_MAX		100
+#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX - EXTCON_PROP_JACK_MIN + 1)
+
+/* Properties of EXTCON_TYPE_DISP. */
+#define EXTCON_PROP_DISP_MIN		150
+#define EXTCON_PROP_DISP_MAX		150
+#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX - EXTCON_PROP_DISP_MIN + 1)
+
+/*
+ * Define the type of property's value.
+ *
+ * Define the property's value as union type. Because each property
+ * would need the different data type to store it.
+ */
+union extcon_property_value {
+	int intval;	/* type : integer (intval) */
+};
+
 struct extcon_cable;
 
 /**
@@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
 				   bool cable_state);
 
 /*
+ * get/set_property access the property value of each external connector.
+ * They are used to access the property of each cable based on the property id.
+ */
+extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value *prop_val);
+extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value prop_val);
+
+/*
  * Following APIs are to monitor every action of a notifier.
  * Registrar gets notified for every external port of a connection device.
  * Probably this could be used to debug an action of notifier; however,
@@ -239,6 +312,19 @@ static inline int extcon_set_cable_state_(struct extcon_dev *edev,
 	return 0;
 }
 
+static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
+					unsigned int prop,
+					union extcon_property_value *prop_val)
+{
+	return 0;
+}
+static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
+					unsigned int prop,
+					union extcon_property_value prop_val)
+{
+	return 0;
+}
+
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
 	return NULL;
-- 
1.9.1

[PATCH v3 3/6] extcon: Add the support for the capability of each property

[Chanwoo Choi]

This patch adds the support of the property capability setting. This function
decides the supported properties of each external connector on extcon provider
driver.

Ths list of new extcon APIs to get/set the capability of property as following:
- int extcon_get_property_capability(struct extcon_dev *edev,
					unsigned int id, unsigned int prop);
- int extcon_set_property_capability(struct extcon_dev *edev,
					unsigned int id, unsigned int prop);

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/extcon.h  |  22 ++++++++
 2 files changed, 163 insertions(+)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index bb6e99fe84c8..7ae0771f29e7 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -201,6 +201,11 @@ struct extcon_cable {
 	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
 	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
 	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
+
+	unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)];
+	unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)];
+	unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)];
+	unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)];
 };
 
 static struct class *extcon_class;
@@ -297,6 +302,39 @@ static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
 	return !!(extcon_info[id].type & type);
 }
 
+static int is_extcon_property_capability(struct extcon_dev *edev,
+				unsigned int id, int index,unsigned int prop)
+{
+	struct extcon_cable *cable;
+	int type, ret;
+
+	/* Check whether the property is supported or not. */
+	type = get_extcon_type(prop);
+	if (type < 0)
+		return type;
+
+	cable = &edev->cables[index];
+
+	switch (type) {
+	case EXTCON_TYPE_USB:
+		ret = test_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);
+		break;
+	case EXTCON_TYPE_CHG:
+		ret = test_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
+		break;
+	case EXTCON_TYPE_JACK:
+		ret = test_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
+		break;
+	case EXTCON_TYPE_DISP:
+		ret = test_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void init_property(struct extcon_dev *edev, unsigned int id, int index)
 {
 	unsigned int type = extcon_info[id].type;
@@ -554,6 +592,12 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id,
 
 	spin_lock_irqsave(&edev->lock, flags);
 
+	/* Check whether the property is available or not. */
+	if (!is_extcon_property_capability(edev, id, index, prop)) {
+		spin_unlock_irqrestore(&edev->lock, flags);
+		return -EPERM;
+	}
+
 	/*
 	 * Check whether the external connector is attached.
 	 * If external connector is detached, the user can not
@@ -626,6 +670,12 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 
 	spin_lock_irqsave(&edev->lock, flags);
 
+	/* Check whether the property is available or not. */
+	if (!is_extcon_property_capability(edev, id, index, prop)) {
+		spin_unlock_irqrestore(&edev->lock, flags);
+		return -EPERM;
+	}
+
 	cable = &edev->cables[index];
 
 	/* Set the property value according to extcon type */
@@ -654,6 +704,97 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 EXPORT_SYMBOL_GPL(extcon_set_property);
 
 /**
+ * extcon_get_property_capability() - Get the capability of property
+ *			of an external connector.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector
+ *			in extcon enumeration.
+ * @prop:		the property id among enum extcon_property.
+ *
+ * Returns 1 if the property is available or 0 if not available.
+ */
+int extcon_get_property_capability(struct extcon_dev *edev, unsigned int id,
+					unsigned int prop)
+{
+	int index;
+
+	if (!edev)
+		return -EINVAL;
+
+	/* Check whether the property is supported or not */
+	if (!is_extcon_property_supported(id, prop))
+		return -EINVAL;
+
+	/* Find the cable index of external connector by using id */
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	return is_extcon_property_capability(edev, id, index, prop);
+}
+EXPORT_SYMBOL_GPL(extcon_get_property_capability);
+
+/**
+ * extcon_set_property_capability() - Set the capability of a property
+ *			of an external connector.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector
+ *			in extcon enumeration.
+ * @prop:		the property id among enum extcon_property.
+ *
+ * This function set the capability of a property for an external connector
+ * to mark the bit in capability bitmap which mean the available state of
+ * a property.
+ *
+ * Returns 0 if success or error number if fail
+ */
+int extcon_set_property_capability(struct extcon_dev *edev, unsigned int id,
+					unsigned int prop)
+{
+	struct extcon_cable *cable;
+	int index, type, ret = 0;
+
+	if (!edev)
+		return -EINVAL;
+
+	/* Check whether the property is supported or not. */
+	if (!is_extcon_property_supported(id, prop))
+		return -EINVAL;
+
+	/* Find the cable index of external connector by using id. */
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	/* Check whether the property is supported or not. */
+	type = get_extcon_type(prop);
+	if (type < 0)
+		return type;
+
+	cable = &edev->cables[index];
+
+	switch (type) {
+	case EXTCON_TYPE_USB:
+		__set_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);
+		break;
+	case EXTCON_TYPE_CHG:
+		__set_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
+		break;
+	case EXTCON_TYPE_JACK:
+		__set_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
+		break;
+	case EXTCON_TYPE_DISP:
+		__set_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_set_property_capability);
+
+/**
  * extcon_get_extcon_dev() - Get the extcon device instance from the name
  * @extcon_name:	The extcon name provided with extcon_dev_register()
  */
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index fe583fca7732..ffd32a8975d5 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -240,6 +240,16 @@ extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 				union extcon_property_value prop_val);
 
 /*
+ * get/set_property_capability set the capability of the property for each
+ * external connector. They are used to set the capability of the property
+ * of each external connector based on the id and property.
+ */
+extern int extcon_get_property_capability(struct extcon_dev *edev,
+				unsigned int id, unsigned int prop);
+extern int extcon_set_property_capability(struct extcon_dev *edev,
+				unsigned int id, unsigned int prop);
+
+/*
  * Following APIs are to monitor every action of a notifier.
  * Registrar gets notified for every external port of a connection device.
  * Probably this could be used to debug an action of notifier; however,
@@ -325,6 +335,18 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 	return 0;
 }
 
+static inline int extcon_get_property_capability(struct extcon_dev *edev,
+					unsigned int id, unsigned int prop)
+{
+	return 0;
+}
+
+static inline int extcon_set_property_capability(struct extcon_dev *edev,
+					unsigned int id, unsigned int prop)
+{
+	return 0;
+}
+
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
 	return NULL;
-- 
1.9.1

[PATCH v3 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern

[Chanwoo Choi]

This patch just renames the existing extcon_get/set_cable_state_()
as following because of maintaining the function naming pattern
like as extcon APIs for property.
- extcon_set_cable_state_() -> extcon_set_state()
- extcon_get_cable_state_() -> extcon_get_state()

But, this patch remains the old extcon_set/get_cable_state_() functions
to prevent the build break. After altering new APIs, remove the old APIs.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 28 +++++++++++++---------------
 include/linux/extcon.h  | 25 ++++++++++++++++++-------
 2 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 7ae0771f29e7..aaf28bc9e4da 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -398,8 +398,7 @@ static ssize_t cable_state_show(struct device *dev,
 	int i = cable->cable_index;
 
 	return sprintf(buf, "%d\n",
-		       extcon_get_cable_state_(cable->edev,
-					       cable->edev->supported_cable[i]));
+		extcon_get_state(cable->edev, cable->edev->supported_cable[i]));
 }
 
 /**
@@ -495,13 +494,14 @@ static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 }
 
 /**
- * extcon_get_cable_state_() - Get the status of a specific cable.
+ * extcon_get_state() - Get the state of a external connector.
  * @edev:	the extcon device that has the cable.
  * @id:		the unique id of each external connector in extcon enumeration.
  */
-int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
+int extcon_get_state(struct extcon_dev *edev, const unsigned int id)
 {
-	int index;
+	int index, state;
+	unsigned long flags;
 
 	if (!edev)
 		return -EINVAL;
@@ -510,22 +510,23 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
 	if (index < 0)
 		return index;
 
-	if (edev->max_supported && edev->max_supported <= index)
-		return -EINVAL;
+	spin_lock_irqsave(&edev->lock, flags);
+	state = is_extcon_attached(edev, index);
+	spin_unlock_irqrestore(&edev->lock, flags);
 
-	return is_extcon_attached(edev, index);
+	return state;
 }
-EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
+EXPORT_SYMBOL_GPL(extcon_get_state);
 
 /**
- * extcon_set_cable_state_() - Set the status of a specific cable.
+ * extcon_set_state() - Set the state of a external connector.
  * @edev:		the extcon device that has the cable.
  * @id:			the unique id of each external connector
  *			in extcon enumeration.
  * @state:		the new cable status. The default semantics is
  *			true: attached / false: detached.
  */
-int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
+int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 				bool cable_state)
 {
 	u32 state;
@@ -538,9 +539,6 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
 	if (index < 0)
 		return index;
 
-	if (edev->max_supported && edev->max_supported <= index)
-		return -EINVAL;
-
 	/*
 	 * Initialize the value of extcon property before setting
 	 * the detached state for an external connector.
@@ -551,7 +549,7 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
 	state = cable_state ? (1 << index) : 0;
 	return extcon_update_state(edev, 1 << index, state);
 }
-EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
+EXPORT_SYMBOL_GPL(extcon_set_state);
 
 /**
  * extcon_get_property() - Get the property value of a specific cable.
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index ffd32a8975d5..4e7664378d5b 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -221,11 +221,11 @@ extern struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
 extern void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
 
 /*
- * get/set_cable_state access each bit of the 32b encoded state value.
+ * get/set_state access each bit of the 32b encoded state value.
  * They are used to access the status of each cable based on the cable id.
  */
-extern int extcon_get_cable_state_(struct extcon_dev *edev, unsigned int id);
-extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
+extern int extcon_get_state(struct extcon_dev *edev, unsigned int id);
+extern int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 				   bool cable_state);
 
 /*
@@ -310,14 +310,14 @@ static inline struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
 
 static inline void devm_extcon_dev_free(struct extcon_dev *edev) { }
 
-static inline int extcon_get_cable_state_(struct extcon_dev *edev,
-					  unsigned int id)
+
+static inline int extcon_get_state(struct extcon_dev *edev, unsigned int id)
 {
 	return 0;
 }
 
-static inline int extcon_set_cable_state_(struct extcon_dev *edev,
-					  unsigned int id, bool cable_state)
+static inline int extcon_set_state(struct extcon_dev *edev, unsigned int id,
+				bool cable_state)
 {
 	return 0;
 }
@@ -407,4 +407,15 @@ static inline int extcon_unregister_interest(struct extcon_specific_cable_nb
 {
 	return -EINVAL;
 }
+
+static inline int extcon_get_cable_state_(struct extcon_dev *edev, unsigned int id)
+{
+	return extcon_get_state(edev, id);
+}
+
+static inline int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
+				   bool cable_state)
+{
+	return extcon_set_state(edev, id, cable_state);
+}
 #endif /* __LINUX_EXTCON_H__ */
-- 
1.9.1

[PATCH v3 5/6] extcon: Add the synchronization extcon APIs to support the notification

[Chanwoo Choi]

This patch adds the synchronization extcon APIs to support the notifications
for both state and property. When extcon_*_sync() functions is called,
the extcon informs the information from extcon provider to extcon client.

The extcon driver may need to change the both state and multiple properties
at the same time. After setting the data of a external connector,
the extcon send the notification to client driver with the extcon_*_sync().

The list of new extcon APIs as following:
- extcon_sync() : Send the notification for each external connector to
		synchronize the information between extcon provider driver
		and extcon client driver.
- extcon_set_state_sync() : Set the state of external connector with noti.
- extcon_set_property_sync() : Set the property of external connector with noti.

For example,
case 1, change the state of external connector and synchronized the data.
	extcon_set_state_sync(edev, EXTCON_USB, 1);

case 2, change both the state and property of external connector
	and synchronized the data.
	extcon_set_state(edev, EXTCON_USB, 1);
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
	extcon_sync(edev, EXTCON_USB);

case 3, change the property of external connector and synchronized the data.
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
	extcon_sync(edev, EXTCON_USB);

case 4, change the property of external connector and synchronized the data.
	extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 210 +++++++++++++++++++++++++++++++-----------------
 include/linux/extcon.h  |  30 ++++++-
 2 files changed, 164 insertions(+), 76 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index aaf28bc9e4da..b9290ab90fcd 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -279,14 +279,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
 	return !!(edev->state & BIT(index));
 }
 
-static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
+static bool is_extcon_changed(struct extcon_dev *edev, int index,
+				bool new_state)
 {
-	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
-		*attached = ((new >> idx) & 0x1) ? true : false;
-		return true;
-	}
-
-	return false;
+	int state = !!(edev->state & BIT(index));
+	return (state != new_state);
 }
 
 static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
@@ -402,21 +399,13 @@ static ssize_t cable_state_show(struct device *dev,
 }
 
 /**
- * extcon_update_state() - Update the cable attach states of the extcon device
- *			   only for the masked bits.
- * @edev:	the extcon device
- * @mask:	the bit mask to designate updated bits.
- * @state:	new cable attach status for @edev
- *
- * Changing the state sends uevent with environment variable containing
- * the name of extcon device (envp[0]) and the state output (envp[1]).
- * Tizen uses this format for extcon device to get events from ports.
- * Android uses this format as well.
+ * extcon_sync()	- Synchronize the states for both the attached/detached
+ * @edev:		the extcon device that has the cable.
  *
- * Note that the notifier provides which bits are changed in the state
- * variable with the val parameter (second) to the callback.
+ * This function send a notification to synchronize the all states of a
+ * specific external connector
  */
-static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
+int extcon_sync(struct extcon_dev *edev, unsigned int id)
 {
 	char name_buf[120];
 	char state_buf[120];
@@ -425,73 +414,58 @@ static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 	int env_offset = 0;
 	int length;
 	int index;
+	int state;
 	unsigned long flags;
-	bool attached;
 
 	if (!edev)
 		return -EINVAL;
 
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
 	spin_lock_irqsave(&edev->lock, flags);
 
-	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
-		u32 old_state;
+	state = !!(edev->state & BIT(index));
+	raw_notifier_call_chain(&edev->nh[index], state, edev);
 
-		if (check_mutually_exclusive(edev, (edev->state & ~mask) |
-						   (state & mask))) {
-			spin_unlock_irqrestore(&edev->lock, flags);
-			return -EPERM;
-		}
+	/* This could be in interrupt handler */
+	prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
+	if (!prop_buf) {
+		/* Unlock early before uevent */
+		spin_unlock_irqrestore(&edev->lock, flags);
 
-		old_state = edev->state;
-		edev->state &= ~mask;
-		edev->state |= state & mask;
+		dev_err(&edev->dev, "out of memory in extcon_set_state\n");
+		kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
 
-		for (index = 0; index < edev->max_supported; index++) {
-			if (is_extcon_changed(old_state, edev->state, index,
-					      &attached))
-				raw_notifier_call_chain(&edev->nh[index],
-							attached, edev);
-		}
-
-		/* This could be in interrupt handler */
-		prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
-		if (prop_buf) {
-			length = name_show(&edev->dev, NULL, prop_buf);
-			if (length > 0) {
-				if (prop_buf[length - 1] == '\n')
-					prop_buf[length - 1] = 0;
-				snprintf(name_buf, sizeof(name_buf),
-					"NAME=%s", prop_buf);
-				envp[env_offset++] = name_buf;
-			}
-			length = state_show(&edev->dev, NULL, prop_buf);
-			if (length > 0) {
-				if (prop_buf[length - 1] == '\n')
-					prop_buf[length - 1] = 0;
-				snprintf(state_buf, sizeof(state_buf),
-					"STATE=%s", prop_buf);
-				envp[env_offset++] = state_buf;
-			}
-			envp[env_offset] = NULL;
-			/* Unlock early before uevent */
-			spin_unlock_irqrestore(&edev->lock, flags);
+		return 0;
+	}
 
-			kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
-			free_page((unsigned long)prop_buf);
-		} else {
-			/* Unlock early before uevent */
-			spin_unlock_irqrestore(&edev->lock, flags);
+	length = name_show(&edev->dev, NULL, prop_buf);
+	if (length > 0) {
+		if (prop_buf[length - 1] == '\n')
+			prop_buf[length - 1] = 0;
+		snprintf(name_buf, sizeof(name_buf), "NAME=%s", prop_buf);
+		envp[env_offset++] = name_buf;
+	}
 
-			dev_err(&edev->dev, "out of memory in extcon_set_state\n");
-			kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
-		}
-	} else {
-		/* No changes */
-		spin_unlock_irqrestore(&edev->lock, flags);
+	length = state_show(&edev->dev, NULL, prop_buf);
+	if (length > 0) {
+		if (prop_buf[length - 1] == '\n')
+			prop_buf[length - 1] = 0;
+		snprintf(state_buf, sizeof(state_buf), "STATE=%s", prop_buf);
+		envp[env_offset++] = state_buf;
 	}
+	envp[env_offset] = NULL;
+
+	/* Unlock early before uevent */
+	spin_unlock_irqrestore(&edev->lock, flags);
+	kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
+	free_page((unsigned long)prop_buf);
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(extcon_sync);
 
 /**
  * extcon_get_state() - Get the state of a external connector.
@@ -520,17 +494,22 @@ EXPORT_SYMBOL_GPL(extcon_get_state);
 
 /**
  * extcon_set_state() - Set the state of a external connector.
+ *			without a notification.
  * @edev:		the extcon device that has the cable.
  * @id:			the unique id of each external connector
  *			in extcon enumeration.
  * @state:		the new cable status. The default semantics is
  *			true: attached / false: detached.
+ *
+ * This function only set the state of a external connector without
+ * a notification. To synchronize the data of a external connector,
+ * use extcon_set_state_sync() and extcon_sync().
  */
 int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 				bool cable_state)
 {
-	u32 state;
-	int index;
+	unsigned long flags;
+	int index, ret = 0;
 
 	if (!edev)
 		return -EINVAL;
@@ -539,6 +518,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 	if (index < 0)
 		return index;
 
+	spin_lock_irqsave(&edev->lock, flags);
+
+	/* Check whether the external connector's state is changed. */
+	if (!is_extcon_changed(edev, index, cable_state))
+		goto out;
+
+	if (check_mutually_exclusive(edev,
+		(edev->state & ~BIT(index)) | (cable_state & BIT(index)))) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	/*
 	 * Initialize the value of extcon property before setting
 	 * the detached state for an external connector.
@@ -546,12 +537,56 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 	if (!cable_state)
 		init_property(edev, id, index);
 
-	state = cable_state ? (1 << index) : 0;
-	return extcon_update_state(edev, 1 << index, state);
+	/* Update the state for a external connector. */
+	if (cable_state)
+		edev->state |= BIT(index);
+	else
+		edev->state &= ~(BIT(index));
+out:
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(extcon_set_state);
 
 /**
+ * extcon_set_state_sync() - Set the state of a external connector
+ *			with a notification.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector
+ *			in extcon enumeration.
+ * @state:		the new cable status. The default semantics is
+ *			true: attached / false: detached.
+ *
+ * This function set the state of external connector and synchronize the data
+ * by usning a notification.
+ */
+int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
+				bool cable_state)
+{
+	int ret, index;
+	unsigned long flags;
+
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	/* Check whether the external connector's state is changed. */
+	spin_lock_irqsave(&edev->lock, flags);
+	ret = is_extcon_changed(edev, index, cable_state);
+	spin_unlock_irqrestore(&edev->lock, flags);
+	if (!ret)
+		return 0;
+
+	ret = extcon_set_state(edev, id, cable_state);
+	if (ret < 0)
+		return ret;
+
+	return extcon_sync(edev, id);
+}
+EXPORT_SYMBOL_GPL(extcon_set_state_sync);
+
+/**
  * extcon_get_property() - Get the property value of a specific cable.
  * @edev:		the extcon device that has the cable.
  * @id:			the unique id of each external connector
@@ -702,6 +737,31 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 EXPORT_SYMBOL_GPL(extcon_set_property);
 
 /**
+ * extcon_set_property_sync() - Set the property value of a specific cable
+			with a notification.
+ * @prop_val:		the pointer including the new value of property.
+ *
+ * When setting the property value of external connector, the external connector
+ * should be attached. The each property should be included in the list of
+ * supported properties according to the type of external connectors.
+ *
+ * Returns 0 if success or error number if fail
+ */
+int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value prop_val)
+{
+	int ret;
+
+	ret = extcon_set_property(edev, id, prop, prop_val);
+	if (ret < 0)
+		return ret;
+
+	return extcon_sync(edev, id);
+}
+EXPORT_SYMBOL_GPL(extcon_set_property_sync);
+
+/**
  * extcon_get_property_capability() - Get the capability of property
  *			of an external connector.
  * @edev:		the extcon device that has the cable.
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 4e7664378d5b..664db79940b4 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -227,6 +227,13 @@ extern void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
 extern int extcon_get_state(struct extcon_dev *edev, unsigned int id);
 extern int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 				   bool cable_state);
+extern int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
+				bool cable_state);
+
+/*
+ * Synchronize the state and property data for a specific external connector.
+ */
+extern int extcon_sync(struct extcon_dev *edev, unsigned int id);
 
 /*
  * get/set_property access the property value of each external connector.
@@ -238,6 +245,9 @@ extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
 extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 				unsigned int prop,
 				union extcon_property_value prop_val);
+extern int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
+				unsigned int prop,
+				union extcon_property_value prop_val);
 
 /*
  * get/set_property_capability set the capability of the property for each
@@ -322,6 +332,17 @@ static inline int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 	return 0;
 }
 
+static inline int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
+				bool cable_state)
+{
+	return 0;
+}
+
+static inline int extcon_sync(struct extcon_dev *edev, unsigned int id)
+{
+	return 0;
+}
+
 static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
 					unsigned int prop,
 					union extcon_property_value *prop_val)
@@ -335,6 +356,13 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 	return 0;
 }
 
+static inline int extcon_set_property_sync(struct extcon_dev *edev,
+					unsigned int id, unsigned int prop,
+					union extcon_property_value prop_val)
+{
+	return 0;
+}
+
 static inline int extcon_get_property_capability(struct extcon_dev *edev,
 					unsigned int id, unsigned int prop)
 {
@@ -416,6 +444,6 @@ static inline int extcon_get_cable_state_(struct extcon_dev *edev, unsigned int
 static inline int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
 				   bool cable_state)
 {
-	return extcon_set_state(edev, id, cable_state);
+	return extcon_set_state_sync(edev, id, cable_state);
 }
 #endif /* __LINUX_EXTCON_H__ */
-- 
1.9.1

[PATCH v3 6/6] extcon: Add EXTCON_DISP_DP and the property for USB Type-C

[Chanwoo Choi]

From: Chris Zhong <zyw@rock-chips.com>

Add EXTCON_DISP_DP for the Display external connector. For Type-C
connector the DisplayPort can work as an Alternate Mode(VESA DisplayPort
Alt Mode on USB Type-C Standard). The Type-C support both normal
and flipped orientation, so add a property to extcon.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Tested-by: Chris Zhong <zyw@rock-chips.com>
Tested-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
---
 drivers/extcon/extcon.c | 5 +++++
 include/linux/extcon.h  | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index b9290ab90fcd..4e7ef66543f6 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -157,6 +157,11 @@ struct __extcon_info {
 		.id = EXTCON_DISP_VGA,
 		.name = "VGA",
 	},
+	[EXTCON_DISP_DP] = {
+		.type = EXTCON_TYPE_DISP | EXTCON_TYPE_USB,
+		.id = EXTCON_DISP_DP,
+		.name = "DP",
+	},
 
 	/* Miscellaneous external connector */
 	[EXTCON_DOCK] = {
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 664db79940b4..950f8491fa6a 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -69,6 +69,7 @@
 #define EXTCON_DISP_MHL		41	/* Mobile High-Definition Link */
 #define EXTCON_DISP_DVI		42	/* Digital Visual Interface */
 #define EXTCON_DISP_VGA		43	/* Video Graphics Array */
+#define EXTCON_DISP_DP		44	/* Display Port */
 
 /* Miscellaneous external connector */
 #define EXTCON_DOCK		60
@@ -106,12 +107,17 @@
  * @type:	integer (intval)
  * @value:	0 (low) or 1 (high)
  * @default:	0 (low)
+ * - EXTCON_PROP_USB_TYPEC_POLARITY
+ * @type:	integer (intval)
+ * @value:	0 (normal) or 1 (flip)
+ * @default:	0 (normal)
  */
 #define EXTCON_PROP_USB_ID		0
 #define EXTCON_PROP_USB_VBUS		1
+#define EXTCON_PROP_USB_TYPEC_POLARITY	2
 
 #define EXTCON_PROP_USB_MIN		0
-#define EXTCON_PROP_USB_MAX		1
+#define EXTCON_PROP_USB_MAX		2
 #define EXTCON_PROP_USB_CNT	(EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1)
 
 /* Properties of EXTCON_TYPE_CHG. */
-- 
1.9.1

Re: [PATCH v3 1/6] extcon: Add the extcon_type to gather each connector into five category

[Roger Quadros]

Hi,

On 02/08/16 04:58, Chanwoo Choi wrote:
> This patch adds the new extcon type to group the each connecotr
> into following five category. This type would be used to handle
> the connectors as a group unit instead of a connector unit.
> - EXTCON_TYPE_USB  : USB connector
> - EXTCON_TYPE_CHG  : Charger connector
> - EXTCON_TYPE_JACK : Jack connector
> - EXTCON_TYPE_DISP : Display connector
> - EXTCON_TYPE_MISC : Miscellaneous connector
> 
> Also, each external connector is possible to belong to one more extcon type.
> In caes of EXTCON_CHG_USB_SDP, it have the EXTCON_TYPE_CHG and EXTCON_TYPE_USB.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Chris Zhong <zyw@rock-chips.com>
> Tested-by: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
>  drivers/extcon/extcon.c | 159 +++++++++++++++++++++++++++++++++++++++---------
>  include/linux/extcon.h  |   9 +++
>  2 files changed, 139 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 9a266e5c7e10..129afc87313e 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -38,43 +38,144 @@
>  #define SUPPORTED_CABLE_MAX	32
>  #define CABLE_NAME_MAX		30
>  
> -static const char *extcon_name[] =  {
> -	[EXTCON_NONE]			= "NONE",
> +struct __extcon_info {
> +	unsigned int type;
> +	unsigned int id;
> +	const char *name;
> +
> +} extcon_info[] = {
> +	[EXTCON_NONE] = {
> +		.type = EXTCON_TYPE_MISC,
> +		.id = EXTCON_NONE,
> +		.name = "NONE",
> +	},
>  
>  	/* USB external connector */
> -	[EXTCON_USB]			= "USB",
> -	[EXTCON_USB_HOST]		= "USB-HOST",
> +	[EXTCON_USB] = {
> +		.type = EXTCON_TYPE_USB,
> +		.id = EXTCON_USB,
> +		.name = "USB",
> +	},
> +	[EXTCON_USB_HOST] = {
> +		.type = EXTCON_TYPE_USB,
> +		.id = EXTCON_USB,
> +		.name = "USB_HOST",
> +	},

EXTCON_USB_HOST should now be redundant as we can get all the
necessary information via EXTCON_USB.

>  
>  	/* Charging external connector */


<snip>

cheers,
-roger

Re: [PATCH v3 1/6] extcon: Add the extcon_type to gather each connector into five category

[Chanwoo Choi]

Hi Roger,

On 2016년 08월 02일 16:27, Roger Quadros wrote:
> Hi,
> 
> On 02/08/16 04:58, Chanwoo Choi wrote:
>> This patch adds the new extcon type to group the each connecotr
>> into following five category. This type would be used to handle
>> the connectors as a group unit instead of a connector unit.
>> - EXTCON_TYPE_USB  : USB connector
>> - EXTCON_TYPE_CHG  : Charger connector
>> - EXTCON_TYPE_JACK : Jack connector
>> - EXTCON_TYPE_DISP : Display connector
>> - EXTCON_TYPE_MISC : Miscellaneous connector
>>
>> Also, each external connector is possible to belong to one more extcon type.
>> In caes of EXTCON_CHG_USB_SDP, it have the EXTCON_TYPE_CHG and EXTCON_TYPE_USB.
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>> Tested-by: Guenter Roeck <groeck@chromium.org>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>> ---
>>  drivers/extcon/extcon.c | 159 +++++++++++++++++++++++++++++++++++++++---------
>>  include/linux/extcon.h  |   9 +++
>>  2 files changed, 139 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 9a266e5c7e10..129afc87313e 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -38,43 +38,144 @@
>>  #define SUPPORTED_CABLE_MAX	32
>>  #define CABLE_NAME_MAX		30
>>  
>> -static const char *extcon_name[] =  {
>> -	[EXTCON_NONE]			= "NONE",
>> +struct __extcon_info {
>> +	unsigned int type;
>> +	unsigned int id;
>> +	const char *name;
>> +
>> +} extcon_info[] = {
>> +	[EXTCON_NONE] = {
>> +		.type = EXTCON_TYPE_MISC,
>> +		.id = EXTCON_NONE,
>> +		.name = "NONE",
>> +	},
>>  
>>  	/* USB external connector */
>> -	[EXTCON_USB]			= "USB",
>> -	[EXTCON_USB_HOST]		= "USB-HOST",
>> +	[EXTCON_USB] = {
>> +		.type = EXTCON_TYPE_USB,
>> +		.id = EXTCON_USB,
>> +		.name = "USB",
>> +	},
>> +	[EXTCON_USB_HOST] = {
>> +		.type = EXTCON_TYPE_USB,
>> +		.id = EXTCON_USB,
>> +		.name = "USB_HOST",
>> +	},
> 
> EXTCON_USB_HOST should now be redundant as we can get all the
> necessary information via EXTCON_USB.

It is my mistake. I'll fix it.
	.id = EXTCON_USB, -> .id = EXTCON_USB_HOST,

On the user-space, the process don't handle the ID and VBUS information directly.
they need the identical type of attached external connector.

For exmaple,
The extcon separate the following two case:
- mobile phone is connected to desktop pc.
- mobile phone is connected to mouse/keyboard with otg cable.

Regards,
Chanwoo Choi

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Roger Quadros]

+Felipe

Hi,

On 02/08/16 04:58, Chanwoo Choi wrote:
> This patch support the extcon property for the external connector
> because each external connector might have the property according to
> the H/W design and the specific characteristics.
> 
> - EXTCON_PROP_USB_[property name]
> - EXTCON_PROP_CHG_[property name]
> - EXTCON_PROP_JACK_[property name]
> - EXTCON_PROP_DISP_[property name]
> 
> Add the new extcon APIs to get/set the property value as following:
> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> 			unsigned int prop,
> 			union extcon_property_value *prop_val)
> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> 			unsigned int prop,
> 			union extcon_property_value prop_val)
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Chris Zhong <zyw@rock-chips.com>
> Tested-by: Guenter Roeck <groeck@chromium.org>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>  2 files changed, 286 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 129afc87313e..bb6e99fe84c8 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -196,6 +196,11 @@ struct extcon_cable {
>  	struct device_attribute attr_state;
>  
>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> +
> +	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
> +	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
> +	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
> +	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>  };
>  
>  static struct class *extcon_class;
> @@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>  	return -EINVAL;
>  }
>  
> +static int get_extcon_type(unsigned int prop)
> +{
> +	switch (prop) {
> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> +		return EXTCON_TYPE_USB;
> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> +		return EXTCON_TYPE_CHG;
> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> +		return EXTCON_TYPE_JACK;
> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> +		return EXTCON_TYPE_DISP;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
> +{
> +	return !!(edev->state & BIT(index));
> +}
> +
>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>  {
>  	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
> @@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>  	return false;
>  }
>  
> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
> +{
> +	int type;
> +
> +	/* Check whether the property is supported or not. */
> +	type = get_extcon_type(prop);
> +	if (type < 0)
> +		return false;
> +
> +	/* Check whether a specific extcon id supports the property or not. */
> +	return !!(extcon_info[id].type & type);
> +}
> +
> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
> +{
> +	unsigned int type = extcon_info[id].type;
> +	struct extcon_cable *cable = &edev->cables[index];
> +
> +	if (EXTCON_TYPE_USB & type)
> +		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
> +	if (EXTCON_TYPE_CHG & type)
> +		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
> +	if (EXTCON_TYPE_JACK & type)
> +		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
> +	if (EXTCON_TYPE_DISP & type)
> +		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
> +}
> +
>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>  			  char *buf)
>  {
> @@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>  	if (edev->max_supported && edev->max_supported <= index)
>  		return -EINVAL;
>  
> -	return !!(edev->state & (1 << index));
> +	return is_extcon_attached(edev, index);
>  }
>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>  
> @@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>  	if (edev->max_supported && edev->max_supported <= index)
>  		return -EINVAL;
>  
> +	/*
> +	 * Initialize the value of extcon property before setting
> +	 * the detached state for an external connector.
> +	 */
> +	if (!cable_state)
> +		init_property(edev, id, index);
> +

I'm a bit concerned about this for USB case. See below why.

>  	state = cable_state ? (1 << index) : 0;
>  	return extcon_update_state(edev, 1 << index, state);
>  }
>  EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>  
>  /**
> + * extcon_get_property() - Get the property value of a specific cable.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector
> + *			in extcon enumeration.
> + * @prop:		the property id among enum extcon_property.
> + * @prop_val:		the pointer which store the value of property.
> + *
> + * When getting the property value of external connector, the external connector
> + * should be attached. If detached state, function just return 0 without
> + * property value. Also, the each property should be included in the list of
> + * supported properties according to the type of external connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value *prop_val)
> +{
> +	struct extcon_cable *cable;
> +	unsigned long flags;
> +	int index, ret = 0;
> +
> +	*prop_val = (union extcon_property_value)(0);
> +
> +	if (!edev)
> +		return -EINVAL;
> +
> +	/* Check whether the property is supported or not */
> +	if (!is_extcon_property_supported(id, prop))
> +		return -EINVAL;
> +
> +	/* Find the cable index of external connector by using id */
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
> +
> +	spin_lock_irqsave(&edev->lock, flags);
> +
> +	/*
> +	 * Check whether the external connector is attached.
> +	 * If external connector is detached, the user can not
> +	 * get the property value.
> +	 */

How will this work for USB case? We need to know VBUS and ID states
even if the USB cable is detached.

Moreover there is no specific mechanism to detect if the USB cable is attached
or not in the extcon-usb-gpio.c case.
One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c.

Is this acceptable?

> +	if (!is_extcon_attached(edev, index)) {
> +		spin_unlock_irqrestore(&edev->lock, flags);
> +		return 0;
> +	}
> +
> +	cable = &edev->cables[index];
> +
> +	/* Get the property value according to extcon type */
> +	switch (prop) {
> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> +		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
> +		break;
> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> +		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
> +		break;
> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> +		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
> +		break;
> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> +		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
> +		break;

All these should be simplified to
		*prop_val = cable->foo_propval[prop];
see below why.

> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_get_property);
> +
> +/**
> + * extcon_set_property() - Set the property value of a specific cable.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector
> + *			in extcon enumeration.
> + * @prop:		the property id among enum extcon_property.
> + * @prop_val:		the pointer including the new value of property.
> + *
> + * The each property should be included in the list of supported properties
> + * according to the type of external connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value prop_val)
> +{
> +	struct extcon_cable *cable;
> +	unsigned long flags;
> +	int index, ret = 0;
> +
> +	if (!edev)
> +		return -EINVAL;
> +
> +	/* Check whether the property is supported or not */
> +	if (!is_extcon_property_supported(id, prop))
> +		return -EINVAL;
> +
> +	/* Find the cable index of external connector by using id */
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
> +
> +	spin_lock_irqsave(&edev->lock, flags);
> +
> +	cable = &edev->cables[index];
> +
> +	/* Set the property value according to extcon type */
> +	switch (prop) {
> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
> +		cable->usb_propval[prop - EXTCON_PROP_USB_MIN] = prop_val;
> +		break;
> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
> +		cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] = prop_val;
> +		break;
> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
> +		cable->jack_propval[prop - EXTCON_PROP_JACK_MIN] = prop_val;
> +		break;
> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
> +		cable->disp_propval[prop - EXTCON_PROP_DISP_MIN] = prop_val;
> +		break;

All these should be simplified to
		*prop_val = cable->foo_propval[prop];
see below why.

> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_property);
> +
> +/**
>   * extcon_get_extcon_dev() - Get the extcon device instance from the name
>   * @extcon_name:	The extcon name provided with extcon_dev_register()
>   */
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 46d802892c82..fe583fca7732 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -77,6 +77,68 @@
>  
>  #define EXTCON_NUM		63
>  
> +/*
> + * Define the property of supported external connectors.
> + *
> + * When adding the new extcon property, they *must* have
> + * the type/value/default information. Also, you *have to*
> + * modify the EXTCON_PROP_[type]_START/END definitions
> + * which mean the range of the supported properties
> + * for each extcon type.
> + *
> + * The naming style of property
> + * : EXTCON_PROP_[type]_[property name]
> + *
> + * EXTCON_PROP_USB_[property name]	: USB property
> + * EXTCON_PROP_CHG_[property name]	: Charger property
> + * EXTCON_PROP_JACK_[property name]	: Jack property
> + * EXTCON_PROP_DISP_[property name]	: Display property
> + */
> +
> +/*
> + * Properties of EXTCON_TYPE_USB.
> + *
> + * - EXTCON_PROP_USB_ID
> + * @type:	integer (intval)
> + * @value:	0 (low) or 1 (high)
> + * @default:	0 (low)
> + * - EXTCON_PROP_USB_VBUS
> + * @type:	integer (intval)
> + * @value:	0 (low) or 1 (high)
> + * @default:	0 (low)
> + */
> +#define EXTCON_PROP_USB_ID		0
> +#define EXTCON_PROP_USB_VBUS		1
> +
> +#define EXTCON_PROP_USB_MIN		0
> +#define EXTCON_PROP_USB_MAX		1
> +#define EXTCON_PROP_USB_CNT	(EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1)

As the properties themselves are separated by the cable type
they can overlap.

So MIN is not needed as it is always 0

#define EXTCON_PROP_USB_ID		0
#define EXTCON_PROP_USB_VBUS		1
#define EXTCON_PROP_USB_MAX		1

#define EXTCON_PROP_CHG_MAX		1
#define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX + 1)

and so on.

> +
> +/* Properties of EXTCON_TYPE_CHG. */
> +#define EXTCON_PROP_CHG_MIN		50
> +#define EXTCON_PROP_CHG_MAX		50
> +#define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1)
> +
> +/* Properties of EXTCON_TYPE_JACK. */
> +#define EXTCON_PROP_JACK_MIN		100
> +#define EXTCON_PROP_JACK_MAX		100
> +#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX - EXTCON_PROP_JACK_MIN + 1)
> +
> +/* Properties of EXTCON_TYPE_DISP. */
> +#define EXTCON_PROP_DISP_MIN		150
> +#define EXTCON_PROP_DISP_MAX		150
> +#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX - EXTCON_PROP_DISP_MIN + 1)
> +
> +/*
> + * Define the type of property's value.
> + *
> + * Define the property's value as union type. Because each property
> + * would need the different data type to store it.
> + */
> +union extcon_property_value {
> +	int intval;	/* type : integer (intval) */
> +};
> +

Why is this a union? There is only one member.

>  struct extcon_cable;
>  
>  /**
> @@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>  				   bool cable_state);
>  
>  /*
> + * get/set_property access the property value of each external connector.
> + * They are used to access the property of each cable based on the property id.
> + */
> +extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value *prop_val);
> +extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value prop_val);
> +
> +/*
>   * Following APIs are to monitor every action of a notifier.
>   * Registrar gets notified for every external port of a connection device.
>   * Probably this could be used to debug an action of notifier; however,
> @@ -239,6 +312,19 @@ static inline int extcon_set_cable_state_(struct extcon_dev *edev,
>  	return 0;
>  }
>  
> +static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
> +					unsigned int prop,
> +					union extcon_property_value *prop_val)
> +{
> +	return 0;
> +}
> +static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
> +					unsigned int prop,
> +					union extcon_property_value prop_val)
> +{
> +	return 0;
> +}
> +
>  static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  {
>  	return NULL;
> 

cheers,
-roger

Re: [PATCH v3 3/6] extcon: Add the support for the capability of each property

[Roger Quadros]

Hi,

On 02/08/16 04:58, Chanwoo Choi wrote:
> This patch adds the support of the property capability setting. This function
> decides the supported properties of each external connector on extcon provider
> driver.
> 
> Ths list of new extcon APIs to get/set the capability of property as following:
> - int extcon_get_property_capability(struct extcon_dev *edev,
> 					unsigned int id, unsigned int prop);
> - int extcon_set_property_capability(struct extcon_dev *edev,
> 					unsigned int id, unsigned int prop);
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Chris Zhong <zyw@rock-chips.com>
> Tested-by: Guenter Roeck <groeck@chromium.org>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
>  drivers/extcon/extcon.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/extcon.h  |  22 ++++++++
>  2 files changed, 163 insertions(+)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index bb6e99fe84c8..7ae0771f29e7 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -201,6 +201,11 @@ struct extcon_cable {
>  	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>  	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>  	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
> +
> +	unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)];
> +	unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)];
> +	unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)];
> +	unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)];

This can be replaced by a single
	unsigned long cap_bits;

As different type cables have separate extcon_cable instances.

>  };
>  
>  static struct class *extcon_class;
> @@ -297,6 +302,39 @@ static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>  	return !!(extcon_info[id].type & type);
>  }
>  
> +static int is_extcon_property_capability(struct extcon_dev *edev,
> +				unsigned int id, int index,unsigned int prop)
> +{
> +	struct extcon_cable *cable;
> +	int type, ret;
> +
> +	/* Check whether the property is supported or not. */
> +	type = get_extcon_type(prop);
> +	if (type < 0)
> +		return type;
> +
> +	cable = &edev->cables[index];
> +
> +	switch (type) {
> +	case EXTCON_TYPE_USB:
> +		ret = test_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);

	ret = test_bit(prop, cable->cap_bits);

> +		break;
> +	case EXTCON_TYPE_CHG:
> +		ret = test_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
> +		break;
> +	case EXTCON_TYPE_JACK:
> +		ret = test_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
> +		break;
> +	case EXTCON_TYPE_DISP:
> +		ret = test_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>  {
>  	unsigned int type = extcon_info[id].type;
> @@ -554,6 +592,12 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>  
>  	spin_lock_irqsave(&edev->lock, flags);
>  
> +	/* Check whether the property is available or not. */
> +	if (!is_extcon_property_capability(edev, id, index, prop)) {
> +		spin_unlock_irqrestore(&edev->lock, flags);
> +		return -EPERM;
> +	}
> +
>  	/*
>  	 * Check whether the external connector is attached.
>  	 * If external connector is detached, the user can not
> @@ -626,6 +670,12 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>  
>  	spin_lock_irqsave(&edev->lock, flags);
>  
> +	/* Check whether the property is available or not. */
> +	if (!is_extcon_property_capability(edev, id, index, prop)) {
> +		spin_unlock_irqrestore(&edev->lock, flags);
> +		return -EPERM;
> +	}
> +
>  	cable = &edev->cables[index];
>  
>  	/* Set the property value according to extcon type */
> @@ -654,6 +704,97 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>  EXPORT_SYMBOL_GPL(extcon_set_property);
>  
>  /**
> + * extcon_get_property_capability() - Get the capability of property
> + *			of an external connector.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector
> + *			in extcon enumeration.
> + * @prop:		the property id among enum extcon_property.
> + *
> + * Returns 1 if the property is available or 0 if not available.
> + */
> +int extcon_get_property_capability(struct extcon_dev *edev, unsigned int id,
> +					unsigned int prop)
> +{
> +	int index;
> +
> +	if (!edev)
> +		return -EINVAL;
> +
> +	/* Check whether the property is supported or not */
> +	if (!is_extcon_property_supported(id, prop))
> +		return -EINVAL;
> +
> +	/* Find the cable index of external connector by using id */
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
> +
> +	return is_extcon_property_capability(edev, id, index, prop);
> +}
> +EXPORT_SYMBOL_GPL(extcon_get_property_capability);
> +
> +/**
> + * extcon_set_property_capability() - Set the capability of a property
> + *			of an external connector.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector
> + *			in extcon enumeration.
> + * @prop:		the property id among enum extcon_property.
> + *
> + * This function set the capability of a property for an external connector
> + * to mark the bit in capability bitmap which mean the available state of
> + * a property.
> + *
> + * Returns 0 if success or error number if fail
> + */

So extcon drivers must explicitly enable all properties?
Isn't this an unnecessary step? Why can't all properties by supported by default.
If the hardware doesn't generate that property, the driver will just not toggle it.
Can you please give an example why this won't work?

> +int extcon_set_property_capability(struct extcon_dev *edev, unsigned int id,
> +					unsigned int prop)
> +{
> +	struct extcon_cable *cable;
> +	int index, type, ret = 0;
> +
> +	if (!edev)
> +		return -EINVAL;
> +
> +	/* Check whether the property is supported or not. */
> +	if (!is_extcon_property_supported(id, prop))
> +		return -EINVAL;
> +
> +	/* Find the cable index of external connector by using id. */
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
> +
> +	/* Check whether the property is supported or not. */
Again? we've already checked whether property is supported or not.

> +	type = get_extcon_type(prop);
> +	if (type < 0)
> +		return type;
> +
> +	cable = &edev->cables[index];
> +
> +	switch (type) {
> +	case EXTCON_TYPE_USB:
> +		__set_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);
> +		break;
> +	case EXTCON_TYPE_CHG:
> +		__set_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
> +		break;
> +	case EXTCON_TYPE_JACK:
> +		__set_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
> +		break;
> +	case EXTCON_TYPE_DISP:
> +		__set_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_property_capability);
> +
> +/**
>   * extcon_get_extcon_dev() - Get the extcon device instance from the name
>   * @extcon_name:	The extcon name provided with extcon_dev_register()
>   */
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index fe583fca7732..ffd32a8975d5 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -240,6 +240,16 @@ extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>  				union extcon_property_value prop_val);
>  
>  /*
> + * get/set_property_capability set the capability of the property for each
> + * external connector. They are used to set the capability of the property
> + * of each external connector based on the id and property.
> + */
> +extern int extcon_get_property_capability(struct extcon_dev *edev,
> +				unsigned int id, unsigned int prop);
> +extern int extcon_set_property_capability(struct extcon_dev *edev,
> +				unsigned int id, unsigned int prop);
> +
> +/*
>   * Following APIs are to monitor every action of a notifier.
>   * Registrar gets notified for every external port of a connection device.
>   * Probably this could be used to debug an action of notifier; however,
> @@ -325,6 +335,18 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>  	return 0;
>  }
>  
> +static inline int extcon_get_property_capability(struct extcon_dev *edev,
> +					unsigned int id, unsigned int prop)
> +{
> +	return 0;
> +}
> +
> +static inline int extcon_set_property_capability(struct extcon_dev *edev,
> +					unsigned int id, unsigned int prop)
> +{
> +	return 0;
> +}
> +
>  static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  {
>  	return NULL;
> 

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi]

Hi,

On 2016년 08월 02일 16:43, Roger Quadros wrote:
> +Felipe
> 
> Hi,
> 
> On 02/08/16 04:58, Chanwoo Choi wrote:
>> This patch support the extcon property for the external connector
>> because each external connector might have the property according to
>> the H/W design and the specific characteristics.
>>
>> - EXTCON_PROP_USB_[property name]
>> - EXTCON_PROP_CHG_[property name]
>> - EXTCON_PROP_JACK_[property name]
>> - EXTCON_PROP_DISP_[property name]
>>
>> Add the new extcon APIs to get/set the property value as following:
>> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> 			unsigned int prop,
>> 			union extcon_property_value *prop_val)
>> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> 			unsigned int prop,
>> 			union extcon_property_value prop_val)
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>> Tested-by: Guenter Roeck <groeck@chromium.org>
>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>> ---
>>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>>  2 files changed, 286 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 129afc87313e..bb6e99fe84c8 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -196,6 +196,11 @@ struct extcon_cable {
>>  	struct device_attribute attr_state;
>>  
>>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
>> +
>> +	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
>> +	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>> +	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>> +	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>>  };
>>  
>>  static struct class *extcon_class;
>> @@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>>  	return -EINVAL;
>>  }
>>  
>> +static int get_extcon_type(unsigned int prop)
>> +{
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		return EXTCON_TYPE_USB;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		return EXTCON_TYPE_CHG;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		return EXTCON_TYPE_JACK;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		return EXTCON_TYPE_DISP;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
>> +{
>> +	return !!(edev->state & BIT(index));
>> +}
>> +
>>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>  {
>>  	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>> @@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>  	return false;
>>  }
>>  
>> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>> +{
>> +	int type;
>> +
>> +	/* Check whether the property is supported or not. */
>> +	type = get_extcon_type(prop);
>> +	if (type < 0)
>> +		return false;
>> +
>> +	/* Check whether a specific extcon id supports the property or not. */
>> +	return !!(extcon_info[id].type & type);
>> +}
>> +
>> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>> +{
>> +	unsigned int type = extcon_info[id].type;
>> +	struct extcon_cable *cable = &edev->cables[index];
>> +
>> +	if (EXTCON_TYPE_USB & type)
>> +		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
>> +	if (EXTCON_TYPE_CHG & type)
>> +		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
>> +	if (EXTCON_TYPE_JACK & type)
>> +		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
>> +	if (EXTCON_TYPE_DISP & type)
>> +		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
>> +}
>> +
>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>  			  char *buf)
>>  {
>> @@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>>  	if (edev->max_supported && edev->max_supported <= index)
>>  		return -EINVAL;
>>  
>> -	return !!(edev->state & (1 << index));
>> +	return is_extcon_attached(edev, index);
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>  
>> @@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>  	if (edev->max_supported && edev->max_supported <= index)
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * Initialize the value of extcon property before setting
>> +	 * the detached state for an external connector.
>> +	 */
>> +	if (!cable_state)
>> +		init_property(edev, id, index);
>> +
> 
> I'm a bit concerned about this for USB case. See below why.
> 
>>  	state = cable_state ? (1 << index) : 0;
>>  	return extcon_update_state(edev, 1 << index, state);
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>  
>>  /**
>> + * extcon_get_property() - Get the property value of a specific cable.
>> + * @edev:		the extcon device that has the cable.
>> + * @id:			the unique id of each external connector
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + * @prop_val:		the pointer which store the value of property.
>> + *
>> + * When getting the property value of external connector, the external connector
>> + * should be attached. If detached state, function just return 0 without
>> + * property value. Also, the each property should be included in the list of
>> + * supported properties according to the type of external connectors.
>> + *
>> + * Returns 0 if success or error number if fail
>> + */
>> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value *prop_val)
>> +{
>> +	struct extcon_cable *cable;
>> +	unsigned long flags;
>> +	int index, ret = 0;
>> +
>> +	*prop_val = (union extcon_property_value)(0);
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	spin_lock_irqsave(&edev->lock, flags);
>> +
>> +	/*
>> +	 * Check whether the external connector is attached.
>> +	 * If external connector is detached, the user can not
>> +	 * get the property value.
>> +	 */
> 
> How will this work for USB case? We need to know VBUS and ID states
> even if the USB cable is detached.

When USB is detached, extcon_get_property return the default value without any operation.
The default value of supported property are 0 (zero). If new property need the differnt default
value, I'll support it.

> 
> Moreover there is no specific mechanism to detect if the USB cable is attached
> or not in the extcon-usb-gpio.c case.
> One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c.
> 
> Is this acceptable?

No. the extcon have to detect the correct state. When USB cable is detached, 
the extcon cannot set the attached state for EXTCON_USB.

> 
>> +	if (!is_extcon_attached(edev, index)) {
>> +		spin_unlock_irqrestore(&edev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	/* Get the property value according to extcon type */
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
>> +		break;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
>> +		break;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
>> +		break;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
>> +		break;
> 
> All these should be simplified to
> 		*prop_val = cable->foo_propval[prop];
> see below why.

You mean that the macro?
I already used the macro. But, as comment, I don't use it.

> 
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_get_property);
>> +
>> +/**
>> + * extcon_set_property() - Set the property value of a specific cable.
>> + * @edev:		the extcon device that has the cable.
>> + * @id:			the unique id of each external connector
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + * @prop_val:		the pointer including the new value of property.
>> + *
>> + * The each property should be included in the list of supported properties
>> + * according to the type of external connectors.
>> + *
>> + * Returns 0 if success or error number if fail
>> + */
>> +int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value prop_val)
>> +{
>> +	struct extcon_cable *cable;
>> +	unsigned long flags;
>> +	int index, ret = 0;
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	spin_lock_irqsave(&edev->lock, flags);
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	/* Set the property value according to extcon type */
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		cable->usb_propval[prop - EXTCON_PROP_USB_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		cable->jack_propval[prop - EXTCON_PROP_JACK_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		cable->disp_propval[prop - EXTCON_PROP_DISP_MIN] = prop_val;
>> +		break;
> 
> All these should be simplified to
> 		*prop_val = cable->foo_propval[prop];
> see below why.

ditto.

> 
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_set_property);
>> +
>> +/**
>>   * extcon_get_extcon_dev() - Get the extcon device instance from the name
>>   * @extcon_name:	The extcon name provided with extcon_dev_register()
>>   */
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 46d802892c82..fe583fca7732 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -77,6 +77,68 @@
>>  
>>  #define EXTCON_NUM		63
>>  
>> +/*
>> + * Define the property of supported external connectors.
>> + *
>> + * When adding the new extcon property, they *must* have
>> + * the type/value/default information. Also, you *have to*
>> + * modify the EXTCON_PROP_[type]_START/END definitions
>> + * which mean the range of the supported properties
>> + * for each extcon type.
>> + *
>> + * The naming style of property
>> + * : EXTCON_PROP_[type]_[property name]
>> + *
>> + * EXTCON_PROP_USB_[property name]	: USB property
>> + * EXTCON_PROP_CHG_[property name]	: Charger property
>> + * EXTCON_PROP_JACK_[property name]	: Jack property
>> + * EXTCON_PROP_DISP_[property name]	: Display property
>> + */
>> +
>> +/*
>> + * Properties of EXTCON_TYPE_USB.
>> + *
>> + * - EXTCON_PROP_USB_ID
>> + * @type:	integer (intval)
>> + * @value:	0 (low) or 1 (high)
>> + * @default:	0 (low)
>> + * - EXTCON_PROP_USB_VBUS
>> + * @type:	integer (intval)
>> + * @value:	0 (low) or 1 (high)
>> + * @default:	0 (low)
>> + */
>> +#define EXTCON_PROP_USB_ID		0
>> +#define EXTCON_PROP_USB_VBUS		1
>> +
>> +#define EXTCON_PROP_USB_MIN		0
>> +#define EXTCON_PROP_USB_MAX		1
>> +#define EXTCON_PROP_USB_CNT	(EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1)
> 
> As the properties themselves are separated by the cable type
> they can overlap.
> 
> So MIN is not needed as it is always 0

Yes. Just adding the EXTCON_PROP_USB_MIN because remaining the
consistent expression for other types (EXTCON_PROP_[type]_MIN).

> 
> #define EXTCON_PROP_USB_ID		0
> #define EXTCON_PROP_USB_VBUS		1
> #define EXTCON_PROP_USB_MAX		1
> 
> #define EXTCON_PROP_CHG_MAX		1
> #define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX + 1)
> 
> and so on.
> 
>> +
>> +/* Properties of EXTCON_TYPE_CHG. */
>> +#define EXTCON_PROP_CHG_MIN		50
>> +#define EXTCON_PROP_CHG_MAX		50
>> +#define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1)
>> +
>> +/* Properties of EXTCON_TYPE_JACK. */
>> +#define EXTCON_PROP_JACK_MIN		100
>> +#define EXTCON_PROP_JACK_MAX		100
>> +#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX - EXTCON_PROP_JACK_MIN + 1)
>> +
>> +/* Properties of EXTCON_TYPE_DISP. */
>> +#define EXTCON_PROP_DISP_MIN		150
>> +#define EXTCON_PROP_DISP_MAX		150
>> +#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX - EXTCON_PROP_DISP_MIN + 1)
>> +
>> +/*
>> + * Define the type of property's value.
>> + *
>> + * Define the property's value as union type. Because each property
>> + * would need the different data type to store it.
>> + */
>> +union extcon_property_value {
>> +	int intval;	/* type : integer (intval) */
>> +};
>> +
> 
> Why is this a union? There is only one member.

No. The extcon support the various type of external connector.
This patch don't support the only USB type.

> 
>>  struct extcon_cable;
>>  
>>  /**
>> @@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>  				   bool cable_state);
>>  
>>  /*
>> + * get/set_property access the property value of each external connector.
>> + * They are used to access the property of each cable based on the property id.
>> + */
>> +extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value *prop_val);
>> +extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value prop_val);
>> +
>> +/*
>>   * Following APIs are to monitor every action of a notifier.
>>   * Registrar gets notified for every external port of a connection device.
>>   * Probably this could be used to debug an action of notifier; however,
>> @@ -239,6 +312,19 @@ static inline int extcon_set_cable_state_(struct extcon_dev *edev,
>>  	return 0;
>>  }
>>  
>> +static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop,
>> +					union extcon_property_value *prop_val)
>> +{
>> +	return 0;
>> +}
>> +static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop,
>> +					union extcon_property_value prop_val)
>> +{
>> +	return 0;
>> +}
>> +
>>  static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>  {
>>  	return NULL;
>>
> 

Thanks,
Chanwoo Choi

Re: [PATCH v3 3/6] extcon: Add the support for the capability of each property

[Chanwoo Choi]

Hi,

On 2016년 08월 02일 17:07, Roger Quadros wrote:
> Hi,
> 
> On 02/08/16 04:58, Chanwoo Choi wrote:
>> This patch adds the support of the property capability setting. This function
>> decides the supported properties of each external connector on extcon provider
>> driver.
>>
>> Ths list of new extcon APIs to get/set the capability of property as following:
>> - int extcon_get_property_capability(struct extcon_dev *edev,
>> 					unsigned int id, unsigned int prop);
>> - int extcon_set_property_capability(struct extcon_dev *edev,
>> 					unsigned int id, unsigned int prop);
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>> Tested-by: Guenter Roeck <groeck@chromium.org>
>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>> ---
>>  drivers/extcon/extcon.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/extcon.h  |  22 ++++++++
>>  2 files changed, 163 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index bb6e99fe84c8..7ae0771f29e7 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -201,6 +201,11 @@ struct extcon_cable {
>>  	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>>  	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>>  	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>> +
>> +	unsigned long usb_bits[BITS_TO_LONGS(EXTCON_PROP_USB_CNT)];
>> +	unsigned long chg_bits[BITS_TO_LONGS(EXTCON_PROP_CHG_CNT)];
>> +	unsigned long jack_bits[BITS_TO_LONGS(EXTCON_PROP_JACK_CNT)];
>> +	unsigned long disp_bits[BITS_TO_LONGS(EXTCON_PROP_DISP_CNT)];
> 
> This can be replaced by a single
> 	unsigned long cap_bits;
> 
> As different type cables have separate extcon_cable instances.

The one external connector is able to have the multiple extcon type.
So, the external connector should have the [type]_bits variable.

> 
>>  };
>>  
>>  static struct class *extcon_class;
>> @@ -297,6 +302,39 @@ static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>>  	return !!(extcon_info[id].type & type);
>>  }
>>  
>> +static int is_extcon_property_capability(struct extcon_dev *edev,
>> +				unsigned int id, int index,unsigned int prop)
>> +{
>> +	struct extcon_cable *cable;
>> +	int type, ret;
>> +
>> +	/* Check whether the property is supported or not. */
>> +	type = get_extcon_type(prop);
>> +	if (type < 0)
>> +		return type;
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	switch (type) {
>> +	case EXTCON_TYPE_USB:
>> +		ret = test_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);
> 
> 	ret = test_bit(prop, cable->cap_bits);

As I mentioned on previous reply, I just want to maintain the
consistent style with other extcon types.

> 
>> +		break;
>> +	case EXTCON_TYPE_CHG:
>> +		ret = test_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
>> +		break;
>> +	case EXTCON_TYPE_JACK:
>> +		ret = test_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
>> +		break;
>> +	case EXTCON_TYPE_DISP:
>> +		ret = test_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>>  {
>>  	unsigned int type = extcon_info[id].type;
>> @@ -554,6 +592,12 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>>  
>>  	spin_lock_irqsave(&edev->lock, flags);
>>  
>> +	/* Check whether the property is available or not. */
>> +	if (!is_extcon_property_capability(edev, id, index, prop)) {
>> +		spin_unlock_irqrestore(&edev->lock, flags);
>> +		return -EPERM;
>> +	}
>> +
>>  	/*
>>  	 * Check whether the external connector is attached.
>>  	 * If external connector is detached, the user can not
>> @@ -626,6 +670,12 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>  
>>  	spin_lock_irqsave(&edev->lock, flags);
>>  
>> +	/* Check whether the property is available or not. */
>> +	if (!is_extcon_property_capability(edev, id, index, prop)) {
>> +		spin_unlock_irqrestore(&edev->lock, flags);
>> +		return -EPERM;
>> +	}
>> +
>>  	cable = &edev->cables[index];
>>  
>>  	/* Set the property value according to extcon type */
>> @@ -654,6 +704,97 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>  EXPORT_SYMBOL_GPL(extcon_set_property);
>>  
>>  /**
>> + * extcon_get_property_capability() - Get the capability of property
>> + *			of an external connector.
>> + * @edev:		the extcon device that has the cable.
>> + * @id:			the unique id of each external connector
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + *
>> + * Returns 1 if the property is available or 0 if not available.
>> + */
>> +int extcon_get_property_capability(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop)
>> +{
>> +	int index;
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	return is_extcon_property_capability(edev, id, index, prop);
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_get_property_capability);
>> +
>> +/**
>> + * extcon_set_property_capability() - Set the capability of a property
>> + *			of an external connector.
>> + * @edev:		the extcon device that has the cable.
>> + * @id:			the unique id of each external connector
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + *
>> + * This function set the capability of a property for an external connector
>> + * to mark the bit in capability bitmap which mean the available state of
>> + * a property.
>> + *
>> + * Returns 0 if success or error number if fail
>> + */
> 
> So extcon drivers must explicitly enable all properties?

The extcon provider driver should enable the capability for each property.

> Isn't this an unnecessary step? Why can't all properties by supported by default.
> If the hardware doesn't generate that property, the driver will just not toggle it.
> Can you please give an example why this won't work?

The extcon support the uevent when connector is attached or detached.
Also, the message of uevent will include the properties value.
If we don't use the capability function, the uevent's message 
may include the unsupported and un-needed property information.

> 
>> +int extcon_set_property_capability(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop)
>> +{
>> +	struct extcon_cable *cable;
>> +	int index, type, ret = 0;
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not. */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id. */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	/* Check whether the property is supported or not. */
> Again? we've already checked whether property is supported or not.

It is wrong description. I'll remove it.

> 
>> +	type = get_extcon_type(prop);
>> +	if (type < 0)
>> +		return type;
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	switch (type) {
>> +	case EXTCON_TYPE_USB:
>> +		__set_bit(prop - EXTCON_PROP_USB_MIN, cable->usb_bits);
>> +		break;
>> +	case EXTCON_TYPE_CHG:
>> +		__set_bit(prop - EXTCON_PROP_CHG_MIN, cable->chg_bits);
>> +		break;
>> +	case EXTCON_TYPE_JACK:
>> +		__set_bit(prop - EXTCON_PROP_JACK_MIN, cable->jack_bits);
>> +		break;
>> +	case EXTCON_TYPE_DISP:
>> +		__set_bit(prop - EXTCON_PROP_DISP_MIN, cable->disp_bits);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_set_property_capability);
>> +
>> +/**
>>   * extcon_get_extcon_dev() - Get the extcon device instance from the name
>>   * @extcon_name:	The extcon name provided with extcon_dev_register()
>>   */
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index fe583fca7732..ffd32a8975d5 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -240,6 +240,16 @@ extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>  				union extcon_property_value prop_val);
>>  
>>  /*
>> + * get/set_property_capability set the capability of the property for each
>> + * external connector. They are used to set the capability of the property
>> + * of each external connector based on the id and property.
>> + */
>> +extern int extcon_get_property_capability(struct extcon_dev *edev,
>> +				unsigned int id, unsigned int prop);
>> +extern int extcon_set_property_capability(struct extcon_dev *edev,
>> +				unsigned int id, unsigned int prop);
>> +
>> +/*
>>   * Following APIs are to monitor every action of a notifier.
>>   * Registrar gets notified for every external port of a connection device.
>>   * Probably this could be used to debug an action of notifier; however,
>> @@ -325,6 +335,18 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>  	return 0;
>>  }
>>  
>> +static inline int extcon_get_property_capability(struct extcon_dev *edev,
>> +					unsigned int id, unsigned int prop)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int extcon_set_property_capability(struct extcon_dev *edev,
>> +					unsigned int id, unsigned int prop)
>> +{
>> +	return 0;
>> +}
>> +
>>  static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>  {
>>  	return NULL;
>>

Thanks,
Chanwoo Choi

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Roger Quadros]

Hi Chanwoo,

On 02/08/16 11:08, Chanwoo Choi wrote:
> Hi,
> 
> On 2016년 08월 02일 16:43, Roger Quadros wrote:
>> +Felipe
>>
>> Hi,
>>
>> On 02/08/16 04:58, Chanwoo Choi wrote:
>>> This patch support the extcon property for the external connector
>>> because each external connector might have the property according to
>>> the H/W design and the specific characteristics.
>>>
>>> - EXTCON_PROP_USB_[property name]
>>> - EXTCON_PROP_CHG_[property name]
>>> - EXTCON_PROP_JACK_[property name]
>>> - EXTCON_PROP_DISP_[property name]
>>>
>>> Add the new extcon APIs to get/set the property value as following:
>>> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>>> 			unsigned int prop,
>>> 			union extcon_property_value *prop_val)
>>> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>> 			unsigned int prop,
>>> 			union extcon_property_value prop_val)
>>>
>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>>> Tested-by: Guenter Roeck <groeck@chromium.org>
>>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>>> ---
>>>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>>>  2 files changed, 286 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 129afc87313e..bb6e99fe84c8 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -196,6 +196,11 @@ struct extcon_cable {
>>>  	struct device_attribute attr_state;
>>>  
>>>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
>>> +
>>> +	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
>>> +	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>>> +	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>>> +	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>>>  };
>>>  
>>>  static struct class *extcon_class;
>>> @@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>>>  	return -EINVAL;
>>>  }
>>>  
>>> +static int get_extcon_type(unsigned int prop)
>>> +{
>>> +	switch (prop) {
>>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>>> +		return EXTCON_TYPE_USB;
>>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>>> +		return EXTCON_TYPE_CHG;
>>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>>> +		return EXTCON_TYPE_JACK;
>>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>>> +		return EXTCON_TYPE_DISP;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +}
>>> +
>>> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
>>> +{
>>> +	return !!(edev->state & BIT(index));
>>> +}
>>> +
>>>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>>  {
>>>  	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>>> @@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>>  	return false;
>>>  }
>>>  
>>> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>>> +{
>>> +	int type;
>>> +
>>> +	/* Check whether the property is supported or not. */
>>> +	type = get_extcon_type(prop);
>>> +	if (type < 0)
>>> +		return false;
>>> +
>>> +	/* Check whether a specific extcon id supports the property or not. */
>>> +	return !!(extcon_info[id].type & type);
>>> +}
>>> +
>>> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>>> +{
>>> +	unsigned int type = extcon_info[id].type;
>>> +	struct extcon_cable *cable = &edev->cables[index];
>>> +
>>> +	if (EXTCON_TYPE_USB & type)
>>> +		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
>>> +	if (EXTCON_TYPE_CHG & type)
>>> +		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
>>> +	if (EXTCON_TYPE_JACK & type)
>>> +		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
>>> +	if (EXTCON_TYPE_DISP & type)
>>> +		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
>>> +}
>>> +
>>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>>  			  char *buf)
>>>  {
>>> @@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>>>  	if (edev->max_supported && edev->max_supported <= index)
>>>  		return -EINVAL;
>>>  
>>> -	return !!(edev->state & (1 << index));
>>> +	return is_extcon_attached(edev, index);
>>>  }
>>>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>>  
>>> @@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>>  	if (edev->max_supported && edev->max_supported <= index)
>>>  		return -EINVAL;
>>>  
>>> +	/*
>>> +	 * Initialize the value of extcon property before setting
>>> +	 * the detached state for an external connector.
>>> +	 */
>>> +	if (!cable_state)
>>> +		init_property(edev, id, index);
>>> +
>>
>> I'm a bit concerned about this for USB case. See below why.
>>
>>>  	state = cable_state ? (1 << index) : 0;
>>>  	return extcon_update_state(edev, 1 << index, state);
>>>  }
>>>  EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>>  
>>>  /**
>>> + * extcon_get_property() - Get the property value of a specific cable.
>>> + * @edev:		the extcon device that has the cable.
>>> + * @id:			the unique id of each external connector
>>> + *			in extcon enumeration.
>>> + * @prop:		the property id among enum extcon_property.
>>> + * @prop_val:		the pointer which store the value of property.
>>> + *
>>> + * When getting the property value of external connector, the external connector
>>> + * should be attached. If detached state, function just return 0 without
>>> + * property value. Also, the each property should be included in the list of
>>> + * supported properties according to the type of external connectors.
>>> + *
>>> + * Returns 0 if success or error number if fail
>>> + */
>>> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>>> +				unsigned int prop,
>>> +				union extcon_property_value *prop_val)
>>> +{
>>> +	struct extcon_cable *cable;
>>> +	unsigned long flags;
>>> +	int index, ret = 0;
>>> +
>>> +	*prop_val = (union extcon_property_value)(0);
>>> +
>>> +	if (!edev)
>>> +		return -EINVAL;
>>> +
>>> +	/* Check whether the property is supported or not */
>>> +	if (!is_extcon_property_supported(id, prop))
>>> +		return -EINVAL;
>>> +
>>> +	/* Find the cable index of external connector by using id */
>>> +	index = find_cable_index_by_id(edev, id);
>>> +	if (index < 0)
>>> +		return index;
>>> +
>>> +	spin_lock_irqsave(&edev->lock, flags);
>>> +
>>> +	/*
>>> +	 * Check whether the external connector is attached.
>>> +	 * If external connector is detached, the user can not
>>> +	 * get the property value.
>>> +	 */
>>
>> How will this work for USB case? We need to know VBUS and ID states
>> even if the USB cable is detached.
> 
> When USB is detached, extcon_get_property return the default value without any operation.
> The default value of supported property are 0 (zero). If new property need the differnt default
> value, I'll support it.

Is the property a property of the connector or of the cable?

In my opinion, ID and VBUS are properties of the USB connector and not of
the USB cable. So extcon must provide valid status for those properties
even if USB cable or USB_HOST cable is detached.

USB controller drivers are interested in raw VBUS and ID states
irrespective of EXTCON_USB attached/detached or EXTCON_USB_HOST
attached/detached state.

We don't want to have a window where both cables are in detached states
and controller cannot get valid VBUS/ID properties.
> 
>>
>> Moreover there is no specific mechanism to detect if the USB cable is attached
>> or not in the extcon-usb-gpio.c case.
>> One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c.
>>
>> Is this acceptable?
> 
> No. the extcon have to detect the correct state. When USB cable is detached, 
> the extcon cannot set the attached state for EXTCON_USB.
> 

OK.
>>
>>> +	if (!is_extcon_attached(edev, index)) {
>>> +		spin_unlock_irqrestore(&edev->lock, flags);
>>> +		return 0;
>>> +	}
>>> +
>>> +	cable = &edev->cables[index];
>>> +
>>> +	/* Get the property value according to extcon type */
>>> +	switch (prop) {
>>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>>> +		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
>>> +		break;
>>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>>> +		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
>>> +		break;
>>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>>> +		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
>>> +		break;
>>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>>> +		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
>>> +		break;

cheers,
-roger

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi]

Hi Roger,

On 2016년 08월 03일 18:46, Roger Quadros wrote:
> Hi Chanwoo,
> 
> On 02/08/16 11:08, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2016년 08월 02일 16:43, Roger Quadros wrote:
>>> +Felipe
>>>
>>> Hi,
>>>
>>> On 02/08/16 04:58, Chanwoo Choi wrote:
>>>> This patch support the extcon property for the external connector
>>>> because each external connector might have the property according to
>>>> the H/W design and the specific characteristics.
>>>>
>>>> - EXTCON_PROP_USB_[property name]
>>>> - EXTCON_PROP_CHG_[property name]
>>>> - EXTCON_PROP_JACK_[property name]
>>>> - EXTCON_PROP_DISP_[property name]
>>>>
>>>> Add the new extcon APIs to get/set the property value as following:
>>>> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>>>> 			unsigned int prop,
>>>> 			union extcon_property_value *prop_val)
>>>> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>>>> 			unsigned int prop,
>>>> 			union extcon_property_value prop_val)
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> Tested-by: Chris Zhong <zyw@rock-chips.com>
>>>> Tested-by: Guenter Roeck <groeck@chromium.org>
>>>> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>>>> ---
>>>>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>>>>  2 files changed, 286 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>>> index 129afc87313e..bb6e99fe84c8 100644
>>>> --- a/drivers/extcon/extcon.c
>>>> +++ b/drivers/extcon/extcon.c
>>>> @@ -196,6 +196,11 @@ struct extcon_cable {
>>>>  	struct device_attribute attr_state;
>>>>  
>>>>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
>>>> +
>>>> +	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
>>>> +	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>>>> +	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>>>> +	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>>>>  };
>>>>  
>>>>  static struct class *extcon_class;
>>>> @@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> +static int get_extcon_type(unsigned int prop)
>>>> +{
>>>> +	switch (prop) {
>>>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>>>> +		return EXTCON_TYPE_USB;
>>>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>>>> +		return EXTCON_TYPE_CHG;
>>>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>>>> +		return EXTCON_TYPE_JACK;
>>>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>>>> +		return EXTCON_TYPE_DISP;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +}
>>>> +
>>>> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
>>>> +{
>>>> +	return !!(edev->state & BIT(index));
>>>> +}
>>>> +
>>>>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>>>  {
>>>>  	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>>>> @@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>>>  	return false;
>>>>  }
>>>>  
>>>> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>>>> +{
>>>> +	int type;
>>>> +
>>>> +	/* Check whether the property is supported or not. */
>>>> +	type = get_extcon_type(prop);
>>>> +	if (type < 0)
>>>> +		return false;
>>>> +
>>>> +	/* Check whether a specific extcon id supports the property or not. */
>>>> +	return !!(extcon_info[id].type & type);
>>>> +}
>>>> +
>>>> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>>>> +{
>>>> +	unsigned int type = extcon_info[id].type;
>>>> +	struct extcon_cable *cable = &edev->cables[index];
>>>> +
>>>> +	if (EXTCON_TYPE_USB & type)
>>>> +		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
>>>> +	if (EXTCON_TYPE_CHG & type)
>>>> +		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
>>>> +	if (EXTCON_TYPE_JACK & type)
>>>> +		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
>>>> +	if (EXTCON_TYPE_DISP & type)
>>>> +		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
>>>> +}
>>>> +
>>>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>>>  			  char *buf)
>>>>  {
>>>> @@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>>>>  	if (edev->max_supported && edev->max_supported <= index)
>>>>  		return -EINVAL;
>>>>  
>>>> -	return !!(edev->state & (1 << index));
>>>> +	return is_extcon_attached(edev, index);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>>>  
>>>> @@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>>>  	if (edev->max_supported && edev->max_supported <= index)
>>>>  		return -EINVAL;
>>>>  
>>>> +	/*
>>>> +	 * Initialize the value of extcon property before setting
>>>> +	 * the detached state for an external connector.
>>>> +	 */
>>>> +	if (!cable_state)
>>>> +		init_property(edev, id, index);
>>>> +
>>>
>>> I'm a bit concerned about this for USB case. See below why.
>>>
>>>>  	state = cable_state ? (1 << index) : 0;
>>>>  	return extcon_update_state(edev, 1 << index, state);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>>>  
>>>>  /**
>>>> + * extcon_get_property() - Get the property value of a specific cable.
>>>> + * @edev:		the extcon device that has the cable.
>>>> + * @id:			the unique id of each external connector
>>>> + *			in extcon enumeration.
>>>> + * @prop:		the property id among enum extcon_property.
>>>> + * @prop_val:		the pointer which store the value of property.
>>>> + *
>>>> + * When getting the property value of external connector, the external connector
>>>> + * should be attached. If detached state, function just return 0 without
>>>> + * property value. Also, the each property should be included in the list of
>>>> + * supported properties according to the type of external connectors.
>>>> + *
>>>> + * Returns 0 if success or error number if fail
>>>> + */
>>>> +int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>>>> +				unsigned int prop,
>>>> +				union extcon_property_value *prop_val)
>>>> +{
>>>> +	struct extcon_cable *cable;
>>>> +	unsigned long flags;
>>>> +	int index, ret = 0;
>>>> +
>>>> +	*prop_val = (union extcon_property_value)(0);
>>>> +
>>>> +	if (!edev)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* Check whether the property is supported or not */
>>>> +	if (!is_extcon_property_supported(id, prop))
>>>> +		return -EINVAL;
>>>> +
>>>> +	/* Find the cable index of external connector by using id */
>>>> +	index = find_cable_index_by_id(edev, id);
>>>> +	if (index < 0)
>>>> +		return index;
>>>> +
>>>> +	spin_lock_irqsave(&edev->lock, flags);
>>>> +
>>>> +	/*
>>>> +	 * Check whether the external connector is attached.
>>>> +	 * If external connector is detached, the user can not
>>>> +	 * get the property value.
>>>> +	 */
>>>
>>> How will this work for USB case? We need to know VBUS and ID states
>>> even if the USB cable is detached.
>>
>> When USB is detached, extcon_get_property return the default value without any operation.
>> The default value of supported property are 0 (zero). If new property need the differnt default
>> value, I'll support it.
> 
> Is the property a property of the connector or of the cable?
> 
> In my opinion, ID and VBUS are properties of the USB connector and not of
> the USB cable. So extcon must provide valid status for those properties
> even if USB cable or USB_HOST cable is detached.

I don't understand about that if USB and USB_HOST are detached,
how can the USB be operating? As you mentioned that, extcon must
provide the valid status for both state and properties. 

So, I already mentioned, When USB and USB_HOST are detached,
extcon return the default value instead of error value.
I think that it is reasonable. Why is it not a valid?

> 
> USB controller drivers are interested in raw VBUS and ID states
> irrespective of EXTCON_USB attached/detached or EXTCON_USB_HOST
> attached/detached state.

As I already mentioned about this issue, the extcon support the
two notification. One is notification to framework/device driver
in kernel-space. The second role send the uevent to user-space
to detect the type of connected external connector.

Firstly, extcon have to check the connected state of any external connector.
When there is no connected connector, extcon cannot check the both
state and property of external connector.

You want that the extcon split out the handling role of state and properties.
But it breaks the basically concept and method of extcon.

So, we need to find the good method to support the user-space and kernel-space
with extcon. Thanks for your comment.

Regards,
Chanwoo Choi

> 
> We don't want to have a window where both cables are in detached states
> and controller cannot get valid VBUS/ID properties.
>>
>>>
>>> Moreover there is no specific mechanism to detect if the USB cable is attached
>>> or not in the extcon-usb-gpio.c case.
>>> One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c.
>>>
>>> Is this acceptable?
>>
>> No. the extcon have to detect the correct state. When USB cable is detached, 
>> the extcon cannot set the attached state for EXTCON_USB.
>>
> 
> OK.
>>>
>>>> +	if (!is_extcon_attached(edev, index)) {
>>>> +		spin_unlock_irqrestore(&edev->lock, flags);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	cable = &edev->cables[index];
>>>> +
>>>> +	/* Get the property value according to extcon type */
>>>> +	switch (prop) {
>>>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>>>> +		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
>>>> +		break;
>>>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>>>> +		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
>>>> +		break;
>>>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>>>> +		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
>>>> +		break;
>>>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>>>> +		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
>>>> +		break;
> 
> cheers,
> -roger
> 
> 
> 

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Guenter Roeck]

On Wed, Aug 3, 2016 at 5:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi Roger,
>
> On 2016년 08월 03일 18:46, Roger Quadros wrote:
>> Hi Chanwoo,
>>

[ ... ]

>>>>> +  /*
>>>>> +   * Check whether the external connector is attached.
>>>>> +   * If external connector is detached, the user can not
>>>>> +   * get the property value.
>>>>> +   */
>>>>
>>>> How will this work for USB case? We need to know VBUS and ID states
>>>> even if the USB cable is detached.
>>>
>>> When USB is detached, extcon_get_property return the default value without any operation.
>>> The default value of supported property are 0 (zero). If new property need the differnt default
>>> value, I'll support it.
>>
>> Is the property a property of the connector or of the cable?
>>
>> In my opinion, ID and VBUS are properties of the USB connector and not of
>> the USB cable. So extcon must provide valid status for those properties
>> even if USB cable or USB_HOST cable is detached.
>
> I don't understand about that if USB and USB_HOST are detached,
> how can the USB be operating? As you mentioned that, extcon must
> provide the valid status for both state and properties.
>

Correct. No cable means that the polarity is unknown, and VBUS must not be
active (for USB_HOST), or it can not be active (for USB).

Only question might be EXTCON_PROP_USB_ID; I am not sure I understand
what it is supposed to return. Maybe it would be worthwhile to document it ?

> So, I already mentioned, When USB and USB_HOST are detached,
> extcon return the default value instead of error value.
> I think that it is reasonable. Why is it not a valid?
>

I agree; I don't know what else could be returned if no cable is
attached, even if we wanted to.

Guenter

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Roger Quadros]

On 04/08/16 07:09, Guenter Roeck wrote:
> On Wed, Aug 3, 2016 at 5:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> Hi Roger,
>>
>> On 2016년 08월 03일 18:46, Roger Quadros wrote:
>>> Hi Chanwoo,
>>>
> 
> [ ... ]
> 
>>>>>> +  /*
>>>>>> +   * Check whether the external connector is attached.
>>>>>> +   * If external connector is detached, the user can not
>>>>>> +   * get the property value.
>>>>>> +   */
>>>>>
>>>>> How will this work for USB case? We need to know VBUS and ID states
>>>>> even if the USB cable is detached.
>>>>
>>>> When USB is detached, extcon_get_property return the default value without any operation.
>>>> The default value of supported property are 0 (zero). If new property need the differnt default
>>>> value, I'll support it.
>>>
>>> Is the property a property of the connector or of the cable?
>>>
>>> In my opinion, ID and VBUS are properties of the USB connector and not of
>>> the USB cable. So extcon must provide valid status for those properties
>>> even if USB cable or USB_HOST cable is detached.
>>
>> I don't understand about that if USB and USB_HOST are detached,
>> how can the USB be operating? As you mentioned that, extcon must
>> provide the valid status for both state and properties.
>>
> 
> Correct. No cable means that the polarity is unknown, and VBUS must not be
> active (for USB_HOST), or it can not be active (for USB).

OK.

> 
> Only question might be EXTCON_PROP_USB_ID; I am not sure I understand
> what it is supposed to return. Maybe it would be worthwhile to document it ?

Agreed. It seems redundant as ID can be easily inferred from USB_HOST cable state.
i.e. if USB_HOST is attached ID is 0. If USB_HOST is detached ID is 1.

> 
>> So, I already mentioned, When USB and USB_HOST are detached,
>> extcon return the default value instead of error value.
>> I think that it is reasonable. Why is it not a valid?
>>
> 
> I agree; I don't know what else could be returned if no cable is
> attached, even if we wanted to.
> 
OK.

I understood now that if kernel USB driver can interpret EXTCON_USB, EXTCON_USB_HOST
and VBUS property, it sufficiently captures ID and VBUS information.

cheers,
-roger

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi]

Hi Guenter and Roger,

On 2016년 08월 04일 17:49, Roger Quadros wrote:
> On 04/08/16 07:09, Guenter Roeck wrote:
>> On Wed, Aug 3, 2016 at 5:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> Hi Roger,
>>>
>>> On 2016년 08월 03일 18:46, Roger Quadros wrote:
>>>> Hi Chanwoo,
>>>>
>>
>> [ ... ]
>>
>>>>>>> +  /*
>>>>>>> +   * Check whether the external connector is attached.
>>>>>>> +   * If external connector is detached, the user can not
>>>>>>> +   * get the property value.
>>>>>>> +   */
>>>>>>
>>>>>> How will this work for USB case? We need to know VBUS and ID states
>>>>>> even if the USB cable is detached.
>>>>>
>>>>> When USB is detached, extcon_get_property return the default value without any operation.
>>>>> The default value of supported property are 0 (zero). If new property need the differnt default
>>>>> value, I'll support it.
>>>>
>>>> Is the property a property of the connector or of the cable?
>>>>
>>>> In my opinion, ID and VBUS are properties of the USB connector and not of
>>>> the USB cable. So extcon must provide valid status for those properties
>>>> even if USB cable or USB_HOST cable is detached.
>>>
>>> I don't understand about that if USB and USB_HOST are detached,
>>> how can the USB be operating? As you mentioned that, extcon must
>>> provide the valid status for both state and properties.
>>>
>>
>> Correct. No cable means that the polarity is unknown, and VBUS must not be
>> active (for USB_HOST), or it can not be active (for USB).
> 
> OK.
> 
>>
>> Only question might be EXTCON_PROP_USB_ID; I am not sure I understand
>> what it is supposed to return. Maybe it would be worthwhile to document it ?
> 
> Agreed. It seems redundant as ID can be easily inferred from USB_HOST cable state.
> i.e. if USB_HOST is attached ID is 0. If USB_HOST is detached ID is 1.

Do you mean the EXTCON_PROP_USB_ID is un-needed?
I'll remove the EXTCON_PROP_USB_ID property on next version.

> 
>>
>>> So, I already mentioned, When USB and USB_HOST are detached,
>>> extcon return the default value instead of error value.
>>> I think that it is reasonable. Why is it not a valid?
>>>
>>
>> I agree; I don't know what else could be returned if no cable is
>> attached, even if we wanted to.
>>
> OK.
> 
> I understood now that if kernel USB driver can interpret EXTCON_USB, EXTCON_USB_HOST
> and VBUS property, it sufficiently captures ID and VBUS information.
> 

Regards,
Chanwoo Choi

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Guenter Roeck]

On Thu, Aug 4, 2016 at 3:57 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi Guenter and Roger,
>
> On 2016년 08월 04일 17:49, Roger Quadros wrote:
>> On 04/08/16 07:09, Guenter Roeck wrote:
>>> On Wed, Aug 3, 2016 at 5:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>> Hi Roger,
>>>>
>>>> On 2016년 08월 03일 18:46, Roger Quadros wrote:
>>>>> Hi Chanwoo,
>>>>>
>>>
>>> [ ... ]
>>>
>>>>>>>> +  /*
>>>>>>>> +   * Check whether the external connector is attached.
>>>>>>>> +   * If external connector is detached, the user can not
>>>>>>>> +   * get the property value.
>>>>>>>> +   */
>>>>>>>
>>>>>>> How will this work for USB case? We need to know VBUS and ID states
>>>>>>> even if the USB cable is detached.
>>>>>>
>>>>>> When USB is detached, extcon_get_property return the default value without any operation.
>>>>>> The default value of supported property are 0 (zero). If new property need the differnt default
>>>>>> value, I'll support it.
>>>>>
>>>>> Is the property a property of the connector or of the cable?
>>>>>
>>>>> In my opinion, ID and VBUS are properties of the USB connector and not of
>>>>> the USB cable. So extcon must provide valid status for those properties
>>>>> even if USB cable or USB_HOST cable is detached.
>>>>
>>>> I don't understand about that if USB and USB_HOST are detached,
>>>> how can the USB be operating? As you mentioned that, extcon must
>>>> provide the valid status for both state and properties.
>>>>
>>>
>>> Correct. No cable means that the polarity is unknown, and VBUS must not be
>>> active (for USB_HOST), or it can not be active (for USB).
>>
>> OK.
>>
>>>
>>> Only question might be EXTCON_PROP_USB_ID; I am not sure I understand
>>> what it is supposed to return. Maybe it would be worthwhile to document it ?
>>
>> Agreed. It seems redundant as ID can be easily inferred from USB_HOST cable state.
>> i.e. if USB_HOST is attached ID is 0. If USB_HOST is detached ID is 1.
>
> Do you mean the EXTCON_PROP_USB_ID is un-needed?
> I'll remove the EXTCON_PROP_USB_ID property on next version.
>

If ID reflects host vs. device state, yes, it is not needed. Or,
alternatively, one of USB and USB_HOST would not be needed if ID is
present. Since that is not feasible because it would modify the ABI to
user space, dropping ID makes sense.

Thanks,
Guenter

>>
>>>
>>>> So, I already mentioned, When USB and USB_HOST are detached,
>>>> extcon return the default value instead of error value.
>>>> I think that it is reasonable. Why is it not a valid?
>>>>
>>>
>>> I agree; I don't know what else could be returned if no cable is
>>> attached, even if we wanted to.
>>>
>> OK.
>>
>> I understood now that if kernel USB driver can interpret EXTCON_USB, EXTCON_USB_HOST
>> and VBUS property, it sufficiently captures ID and VBUS information.
>>
>
> Regards,
> Chanwoo Choi
>

Re: [PATCH v3 2/6] extcon: Add the support for extcon property according to extcon type

[Chanwoo Choi]

Hi Guenter,

On 2016년 08월 04일 23:47, Guenter Roeck wrote:
> On Thu, Aug 4, 2016 at 3:57 AM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> Hi Guenter and Roger,
>>
>> On 2016년 08월 04일 17:49, Roger Quadros wrote:
>>> On 04/08/16 07:09, Guenter Roeck wrote:
>>>> On Wed, Aug 3, 2016 at 5:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>> Hi Roger,
>>>>>
>>>>> On 2016년 08월 03일 18:46, Roger Quadros wrote:
>>>>>> Hi Chanwoo,
>>>>>>
>>>>
>>>> [ ... ]
>>>>
>>>>>>>>> +  /*
>>>>>>>>> +   * Check whether the external connector is attached.
>>>>>>>>> +   * If external connector is detached, the user can not
>>>>>>>>> +   * get the property value.
>>>>>>>>> +   */
>>>>>>>>
>>>>>>>> How will this work for USB case? We need to know VBUS and ID states
>>>>>>>> even if the USB cable is detached.
>>>>>>>
>>>>>>> When USB is detached, extcon_get_property return the default value without any operation.
>>>>>>> The default value of supported property are 0 (zero). If new property need the differnt default
>>>>>>> value, I'll support it.
>>>>>>
>>>>>> Is the property a property of the connector or of the cable?
>>>>>>
>>>>>> In my opinion, ID and VBUS are properties of the USB connector and not of
>>>>>> the USB cable. So extcon must provide valid status for those properties
>>>>>> even if USB cable or USB_HOST cable is detached.
>>>>>
>>>>> I don't understand about that if USB and USB_HOST are detached,
>>>>> how can the USB be operating? As you mentioned that, extcon must
>>>>> provide the valid status for both state and properties.
>>>>>
>>>>
>>>> Correct. No cable means that the polarity is unknown, and VBUS must not be
>>>> active (for USB_HOST), or it can not be active (for USB).
>>>
>>> OK.
>>>
>>>>
>>>> Only question might be EXTCON_PROP_USB_ID; I am not sure I understand
>>>> what it is supposed to return. Maybe it would be worthwhile to document it ?
>>>
>>> Agreed. It seems redundant as ID can be easily inferred from USB_HOST cable state.
>>> i.e. if USB_HOST is attached ID is 0. If USB_HOST is detached ID is 1.
>>
>> Do you mean the EXTCON_PROP_USB_ID is un-needed?
>> I'll remove the EXTCON_PROP_USB_ID property on next version.
>>
> 
> If ID reflects host vs. device state, yes, it is not needed. Or,
> alternatively, one of USB and USB_HOST would not be needed if ID is
> present. Since that is not feasible because it would modify the ABI to
> user space, dropping ID makes sense.

OK. I'll drop the EXTCON_PROP_USB_ID. Thanks.

Regards,
Chanwoo Choi

> 
> Thanks,
> Guenter
> 
>>>
>>>>
>>>>> So, I already mentioned, When USB and USB_HOST are detached,
>>>>> extcon return the default value instead of error value.
>>>>> I think that it is reasonable. Why is it not a valid?
>>>>>
>>>>
>>>> I agree; I don't know what else could be returned if no cable is
>>>> attached, even if we wanted to.
>>>>
>>> OK.
>>>
>>> I understood now that if kernel USB driver can interpret EXTCON_USB, EXTCON_USB_HOST
>>> and VBUS property, it sufficiently captures ID and VBUS information.
>>>
>>
>> Regards,
>> Chanwoo Choi
>>
> 
> 
>