[PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Previously, I discussed how to inform the changed state of both ID
and VBUS pin for USB connector on patch-set[1].
[1] https://lkml.org/lkml/2015/4/2/310

So, this patch adds the extcon_set_cable_line_state() function to inform
the additional state of external connectors without additional register/
unregister functions. This function uses the existing notifier chain
which is registered by extcon_register_notifier() / extcon_register_interest().

The extcon_set_cable_line_state() can inform the new state of both
ID and VBUS pin state through extcon_set_cable_line_state().

For exmaple:
- On extcon-usb-gpio.c as extcon provider driver as following:
        static void usb_extcon_detect_cable(struct work_struct *work)
        {
                ...
                /* check ID and update cable state */
                id = gpiod_get_value_cansleep(info->id_gpiod);
                if (id) {
                        extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
                        extcon_set_cable_state_(info->edev, EXTCON_USB, true);

                        extcon_set_cable_line_state(info->edev, EXTCON_USB,
                                                        EXTCON_USB_ID_HIGH);
                } else {
                        extcon_set_cable_state_(info->edev, EXTCON_USB, false);
                        extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);

                        extcon_set_cable_line_state(info->edev, EXTCON_USB,
                                                        EXTCON_USB_ID_LOW);
                }
        }

- On specific extcon consumder driver as following:
        static int xxx_probe(struct platform_device *pdev)
        {
                struct notifier_chain nh;

                nb.notifier_call = extcon_usb_notifier;
                ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
                ...
        }

        static int extcon_usb_notifier(struct notifier_block *self,
                unsigned long event, void *ptr)
        {
                switch (event) {
                case EXTCON_DETACHED:
                        printk("USB is detached\n");
                        break;
                case EXTCON_ATTACHED:
                        printk("USB is attached\n");
                        break;

                case EXTCON_USB_ID_LOW:
                        printk("USB's ID pin is low state\n");
                        break;
                case EXTCON_USB_ID_HIGH:
                        printk("USB's ID pin is high state\n");
                        break;
                case EXTCON_USB_VBUS_LOW:
                        printk("USB's VBUS pin is high state\n");
                        break;
                case EXTCON_USB_VBUS_HIGH:
                        printk("USB's VBUS pin is high state\n");
                        break;
                default:
                        return -EINVAL;
                };
        }

Changes from v1:
- Use dev_warn() instead of dev_info() if set the same extcon_line_state value.

Chanwoo Choi (2):
  extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors
  extcon: usb-gpio: Update the ID pin state of USB when cable state is changed

 drivers/extcon/extcon-usb-gpio.c |  6 ++++
 drivers/extcon/extcon.c          | 74 +++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h           | 24 +++++++++++++
 3 files changed, 103 insertions(+), 1 deletion(-)

-- 
2.2.0.GIT

[PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Chanwoo Choi]

This patch adds the extcon_set_cable_line_state() function to inform
the additional state of each external connector and 'enum extcon_line_state'
enumeration which include the specific states of each external connector.

The each external connector might need the different line state. So, current
'extcon_line_state' enumeration contains the specific state for USB as
following:

- Following the state mean the state of both ID and VBUS line for USB:
enum extcon_line_state {
	EXTCON_USB_ID_LOW	= BIT(1),	/* ID line is low. */
	EXTCON_USB_ID_HIGH	= BIT(2),	/* ID line is high. */
	EXTCON_USB_VBUS_LOW	= BIT(3),	/* VBUS line is low. */
	EXTCON_USB_VBUS_HIGH	= BIT(4),	/* VBUS line is high. */
};

Cc: Myungjoo Ham <cw00.choi@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/extcon.h  | 24 ++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 5099c11..142bf0f 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
 
 		for (index = 0; index < edev->max_supported; index++) {
 			if (is_extcon_changed(edev->state, state, index, &attached))
-				raw_notifier_call_chain(&edev->nh[index], attached, edev);
+				raw_notifier_call_chain(&edev->nh[index],
+					attached ? EXTCON_ATTACHED :
+					EXTCON_DETACHED, edev);
 		}
 
 		edev->state &= ~mask;
@@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev,
 EXPORT_SYMBOL_GPL(extcon_set_cable_state);
 
 /**
+ * extcon_set_cable_line_state() - Set the line state of specific cable.
+ * @edev:		the extcon device that has the cable.
+ * @id:			the unique id of each external connector.
+ * @state:		the line state for specific cable.
+ *
+ * Note that this function support the only USB connector to inform the state
+ * of both ID and VBUS line until now. This function may be extended to support
+ * the additional external connectors.
+ *
+ * If the id is EXTCON_USB, it can support only following line states:
+ * - EXTCON_USB_ID_LOW
+ * - EXTCON_USB_ID_HIGH,
+ * - EXTCON_USB_VBUS_LOW
+ * - EXTCON_USB_VBUS_HIGH
+ */
+int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
+				enum extcon_line_state state)
+{
+	unsigned long flags;
+	unsigned long line_state;
+	int ret = 0, index;
+
+	index = find_cable_index_by_id(edev, id);
+	if (index < 0)
+		return index;
+
+	spin_lock_irqsave(&edev->lock, flags);
+	line_state = edev->line_state[index];
+
+	switch (id) {
+	case EXTCON_USB:
+		if (line_state & state) {
+			dev_warn(&edev->dev,
+				"0x%x state is already set for %s\n",
+				state, extcon_name[id]);
+			goto err;
+		}
+
+		if ((state & EXTCON_USB_ID_LOW) || (state & EXTCON_USB_ID_HIGH))
+			line_state &= ~(EXTCON_USB_ID_LOW | EXTCON_USB_ID_HIGH);
+
+		if ((state & EXTCON_USB_VBUS_LOW)
+			|| (state & EXTCON_USB_VBUS_HIGH))
+			line_state &=
+				~(EXTCON_USB_VBUS_LOW | EXTCON_USB_VBUS_HIGH);
+
+		line_state |= state;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+	edev->line_state[index] = line_state;
+
+	ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev);
+err:
+	spin_unlock_irqrestore(&edev->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(extcon_set_cable_line_state);
+
+/**
  * extcon_get_extcon_dev() - Get the extcon device instance from the name
  * @extcon_name:	The extcon name provided with extcon_dev_register()
  */
@@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev)
 		goto err_dev;
 	}
 
+	edev->line_state = devm_kzalloc(&edev->dev,
+		sizeof(*edev->line_state) * edev->max_supported, GFP_KERNEL);
+	if (!edev->line_state) {
+		ret = -ENOMEM;
+		goto err_dev;
+	}
+
 	for (index = 0; index < edev->max_supported; index++)
 		RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
 
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index be9652b..79e5073 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -66,6 +66,19 @@ enum extcon {
 	EXTCON_END,
 };
 
+enum extcon_line_state {
+	/* Following two definition are used for whether external connectors
+	 * is attached or detached. */
+	EXTCON_DETACHED		= 0x0,
+	EXTCON_ATTACHED		= 0x1,
+
+	/* Following states are only used for EXTCON_USB. */
+	EXTCON_USB_ID_LOW	= BIT(1),	/* ID line is low. */
+	EXTCON_USB_ID_HIGH	= BIT(2),	/* ID line is high. */
+	EXTCON_USB_VBUS_LOW	= BIT(3),	/* VBUS line is low. */
+	EXTCON_USB_VBUS_HIGH	= BIT(4),	/* VBUS line is high. */
+};
+
 struct extcon_cable;
 
 /**
@@ -90,6 +103,8 @@ struct extcon_cable;
  * @dev:		Device of this extcon.
  * @state:		Attach/detach state of this extcon. Do not provide at
  *			register-time.
+ * @line_state:		Line state for each external connecotrs are included in
+ *			this extcon device.
  * @nh:			Notifier for the state change events from this extcon
  * @entry:		To support list of extcon devices so that users can
  *			search for extcon devices based on the extcon name.
@@ -121,6 +136,7 @@ struct extcon_dev {
 	int max_supported;
 	spinlock_t lock;	/* could be called by irq handler */
 	u32 state;
+	unsigned long *line_state;
 
 	/* /sys/class/extcon/.../cable.n/... */
 	struct device_type extcon_dev_type;
@@ -217,6 +233,8 @@ extern int extcon_get_cable_state(struct extcon_dev *edev,
 				  const char *cable_name);
 extern int extcon_set_cable_state(struct extcon_dev *edev,
 				  const char *cable_name, bool cable_state);
+extern int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
+					enum extcon_line_state state);
 
 /*
  * Following APIs are for notifiees (those who want to be notified)
@@ -324,6 +342,12 @@ static inline int extcon_set_cable_state(struct extcon_dev *edev,
 	return 0;
 }
 
+static inline int extcon_set_cable_line_state(struct extcon_dev *edev,
+			enum extcon id, enum extcon_line_state state)
+{
+	return 0;
+}
+
 static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 {
 	return NULL;
-- 
2.2.0.GIT

[PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed

[Chanwoo Choi]

This patch updates the ID pin state of USB when cable state is changed
by using the extcon_set_cable_line_state() function. The extcon consumer driver
can receive the changed ID pin state through registered notifier chain of
extcon consumer driver.

Cc: Roger Quadros <rogerq@ti.com>
Cc: Robert Baldyga <r.baldyga@samsung.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 14da94c..9ff3171 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -62,12 +62,18 @@ static void usb_extcon_detect_cable(struct work_struct *work)
 		 */
 		extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
 		extcon_set_cable_state_(info->edev, EXTCON_USB, true);
+
+		extcon_set_cable_line_state(info->edev, EXTCON_USB,
+						EXTCON_USB_ID_HIGH);
 	} else {
 		/*
 		 * ID = 0 means USB HOST cable attached.
 		 * As we don't have event for USB peripheral cable detached,
 		 * we simulate USB peripheral detach here.
 		 */
+		extcon_set_cable_line_state(info->edev, EXTCON_USB,
+						EXTCON_USB_ID_LOW);
+
 		extcon_set_cable_state_(info->edev, EXTCON_USB, false);
 		extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
 	}
-- 
2.2.0.GIT

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Roger Quadros]

Chanwoo,

