All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] extcon: Add the support for extcon type and property
@ 2016-07-26 12:09 Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category Chanwoo Choi
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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.

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);

Depends on:
This patch depend on the extcon patches[2].

[1] https://en.wikipedia.org/wiki/DisplayPort
[2] http://comments.gmane.org/gmane.linux.kernel/2270818

Chanwoo Choi (5):
  extcon: Add the extcon_type to group 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 | 707 ++++++++++++++++++++++++++++++++++++++++--------
 include/linux/extcon.h  | 176 +++++++++++-
 2 files changed, 766 insertions(+), 117 deletions(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 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 5b61000cde26..b1e6ee6194bc 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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
  2016-07-26 22:06   ` Guenter Roeck
  2016-07-27 17:24   ` Guenter Roeck
  2016-07-26 12:09 ` [PATCH 3/6] extcon: Add the support for the capability of each property Chanwoo Choi
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h  |  86 ++++++++++++++++++++
 2 files changed, 291 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index b1e6ee6194bc..2317aaea063f 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,28 @@ 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 id,
+				unsigned int index)
+{
+	return ((!!(edev->state & (1 << index))) == 1) ? true : false;
+}
+
 static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
 {
 	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
@@ -258,6 +285,41 @@ 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)
+{
+	unsigned 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. */
+	if (extcon_info[id].type | type)
+		return true;
+
+	return false;
+}
+
+#define INIT_PROPERTY(name, name_lower, type)				\
+	if (EXTCON_TYPE_##name || type) {				\
+		for (i = 0; i < EXTCON_PROP_##name##_CNT; i++)		\
+			cable->name_lower##_propval[i] = val;		\
+	}								\
+
+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];
+	union extcon_property_value val = (union extcon_property_value)(0);
+	int i;
+
+	INIT_PROPERTY(USB, usb, type);
+	INIT_PROPERTY(CHG, chg, type);
+	INIT_PROPERTY(JACK, jack, type);
+	INIT_PROPERTY(DISP, disp, type);
+}
+
 static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
@@ -421,7 +483,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 (int)(is_extcon_attached(edev, id, index));
 }
 EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
 
@@ -449,12 +511,154 @@ 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);
+
+	/* Set the state for external connector as the detached state. */
 	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;
+
+	/*
+	 * 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, id, index))
+		return 0;
+
+	cable = &edev->cables[index];
+	spin_lock_irqsave(&edev->lock, flags);
+
+	/* 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;
+
+	cable = &edev->cables[index];
+	spin_lock_irqsave(&edev->lock, flags);
+
+	/* 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..296d1452dcb4 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)
+
+/* 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)
+
+/* 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)
+
+/* 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)
+
+/*
+ * 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 : interger (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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/6] extcon: Add the support for the capability of each property
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern Chanwoo Choi
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 drivers/extcon/extcon.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h  |  22 ++++++++
 2 files changed, 155 insertions(+), 2 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 2317aaea063f..f9522fcdb99c 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;
@@ -545,7 +550,7 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id,
 {
 	struct extcon_cable *cable;
 	unsigned long flags;
-	int index, ret = 0;
+	int index, capability, ret = 0;
 
 	*prop_val = (union extcon_property_value)(0);
 
@@ -561,6 +566,11 @@ int extcon_get_property(struct extcon_dev *edev, unsigned int id,
 	if (index < 0)
 		return index;
 
+	/* Check whether the property is available or not. */
+	capability = extcon_get_property_capability(edev, id, prop);
+	if (!capability)
+		return -EPERM;
+
 	/*
 	 * Check whether the external connector is attached.
 	 * If external connector is detached, the user can not
@@ -616,7 +626,7 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 {
 	struct extcon_cable *cable;
 	unsigned long flags;
-	int index, ret = 0;
+	int index, capability, ret = 0;
 
 	if (!edev)
 		return -EINVAL;
@@ -630,6 +640,11 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
 	if (index < 0)
 		return index;
 
+	/* Check whether the property is available or not. */
+	capability = extcon_get_property_capability(edev, id, prop);
+	if (!capability)
+		return -EPERM;
+
 	cable = &edev->cables[index];
 	spin_lock_irqsave(&edev->lock, flags);
 
@@ -659,6 +674,122 @@ 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)
+{
+	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:
+		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;
+}
+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 296d1452dcb4..ee34a644594a 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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
                   ` (2 preceding siblings ...)
  2016-07-26 12:09 ` [PATCH 3/6] extcon: Add the support for the capability of each property Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
  2016-07-26 12:09 ` [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification Chanwoo Choi
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 drivers/extcon/extcon.c | 15 +++++++--------
 include/linux/extcon.h  | 25 ++++++++++++++++++-------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index f9522fcdb99c..73c6981bf559 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -373,8 +373,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]));
 }
 
 /**
@@ -470,11 +469,11 @@ 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;
 
@@ -490,17 +489,17 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
 
 	return (int)(is_extcon_attached(edev, id, index));
 }
-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;
@@ -527,7 +526,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 ee34a644594a..26f4481b3788 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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
                   ` (3 preceding siblings ...)
  2016-07-26 12:09 ` [PATCH 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
  2016-07-27  7:30   ` Chris Zhong
  2016-07-26 12:09 ` [PATCH 6/6] extcon: Add EXTCON_DISP_DP and the property for USB Type-C Chanwoo Choi
       [not found] ` <CGME20160726120955epcas1p1379233158fe6612133799eb2255a5247@epcas1p1.samsung.com>
  6 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 drivers/extcon/extcon.c | 199 ++++++++++++++++++++++++++++++------------------
 include/linux/extcon.h  |  30 +++++++-
 2 files changed, 152 insertions(+), 77 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 73c6981bf559..8572523bfd40 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -280,14 +280,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id,
 	return ((!!(edev->state & (1 << index))) == 1) ? true : false;
 }
 
-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 & (1 << index));
+	return (state != new_state) ? true : false;
 }
 
 static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
@@ -377,21 +374,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];
@@ -400,73 +389,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;
 
-	spin_lock_irqsave(&edev->lock, flags);
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
 
-	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
-		u32 old_state;
+	state = !!(edev->state & (1 << 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;
-		}
+	spin_lock_irqsave(&edev->lock, flags);
 
-		old_state = edev->state;
-		edev->state &= ~mask;
-		edev->state |= state & mask;
+	/* 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);
 
-		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);
-		}
+		dev_err(&edev->dev, "out of memory in extcon_set_state\n");
+		kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
 
-		/* 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.
@@ -493,17 +467,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;
@@ -515,6 +494,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 	if (edev->max_supported && edev->max_supported <= index)
 		return -EINVAL;
 
+	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 & ~(1 << index)) | (cable_state & (1 << index)))) {
+		ret = -EPERM;
+		goto out;
+	}
+
 	/*
 	 * Initialize the value of extcon property before setting
 	 * the detached state for an external connector.
@@ -522,13 +513,44 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
 	if (!cable_state)
 		init_property(edev, id, index);
 
-	/* Set the state for external connector as the detached state. */
-	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 |= (1 << index);
+	else
+		edev->state &= ~(1 << 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;
+
+	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
@@ -673,6 +695,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 26f4481b3788..00ab1180346c 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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/6] extcon: Add EXTCON_DISP_DP and the property for USB Type-C
  2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
                   ` (4 preceding siblings ...)
  2016-07-26 12:09 ` [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification Chanwoo Choi
@ 2016-07-26 12:09 ` Chanwoo Choi
       [not found] ` <CGME20160726120955epcas1p1379233158fe6612133799eb2255a5247@epcas1p1.samsung.com>
  6 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-26 12:09 UTC (permalink / raw)
  To: linux-kernel; +Cc: cw00.choi, myungjoo.ham, zyw, groeck, cwchoi00

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>
---
 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 8572523bfd40..00711e811821 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 00ab1180346c..2f4cde8c38d7 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)
 
 /* Properties of EXTCON_TYPE_CHG. */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
