Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453541 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 2/6] extcon: Add the support for extcon property according  to extcon type Chanwoo Choi <cw00.choi@samsung.com> - 2016-08-02 04:00 +0200
    Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Roger Quadros <rogerq@ti.com> - 2016-08-02 10:00 +0200
      Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Chanwoo Choi <cw00.choi@samsung.com> - 2016-08-02 10:10 +0200
        Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Roger Quadros <rogerq@ti.com> - 2016-08-03 12:30 +0200
          Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Chanwoo Choi <cw00.choi@samsung.com> - 2016-08-04 03:00 +0200
            Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Guenter Roeck <groeck@google.com> - 2016-08-04 06:10 +0200
              Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Roger Quadros <rogerq@ti.com> - 2016-08-04 11:00 +0200
                Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Chanwoo Choi <cw00.choi@samsung.com> - 2016-08-04 13:00 +0200
                  Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Guenter Roeck <groeck@google.com> - 2016-08-04 16:50 +0200
                    Re: [PATCH v3 2/6] extcon: Add the support for extcon property  according to extcon type Chanwoo Choi <cw00.choi@samsung.com> - 2016-08-05 02:50 +0200

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

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

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

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

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

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

[toc] | [next] | [standalone]

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

+Felipe

Hi,

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

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

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

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

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

Is this acceptable?

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

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

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

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

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

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

So MIN is not needed as it is always 0

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

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

and so on.

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

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

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

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

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

Hi,

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

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

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

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

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

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

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

ditto.

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

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

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

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

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

Thanks,
Chanwoo Choi

[toc] | [prev] | [next] | [standalone]

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

Hi Chanwoo,

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

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

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

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

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

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

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

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

Hi Roger,

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

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

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

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

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

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

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

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

Regards,
Chanwoo Choi

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

[toc] | [prev] | [next] | [standalone]

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

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

[ ... ]

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

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

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

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

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

Guenter

[toc] | [prev] | [next] | [standalone]

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

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

OK.

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

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

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

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

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

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

Hi Guenter and Roger,

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

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

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

Regards,
Chanwoo Choi

[toc] | [prev] | [next] | [standalone]

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

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

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

Thanks,
Guenter

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

[toc] | [prev] | [next] | [standalone]

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

Hi Guenter,

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

OK. I'll drop the EXTCON_PROP_USB_ID. Thanks.

Regards,
Chanwoo Choi

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web