On 27/05/15 15:15, Chanwoo Choi wrote:
> Previously, I discussed how to inform the changed state of both ID
> and VBUS pin for USB connector on patch-set[1].
> [1] https://lkml.org/lkml/2015/4/2/310
>
> So, this patch adds the extcon_set_cable_line_state() function to inform
> the additional state of external connectors without additional register/
> unregister functions. This function uses the existing notifier chain
> which is registered by extcon_register_notifier() / extcon_register_interest().
>
> The extcon_set_cable_line_state() can inform the new state of both
> ID and VBUS pin state through extcon_set_cable_line_state().
>
> For exmaple:
> - On extcon-usb-gpio.c as extcon provider driver as following:
>          static void usb_extcon_detect_cable(struct work_struct *work)
>          {
>                  ...
>                  /* check ID and update cable state */
>                  id = gpiod_get_value_cansleep(info->id_gpiod);
>                  if (id) {
>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);

Now that all USB line states can be captured by a single cable type
can we get rid of EXTCON_USB_HOST?

That way the extcon driver doesn't need to make any decisions as to
what mode we're in (host/cable) and this is best left to the USB driver.

cheers,
-roger

>                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>
>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>                                                          EXTCON_USB_ID_HIGH);
>                  } else {
>                          extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
>
>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>                                                          EXTCON_USB_ID_LOW);
>                  }
>          }
>
> - On specific extcon consumder driver as following:
>          static int xxx_probe(struct platform_device *pdev)
>          {
>                  struct notifier_chain nh;
>
>                  nb.notifier_call = extcon_usb_notifier;
>                  ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
>                  ...
>          }
>
>          static int extcon_usb_notifier(struct notifier_block *self,
>                  unsigned long event, void *ptr)
>          {
>                  switch (event) {
>                  case EXTCON_DETACHED:
>                          printk("USB is detached\n");
>                          break;
>                  case EXTCON_ATTACHED:
>                          printk("USB is attached\n");
>                          break;
>
>                  case EXTCON_USB_ID_LOW:
>                          printk("USB's ID pin is low state\n");
>                          break;
>                  case EXTCON_USB_ID_HIGH:
>                          printk("USB's ID pin is high state\n");
>                          break;
>                  case EXTCON_USB_VBUS_LOW:
>                          printk("USB's VBUS pin is high state\n");
>                          break;
>                  case EXTCON_USB_VBUS_HIGH:
>                          printk("USB's VBUS pin is high state\n");
>                          break;
>                  default:
>                          return -EINVAL;
>                  };
>          }
>
> Changes from v1:
> - Use dev_warn() instead of dev_info() if set the same extcon_line_state value.
>
> Chanwoo Choi (2):
>    extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors
>    extcon: usb-gpio: Update the ID pin state of USB when cable state is changed
>
>   drivers/extcon/extcon-usb-gpio.c |  6 ++++
>   drivers/extcon/extcon.c          | 74 +++++++++++++++++++++++++++++++++++++++-
>   include/linux/extcon.h           | 24 +++++++++++++
>   3 files changed, 103 insertions(+), 1 deletion(-)
>

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

On Wed, May 27, 2015 at 11:09 PM, Roger Quadros <rogerq@ti.com> wrote:
> Chanwoo,
>
> On 27/05/15 15:15, Chanwoo Choi wrote:
>>
>> Previously, I discussed how to inform the changed state of both ID
>> and VBUS pin for USB connector on patch-set[1].
>> [1] https://lkml.org/lkml/2015/4/2/310
>>
>> So, this patch adds the extcon_set_cable_line_state() function to inform
>> the additional state of external connectors without additional register/
>> unregister functions. This function uses the existing notifier chain
>> which is registered by extcon_register_notifier() /
>> extcon_register_interest().
>>
>> The extcon_set_cable_line_state() can inform the new state of both
>> ID and VBUS pin state through extcon_set_cable_line_state().
>>
>> For exmaple:
>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>          static void usb_extcon_detect_cable(struct work_struct *work)
>>          {
>>                  ...
>>                  /* check ID and update cable state */
>>                  id = gpiod_get_value_cansleep(info->id_gpiod);
>>                  if (id) {
>>                          extcon_set_cable_state_(info->edev,
>> EXTCON_USB_HOST, false);
>
>
> Now that all USB line states can be captured by a single cable type
> can we get rid of EXTCON_USB_HOST?

EXTCON_USB_HOST is necessary to inform the event to the user-space by uevent.
For example, when the USB mouse device is attached, extcon have to inform
the staet to user-space. The role of extcon should inform the state of
all external connectors.

>
> That way the extcon driver doesn't need to make any decisions as to
> what mode we're in (host/cable) and this is best left to the USB driver.

The extcon just infrom the new event to the user-space.
If the notification of USB_HOST is not necessary in kernel space,
both device driver and framework in kernel space cannot register the notifier.

Thanks,
Chanwoo Choi

>
> cheers,
> -roger
>
>
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB,
>> true);
>>
>>                          extcon_set_cable_line_state(info->edev,
>> EXTCON_USB,
>>
>> EXTCON_USB_ID_HIGH);
>>                  } else {
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB,
>> false);
>>                          extcon_set_cable_state_(info->edev,
>> EXTCON_USB_HOST, true);
>>
>>                          extcon_set_cable_line_state(info->edev,
>> EXTCON_USB,
>>
>> EXTCON_USB_ID_LOW);
>>                  }
>>          }
>>
>> - On specific extcon consumder driver as following:
>>          static int xxx_probe(struct platform_device *pdev)
>>          {
>>                  struct notifier_chain nh;
>>
>>                  nb.notifier_call = extcon_usb_notifier;
>>                  ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
>>                  ...
>>          }
>>
>>          static int extcon_usb_notifier(struct notifier_block *self,
>>                  unsigned long event, void *ptr)
>>          {
>>                  switch (event) {
>>                  case EXTCON_DETACHED:
>>                          printk("USB is detached\n");
>>                          break;
>>                  case EXTCON_ATTACHED:
>>                          printk("USB is attached\n");
>>                          break;
>>
>>                  case EXTCON_USB_ID_LOW:
>>                          printk("USB's ID pin is low state\n");
>>                          break;
>>                  case EXTCON_USB_ID_HIGH:
>>                          printk("USB's ID pin is high state\n");
>>                          break;
>>                  case EXTCON_USB_VBUS_LOW:
>>                          printk("USB's VBUS pin is high state\n");
>>                          break;
>>                  case EXTCON_USB_VBUS_HIGH:
>>                          printk("USB's VBUS pin is high state\n");
>>                          break;
>>                  default:
>>                          return -EINVAL;
>>                  };
>>          }
>>
>> Changes from v1:
>> - Use dev_warn() instead of dev_info() if set the same extcon_line_state
>> value.
>>
>> Chanwoo Choi (2):
>>    extcon: Add extcon_set_cable_line_state() to inform the additional
>> state of external connectors
>>    extcon: usb-gpio: Update the ID pin state of USB when cable state is
>> changed
>>
>>   drivers/extcon/extcon-usb-gpio.c |  6 ++++
>>   drivers/extcon/extcon.c          | 74
>> +++++++++++++++++++++++++++++++++++++++-
>>   include/linux/extcon.h           | 24 +++++++++++++
>>   3 files changed, 103 insertions(+), 1 deletion(-)
>>
>

Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Roger Quadros]

Chanwoo,

On 27/05/15 15:15, Chanwoo Choi wrote:
> This patch adds the extcon_set_cable_line_state() function to inform
> the additional state of each external connector and 'enum extcon_line_state'
> enumeration which include the specific states of each external connector.
>
> The each external connector might need the different line state. So, current
> 'extcon_line_state' enumeration contains the specific state for USB as
> following:
>
> - Following the state mean the state of both ID and VBUS line for USB:
> enum extcon_line_state {
> 	EXTCON_USB_ID_LOW	= BIT(1),	/* ID line is low. */
> 	EXTCON_USB_ID_HIGH	= BIT(2),	/* ID line is high. */

ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit position.
if the bit is set means it is high, if it is cleared means it is low.

> 	EXTCON_USB_VBUS_LOW	= BIT(3),	/* VBUS line is low. */
> 	EXTCON_USB_VBUS_HIGH	= BIT(4),	/* VBUS line is high. */

same here.

enum doesn't look like the right type for extcon_line_state as it is more of a bitmask.

> };
>
> Cc: Myungjoo Ham <cw00.choi@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>   drivers/extcon/extcon.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++-
>   include/linux/extcon.h  | 24 ++++++++++++++++
>   2 files changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 5099c11..142bf0f 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
>
>   		for (index = 0; index < edev->max_supported; index++) {
>   			if (is_extcon_changed(edev->state, state, index, &attached))
> -				raw_notifier_call_chain(&edev->nh[index], attached, edev);
> +				raw_notifier_call_chain(&edev->nh[index],
> +					attached ? EXTCON_ATTACHED :
> +					EXTCON_DETACHED, edev);
>   		}
>
>   		edev->state &= ~mask;
> @@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev,
>   EXPORT_SYMBOL_GPL(extcon_set_cable_state);
>
>   /**
> + * extcon_set_cable_line_state() - Set the line state of specific cable.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector.
> + * @state:		the line state for specific cable.
> + *
> + * Note that this function support the only USB connector to inform the state
> + * of both ID and VBUS line until now. This function may be extended to support
> + * the additional external connectors.
> + *
> + * If the id is EXTCON_USB, it can support only following line states:
> + * - EXTCON_USB_ID_LOW
> + * - EXTCON_USB_ID_HIGH,
> + * - EXTCON_USB_VBUS_LOW
> + * - EXTCON_USB_VBUS_HIGH
> + */
> +int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
> +				enum extcon_line_state state)
> +{
> +	unsigned long flags;
> +	unsigned long line_state;
> +	int ret = 0, index;
> +
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
> +
> +	spin_lock_irqsave(&edev->lock, flags);
> +	line_state = edev->line_state[index];
> +
> +	switch (id) {
> +	case EXTCON_USB:
> +		if (line_state & state) {
> +			dev_warn(&edev->dev,
> +				"0x%x state is already set for %s\n",
> +				state, extcon_name[id]);
> +			goto err;
> +		}
> +
> +		if ((state & EXTCON_USB_ID_LOW) || (state & EXTCON_USB_ID_HIGH))
> +			line_state &= ~(EXTCON_USB_ID_LOW | EXTCON_USB_ID_HIGH);
> +
> +		if ((state & EXTCON_USB_VBUS_LOW)
> +			|| (state & EXTCON_USB_VBUS_HIGH))
> +			line_state &=
> +				~(EXTCON_USB_VBUS_LOW | EXTCON_USB_VBUS_HIGH);
> +
> +		line_state |= state;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +	edev->line_state[index] = line_state;
> +
> +	ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev);
> +err:
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_cable_line_state);
> +
> +/**
>    * extcon_get_extcon_dev() - Get the extcon device instance from the name
>    * @extcon_name:	The extcon name provided with extcon_dev_register()
>    */
> @@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev)
>   		goto err_dev;
>   	}
>
> +	edev->line_state = devm_kzalloc(&edev->dev,
> +		sizeof(*edev->line_state) * edev->max_supported, GFP_KERNEL);
> +	if (!edev->line_state) {
> +		ret = -ENOMEM;
> +		goto err_dev;
> +	}
> +
>   	for (index = 0; index < edev->max_supported; index++)
>   		RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index be9652b..79e5073 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -66,6 +66,19 @@ enum extcon {
>   	EXTCON_END,
>   };
>
> +enum extcon_line_state {
> +	/* Following two definition are used for whether external connectors
> +	 * is attached or detached. */
> +	EXTCON_DETACHED		= 0x0,
> +	EXTCON_ATTACHED		= 0x1,
> +
> +	/* Following states are only used for EXTCON_USB. */
> +	EXTCON_USB_ID_LOW	= BIT(1),	/* ID line is low. */
> +	EXTCON_USB_ID_HIGH	= BIT(2),	/* ID line is high. */
> +	EXTCON_USB_VBUS_LOW	= BIT(3),	/* VBUS line is low. */
> +	EXTCON_USB_VBUS_HIGH	= BIT(4),	/* VBUS line is high. */
> +};