@ 2016-07-26 22:06   ` Guenter Roeck
       [not found]     ` <CAGTfZH2rPyhSiXU=62RKTaE40fok5iiWM+G=+V147gGq8wv_Bw@mail.gmail.com>
  2016-07-27 17:24   ` Guenter Roeck
  1 sibling, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2016-07-26 22:06 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: linux-kernel, myungjoo.ham, Chris Zhong, Guenter Roeck, cwchoi00

On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@samsung.com> 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>
> ---
>  drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/extcon.h  |  86 ++++++++++++++++++++
>  2 files changed, 291 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index b1e6ee6194bc..2317aaea063f 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,28 @@ 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 id,
> +                               unsigned int index)
> +{
> +       return ((!!(edev->state & (1 << index))) == 1) ? true : false;
> +}
> +
>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>  {
>         if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
> @@ -258,6 +285,41 @@ 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)
> +{
> +       unsigned int type;
> +

int

> +       /* Check whether the property is supported or not. */
> +       type = get_extcon_type(prop);
> +       if (type < 0)

otherwise type is never < 0

> +               return false;
> +
> +       /* Check whether a specific extcon id supports the property or not. */
> +       if (extcon_info[id].type | type)

This is always true ?

> +               return true;
> +
> +       return false;
> +}
> +
> +#define INIT_PROPERTY(name, name_lower, type)                          \
> +       if (EXTCON_TYPE_##name || type) {                               \

This is always true ?

> +               for (i = 0; i < EXTCON_PROP_##name##_CNT; i++)          \
> +                       cable->name_lower##_propval[i] = val;           \
> +       }                                                               \
> +
> +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];
> +       union extcon_property_value val = (union extcon_property_value)(0);
> +       int i;
> +
> +       INIT_PROPERTY(USB, usb, type);
> +       INIT_PROPERTY(CHG, chg, type);
> +       INIT_PROPERTY(JACK, jack, type);
> +       INIT_PROPERTY(DISP, disp, type);
> +}
> +
>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>                           char *buf)
>  {
> @@ -421,7 +483,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 (int)(is_extcon_attached(edev, id, index));
>  }
>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>
> @@ -449,12 +511,154 @@ 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);
> +
> +       /* Set the state for external connector as the detached state. */
>         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;
> +
> +       /*
> +        * 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, id, index))
> +               return 0;
> +
> +       cable = &edev->cables[index];
> +       spin_lock_irqsave(&edev->lock, flags);
> +
> +       /* 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;
> +
> +       cable = &edev->cables[index];
> +       spin_lock_irqsave(&edev->lock, flags);
> +
> +       /* 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..296d1452dcb4 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)

        EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1

Otherwise the array won't have enough entries, and writing the last
property will end up overwriting usb_bits (because all other arrays
have a size of 0).

> +
> +/* 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)

        EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1

Otherwise the array won't have any entries.

> +/* 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 : interger (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
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
       [not found]     ` <CAGTfZH2rPyhSiXU=62RKTaE40fok5iiWM+G=+V147gGq8wv_Bw@mail.gmail.com>
