* [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB @ 2015-05-27 12:15 Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors Chanwoo Choi ` (3 more replies) 0 siblings, 4 replies; 24+ messages in thread From: Chanwoo Choi @ 2015-05-27 12:15 UTC (permalink / raw) To: linux-kernel Cc: rogerq, r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors 2015-05-27 12:15 [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Chanwoo Choi @ 2015-05-27 12:15 ` Chanwoo Choi 2015-05-27 14:38 ` Roger Quadros 2015-05-27 12:15 ` [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed Chanwoo Choi ` (2 subsequent siblings) 3 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-27 12:15 UTC (permalink / raw) To: linux-kernel Cc: rogerq, r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors 2015-05-27 12:15 ` [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors Chanwoo Choi @ 2015-05-27 14:38 ` Roger Quadros 2015-05-27 15:06 ` Chanwoo Choi 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2015-05-27 14:38 UTC (permalink / raw) To: Chanwoo Choi, linux-kernel Cc: r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors 2015-05-27 14:38 ` Roger Quadros @ 2015-05-27 15:06 ` Chanwoo Choi 2015-05-28 9:02 ` Ivan T. Ivanov 0 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-27 15:06 UTC (permalink / raw) To: Roger Quadros Cc: linux-kernel, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, iivanov, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors 2015-05-27 15:06 ` Chanwoo Choi @ 2015-05-28 9:02 ` Ivan T. Ivanov [not found] ` <CAGTfZH2rn7OfqaTmr0d5-MfWW3ZFdt05_7vtLKqbEQee53999w@mail.gmail.com> 0 siblings, 1 reply; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-28 9:02 UTC (permalink / raw) To: cw00.choi Cc: Roger Quadros, linux-kernel, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <CAGTfZH2rn7OfqaTmr0d5-MfWW3ZFdt05_7vtLKqbEQee53999w@mail.gmail.com>]
* Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors [not found] ` <CAGTfZH2rn7OfqaTmr0d5-MfWW3ZFdt05_7vtLKqbEQee53999w@mail.gmail.com> @ 2015-05-28 9:37 ` Ivan T. Ivanov 2015-05-29 7:58 ` Chanwoo Choi 0 siblings, 1 reply; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-28 9:37 UTC (permalink / raw) To: cw00.choi Cc: Roger Quadros, linux-kernel, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors 2015-05-28 9:37 ` Ivan T. Ivanov @ 2015-05-29 7:58 ` Chanwoo Choi 0 siblings, 0 replies; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 7:58 UTC (permalink / raw) To: Ivan T. Ivanov Cc: Roger Quadros, linux-kernel, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed 2015-05-27 12:15 [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors Chanwoo Choi @ 2015-05-27 12:15 ` Chanwoo Choi 2015-05-27 14:40 ` Roger Quadros 2015-05-27 14:09 ` [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Roger Quadros 2015-05-28 8:45 ` Ivan T. Ivanov 3 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-27 12:15 UTC (permalink / raw) To: linux-kernel Cc: rogerq, r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed 2015-05-27 12:15 ` [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed Chanwoo Choi @ 2015-05-27 14:40 ` Roger Quadros 0 siblings, 0 replies; 24+ messages in thread From: Roger Quadros @ 2015-05-27 14:40 UTC (permalink / raw) To: Chanwoo Choi, linux-kernel Cc: r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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); > } > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-27 12:15 [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed Chanwoo Choi @ 2015-05-27 14:09 ` Roger Quadros 2015-05-27 14:19 ` Chanwoo Choi 2015-05-28 8:45 ` Ivan T. Ivanov 3 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2015-05-27 14:09 UTC (permalink / raw) To: Chanwoo Choi, linux-kernel Cc: r.baldyga, peter.chen, kishon, balbi, iivanov, cw00.choi, myungjoo.ham 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(-) > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-27 14:09 ` [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Roger Quadros @ 2015-05-27 14:19 ` Chanwoo Choi 0 siblings, 0 replies; 24+ messages in thread From: Chanwoo Choi @ 2015-05-27 14:19 UTC (permalink / raw) To: Roger Quadros Cc: linux-kernel, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, iivanov, myungjoo.ham 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(-) >> > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-27 12:15 [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Chanwoo Choi ` (2 preceding siblings ...) 2015-05-27 14:09 ` [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Roger Quadros @ 2015-05-28 8:45 ` Ivan T. Ivanov 2015-05-28 14:23 ` Roger Quadros 2015-05-29 10:44 ` Chanwoo Choi 3 siblings, 2 replies; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-28 8:45 UTC (permalink / raw) To: Chanwoo Choi Cc: linux-kernel, rogerq, r.baldyga, peter.chen, kishon, balbi, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 8:45 ` Ivan T. Ivanov @ 2015-05-28 14:23 ` Roger Quadros 2015-05-29 1:22 ` Peter Chen ` (3 more replies) 2015-05-29 10:44 ` Chanwoo Choi 1 sibling, 4 replies; 24+ messages in thread From: Roger Quadros @ 2015-05-28 14:23 UTC (permalink / raw) To: Ivan T. Ivanov, Chanwoo Choi, balbi, peter.chen, jun.li Cc: linux-kernel, r.baldyga, peter.chen, kishon, cw00.choi, myungjoo.ham +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 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 14:23 ` Roger Quadros @ 2015-05-29 1:22 ` Peter Chen 2015-05-29 10:53 ` Chanwoo Choi 2015-05-29 7:35 ` Ivan T. Ivanov ` (2 subsequent siblings) 3 siblings, 1 reply; 24+ messages in thread From: Peter Chen @ 2015-05-29 1:22 UTC (permalink / raw) To: Roger Quadros Cc: Ivan T. Ivanov, Chanwoo Choi, balbi, jun.li, linux-kernel, r.baldyga, kishon, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 1:22 ` Peter Chen @ 2015-05-29 10:53 ` Chanwoo Choi 2015-05-29 12:15 ` Chanwoo Choi 0 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 10:53 UTC (permalink / raw) To: Peter Chen Cc: Roger Quadros, Ivan T. Ivanov, Chanwoo Choi, balbi, jun.li, linux-kernel, r.baldyga, kishon, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 10:53 ` Chanwoo Choi @ 2015-05-29 12:15 ` Chanwoo Choi 2015-06-02 6:51 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 12:15 UTC (permalink / raw) To: Peter Chen, Roger Quadros, Ivan T. Ivanov Cc: Chanwoo Choi, balbi, jun.li, linux-kernel, r.baldyga, kishon, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 12:15 ` Chanwoo Choi @ 2015-06-02 6:51 ` Roger Quadros 0 siblings, 0 replies; 24+ messages in thread From: Roger Quadros @ 2015-06-02 6:51 UTC (permalink / raw) To: Chanwoo Choi, Peter Chen, Ivan T. Ivanov Cc: Chanwoo Choi, balbi, jun.li, linux-kernel, r.baldyga, kishon, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 14:23 ` Roger Quadros 2015-05-29 1:22 ` Peter Chen @ 2015-05-29 7:35 ` Ivan T. Ivanov 2015-05-29 7:36 ` Ivan T. Ivanov 2015-05-29 10:39 ` Chanwoo Choi 3 siblings, 0 replies; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-29 7:35 UTC (permalink / raw) To: Roger Quadros Cc: Chanwoo Choi, balbi, peter.chen, jun.li, linux-kernel, r.baldyga, kishon, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 14:23 ` Roger Quadros 2015-05-29 1:22 ` Peter Chen 2015-05-29 7:35 ` Ivan T. Ivanov @ 2015-05-29 7:36 ` Ivan T. Ivanov 2015-05-29 10:39 ` Chanwoo Choi 3 siblings, 0 replies; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-29 7:36 UTC (permalink / raw) To: Roger Quadros Cc: Chanwoo Choi, balbi, peter.chen, jun.li, linux-kernel, r.baldyga, kishon, cw00.choi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 14:23 ` Roger Quadros ` (2 preceding siblings ...) 2015-05-29 7:36 ` Ivan T. Ivanov @ 2015-05-29 10:39 ` Chanwoo Choi 3 siblings, 0 replies; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 10:39 UTC (permalink / raw) To: Roger Quadros Cc: Ivan T. Ivanov, Chanwoo Choi, balbi, peter.chen, jun.li, linux-kernel, r.baldyga, kishon, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-28 8:45 ` Ivan T. Ivanov 2015-05-28 14:23 ` Roger Quadros @ 2015-05-29 10:44 ` Chanwoo Choi 2015-05-29 14:32 ` Ivan T. Ivanov 1 sibling, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 10:44 UTC (permalink / raw) To: Ivan T. Ivanov Cc: Chanwoo Choi, linux-kernel, rogerq, r.baldyga, peter.chen, kishon, balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 10:44 ` Chanwoo Choi @ 2015-05-29 14:32 ` Ivan T. Ivanov 2015-05-29 17:15 ` Chanwoo Choi 0 siblings, 1 reply; 24+ messages in thread From: Ivan T. Ivanov @ 2015-05-29 14:32 UTC (permalink / raw) To: Chanwoo Choi Cc: Chanwoo Choi, linux-kernel, rogerq, r.baldyga, peter.chen, kishon, balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 14:32 ` Ivan T. Ivanov @ 2015-05-29 17:15 ` Chanwoo Choi 2015-05-29 17:39 ` Chanwoo Choi 0 siblings, 1 reply; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 17:15 UTC (permalink / raw) To: Ivan T. Ivanov Cc: linux-kernel, Roger Quadros, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB 2015-05-29 17:15 ` Chanwoo Choi @ 2015-05-29 17:39 ` Chanwoo Choi 0 siblings, 0 replies; 24+ messages in thread From: Chanwoo Choi @ 2015-05-29 17:39 UTC (permalink / raw) To: Chanwoo Choi Cc: linux-kernel, Roger Quadros, Robert Baldyga, Peter Chen, Kishon Vijay Abraham I, Felipe Balbi, myungjoo.ham 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-06-02 6:52 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-27 12:15 [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 1/2] extcon: Add extcon_set_cable_line_state() to inform the additional state of external connectors Chanwoo Choi 2015-05-27 14:38 ` Roger Quadros 2015-05-27 15:06 ` Chanwoo Choi 2015-05-28 9:02 ` Ivan T. Ivanov [not found] ` <CAGTfZH2rn7OfqaTmr0d5-MfWW3ZFdt05_7vtLKqbEQee53999w@mail.gmail.com> 2015-05-28 9:37 ` Ivan T. Ivanov 2015-05-29 7:58 ` Chanwoo Choi 2015-05-27 12:15 ` [PATCH v2 2/2] extcon: usb-gpio: Update the ID pin state of USB when cable state is changed Chanwoo Choi 2015-05-27 14:40 ` Roger Quadros 2015-05-27 14:09 ` [PATCH v2 0/2] extcon: Inform the state of both ID and VBUS pin for USB Roger Quadros 2015-05-27 14:19 ` Chanwoo Choi 2015-05-28 8:45 ` Ivan T. Ivanov 2015-05-28 14:23 ` Roger Quadros 2015-05-29 1:22 ` Peter Chen 2015-05-29 10:53 ` Chanwoo Choi 2015-05-29 12:15 ` Chanwoo Choi 2015-06-02 6:51 ` Roger Quadros 2015-05-29 7:35 ` Ivan T. Ivanov 2015-05-29 7:36 ` Ivan T. Ivanov 2015-05-29 10:39 ` Chanwoo Choi 2015-05-29 10:44 ` Chanwoo Choi 2015-05-29 14:32 ` Ivan T. Ivanov 2015-05-29 17:15 ` Chanwoo Choi 2015-05-29 17:39 ` Chanwoo Choi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).