Why do you use enum? How about the following bit definitions for line state.

#define EXTCON_ATTACHED_DETACHED_BIT	BIT(0)
#define EXTCON_USB_ID_BIT		BIT(1)
#define EXTCON_USB_VBUS_BIT		BIT(2)
...

code must check if appropriate bit is set or cleared to get the high/low state.

> +
>   struct extcon_cable;
>
>   /**
> @@ -90,6 +103,8 @@ struct extcon_cable;
>    * @dev:		Device of this extcon.
>    * @state:		Attach/detach state of this extcon. Do not provide at
>    *			register-time.
> + * @line_state:		Line state for each external connecotrs are included in
> + *			this extcon device.
>    * @nh:			Notifier for the state change events from this extcon
>    * @entry:		To support list of extcon devices so that users can
>    *			search for extcon devices based on the extcon name.
> @@ -121,6 +136,7 @@ struct extcon_dev {
>   	int max_supported;
>   	spinlock_t lock;	/* could be called by irq handler */
>   	u32 state;
> +	unsigned long *line_state;
>
>   	/* /sys/class/extcon/.../cable.n/... */
>   	struct device_type extcon_dev_type;
> @@ -217,6 +233,8 @@ extern int extcon_get_cable_state(struct extcon_dev *edev,
>   				  const char *cable_name);
>   extern int extcon_set_cable_state(struct extcon_dev *edev,
>   				  const char *cable_name, bool cable_state);
> +extern int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
> +					enum extcon_line_state state);
>
>   /*
>    * Following APIs are for notifiees (those who want to be notified)
> @@ -324,6 +342,12 @@ static inline int extcon_set_cable_state(struct extcon_dev *edev,
>   	return 0;
>   }
>
> +static inline int extcon_set_cable_line_state(struct extcon_dev *edev,
> +			enum extcon id, enum extcon_line_state state)
> +{
> +	return 0;
> +}
> +
>   static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>   {
>   	return NULL;
>

cheers,
-roger

Re: [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed

[Roger Quadros]

On 27/05/15 15:15, Chanwoo Choi wrote:
> This patch updates the ID pin state of USB when cable state is changed
> by using the extcon_set_cable_line_state() function. The extcon consumer driver
> can receive the changed ID pin state through registered notifier chain of
> extcon consumer driver.
>
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Robert Baldyga <r.baldyga@samsung.com>
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>   drivers/extcon/extcon-usb-gpio.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index 14da94c..9ff3171 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -62,12 +62,18 @@ static void usb_extcon_detect_cable(struct work_struct *work)
>   		 */
>   		extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>   		extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> +
> +		extcon_set_cable_line_state(info->edev, EXTCON_USB,
> +						EXTCON_USB_ID_HIGH);
>   	} else {
>   		/*
>   		 * ID = 0 means USB HOST cable attached.
>   		 * As we don't have event for USB peripheral cable detached,
>   		 * we simulate USB peripheral detach here.
>   		 */
> +		extcon_set_cable_line_state(info->edev, EXTCON_USB,
> +						EXTCON_USB_ID_LOW);
> +
>   		extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>   		extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
>   	}
>

Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Chanwoo Choi]

On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@ti.com> wrote:
> Chanwoo,
>
> On 27/05/15 15:15, Chanwoo Choi wrote:
>>
>> This patch adds the extcon_set_cable_line_state() function to inform
>> the additional state of each external connector and 'enum
>> extcon_line_state'
>> enumeration which include the specific states of each external connector.
>>
>> The each external connector might need the different line state. So,
>> current
>> 'extcon_line_state' enumeration contains the specific state for USB as
>> following:
>>
>> - Following the state mean the state of both ID and VBUS line for USB:
>> enum extcon_line_state {
>>         EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
>>         EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
>
>
> ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit
> position.
> if the bit is set means it is high, if it is cleared means it is low.

These are the notifier event. So, extcon_line_state have to include
each event for both low or high state of ID.
because the extcon consumer driver using the
extcon_register_notifier() need the each event to distinguish the type
of event.

>
>>         EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
>>         EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
>
>
> same here.

ditto.

>
> enum doesn't look like the right type for extcon_line_state as it is more of
> a bitmask.

Why? I prefer to use the enum if there are no problem.

>
>
>> };
>>
>> Cc: Myungjoo Ham <cw00.choi@samsung.com>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>   drivers/extcon/extcon.c | 74
>> ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/extcon.h  | 24 ++++++++++++++++
>>   2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 5099c11..142bf0f 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -279,7 +279,9 @@ int extcon_update_state(struct extcon_dev *edev, u32
>> mask, u32 state)
>>
>>                 for (index = 0; index < edev->max_supported; index++) {
>>                         if (is_extcon_changed(edev->state, state, index,
>> &attached))
>> -                               raw_notifier_call_chain(&edev->nh[index],
>> attached, edev);
>> +                               raw_notifier_call_chain(&edev->nh[index],
>> +                                       attached ? EXTCON_ATTACHED :
>> +                                       EXTCON_DETACHED, edev);
>>                 }
>>
>>                 edev->state &= ~mask;
>> @@ -418,6 +420,69 @@ int extcon_set_cable_state(struct extcon_dev *edev,
>>   EXPORT_SYMBOL_GPL(extcon_set_cable_state);
>>
>>   /**
>> + * extcon_set_cable_line_state() - Set the line state of specific cable.
>> + * @edev:              the extcon device that has the cable.
>> + * @id:                        the unique id of each external connector.
>> + * @state:             the line state for specific cable.
>> + *
>> + * Note that this function support the only USB connector to inform the
>> state
>> + * of both ID and VBUS line until now. This function may be extended to
>> support
>> + * the additional external connectors.
>> + *
>> + * If the id is EXTCON_USB, it can support only following line states:
>> + * - EXTCON_USB_ID_LOW
>> + * - EXTCON_USB_ID_HIGH,
>> + * - EXTCON_USB_VBUS_LOW
>> + * - EXTCON_USB_VBUS_HIGH
>> + */
>> +int extcon_set_cable_line_state(struct extcon_dev *edev, enum extcon id,
>> +                               enum extcon_line_state state)
>> +{
>> +       unsigned long flags;
>> +       unsigned long line_state;
>> +       int ret = 0, index;
>> +
>> +       index = find_cable_index_by_id(edev, id);
>> +       if (index < 0)
>> +               return index;
>> +
>> +       spin_lock_irqsave(&edev->lock, flags);
>> +       line_state = edev->line_state[index];
>> +
>> +       switch (id) {
>> +       case EXTCON_USB:
>> +               if (line_state & state) {
>> +                       dev_warn(&edev->dev,
>> +                               "0x%x state is already set for %s\n",
>> +                               state, extcon_name[id]);
>> +                       goto err;
>> +               }
>> +
>> +               if ((state & EXTCON_USB_ID_LOW) || (state &
>> EXTCON_USB_ID_HIGH))
>> +                       line_state &= ~(EXTCON_USB_ID_LOW |
>> EXTCON_USB_ID_HIGH);
>> +
>> +               if ((state & EXTCON_USB_VBUS_LOW)
>> +                       || (state & EXTCON_USB_VBUS_HIGH))
>> +                       line_state &=
>> +                               ~(EXTCON_USB_VBUS_LOW |
>> EXTCON_USB_VBUS_HIGH);
>> +
>> +               line_state |= state;
>> +               break;
>> +       default:
>> +               ret = -EINVAL;
>> +               goto err;
>> +       }
>> +       edev->line_state[index] = line_state;
>> +
>> +       ret = raw_notifier_call_chain(&edev->nh[index], line_state, edev);
>> +err:
>> +       spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_set_cable_line_state);
>> +
>> +/**
>>    * extcon_get_extcon_dev() - Get the extcon device instance from the
>> name
>>    * @extcon_name:      The extcon name provided with
>> extcon_dev_register()
>>    */
>> @@ -897,6 +962,13 @@ int extcon_dev_register(struct extcon_dev *edev)
>>                 goto err_dev;
>>         }
>>
>> +       edev->line_state = devm_kzalloc(&edev->dev,
>> +               sizeof(*edev->line_state) * edev->max_supported,
>> GFP_KERNEL);
>> +       if (!edev->line_state) {
>> +               ret = -ENOMEM;
>> +               goto err_dev;
>> +       }
>> +
>>         for (index = 0; index < edev->max_supported; index++)
>>                 RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]);
>>
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index be9652b..79e5073 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -66,6 +66,19 @@ enum extcon {
>>         EXTCON_END,
>>   };
>>
>> +enum extcon_line_state {
>> +       /* Following two definition are used for whether external
>> connectors
>> +        * is attached or detached. */
>> +       EXTCON_DETACHED         = 0x0,
>> +       EXTCON_ATTACHED         = 0x1,
>> +
>> +       /* Following states are only used for EXTCON_USB. */
>> +       EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
>> +       EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
>> +       EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
>> +       EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
>> +};
>
>
> Why do you use enum? How about the following bit definitions for line state.
>
> #define EXTCON_ATTACHED_DETACHED_BIT    BIT(0)
> #define EXTCON_USB_ID_BIT               BIT(1)
> #define EXTCON_USB_VBUS_BIT             BIT(2)
> ...
>
> code must check if appropriate bit is set or cleared to get the high/low
> state.

I answer about it on upper.

Cheers,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Ivan T. Ivanov]

Hi Chanwoo,