@ 2016-07-27  1:15       ` Chris Zhong
  2016-07-27  1:44         ` Guenter Roeck
  0 siblings, 1 reply; 21+ messages in thread
From: Chris Zhong @ 2016-07-27  1:15 UTC (permalink / raw)
  To: chanwoo, Guenter Roeck
  Cc: Chanwoo Choi, linux-kernel, myungjoo.ham, Guenter Roeck

Hi Chanwoo

On 07/27/2016 08:31 AM, Chanwoo Choi wrote:
> Hi Guenter,
>
> 2016년 7월 27일 수요일, Guenter Roeck<groeck@google.com 
> <mailto:groeck@google.com>>님이 작성한 메시지:
>
>     On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi
>     <cw00.choi@samsung.com <javascript:;>> 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 <javascript:;>>
>     > ---
>     >  drivers/extcon/extcon.c | 206
>     +++++++++++++++++++++++++++++++++++++++++++++++-
>     >  include/linux/extcon.h  |  86 ++++++++++++++++++++
>     >  2 files changed, 291 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>     > index b1e6ee6194bc..2317aaea063f 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,28 @@ 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 id,
>     > +                               unsigned int index)
>     > +{
>     > +       return ((!!(edev->state & (1 << index))) == 1) ? true :
>     false;
>     > +}
>     > +
>     >  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool
>     *attached)
>     >  {
>     >         if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>     > @@ -258,6 +285,41 @@ 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)
>     > +{
>     > +       unsigned int type;
>     > +
>
>     int
>
>
> ok.
>
>
>     > +       /* Check whether the property is supported or not. */
>     > +       type = get_extcon_type(prop);
>     > +       if (type < 0)
>
>     otherwise type is never < 0
>
>
> you're right.
>
>
>     > +               return false;
>     > +
>     > +       /* Check whether a specific extcon id supports the
>     property or not. */
>     > +       if (extcon_info[id].type | type)
>
>     This is always true ?
>
>
> It is my mistake. Use '&' instead of '|'.
>
>
>     > +               return true;
>     > +
>     > +       return false;
>     > +}
>     > +
>     > +#define INIT_PROPERTY(name, name_lower, type)             \
>     > +       if (EXTCON_TYPE_##name || type) {              \
>
>     This is always true ?
>
>
> It is my mistake. Use '&' instead of '||'.
>
>
>     > +               for (i = 0; i < EXTCON_PROP_##name##_CNT; i++) 
>             \
>     > +                       cable->name_lower##_propval[i] = val;   
>            \
>     > +       }              \
>     > +
>     > +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];
>     > +       union extcon_property_value val = (union
>     extcon_property_value)(0);
>     > +       int i;
>     > +
>     > +       INIT_PROPERTY(USB, usb, type);
>     > +       INIT_PROPERTY(CHG, chg, type);
>     > +       INIT_PROPERTY(JACK, jack, type);
>     > +       INIT_PROPERTY(DISP, disp, type);
>     > +}
>     > +
>     >  static ssize_t state_show(struct device *dev, struct
>     device_attribute *attr,
>     >                           char *buf)
>     >  {
>     > @@ -421,7 +483,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 (int)(is_extcon_attached(edev, id, index));
>     >  }
>     >  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>     >
>     > @@ -449,12 +511,154 @@ 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);
>     > +
>     > +       /* Set the state for external connector as the detached
>     state. */
>     >         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;
>     > +
>     > +       /*
>     > +        * 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, id, index))
>     > +               return 0;
>     > +
>     > +       cable = &edev->cables[index];
>     > +       spin_lock_irqsave(&edev->lock, flags);
>     > +
>     > +       /* 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;
>     > +
>     > +       cable = &edev->cables[index];
>     > +       spin_lock_irqsave(&edev->lock, flags);
>     > +
>     > +       /* 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..296d1452dcb4 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)
>
>             EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1
>
>     Otherwise the array won't have enough entries, and writing the last
>     property will end up overwriting usb_bits (because all other arrays
>     have a size of 0)
>
>
> You're right. I'll fix it.
>
>
>     > +
>     > +/* 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)
>
>             EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1
>
>     Otherwise the array won't have any entries.
>
>
> ok.
>
>
>     > +/* 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
>
>
> ok.
>
>
>     > +
>     > +/* 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
>
>
> ok.

Tested with these "+1", it works for my DP patch.

>
>     > +
>     > +/*
>     > + * 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 : interger (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
>     >
>
>
> Regards,
> Chanwoo Choi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  1:15       ` Chris Zhong
@ 2016-07-27  1:44         ` Guenter Roeck
  2016-07-27  2:09           ` Chris Zhong
  0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2016-07-27  1:44 UTC (permalink / raw)
  To: Chris Zhong
  Cc: chanwoo, Chanwoo Choi, linux-kernel, myungjoo.ham, Guenter Roeck

Hi Chris,

On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:

[ ... ]

>>
>>     > +
>>     > +/* 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
>>
>>
>> ok.
>
>
> Tested with these "+1", it works for my DP patch.
>

You should be able to use
https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
you didn't do that already).

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  1:44         ` Guenter Roeck
@ 2016-07-27  2:09           ` Chris Zhong
  2016-07-27  3:42             ` Chanwoo Choi
  0 siblings, 1 reply; 21+ messages in thread
From: Chris Zhong @ 2016-07-27  2:09 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: chanwoo, Chanwoo Choi, linux-kernel, myungjoo.ham, Guenter Roeck

Hi Guernter

On 07/27/2016 09:44 AM, Guenter Roeck wrote:
> Hi Chris,
>
> On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>
> [ ... ]
>
>>>      > +
>>>      > +/* 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
>>>
>>>
>>> ok.
>>
>> Tested with these "+1", it works for my DP patch.
>>
> You should be able to use
> https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
> you didn't do that already).
>
> Thanks,
> Guenter

Thanks Guenter, and I saw this bug has fixed in extcon-test branch.


Thanks
Chris Zhong
>
>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  2:09           ` Chris Zhong
@ 2016-07-27  3:42             ` Chanwoo Choi
  2016-07-27  3:51               ` Guenter Roeck
  0 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-27  3:42 UTC (permalink / raw)
  To: Chris Zhong, Guenter Roeck
  Cc: chanwoo, linux-kernel, myungjoo.ham, Guenter Roeck, cpgs

Hi Chris,

On 2016년 07월 27일 11:09, Chris Zhong wrote:
> Hi Guernter
> 
> On 07/27/2016 09:44 AM, Guenter Roeck wrote:
>> Hi Chris,
>>
>> On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>>
>> [ ... ]
>>
>>>>      > +
>>>>      > +/* 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
>>>>
>>>>
>>>> ok.
>>>
>>> Tested with these "+1", it works for my DP patch.
>>>
>> You should be able to use
>> https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
>> you didn't do that already).
>>
>> Thanks,
>> Guenter
> 
> Thanks Guenter, and I saw this bug has fixed in extcon-test branch.
> 
> 

Do you test it with extcon_set_property_capability()?
And if you test this patch-es, could you send the tested-by tag for these patches?

I'll send the next version after a few days.

Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  3:42             ` Chanwoo Choi
@ 2016-07-27  3:51               ` Guenter Roeck
  2016-07-27  3:57                 ` Chanwoo Choi
  0 siblings, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2016-07-27  3:51 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: Chris Zhong, chanwoo, linux-kernel, myungjoo.ham, Guenter Roeck, cpgs

On Tue, Jul 26, 2016 at 8:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
> Hi Chris,
>
> On 2016년 07월 27일 11:09, Chris Zhong wrote:
>> Hi Guernter
>>
>> On 07/27/2016 09:44 AM, Guenter Roeck wrote:
>>> Hi Chris,
>>>
>>> On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>>>
>>> [ ... ]
>>>
>>>>>      > +
>>>>>      > +/* 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
>>>>>
>>>>>
>>>>> ok.
>>>>
>>>> Tested with these "+1", it works for my DP patch.
>>>>
>>> You should be able to use
>>> https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
>>> you didn't do that already).
>>>
>>> Thanks,
>>> Guenter
>>
>> Thanks Guenter, and I saw this bug has fixed in extcon-test branch.
>>
>>
>
> Do you test it with extcon_set_property_capability()?
> And if you test this patch-es, could you send the tested-by tag for these patches?
>

For my part I did. Above link is public, so you should be able to see
the complete patch set which uses the new API from
drivers/extcon/extcon-cros_ec.c.

I'll re-test tomorrow with the updated patches from your test branch.

> I'll send the next version after a few days.

Great.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  3:51               ` Guenter Roeck
@ 2016-07-27  3:57                 ` Chanwoo Choi
  2016-07-27  4:30                   ` Chris Zhong
  0 siblings, 1 reply; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-27  3:57 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Chris Zhong, chanwoo, linux-kernel, myungjoo.ham, Guenter Roeck, cpgs

On 2016년 07월 27일 12:51, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 8:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> Hi Chris,
>>
>> On 2016년 07월 27일 11:09, Chris Zhong wrote:
>>> Hi Guernter
>>>
>>> On 07/27/2016 09:44 AM, Guenter Roeck wrote:
>>>> Hi Chris,
>>>>
>>>> On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>      > +
>>>>>>      > +/* 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
>>>>>>
>>>>>>
>>>>>> ok.
>>>>>
>>>>> Tested with these "+1", it works for my DP patch.
>>>>>
>>>> You should be able to use
>>>> https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
>>>> you didn't do that already).
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> Thanks Guenter, and I saw this bug has fixed in extcon-test branch.
>>>
>>>
>>
>> Do you test it with extcon_set_property_capability()?
>> And if you test this patch-es, could you send the tested-by tag for these patches?
>>
> 
> For my part I did. Above link is public, so you should be able to see
> the complete patch set which uses the new API from
> drivers/extcon/extcon-cros_ec.c.
> 
> I'll re-test tomorrow with the updated patches from your test branch.

OK. Thanks. 

Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category
       [not found] ` <CGME20160726120955epcas1p1379233158fe6612133799eb2255a5247@epcas1p1.samsung.com>
@ 2016-07-27  4:27   ` MyungJoo Ham
  2016-07-27  5:35     ` Chanwoo Choi
  0 siblings, 1 reply; 21+ messages in thread
From: MyungJoo Ham @ 2016-07-27  4:27 UTC (permalink / raw)
  To: Chanwoo Choi, linux-kernel; +Cc: zyw, groeck, cwchoi00

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

> 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

"Jack" seems to be an internal jargon that many people won't recognize.
It's, in fact, 3.5-pi, isn't it?

Anyway, this is already being used as an enum with drivers,
I'd suggest to add a comment in extcon.h stating that 
"Jack connector" is usually the 3.5-pi earbud connector.

Anyway, I like the direction of this patch.


Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>

Cheers,
MyungJoo

> --- 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 */
+                                /* Usually, this is a 3.5-pi earbud conector */
> +#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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27  3:57                 ` Chanwoo Choi
@ 2016-07-27  4:30                   ` Chris Zhong
  0 siblings, 0 replies; 21+ messages in thread
From: Chris Zhong @ 2016-07-27  4:30 UTC (permalink / raw)
  To: Chanwoo Choi, Guenter Roeck
  Cc: chanwoo, linux-kernel, myungjoo.ham, Guenter Roeck, cpgs

Hi Chanwoo

On 07/27/2016 11:57 AM, Chanwoo Choi wrote:
> On 2016년 07월 27일 12:51, Guenter Roeck wrote:
>> On Tue, Jul 26, 2016 at 8:42 PM, Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>> Hi Chris,
>>>
>>> On 2016년 07월 27일 11:09, Chris Zhong wrote:
>>>> Hi Guernter
>>>>
>>>> On 07/27/2016 09:44 AM, Guenter Roeck wrote:
>>>>> Hi Chris,
>>>>>
>>>>> On Tue, Jul 26, 2016 at 6:15 PM, Chris Zhong <zyw@rock-chips.com> wrote:
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>>       > +
>>>>>>>       > +/* 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
>>>>>>>
>>>>>>>
>>>>>>> ok.
>>>>>> Tested with these "+1", it works for my DP patch.
>>>>>>
>>>>> You should be able to use
>>>>> https://chromium-review.googlesource.com/#/c/363623/1 as baseline (if
>>>>> you didn't do that already).
>>>>>
>>>>> Thanks,
>>>>> Guenter
>>>> Thanks Guenter, and I saw this bug has fixed in extcon-test branch.
>>>>
>>>>
>>> Do you test it with extcon_set_property_capability()?
>>> And if you test this patch-es, could you send the tested-by tag for these patches?
>>>
Yes, the new API need this extcon_set_property_capability to be called 
before setting property.
On this basis, My DP patches works well with a little bit modification. 
Then, I will post the V7 version soon, base on this patches and 
Guenter's FIXUP patch.
Please feel free to add my Tested-by: Chris Zhong <zyw@rock-chips.com> 
tag in the next version.


>> For my part I did. Above link is public, so you should be able to see
>> the complete patch set which uses the new API from
>> drivers/extcon/extcon-cros_ec.c.
>>
>> I'll re-test tomorrow with the updated patches from your test branch.




> OK. Thanks.
>
> Regards,
> Chanwoo Choi
>
>
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category
  2016-07-27  4:27   ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category MyungJoo Ham
@ 2016-07-27  5:35     ` Chanwoo Choi
  0 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-27  5:35 UTC (permalink / raw)
  To: myungjoo.ham, linux-kernel; +Cc: zyw, groeck, cwchoi00