On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
> Previously, I discussed how to inform the changed state of both ID
> and VBUS pin for USB connector on patch-set[1].
> [1] https://lkml.org/lkml/2015/4/2/310
> 
> So, this patch adds the extcon_set_cable_line_state() function to inform
> the additional state of external connectors without additional register/
> unregister functions. This function uses the existing notifier chain
> which is registered by extcon_register_notifier() / extcon_register_interest().
> 
> The extcon_set_cable_line_state() can inform the new state of both
> ID and VBUS pin state through extcon_set_cable_line_state().
> 
> For exmaple:
> - On extcon-usb-gpio.c as extcon provider driver as following:
>         static void usb_extcon_detect_cable(struct work_struct *work)
>         {
>                 ...
>                 /* check ID and update cable state */
>                 id = gpiod_get_value_cansleep(info->id_gpiod);
>                 if (id) {
>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> 
>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>                                                         EXTCON_USB_ID_HIGH);

I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
required information from the function one line above, do I miss something?   

>                 } else {
>                         extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
> 
>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>                                                         EXTCON_USB_ID_LOW);
>                 }
>         }
> 
> - On specific extcon consumder driver as following:
>         static int xxx_probe(struct platform_device *pdev)
>         {
>                 struct notifier_chain nh;
> 
>                 nb.notifier_call = extcon_usb_notifier;
>                 ret = extcon_register_notifier(edev, EXTCON_USB, &nb);

This is bit misleading. 'nb' could not be in stack.

Regards,
Ivan

Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Ivan T. Ivanov]

Hi Chanwoo, 

On Thu, 2015-05-28 at 00:06 +0900, Chanwoo Choi wrote:
> On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@ti.com> wrote:
> > Chanwoo,
> > 
> > On 27/05/15 15:15, Chanwoo Choi wrote:
> > > This patch adds the extcon_set_cable_line_state() function to inform
> > > the additional state of each external connector and 'enum
> > > extcon_line_state'
> > > enumeration which include the specific states of each external connector.
> > > 
> > > The each external connector might need the different line state. So,
> > > current
> > > 'extcon_line_state' enumeration contains the specific state for USB as
> > > following:
> > > 
> > > - Following the state mean the state of both ID and VBUS line for USB:
> > > enum extcon_line_state {
> > >         EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
> > >         EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
> > 
> > 
> > ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit
> > position.
> > if the bit is set means it is high, if it is cleared means it is low.
> 
> These are the notifier event. So, extcon_line_state have to include
> each event for both low or high state of ID.
> because the extcon consumer driver using the
> extcon_register_notifier() need the each event to distinguish the type
> of event.
> 
> > >         EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
> > >         EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
> > 
> > 
> > same here.
> 
> ditto.
> 
> > enum doesn't look like the right type for extcon_line_state as it is more of
> > a bitmask.
> 
> Why? I prefer to use the enum if there are no problem.

I do agree with Roger.
 
> > > 
> > > +enum extcon_line_state {
> > > +       /* Following two definition are used for whether external
> > > connectors
> > > +        * is attached or detached. */
> > > +       EXTCON_DETACHED         = 0x0,
> > > +       EXTCON_ATTACHED         = 0x1,
> > > +
> > > +       /* Following states are only used for EXTCON_USB. */
> > > +       EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
> > > +       EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
> > > +       EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
> > > +       EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
> > > +};
> > 
> > 
> > Why do you use enum? How about the following bit definitions for line state.
> > 
> > #define EXTCON_ATTACHED_DETACHED_BIT    BIT(0)
> > #define EXTCON_USB_ID_BIT               BIT(1)
> > #define EXTCON_USB_VBUS_BIT             BIT(2)
> > ...
> > 
> > code must check if appropriate bit is set or cleared to get the high/low
> > state.
> 
> I answer about it on upper.
> 

Funny. We can use the one bit for DETACHED/ATTACHED, but we
can not use the one bit for LOW/HIGH?

Regards,
Ivan

Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Ivan T. Ivanov]

Hi, 

On Thu, 2015-05-28 at 18:17 +0900, Chanwoo Choi wrote:
> 
> 
> 2015년 5월 28일 목요일, Ivan T. Ivanov<iivanov@mm-sol.com>님이 작성한 메시지:
> > Hi Chanwoo,
> > 
> > On Thu, 2015-05-28 at 00:06 +0900, Chanwoo Choi wrote:
> > > On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@ti.com> wrote:
> > > > Chanwoo,
> > > >
> > > > On 27/05/15 15:15, Chanwoo Choi wrote:
> > > > > This patch adds the extcon_set_cable_line_state() function to inform
> > > > > the additional state of each external connector and 'enum
> > > > > extcon_line_state'
> > > > > enumeration which include the specific states of each external connector.
> > > > >
> > > > > The each external connector might need the different line state. So,
> > > > > current
> > > > > 'extcon_line_state' enumeration contains the specific state for USB as
> > > > > following:
> > > > >
> > > > > - Following the state mean the state of both ID and VBUS line for USB:
> > > > > enum extcon_line_state {
> > > > >         EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
> > > > >         EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
> > > >
> > > >
> > > > ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit
> > > > position.
> > > > if the bit is set means it is high, if it is cleared means it is low.
> > >
> > > These are the notifier event. So, extcon_line_state have to include
> > > each event for both low or high state of ID.
> > > because the extcon consumer driver using the
> > > extcon_register_notifier() need the each event to distinguish the type
> > > of event.
> > >
> > > > >         EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
> > > > >         EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
> > > >
> > > >
> > > > same here.
> > >
> > > ditto.
> > >
> > > > enum doesn't look like the right type for extcon_line_state as it is more of
> > > > a bitmask.
> > >
> > > Why? I prefer to use the enum if there are no problem.
> > 
> If you insist your opinion, you shoud suggest the reason. Could you tell me the reason?

It looks unreadable. There is no need to mix interface API (enum extcon_line_state)
with implementation details. You can use plain enum at interface level and hide
shifting inside implementation.

> 
> 
> > > > >
> > > > > +enum extcon_line_state {
> > > > > +       /* Following two definition are used for whether external
> > > > > connectors
> > > > > +        * is attached or detached. */
> > > > > +       EXTCON_DETACHED         = 0x0,
> > > > > +       EXTCON_ATTACHED         = 0x1,
> > > > > +
> > > > > +       /* Following states are only used for EXTCON_USB. */
> > > > > +       EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
> > > > > +       EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
> > > > > +       EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
> > > > > +       EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
> > > > > +};
> > > >
> > > >
> > > > Why do you use enum? How about the following bit definitions for line state.
> > > >
> > > > #define EXTCON_ATTACHED_DETACHED_BIT    BIT(0)
> > > > #define EXTCON_USB_ID_BIT               BIT(1)
> > > > #define EXTCON_USB_VBUS_BIT             BIT(2)
> > > > ...
> > > >
> > > > code must check if appropriate bit is set or cleared to get the high/low
> > > > state.
> > >
> > > I answer about it on upper.
> > >
> > 
> > Funny. We can use the one bit for DETACHED/ATTACHED, but we
> hmm. Did you read my answer about low/high?
> 
> But, I'm considering again about using the detached/attached value.

Ok, but consumers will receive one event at time, right?

Regards,
Ivan

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Roger Quadros]

+Peter & Li,

Ivan,

On 28/05/15 11:45, Ivan T. Ivanov wrote:
>
> Hi Chanwoo,
>
> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>> Previously, I discussed how to inform the changed state of both ID
>> and VBUS pin for USB connector on patch-set[1].
>> [1] https://lkml.org/lkml/2015/4/2/310
>>
>> So, this patch adds the extcon_set_cable_line_state() function to inform
>> the additional state of external connectors without additional register/
>> unregister functions. This function uses the existing notifier chain
>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>
>> The extcon_set_cable_line_state() can inform the new state of both
>> ID and VBUS pin state through extcon_set_cable_line_state().
>>
>> For exmaple:
>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>          static void usb_extcon_detect_cable(struct work_struct *work)
>>          {
>>                  ...
>>                  /* check ID and update cable state */
>>                  id = gpiod_get_value_cansleep(info->id_gpiod);
>>                  if (id) {
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>
>>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>                                                          EXTCON_USB_ID_HIGH);
>
> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> required information from the function one line above, do I miss something?

This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
the 4 states of ID and VBUS pins that we need for a real USB driver to work.

It looks like it was designed from user space users perspective where they are
only interested in USB role. i.e. host or peripheral.

Right now we are mixing both ID/VBUS and HOST/Peripheral states.
This will break when we consider OTG role switching.
With role switching, the USB device might start as a peripheral but switch role to host
on the fly and the existing setup (including these patches) can't cater to that
if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
Because they are hard-wired to the ID pin state which doesn't change during
role switch without cable switch.

The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
It just needs ID/VBUS states and should decide the Host/Peripheral state from
that and other inputs (like HNP/user request/etc).

The flow could be like this

(extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]

If that is not going to happen then we will have to switch to this

(usb phy driver) -> [ID/VBUS states] -> (USB driver) -> (extcon f/w) -> [Host/Peripheral states]


Felipe, Peter, Li,

what do you guys suggest?

cheers,
-roger

>
>>                  } else {
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
>>
>>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>                                                          EXTCON_USB_ID_LOW);
>>                  }
>>          }
>>
>> - On specific extcon consumder driver as following:
>>          static int xxx_probe(struct platform_device *pdev)
>>          {
>>                  struct notifier_chain nh;
>>
>>                  nb.notifier_call = extcon_usb_notifier;
>>                  ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
>
> This is bit misleading. 'nb' could not be in stack.
>
> Regards,
> Ivan
>

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Peter Chen]