Hi Myungjoo,

On 2016년 07월 27일 13:27, MyungJoo Ham 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
> 
> "Jack" seems to be an internal jargon that many people won't recognize.
> It's, in fact, 3.5-pi, isn't it?

You're right. But, the ALSA framework already used the 'jack' word
for headphone/headset/microphone for a long time. (sound/soc/soc-jack.c)
So, To prevent the confusion, I use the 'jack' word.

> 
> Anyway, this is already being used as an enum with drivers,
> I'd suggest to add a comment in extcon.h stating that 
> "Jack connector" is usually the 3.5-pi earbud connector.

Additionally,  jack means the the LINE_IN/OUT, VIDEO_IN/OUT,
SPDIF_IN/OUT in the extcon.

> 
> Anyway, I like the direction of this patch.

Thanks.

> 
> 
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>

Regards,
Chanwoo Choi

> 
> Cheers,
> MyungJoo
> 
>> --- 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 */
> +                                /* Usually, this is a 3.5-pi earbud conector */
>> +#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
>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification
  2016-07-26 12:09 ` [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification Chanwoo Choi
@ 2016-07-27  7:30   ` Chris Zhong
  2016-07-27  7:41     ` Chanwoo Choi
  0 siblings, 1 reply; 21+ messages in thread
From: Chris Zhong @ 2016-07-27  7:30 UTC (permalink / raw)
  To: Chanwoo Choi, linux-kernel; +Cc: myungjoo.ham, groeck, cwchoi00

Hi Chanwoo

On 07/26/2016 08:09 PM, Chanwoo Choi wrote:
> 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>
> ---
>   drivers/extcon/extcon.c | 199 ++++++++++++++++++++++++++++++------------------
>   include/linux/extcon.h  |  30 +++++++-
>   2 files changed, 152 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 73c6981bf559..8572523bfd40 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -280,14 +280,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id,
>   	return ((!!(edev->state & (1 << index))) == 1) ? true : false;
>   }
>   
> -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 & (1 << index));
> +	return (state != new_state) ? true : false;
>   }
>   
>   static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
> @@ -377,21 +374,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];
> @@ -400,73 +389,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;
>   
> -	spin_lock_irqsave(&edev->lock, flags);
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
>   
> -	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
> -		u32 old_state;
> +	state = !!(edev->state & (1 << 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;
> -		}
> +	spin_lock_irqsave(&edev->lock, flags);
>   
> -		old_state = edev->state;
> -		edev->state &= ~mask;
> -		edev->state |= state & mask;
> +	/* 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);
>   
> -		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);
> -		}
> +		dev_err(&edev->dev, "out of memory in extcon_set_state\n");
> +		kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
>   
> -		/* 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.
> @@ -493,17 +467,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;
> @@ -515,6 +494,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   	if (edev->max_supported && edev->max_supported <= index)
>   		return -EINVAL;
>   
> +	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 & ~(1 << index)) | (cable_state & (1 << index)))) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
>   	/*
>   	 * Initialize the value of extcon property before setting
>   	 * the detached state for an external connector.
> @@ -522,13 +513,44 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   	if (!cable_state)
>   		init_property(edev, id, index);
>   
> -	/* Set the state for external connector as the detached state. */
> -	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 |= (1 << index);
> +	else
> +		edev->state &= ~(1 << 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;
> +
> +	ret = extcon_set_state(edev, id, cable_state);
> +	if (ret < 0)
> +		return ret;
> +
> +	return extcon_sync(edev, id);
So, Regardless of whether the state change, the notifier chain will be 
called,
If this function can decide whether to call a notice, according to the 
state change. I think it would be better.
The old extcon_set_cable_state_ was working like this.

> +}
> +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
> @@ -673,6 +695,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 26f4481b3788..00ab1180346c 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__ */

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification
  2016-07-27  7:30   ` Chris Zhong