On Thu, May 28, 2015 at 05:23:57PM +0300, Roger Quadros wrote:
> +Peter & Li,
> 
> Ivan,
> 
> On 28/05/15 11:45, Ivan T. Ivanov wrote:
> >
> >Hi Chanwoo,
> >
> >On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
> >>Previously, I discussed how to inform the changed state of both ID
> >>and VBUS pin for USB connector on patch-set[1].
> >>[1] https://lkml.org/lkml/2015/4/2/310
> >>
> >>So, this patch adds the extcon_set_cable_line_state() function to inform
> >>the additional state of external connectors without additional register/
> >>unregister functions. This function uses the existing notifier chain
> >>which is registered by extcon_register_notifier() / extcon_register_interest().
> >>
> >>The extcon_set_cable_line_state() can inform the new state of both
> >>ID and VBUS pin state through extcon_set_cable_line_state().
> >>
> >>For exmaple:
> >>- On extcon-usb-gpio.c as extcon provider driver as following:
> >>         static void usb_extcon_detect_cable(struct work_struct *work)
> >>         {
> >>                 ...
> >>                 /* check ID and update cable state */
> >>                 id = gpiod_get_value_cansleep(info->id_gpiod);
> >>                 if (id) {
> >>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
> >>                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> >>
> >>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
> >>                                                         EXTCON_USB_ID_HIGH);
> >
> >I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> >It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> >required information from the function one line above, do I miss something?
> 
> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
> the 4 states of ID and VBUS pins that we need for a real USB driver to work.
> 
> It looks like it was designed from user space users perspective where they are
> only interested in USB role. i.e. host or peripheral.
> 
> Right now we are mixing both ID/VBUS and HOST/Peripheral states.
> This will break when we consider OTG role switching.
> With role switching, the USB device might start as a peripheral but switch role to host
> on the fly and the existing setup (including these patches) can't cater to that
> if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
> Because they are hard-wired to the ID pin state which doesn't change during
> role switch without cable switch.
> 
> The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
> It just needs ID/VBUS states and should decide the Host/Peripheral state from
> that and other inputs (like HNP/user request/etc).
> 
> The flow could be like this
> 
> (extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]

Agree. Chanwoo, USB driver knows better than extcon driver about USB
role (host/peripheral), so the app should use USB interface to know it,
in fact, I don't be aware any use case needs to know USB role?
Are there any users for EXTCON_USB and EXTCON_USB_HOST currently?

> 
> If that is not going to happen then we will have to switch to this
> 
> (usb phy driver) -> [ID/VBUS states] -> (USB driver) -> (extcon f/w) -> [Host/Peripheral states]
> 
> 
> Felipe, Peter, Li,
> 
> what do you guys suggest?
> 
> cheers,
> -roger
> 
> >
> >>                 } else {
> >>                         extcon_set_cable_state_(info->edev, EXTCON_USB, false);
> >>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
> >>
> >>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
> >>                                                         EXTCON_USB_ID_LOW);
> >>                 }
> >>         }
> >>
> >>- On specific extcon consumder driver as following:
> >>         static int xxx_probe(struct platform_device *pdev)
> >>         {
> >>                 struct notifier_chain nh;
> >>
> >>                 nb.notifier_call = extcon_usb_notifier;
> >>                 ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
> >
> >This is bit misleading. 'nb' could not be in stack.
> >
> >Regards,
> >Ivan
> >

-- 

Best Regards,
Peter Chen

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Ivan T. Ivanov]

Hi, 

On Thu, 2015-05-28 at 17:23 +0300, Roger Quadros wrote:
> +Peter & Li,
> 
> Ivan,
> 
> On 28/05/15 11:45, Ivan T. Ivanov wrote:
> > Hi Chanwoo,
> > 
> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
> > > Previously, I discussed how to inform the changed state of both ID
> > > and VBUS pin for USB connector on patch-set[1].
> > > [1] https://lkml.org/lkml/2015/4/2/310
> > > 
> > > So, this patch adds the extcon_set_cable_line_state() function to inform
> > > the additional state of external connectors without additional register/
> > > unregister functions. This function uses the existing notifier chain
> > > which is registered by extcon_register_notifier() / extcon_register_interest().
> > > 
> > > The extcon_set_cable_line_state() can inform the new state of both
> > > ID and VBUS pin state through extcon_set_cable_line_state().
> > > 
> > > For exmaple:
> > > - On extcon-usb-gpio.c as extcon provider driver as following:
> > >          static void usb_extcon_detect_cable(struct work_struct *work)
> > >          {
> > >                  ...
> > >                  /* check ID and update cable state */
> > >                  id = gpiod_get_value_cansleep(info->id_gpiod);
> > >                  if (id) {
> > >                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
> > >                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> > > 
> > >                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
> > >                                                          EXTCON_USB_ID_HIGH);
> > 
> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> > required information from the function one line above, do I miss something?
> 
> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
> the 4 states of ID and VBUS pins that we need for a real USB driver to work.

Are they any producers or consumers of "USB-HOST" and "USB" which are using these
for anything different than ID and VBUS state tracking, except the user space?
If not, could we just rename in kernel definitions, keeping user space notification
strings and be done? 

Regadrs,
Ivan

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Ivan T. Ivanov]

Hi, 