@ 2016-07-27  7:41     ` Chanwoo Choi
  0 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-27  7:41 UTC (permalink / raw)
  To: Chris Zhong, linux-kernel; +Cc: myungjoo.ham, groeck, cwchoi00

Hi Chris,

On 2016년 07월 27일 16:30, Chris Zhong wrote:
> Hi Chanwoo
> 
> On 07/26/2016 08:09 PM, Chanwoo Choi wrote:
>> 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>
>> ---
>>   drivers/extcon/extcon.c | 199 ++++++++++++++++++++++++++++++------------------
>>   include/linux/extcon.h  |  30 +++++++-
>>   2 files changed, 152 insertions(+), 77 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 73c6981bf559..8572523bfd40 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -280,14 +280,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id,
>>       return ((!!(edev->state & (1 << index))) == 1) ? true : false;
>>   }
>>   -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 & (1 << index));
>> +    return (state != new_state) ? true : false;
>>   }
>>     static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>> @@ -377,21 +374,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];
>> @@ -400,73 +389,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;
>>   -    spin_lock_irqsave(&edev->lock, flags);
>> +    index = find_cable_index_by_id(edev, id);
>> +    if (index < 0)
>> +        return index;
>>   -    if (edev->state != ((edev->state & ~mask) | (state & mask))) {
>> -        u32 old_state;
>> +    state = !!(edev->state & (1 << 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;
>> -        }
>> +    spin_lock_irqsave(&edev->lock, flags);
>>   -        old_state = edev->state;
>> -        edev->state &= ~mask;
>> -        edev->state |= state & mask;
>> +    /* 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);
>>   -        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);
>> -        }
>> +        dev_err(&edev->dev, "out of memory in extcon_set_state\n");
>> +        kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
>>   -        /* 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.
>> @@ -493,17 +467,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;
>> @@ -515,6 +494,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>>       if (edev->max_supported && edev->max_supported <= index)
>>           return -EINVAL;
>>   +    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 & ~(1 << index)) | (cable_state & (1 << index)))) {
>> +        ret = -EPERM;
>> +        goto out;
>> +    }
>> +
>>       /*
>>        * Initialize the value of extcon property before setting
>>        * the detached state for an external connector.
>> @@ -522,13 +513,44 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>>       if (!cable_state)
>>           init_property(edev, id, index);
>>   -    /* Set the state for external connector as the detached state. */
>> -    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 |= (1 << index);
>> +    else
>> +        edev->state &= ~(1 << 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;
>> +
>> +    ret = extcon_set_state(edev, id, cable_state);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return extcon_sync(edev, id);
> So, Regardless of whether the state change, the notifier chain will be called,
> If this function can decide whether to call a notice, according to the state change. I think it would be better.
> The old extcon_set_cable_state_ was working like this.

OK. I'll modify it on next version.

[snip]

Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
  2016-07-26 22:06   ` Guenter Roeck