On Thu, 2015-05-28 at 17:23 +0300, Roger Quadros wrote:
> +Peter & Li,
> 
> Ivan,
> 
> On 28/05/15 11:45, Ivan T. Ivanov wrote:
> > Hi Chanwoo,
> > 
> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
> > > Previously, I discussed how to inform the changed state of both ID
> > > and VBUS pin for USB connector on patch-set[1].
> > > [1] https://lkml.org/lkml/2015/4/2/310
> > > 
> > > So, this patch adds the extcon_set_cable_line_state() function to inform
> > > the additional state of external connectors without additional register/
> > > unregister functions. This function uses the existing notifier chain
> > > which is registered by extcon_register_notifier() / extcon_register_interest().
> > > 
> > > The extcon_set_cable_line_state() can inform the new state of both
> > > ID and VBUS pin state through extcon_set_cable_line_state().
> > > 
> > > For exmaple:
> > > - On extcon-usb-gpio.c as extcon provider driver as following:
> > >          static void usb_extcon_detect_cable(struct work_struct *work)
> > >          {
> > >                  ...
> > >                  /* check ID and update cable state */
> > >                  id = gpiod_get_value_cansleep(info->id_gpiod);
> > >                  if (id) {
> > >                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
> > >                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> > > 
> > >                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
> > >                                                          EXTCON_USB_ID_HIGH);
> > 
> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> > required information from the function one line above, do I miss something?
> 
> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
> the 4 states of ID and VBUS pins that we need for a real USB driver to work.

Are they any producers or consumers of "USB-HOST" and "USB" which are using these
for anything different than ID and VBUS state tracking, except the user space?
If not, could we just rename in kernel definitions, keeping user space notification
strings and be done? 

Regadrs,
Ivan

Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors

[Chanwoo Choi]

On 05/28/2015 06:37 PM, Ivan T. Ivanov wrote:
> Hi, 
> 
> On Thu, 2015-05-28 at 18:17 +0900, Chanwoo Choi wrote:
>>
>>
>> 2015년 5월 28일 목요일, Ivan T. Ivanov<iivanov@mm-sol.com>님이 작성한 메시지:
>>> Hi Chanwoo,
>>>
>>> On Thu, 2015-05-28 at 00:06 +0900, Chanwoo Choi wrote:
>>>> On Wed, May 27, 2015 at 11:38 PM, Roger Quadros <rogerq@ti.com> wrote:
>>>>> Chanwoo,
>>>>>
>>>>> On 27/05/15 15:15, Chanwoo Choi wrote:
>>>>>> This patch adds the extcon_set_cable_line_state() function to inform
>>>>>> the additional state of each external connector and 'enum
>>>>>> extcon_line_state'
>>>>>> enumeration which include the specific states of each external connector.
>>>>>>
>>>>>> The each external connector might need the different line state. So,
>>>>>> current
>>>>>> 'extcon_line_state' enumeration contains the specific state for USB as
>>>>>> following:
>>>>>>
>>>>>> - Following the state mean the state of both ID and VBUS line for USB:
>>>>>> enum extcon_line_state {
>>>>>>         EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
>>>>>>         EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
>>>>>
>>>>>
>>>>> ID_LOW and ID_HIGH cannot happen simultaneously so they can use the same bit
>>>>> position.
>>>>> if the bit is set means it is high, if it is cleared means it is low.
>>>>
>>>> These are the notifier event. So, extcon_line_state have to include
>>>> each event for both low or high state of ID.
>>>> because the extcon consumer driver using the
>>>> extcon_register_notifier() need the each event to distinguish the type
>>>> of event.
>>>>
>>>>>>         EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
>>>>>>         EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
>>>>>
>>>>>
>>>>> same here.
>>>>
>>>> ditto.
>>>>
>>>>> enum doesn't look like the right type for extcon_line_state as it is more of
>>>>> a bitmask.
>>>>
>>>> Why? I prefer to use the enum if there are no problem.
>>>
>> If you insist your opinion, you shoud suggest the reason. Could you tell me the reason?
> 
> It looks unreadable. There is no need to mix interface API (enum extcon_line_state)
> with implementation details. You can use plain enum at interface level and hide
> shifting inside implementation.

OK, I define it instead of enum.

> 
>>
>>
>>>>>>
>>>>>> +enum extcon_line_state {
>>>>>> +       /* Following two definition are used for whether external
>>>>>> connectors
>>>>>> +        * is attached or detached. */
>>>>>> +       EXTCON_DETACHED         = 0x0,
>>>>>> +       EXTCON_ATTACHED         = 0x1,
>>>>>> +
>>>>>> +       /* Following states are only used for EXTCON_USB. */
>>>>>> +       EXTCON_USB_ID_LOW       = BIT(1),       /* ID line is low. */
>>>>>> +       EXTCON_USB_ID_HIGH      = BIT(2),       /* ID line is high. */
>>>>>> +       EXTCON_USB_VBUS_LOW     = BIT(3),       /* VBUS line is low. */
>>>>>> +       EXTCON_USB_VBUS_HIGH    = BIT(4),       /* VBUS line is high. */
>>>>>> +};
>>>>>
>>>>>
>>>>> Why do you use enum? How about the following bit definitions for line state.
>>>>>
>>>>> #define EXTCON_ATTACHED_DETACHED_BIT    BIT(0)
>>>>> #define EXTCON_USB_ID_BIT               BIT(1)
>>>>> #define EXTCON_USB_VBUS_BIT             BIT(2)
>>>>> ...
>>>>>
>>>>> code must check if appropriate bit is set or cleared to get the high/low
>>>>> state.
>>>>
>>>> I answer about it on upper.
>>>>
>>>
>>> Funny. We can use the one bit for DETACHED/ATTACHED, but we
>> hmm. Did you read my answer about low/high?
>>
>> But, I'm considering again about using the detached/attached value.
> 
> Ok, but consumers will receive one event at time, right?

Yes.

Cheers,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Hi Roger,

On 05/28/2015 11:23 PM, Roger Quadros wrote:
> +Peter & Li,
> 
> Ivan,
> 
> On 28/05/15 11:45, Ivan T. Ivanov wrote:
>>
>> Hi Chanwoo,
>>
>> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>>> Previously, I discussed how to inform the changed state of both ID
>>> and VBUS pin for USB connector on patch-set[1].
>>> [1] https://lkml.org/lkml/2015/4/2/310
>>>
>>> So, this patch adds the extcon_set_cable_line_state() function to inform
>>> the additional state of external connectors without additional register/
>>> unregister functions. This function uses the existing notifier chain
>>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>>
>>> The extcon_set_cable_line_state() can inform the new state of both
>>> ID and VBUS pin state through extcon_set_cable_line_state().
>>>
>>> For exmaple:
>>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>>          static void usb_extcon_detect_cable(struct work_struct *work)
>>>          {
>>>                  ...
>>>                  /* check ID and update cable state */
>>>                  id = gpiod_get_value_cansleep(info->id_gpiod);
>>>                  if (id) {
>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>>
>>>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>>                                                          EXTCON_USB_ID_HIGH);
>>
>> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>> required information from the function one line above, do I miss something?
> 
> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
> the 4 states of ID and VBUS pins that we need for a real USB driver to work.
> 
> It looks like it was designed from user space users perspective where they are
> only interested in USB role. i.e. host or peripheral.
> 
> Right now we are mixing both ID/VBUS and HOST/Peripheral states.
> This will break when we consider OTG role switching.
> With role switching, the USB device might start as a peripheral but switch role to host
> on the fly and the existing setup (including these patches) can't cater to that
> if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
> Because they are hard-wired to the ID pin state which doesn't change during
> role switch without cable switch.
> 
> The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
> It just needs ID/VBUS states and should decide the Host/Peripheral state from
> that and other inputs (like HNP/user request/etc).
> 
> The flow could be like this
> 
> (extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]
> 
> If that is not going to happen then we will have to switch to this
> 
> (usb phy driver) -> [ID/VBUS states] -> (USB driver) -> (extcon f/w) -> [Host/Peripheral states]
> 
> 
> Felipe, Peter, Li,
> 
> what do you guys suggest?

The EXTCON framework have to definitely distinguish the correct type of external connectors which is connected to H/W target or other machines. And then the EXTCON framework inform the state and type of attached external connector to the extcon consumer and user-space. It is absolutely necessary role of EXTCON framework.

The EXTCON_USB_HOST connector is different from EXTCON_USB connector. When USB mouse or keyboard is attached to H/W target, EXTCON fwk will inform both correct type(USB_HOST) and state(1) of attached external connector. If EXTCON fwk don't support the EXTCON_USB_HOST, when USB mouse/keyboard is attached, EXTCON cannot provide the information of attached external connectors.

The EXTCON don't have the permission of USB operation but have the role to inform the correct type of external connectors to the extcon consumer.

As you said, USB driver and framework can receive the notifier event of only ID/VBUS pin state. If USB driver don't need the state of EXTCON_USB_HOST, USB driver don't have to recevie the event of EXTCON_USB_HOST. Just USB driver might receive the ID/VBUS event by extcon_register_notifier().

> 
> cheers,
> -roger
> 
>>
>>>                  } else {
>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
>>>
>>>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>>                                                          EXTCON_USB_ID_LOW);
>>>                  }
>>>          }
>>>
>>> - On specific extcon consumder driver as following:
>>>          static int xxx_probe(struct platform_device *pdev)
>>>          {
>>>                  struct notifier_chain nh;
>>>
>>>                  nb.notifier_call = extcon_usb_notifier;
>>>                  ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
>>
>> This is bit misleading. 'nb' could not be in stack.

Right. It is my mistake. I'll fix it for example.

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Hi Ivan,

On 05/28/2015 05:45 PM, Ivan T. Ivanov wrote:
> 
> Hi Chanwoo,
> 
> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>> Previously, I discussed how to inform the changed state of both ID
>> and VBUS pin for USB connector on patch-set[1].
>> [1] https://lkml.org/lkml/2015/4/2/310
>>
>> So, this patch adds the extcon_set_cable_line_state() function to inform
>> the additional state of external connectors without additional register/
>> unregister functions. This function uses the existing notifier chain
>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>
>> The extcon_set_cable_line_state() can inform the new state of both
>> ID and VBUS pin state through extcon_set_cable_line_state().
>>
>> For exmaple:
>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>         static void usb_extcon_detect_cable(struct work_struct *work)
>>         {
>>                 ...
>>                 /* check ID and update cable state */
>>                 id = gpiod_get_value_cansleep(info->id_gpiod);
>>                 if (id) {
>>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>
>>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>                                                         EXTCON_USB_ID_HIGH);
> 
> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> required information from the function one line above, do I miss something?   

The EXTCON fwk has the follwoing different functions:
- extcon_set_cable_state()
: Send whether external connectors is attached or detached to the extcon consumer driver in kernel space and to the user-space by using the uevent.
- extcon_set_cable_line_state()
: Send the specific line state of both ID and VBUS pin state of USB connector to only the extcon consumer driver in kernel space. This function don't send the uevent to the user-space because user-space process don't consider the h/w pin state.

> 
>>                 } else {
>>                         extcon_set_cable_state_(info->edev, EXTCON_USB, false);
>>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, true);
>>
>>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>                                                         EXTCON_USB_ID_LOW);
>>                 }
>>         }
>>
>> - On specific extcon consumder driver as following:
>>         static int xxx_probe(struct platform_device *pdev)
>>         {
>>                 struct notifier_chain nh;
>>
>>                 nb.notifier_call = extcon_usb_notifier;
>>                 ret = extcon_register_notifier(edev, EXTCON_USB, &nb);
> 
> This is bit misleading. 'nb' could not be in stack.

Right. It is my mistake. I'll fix it for example.

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Hi Peter,

On 05/29/2015 10:22 AM, Peter Chen wrote:
> On Thu, May 28, 2015 at 05:23:57PM +0300, Roger Quadros wrote:
>> +Peter & Li,
>>
>> Ivan,
>>
>> On 28/05/15 11:45, Ivan T. Ivanov wrote:
>>>
>>> Hi Chanwoo,
>>>
>>> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>>>> Previously, I discussed how to inform the changed state of both ID
>>>> and VBUS pin for USB connector on patch-set[1].
>>>> [1] https://lkml.org/lkml/2015/4/2/310
>>>>
>>>> So, this patch adds the extcon_set_cable_line_state() function to inform
>>>> the additional state of external connectors without additional register/
>>>> unregister functions. This function uses the existing notifier chain
>>>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>>>
>>>> The extcon_set_cable_line_state() can inform the new state of both
>>>> ID and VBUS pin state through extcon_set_cable_line_state().
>>>>
>>>> For exmaple:
>>>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>>>         static void usb_extcon_detect_cable(struct work_struct *work)
>>>>         {
>>>>                 ...
>>>>                 /* check ID and update cable state */
>>>>                 id = gpiod_get_value_cansleep(info->id_gpiod);
>>>>                 if (id) {
>>>>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>>>                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>>>
>>>>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>>>                                                         EXTCON_USB_ID_HIGH);
>>>
>>> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>>> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>>> required information from the function one line above, do I miss something?
>>
>> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
>> the 4 states of ID and VBUS pins that we need for a real USB driver to work.
>>
>> It looks like it was designed from user space users perspective where they are
>> only interested in USB role. i.e. host or peripheral.
>>
>> Right now we are mixing both ID/VBUS and HOST/Peripheral states.
>> This will break when we consider OTG role switching.
>> With role switching, the USB device might start as a peripheral but switch role to host
>> on the fly and the existing setup (including these patches) can't cater to that
>> if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
>> Because they are hard-wired to the ID pin state which doesn't change during
>> role switch without cable switch.
>>
>> The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
>> It just needs ID/VBUS states and should decide the Host/Peripheral state from
>> that and other inputs (like HNP/user request/etc).
>>
>> The flow could be like this
>>
>> (extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]
> 
> Agree. Chanwoo, USB driver knows better than extcon driver about USB
> role (host/peripheral), so the app should use USB interface to know it,
> in fact, I don't be aware any use case needs to know USB role?
> Are there any users for EXTCON_USB and EXTCON_USB_HOST currently?

You're right. But, extcon can just distinguish the type of external connectors
and inform the type to the user-space and extcon consumer driver in kernel-space.

When USB mouse or keyboard is attached, user-space can check the state of externel connector which is attaced to the H/W target as following:
- /sys/class/extcon/extconX/cable.0/name -> USB
- /sys/class/extcon/extconX/cable.0/state -> 0
- /sys/class/extcon/extconX/cable.1/name -> USB-HOST
- /sys/class/extcon/extconX/cable.1/state -> 1

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Dear all,

On 05/29/2015 07:53 PM, Chanwoo Choi wrote:
> Hi Peter,
> 
> On 05/29/2015 10:22 AM, Peter Chen wrote:
>> On Thu, May 28, 2015 at 05:23:57PM +0300, Roger Quadros wrote:
>>> +Peter & Li,
>>>
>>> Ivan,
>>>
>>> On 28/05/15 11:45, Ivan T. Ivanov wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>>>>> Previously, I discussed how to inform the changed state of both ID
>>>>> and VBUS pin for USB connector on patch-set[1].
>>>>> [1] https://lkml.org/lkml/2015/4/2/310
>>>>>
>>>>> So, this patch adds the extcon_set_cable_line_state() function to inform
>>>>> the additional state of external connectors without additional register/
>>>>> unregister functions. This function uses the existing notifier chain
>>>>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>>>>
>>>>> The extcon_set_cable_line_state() can inform the new state of both
>>>>> ID and VBUS pin state through extcon_set_cable_line_state().
>>>>>
>>>>> For exmaple:
>>>>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>>>>         static void usb_extcon_detect_cable(struct work_struct *work)
>>>>>         {
>>>>>                 ...
>>>>>                 /* check ID and update cable state */
>>>>>                 id = gpiod_get_value_cansleep(info->id_gpiod);
>>>>>                 if (id) {
>>>>>                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>>>>                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>>>>
>>>>>                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>>>>                                                         EXTCON_USB_ID_HIGH);
>>>>
>>>> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>>>> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>>>> required information from the function one line above, do I miss something?
>>>
>>> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
>>> the 4 states of ID and VBUS pins that we need for a real USB driver to work.
>>>
>>> It looks like it was designed from user space users perspective where they are
>>> only interested in USB role. i.e. host or peripheral.
>>>
>>> Right now we are mixing both ID/VBUS and HOST/Peripheral states.
>>> This will break when we consider OTG role switching.
>>> With role switching, the USB device might start as a peripheral but switch role to host
>>> on the fly and the existing setup (including these patches) can't cater to that
>>> if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
>>> Because they are hard-wired to the ID pin state which doesn't change during
>>> role switch without cable switch.
>>>
>>> The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
>>> It just needs ID/VBUS states and should decide the Host/Peripheral state from
>>> that and other inputs (like HNP/user request/etc).
>>>
>>> The flow could be like this
>>>
>>> (extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]
>>
>> Agree. Chanwoo, USB driver knows better than extcon driver about USB
>> role (host/peripheral), so the app should use USB interface to know it,
>> in fact, I don't be aware any use case needs to know USB role?
>> Are there any users for EXTCON_USB and EXTCON_USB_HOST currently?
> 
> You're right. But, extcon can just distinguish the type of external connectors
> and inform the type to the user-space and extcon consumer driver in kernel-space.
> 
> When USB mouse or keyboard is attached, user-space can check the state of externel connector which is attaced to the H/W target as following:
> - /sys/class/extcon/extconX/cable.0/name -> USB
> - /sys/class/extcon/extconX/cable.0/state -> 0
> - /sys/class/extcon/extconX/cable.1/name -> USB-HOST
> - /sys/class/extcon/extconX/cable.1/state -> 1
> 

On last month, Robert Baldyga sent the patch[1] for extcon-usb-gpio driver.
[1] https://lkml.org/lkml/2015/4/2/311
- [PATCH v3 2/4] extcon: usb-gpio: add support for VBUS detection

This patch[1] included a following table which show the type of external connectors
according to both ID and VBUS pin state.

 State              |    ID   |   VBUS
----------------------------------------
 [1] USB            |    H    |    H
 [2] none           |    H    |    L
 [3] USB & USB-HOST |    L    |    H
 [4] USB-HOST       |    L    |    L

So, I think that extcon-usb-gpio.c could set below two status with set:
- the state of whether USB/USB-HOST are attached or detached -> send the state to both kernel-space and user-space.
: extcon_set_cable_state_()
- the h/w line state of both ID and VBUS pin state -> send the state to only kernel space.
: extcon_set_cable_line_state()

[1]
 State              |    ID   |   VBUS
----------------------------------------
 [1] USB            |    H    |    H

extcon_set_cable_state_(edev, EXTCON_USB, true);
extcon_set_cable_state_(edev, EXTCON_USB_HOST, false);

extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH);
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_HIGH);


[2]
 State              |    ID   |   VBUS
----------------------------------------
 [2] none           |    H    |    L

extcon_set_cable_state_(edev, EXTCON_USB, false);
extcon_set_cable_state_(edev, EXTCON_USB_HOST, false);

extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH);
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_LOW);