@ 2016-07-27 17:24   ` Guenter Roeck
  2016-07-29  7:02     ` Chanwoo Choi
  1 sibling, 1 reply; 21+ messages in thread
From: Guenter Roeck @ 2016-07-27 17:24 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: linux-kernel, myungjoo.ham, Chris Zhong, Guenter Roeck, cwchoi00

On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@samsung.com> 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>
> ---
>  drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/extcon.h  |  86 ++++++++++++++++++++
>  2 files changed, 291 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index b1e6ee6194bc..2317aaea063f 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,28 @@ 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 id,

'id' isn't used.

> +                               unsigned int index)
> +{
> +       return ((!!(edev->state & (1 << index))) == 1) ? true : false;

Minor comment: This is identical to

             return !!(edev->state & (1 << index));
or, with bitops
             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 +285,41 @@ 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)
> +{
> +       unsigned 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. */
> +       if (extcon_info[id].type | type)
> +               return true;
> +
> +       return false;

simpler:
            return !!(extcon_info[id].type & type);

Strictly speaking, the !! isn't even necessary in those mask
operations since C auto-converts to bool, though people sometimes get
confused by that.

> +}
> +
> +#define INIT_PROPERTY(name, name_lower, type)                          \
> +       if (EXTCON_TYPE_##name || type) {                               \
> +               for (i = 0; i < EXTCON_PROP_##name##_CNT; i++)          \
> +                       cable->name_lower##_propval[i] = val;           \
> +       }                                                               \
> +
> +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];
> +       union extcon_property_value val = (union extcon_property_value)(0);
> +       int i;
> +
> +       INIT_PROPERTY(USB, usb, type);
> +       INIT_PROPERTY(CHG, chg, type);
> +       INIT_PROPERTY(JACK, jack, type);
> +       INIT_PROPERTY(DISP, disp, type);