[3]
 State              |    ID   |   VBUS
----------------------------------------
 [3] USB & USB-HOST |    L    |    H

extcon_set_cable_state_(edev, EXTCON_USB, false);
extcon_set_cable_state_(edev, EXTCON_USB_HOST, true);

extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_LOW);
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_HIGH);


[4]
 State              |    ID   |   VBUS
----------------------------------------
 [4] USB-HOST       |    L    |    L

extcon_set_cable_state_(edev, EXTCON_USB, false);
extcon_set_cable_state_(edev, EXTCON_USB_HOST, true);

extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_LOW);
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_LOW);

If I know the wrong knowledge, please let me know your comment and advice.

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Ivan T. Ivanov]

On Fri, 2015-05-29 at 19:44 +0900, Chanwoo Choi wrote:
> Hi Ivan,
> 
> On 05/28/2015 05:45 PM, Ivan T. Ivanov wrote:
> > Hi Chanwoo,
> > 
> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
> > > Previously, I discussed how to inform the changed state of both ID
> > > and VBUS pin for USB connector on patch-set[1].
> > > [1] https://lkml.org/lkml/2015/4/2/310
> > > 
> > > So, this patch adds the extcon_set_cable_line_state() function to inform
> > > the additional state of external connectors without additional register/
> > > unregister functions. This function uses the existing notifier chain
> > > which is registered by extcon_register_notifier() / extcon_register_interest().
> > > 
> > > The extcon_set_cable_line_state() can inform the new state of both
> > > ID and VBUS pin state through extcon_set_cable_line_state().
> > > 
> > > For exmaple:
> > > - On extcon-usb-gpio.c as extcon provider driver as following:
> > >         static void usb_extcon_detect_cable(struct work_struct *work)
> > >         {
> > >                 ...
> > >                 /* check ID and update cable state */
> > >                 id = gpiod_get_value_cansleep(info->id_gpiod);
> > >                 if (id) {
> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
> > > 
> > >                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
> > >                                                         EXTCON_USB_ID_HIGH);
> > 
> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
> > required information from the function one line above, do I miss something?
> 
> The EXTCON fwk has the follwoing different functions:
> - extcon_set_cable_state()
> : Send whether external connectors is attached or detached to the extcon consumer driver in kernel space and to the user-space by using the uevent.
> - extcon_set_cable_line_state()
> : Send the specific line state of both ID and VBUS pin state of USB connector to only the extcon consumer driver in kernel space. This function don't send the uevent to the user-space because user-
> space process don't consider the h/w pin state.

My understanding, from discussion several letters back, is that clients
will receive single event per change (EXTCON_USB_ID_HIGH, EXTCON_USB_ID_LOW...)
and not some combined bit mask, right?

My point was, why we need another function (extcon_set_cable_line_state) if 
extcon_set_cable_state_ already have required information? Or the plan is to 
move extcon_set_cable_state_ to USB drivers and use only extcon_set_cable_line_state
by extcon drivers?

Regards,
Ivan

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

On Fri, May 29, 2015 at 11:32 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>
> On Fri, 2015-05-29 at 19:44 +0900, Chanwoo Choi wrote:
>> Hi Ivan,
>>
>> On 05/28/2015 05:45 PM, Ivan T. Ivanov wrote:
>> > Hi Chanwoo,
>> >
>> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>> > > Previously, I discussed how to inform the changed state of both ID
>> > > and VBUS pin for USB connector on patch-set[1].
>> > > [1] https://lkml.org/lkml/2015/4/2/310
>> > >
>> > > So, this patch adds the extcon_set_cable_line_state() function to inform
>> > > the additional state of external connectors without additional register/
>> > > unregister functions. This function uses the existing notifier chain
>> > > which is registered by extcon_register_notifier() / extcon_register_interest().
>> > >
>> > > The extcon_set_cable_line_state() can inform the new state of both
>> > > ID and VBUS pin state through extcon_set_cable_line_state().
>> > >
>> > > For exmaple:
>> > > - On extcon-usb-gpio.c as extcon provider driver as following:
>> > >         static void usb_extcon_detect_cable(struct work_struct *work)
>> > >         {
>> > >                 ...
>> > >                 /* check ID and update cable state */
>> > >                 id = gpiod_get_value_cansleep(info->id_gpiod);
>> > >                 if (id) {
>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>> > >
>> > >                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>> > >                                                         EXTCON_USB_ID_HIGH);
>> >
>> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>> > required information from the function one line above, do I miss something?
>>
>> The EXTCON fwk has the follwoing different functions:
>> - extcon_set_cable_state()
>> : Send whether external connectors is attached or detached to the extcon consumer driver in kernel space and to the user-space by using the uevent.
>> - extcon_set_cable_line_state()
>> : Send the specific line state of both ID and VBUS pin state of USB connector to only the extcon consumer driver in kernel space. This function don't send the uevent to the user-space because user-
>> space process don't consider the h/w pin state.
>
> My understanding, from discussion several letters back, is that clients
> will receive single event per change (EXTCON_USB_ID_HIGH, EXTCON_USB_ID_LOW...),

There are following constraints between events for EXTCON_USB:
- EXTCON_USB_ID_HIGH and EXTCON_USB_ID_LOW are mutually exclusive.
- EXTCON_USB_VBUS_HIGH and EXTCON_USB_VBUS_LOW are mutually exclusive.

If extcon provider (e.g., extcon-usb-gpio) don't violate the constraints,
extcon provider can send the multiple events as following. Namely,
extcon consumer/clients will receive the the multiple events.

For example:
extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH |
EXTCON_USB_VBUS_LOW);

>
> My point was, why we need another function (extcon_set_cable_line_state) if
> extcon_set_cable_state_ already have required information? Or the plan is to
> move extcon_set_cable_state_ to USB drivers and use only extcon_set_cable_line_state
> by extcon drivers?

You pointed out appropriately. I worried about adding new
extcon_set_cable_line_state().

I'll use extcon_set_cable_state_() without adding
extcon_set_cable_line_state() as following but it need the
modification of function prototype.

- extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, bool state)
-> extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, u64 state)

And extcon use the bit mask for both attached state and detached state
of external connectors instead of boolean.

#define EXTCON_DETACHED             BIT(0)
#define EXTCON_ATTACHED              BIT(1)

#define EXTCON_USB_ID_LOW          BIT(2)
#define EXTCON_USB_ID_HIGH         BIT(3)
#define EXTCON_USB_VBUS_LOW    BIT(4)
#define EXTCON_USB_VBUS_HIGH   BIT(5)

After it, extcon_set_cable_state() would send multiple events at time
as following:

e.g., extcon_set_cable_state(edev, EXTCON_USB, EXTCON_ATTACHED |
EXTCON_USB_ID_HIGH | EXTCON_USB_VBUS_HIGH);

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Chanwoo Choi]

Hi Ivan,

On Sat, May 30, 2015 at 2:15 AM, Chanwoo Choi <cwchoi00@gmail.com> wrote:
> On Fri, May 29, 2015 at 11:32 PM, Ivan T. Ivanov <iivanov@mm-sol.com> wrote:
>>
>> On Fri, 2015-05-29 at 19:44 +0900, Chanwoo Choi wrote:
>>> Hi Ivan,
>>>
>>> On 05/28/2015 05:45 PM, Ivan T. Ivanov wrote:
>>> > Hi Chanwoo,
>>> >
>>> > On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>>> > > Previously, I discussed how to inform the changed state of both ID
>>> > > and VBUS pin for USB connector on patch-set[1].
>>> > > [1] https://lkml.org/lkml/2015/4/2/310
>>> > >
>>> > > So, this patch adds the extcon_set_cable_line_state() function to inform
>>> > > the additional state of external connectors without additional register/
>>> > > unregister functions. This function uses the existing notifier chain
>>> > > which is registered by extcon_register_notifier() / extcon_register_interest().
>>> > >
>>> > > The extcon_set_cable_line_state() can inform the new state of both
>>> > > ID and VBUS pin state through extcon_set_cable_line_state().
>>> > >
>>> > > For exmaple:
>>> > > - On extcon-usb-gpio.c as extcon provider driver as following:
>>> > >         static void usb_extcon_detect_cable(struct work_struct *work)
>>> > >         {
>>> > >                 ...
>>> > >                 /* check ID and update cable state */
>>> > >                 id = gpiod_get_value_cansleep(info->id_gpiod);
>>> > >                 if (id) {
>>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>> > >                         extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>> > >
>>> > >                         extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>> > >                                                         EXTCON_USB_ID_HIGH);
>>> >
>>> > I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>>> > It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>>> > required information from the function one line above, do I miss something?
>>>
>>> The EXTCON fwk has the follwoing different functions:
>>> - extcon_set_cable_state()
>>> : Send whether external connectors is attached or detached to the extcon consumer driver in kernel space and to the user-space by using the uevent.
>>> - extcon_set_cable_line_state()
>>> : Send the specific line state of both ID and VBUS pin state of USB connector to only the extcon consumer driver in kernel space. This function don't send the uevent to the user-space because user-
>>> space process don't consider the h/w pin state.
>>
>> My understanding, from discussion several letters back, is that clients
>> will receive single event per change (EXTCON_USB_ID_HIGH, EXTCON_USB_ID_LOW...),
>
> There are following constraints between events for EXTCON_USB:
> - EXTCON_USB_ID_HIGH and EXTCON_USB_ID_LOW are mutually exclusive.
> - EXTCON_USB_VBUS_HIGH and EXTCON_USB_VBUS_LOW are mutually exclusive.
>
> If extcon provider (e.g., extcon-usb-gpio) don't violate the constraints,
> extcon provider can send the multiple events as following. Namely,
> extcon consumer/clients will receive the the multiple events.
>
> For example:
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH |
> EXTCON_USB_VBUS_LOW);
>
>>
>> My point was, why we need another function (extcon_set_cable_line_state) if
>> extcon_set_cable_state_ already have required information? Or the plan is to
>> move extcon_set_cable_state_ to USB drivers and use only extcon_set_cable_line_state
>> by extcon drivers?
>
> You pointed out appropriately. I worried about adding new
> extcon_set_cable_line_state().
>
> I'll use extcon_set_cable_state_() without adding
> extcon_set_cable_line_state() as following but it need the
> modification of function prototype.
>
> - extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, bool state)
> -> extcon_set_cable_state(struct extcon_dev *edev, enum extcon id, u64 state)
>
> And extcon use the bit mask for both attached state and detached state
> of external connectors instead of boolean.
>
> #define EXTCON_DETACHED             BIT(0)
> #define EXTCON_ATTACHED              BIT(1)
>
> #define EXTCON_USB_ID_LOW          BIT(2)
> #define EXTCON_USB_ID_HIGH         BIT(3)
> #define EXTCON_USB_VBUS_LOW    BIT(4)
> #define EXTCON_USB_VBUS_HIGH   BIT(5)
>
> After it, extcon_set_cable_state() would send multiple events at time
> as following:
>
> e.g., extcon_set_cable_state(edev, EXTCON_USB, EXTCON_ATTACHED |
> EXTCON_USB_ID_HIGH | EXTCON_USB_VBUS_HIGH);

This design has the problem.

If extcon uses only extcon_set_cable_state_(), the prototype of
extcon_{set|get}_cable_state_() functions should be changed.

extcon_set_cable_state_(struct extcon_dev *edev, enum extcon id, bool state)
-> extcon_set_cable_state_(struct extcon_dev *edev, enum extcon id, u64 state)

bool extcon_get_cable_state_(struct extcon_dev *edev, enum extcon id)
-> u64 extcon_get_cable_state_(struct extcon_dev *edev, enum extcon id)

The extcon client should do the bit masking operation to get the
wanted vaule of extcon_get_cable_state_(). I think it makes the
complicated on extcon client aspect.

If extcon add the new extcon_set_cable_line_state(), the extcon client
can get the state of whether external connector is attached or
detached by just calling the extcon_get_cable_state_() without any bit
operation.

Thanks,
Chanwoo Choi

Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB

[Roger Quadros]

Chanwoo,

On 29/05/15 15:15, Chanwoo Choi wrote:
> Dear all,
>
> On 05/29/2015 07:53 PM, Chanwoo Choi wrote:
>> Hi Peter,
>>
>> On 05/29/2015 10:22 AM, Peter Chen wrote:
>>> On Thu, May 28, 2015 at 05:23:57PM +0300, Roger Quadros wrote:
>>>> +Peter & Li,
>>>>
>>>> Ivan,
>>>>
>>>> On 28/05/15 11:45, Ivan T. Ivanov wrote:
>>>>>
>>>>> Hi Chanwoo,
>>>>>
>>>>> On Wed, 2015-05-27 at 21:15 +0900, Chanwoo Choi wrote:
>>>>>> Previously, I discussed how to inform the changed state of both ID
>>>>>> and VBUS pin for USB connector on patch-set[1].
>>>>>> [1] https://lkml.org/lkml/2015/4/2/310
>>>>>>
>>>>>> So, this patch adds the extcon_set_cable_line_state() function to inform
>>>>>> the additional state of external connectors without additional register/
>>>>>> unregister functions. This function uses the existing notifier chain
>>>>>> which is registered by extcon_register_notifier() / extcon_register_interest().
>>>>>>
>>>>>> The extcon_set_cable_line_state() can inform the new state of both
>>>>>> ID and VBUS pin state through extcon_set_cable_line_state().
>>>>>>
>>>>>> For exmaple:
>>>>>> - On extcon-usb-gpio.c as extcon provider driver as following:
>>>>>>          static void usb_extcon_detect_cable(struct work_struct *work)
>>>>>>          {
>>>>>>                  ...
>>>>>>                  /* check ID and update cable state */
>>>>>>                  id = gpiod_get_value_cansleep(info->id_gpiod);
>>>>>>                  if (id) {
>>>>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB_HOST, false);
>>>>>>                          extcon_set_cable_state_(info->edev, EXTCON_USB, true);
>>>>>>
>>>>>>                          extcon_set_cable_line_state(info->edev, EXTCON_USB,
>>>>>>                                                          EXTCON_USB_ID_HIGH);
>>>>>
>>>>> I am getting more and more confused :-). Why EXTCON_USB is now used for ID notifications?
>>>>> It should be EXTCON_USB_HOST, no? Why we need another function, framework already have
>>>>> required information from the function one line above, do I miss something?
>>>>
>>>> This is because the existing EXTCON_USB_HOST and EXTCON_USB do not capture all
>>>> the 4 states of ID and VBUS pins that we need for a real USB driver to work.
>>>>
>>>> It looks like it was designed from user space users perspective where they are
>>>> only interested in USB role. i.e. host or peripheral.
>>>>
>>>> Right now we are mixing both ID/VBUS and HOST/Peripheral states.
>>>> This will break when we consider OTG role switching.
>>>> With role switching, the USB device might start as a peripheral but switch role to host
>>>> on the fly and the existing setup (including these patches) can't cater to that
>>>> if user space is relying on EXTCON_USB_HOST/EXTCON_USB events.
>>>> Because they are hard-wired to the ID pin state which doesn't change during
>>>> role switch without cable switch.
>>>>
>>>> The USB driver doesn't care about EXTCON_USB_HOST/peripheral states.
>>>> It just needs ID/VBUS states and should decide the Host/Peripheral state from
>>>> that and other inputs (like HNP/user request/etc).
>>>>
>>>> The flow could be like this
>>>>
>>>> (extcon-usb-driver) -> [ID/VBUS states] -> (USB driver) -> [HOST/Peripheral states]
>>>
>>> Agree. Chanwoo, USB driver knows better than extcon driver about USB
>>> role (host/peripheral), so the app should use USB interface to know it,
>>> in fact, I don't be aware any use case needs to know USB role?
>>> Are there any users for EXTCON_USB and EXTCON_USB_HOST currently?
>>
>> You're right. But, extcon can just distinguish the type of external connectors
>> and inform the type to the user-space and extcon consumer driver in kernel-space.
>>
>> When USB mouse or keyboard is attached, user-space can check the state of externel connector which is attaced to the H/W target as following:
>> - /sys/class/extcon/extconX/cable.0/name -> USB
>> - /sys/class/extcon/extconX/cable.0/state -> 0
>> - /sys/class/extcon/extconX/cable.1/name -> USB-HOST
>> - /sys/class/extcon/extconX/cable.1/state -> 1
>>
>
> On last month, Robert Baldyga sent the patch[1] for extcon-usb-gpio driver.
> [1] https://lkml.org/lkml/2015/4/2/311
> - [PATCH v3 2/4] extcon: usb-gpio: add support for VBUS detection
>
> This patch[1] included a following table which show the type of external connectors
> according to both ID and VBUS pin state.
>
>   State              |    ID   |   VBUS
> ----------------------------------------
>   [1] USB            |    H    |    H
>   [2] none           |    H    |    L
>   [3] USB & USB-HOST |    L    |    H
>   [4] USB-HOST       |    L    |    L
>
> So, I think that extcon-usb-gpio.c could set below two status with set:
> - the state of whether USB/USB-HOST are attached or detached -> send the state to both kernel-space and user-space.
> : extcon_set_cable_state_()
> - the h/w line state of both ID and VBUS pin state -> send the state to only kernel space.
> : extcon_set_cable_line_state()
>
> [1]
>   State              |    ID   |   VBUS
> ----------------------------------------
>   [1] USB            |    H    |    H
>
> extcon_set_cable_state_(edev, EXTCON_USB, true);
> extcon_set_cable_state_(edev, EXTCON_USB_HOST, false);
>
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH);
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_HIGH);
>
>
> [2]
>   State              |    ID   |   VBUS
> ----------------------------------------
>   [2] none           |    H    |    L
>
> extcon_set_cable_state_(edev, EXTCON_USB, false);
> extcon_set_cable_state_(edev, EXTCON_USB_HOST, false);
>
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_HIGH);
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_LOW);
>
>
> [3]
>   State              |    ID   |   VBUS
> ----------------------------------------
>   [3] USB & USB-HOST |    L    |    H
>
> extcon_set_cable_state_(edev, EXTCON_USB, false);
> extcon_set_cable_state_(edev, EXTCON_USB_HOST, true);
>
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_LOW);
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_HIGH);
>
>
> [4]
>   State              |    ID   |   VBUS
> ----------------------------------------
>   [4] USB-HOST       |    L    |    L
>
> extcon_set_cable_state_(edev, EXTCON_USB, false);
> extcon_set_cable_state_(edev, EXTCON_USB_HOST, true);
>
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_ID_LOW);
> extcon_set_cable_line_state(edev, EXTCON_USB, EXTCON_USB_VBUS_LOW);
>
> If I know the wrong knowledge, please let me know your comment and advice.

I don't see any issues with extcon_get_cable_line_state(). I would prefer that
than changing existing APIS.

User space users will still fail in the dynamic role swap case [3]. But probably nobody
cares about it as nobody seems to be complaining or using role-swap :)
So we can choose to ignore that and solve it when someone complains.

cheers,
-roger