Just wondering if this would be a bit cleaner/simpler.

        switch(type) {
        case EXTCON_TYPE_USB:
                memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
                break;
        case EXTCON_TYPE_CHG:
                memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
                break;
        case EXTCON_TYPE_JACK:
                memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
                break;
        case EXTCON_TYPE_DISP:
                memset(cable->disp_propval, sizeof(cable->disp_propval), 0);
                break;
        }

> +}
> +
>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>                           char *buf)
>  {
> @@ -421,7 +483,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 (int)(is_extcon_attached(edev, id, index));
>  }
>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>
> @@ -449,12 +511,154 @@ 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);
> +
> +       /* Set the state for external connector as the detached state. */
>         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;
> +
> +       /*
> +        * 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, id, index))
> +               return 0;
> +

Wonder if this should be inside the lock. Otherwise the cable state
might change after the check, but before the lock is acquired.

> +       cable = &edev->cables[index];
> +       spin_lock_irqsave(&edev->lock, flags);
> +
> +       /* 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;
> +
> +       cable = &edev->cables[index];
> +       spin_lock_irqsave(&edev->lock, flags);
> +
> +       /* 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..296d1452dcb4 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)
> +
> +/* 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)
> +
> +/* 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)
> +
> +/* 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)
> +
> +/*
> + * 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 : interger (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
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/6] extcon: Add the support for extcon property according to extcon type
  2016-07-27 17:24   ` Guenter Roeck
@ 2016-07-29  7:02     ` Chanwoo Choi
  0 siblings, 0 replies; 21+ messages in thread
From: Chanwoo Choi @ 2016-07-29  7:02 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-kernel, myungjoo.ham, Chris Zhong, Guenter Roeck, cwchoi00,
	cpgs (cpgs@samsung.com)

Hi Guenter,

On 2016년 07월 28일 02:24, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 5:09 AM, Chanwoo Choi <cw00.choi@samsung.com> 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>
>> ---
>>  drivers/extcon/extcon.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/extcon.h  |  86 ++++++++++++++++++++
>>  2 files changed, 291 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index b1e6ee6194bc..2317aaea063f 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,28 @@ 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 id,
> 
> 'id' isn't used.

I'll remove it on parameter list.

> 
>> +                               unsigned int index)
>> +{
>> +       return ((!!(edev->state & (1 << index))) == 1) ? true : false;
> 
> Minor comment: This is identical to
> 
>              return !!(edev->state & (1 << index));
> or, with bitops
>              return !!(edev->state & BIT(index));

I'll use the bitops as you comment.
	
	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 +285,41 @@ 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)
>> +{
>> +       unsigned 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. */
>> +       if (extcon_info[id].type | type)
>> +               return true;
>> +
>> +       return false;
> 
> simpler:
>             return !!(extcon_info[id].type & type);


OK. I'll use it as you comment.

> 
> Strictly speaking, the !! isn't even necessary in those mask
> operations since C auto-converts to bool, though people sometimes get
> confused by that.

Thanks for your explanation. For readability, I remain the '!!' operation.

> 
>> +}
>> +
>> +#define INIT_PROPERTY(name, name_lower, type)                          \
>> +       if (EXTCON_TYPE_##name || type) {                               \
>> +               for (i = 0; i < EXTCON_PROP_##name##_CNT; i++)          \
>> +                       cable->name_lower##_propval[i] = val;           \
>> +       }                                                               \
>> +
>> +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];
>> +       union extcon_property_value val = (union extcon_property_value)(0);
>> +       int i;
>> +
>> +       INIT_PROPERTY(USB, usb, type);
>> +       INIT_PROPERTY(CHG, chg, type);
>> +       INIT_PROPERTY(JACK, jack, type);
>> +       INIT_PROPERTY(DISP, disp, type);
> 
> Just wondering if this would be a bit cleaner/simpler.
> 
>         switch(type) {
>         case EXTCON_TYPE_USB:
>                 memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
>                 break;
>         case EXTCON_TYPE_CHG:
>                 memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
>                 break;
>         case EXTCON_TYPE_JACK:
>                 memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
>                 break;
>         case EXTCON_TYPE_DISP:
>                 memset(cable->disp_propval, sizeof(cable->disp_propval), 0);
>                 break;
>         }

As you comment, I'll modify it as following:
But the each id is able to have the one more extcon type. So, I use the
'if' instead of 'switch'.

	if (EXTCON_TYPE_USB & type)
		memset(cable->usb_propval, sizeof(cable->usb_propval), 0);
	if (EXTCON_TYPE_CHG & type)
		memset(cable->chg_propval, sizeof(cable->chg_propval), 0);
	if (EXTCON_TYPE_JACK & type)
		memset(cable->jack_propval, sizeof(cable->jack_propval), 0);
	if (EXTCON_TYPE_DISP & type)
		memset(cable->disp_propval, sizeof(cable->disp_propval), 0);

> 
>> +}
>> +
>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>                           char *buf)
>>  {
>> @@ -421,7 +483,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 (int)(is_extcon_attached(edev, id, index));
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>
>> @@ -449,12 +511,154 @@ 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);
>> +
>> +       /* Set the state for external connector as the detached state. */
>>         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;
>> +
>> +       /*
>> +        * 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, id, index))
>> +               return 0;
>> +
> 
> Wonder if this should be inside the lock. Otherwise the cable state
> might change after the check, but before the lock is acquired.
> 
>> +       cable = &edev->cables[index];
>> +       spin_lock_irqsave(&edev->lock, flags);

You're right. I'll fix it as following.

	spin_lock_irqsave(&edev->lock, flags);
	if (!is_extcon_attached(edev, id, 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);

[snip]

Regards,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2016-07-29  7:02 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-26 12:09 [PATCH 0/6] extcon: Add the support for extcon type and property Chanwoo Choi
2016-07-26 12:09 ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category Chanwoo Choi
2016-07-26 12:09 ` [PATCH 2/6] extcon: Add the support for extcon property according to extcon type Chanwoo Choi
2016-07-26 22:06   ` Guenter Roeck
     [not found]     ` <CAGTfZH2rPyhSiXU=62RKTaE40fok5iiWM+G=+V147gGq8wv_Bw@mail.gmail.com>
2016-07-27  1:15       ` Chris Zhong
2016-07-27  1:44         ` Guenter Roeck
2016-07-27  2:09           ` Chris Zhong
2016-07-27  3:42             ` Chanwoo Choi
2016-07-27  3:51               ` Guenter Roeck
2016-07-27  3:57                 ` Chanwoo Choi
2016-07-27  4:30                   ` Chris Zhong
2016-07-27 17:24   ` Guenter Roeck
2016-07-29  7:02     ` Chanwoo Choi
2016-07-26 12:09 ` [PATCH 3/6] extcon: Add the support for the capability of each property Chanwoo Choi
2016-07-26 12:09 ` [PATCH 4/6] extcon: Rename the extcon_set/get_state() to maintain the function naming pattern Chanwoo Choi
2016-07-26 12:09 ` [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification Chanwoo Choi
2016-07-27  7:30   ` Chris Zhong
2016-07-27  7:41     ` Chanwoo Choi
2016-07-26 12:09 ` [PATCH 6/6] extcon: Add EXTCON_DISP_DP and the property for USB Type-C Chanwoo Choi
     [not found] ` <CGME20160726120955epcas1p1379233158fe6612133799eb2255a5247@epcas1p1.samsung.com>
2016-07-27  4:27   ` [PATCH 1/6] extcon: Add the extcon_type to group each connector into five category MyungJoo Ham
2016-07-27  5:35     ` Chanwoo Choi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.