Hi Chris, On 2016년 07월 27일 16:30, Chris Zhong wrote: > Hi Chanwoo > > On 07/26/2016 08:09 PM, Chanwoo Choi wrote: >> This patch adds the synchronization extcon APIs to support the notifications >> for both state and property. When extcon_*_sync() functions is called, >> the extcon informs the information from extcon provider to extcon client. >> >> The extcon driver may need to change the both state and multiple properties >> at the same time. After setting the data of a external connector, >> the extcon send the notification to client driver with the extcon_*_sync(). >> >> The list of new extcon APIs as following: >> - extcon_sync() : Send the notification for each external connector to >> synchronize the information between extcon provider driver >> and extcon client driver. >> - extcon_set_state_sync() : Set the state of external connector with noti. >> - extcon_set_property_sync() : Set the property of external connector with >> noti. >> >> For example, >> case 1, change the state of external connector and synchronized the data. >> extcon_set_state_sync(edev, EXTCON_USB, 1); >> >> case 2, change both the state and property of external connector >> and synchronized the data. >> extcon_set_state(edev, EXTCON_USB, 1); >> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1); >> extcon_sync(edev, EXTCON_USB); >> >> case 3, change the property of external connector and synchronized the data. >> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0); >> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1); >> extcon_sync(edev, EXTCON_USB); >> >> case 4, change the property of external connector and synchronized the data. >> extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0); >> >> Signed-off-by: Chanwoo Choi <[email protected]> >> --- >> drivers/extcon/extcon.c | 199 >> ++++++++++++++++++++++++++++++------------------ >> include/linux/extcon.h | 30 +++++++- >> 2 files changed, 152 insertions(+), 77 deletions(-) >> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 73c6981bf559..8572523bfd40 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -280,14 +280,11 @@ static bool is_extcon_attached(struct extcon_dev >> *edev, unsigned int id, >> return ((!!(edev->state & (1 << index))) == 1) ? true : false; >> } >> -static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached) >> +static bool is_extcon_changed(struct extcon_dev *edev, int index, >> + bool new_state) >> { >> - if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) { >> - *attached = ((new >> idx) & 0x1) ? true : false; >> - return true; >> - } >> - >> - return false; >> + int state = !!(edev->state & (1 << index)); >> + return (state != new_state) ? true : false; >> } >> static bool is_extcon_property_supported(unsigned int id, unsigned int >> prop) >> @@ -377,21 +374,13 @@ static ssize_t cable_state_show(struct device *dev, >> } >> /** >> - * extcon_update_state() - Update the cable attach states of the extcon >> device >> - * only for the masked bits. >> - * @edev: the extcon device >> - * @mask: the bit mask to designate updated bits. >> - * @state: new cable attach status for @edev >> - * >> - * Changing the state sends uevent with environment variable containing >> - * the name of extcon device (envp[0]) and the state output (envp[1]). >> - * Tizen uses this format for extcon device to get events from ports. >> - * Android uses this format as well. >> + * extcon_sync() - Synchronize the states for both the attached/detached >> + * @edev: the extcon device that has the cable. >> * >> - * Note that the notifier provides which bits are changed in the state >> - * variable with the val parameter (second) to the callback. >> + * This function send a notification to synchronize the all states of a >> + * specific external connector >> */ >> -static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state) >> +int extcon_sync(struct extcon_dev *edev, unsigned int id) >> { >> char name_buf[120]; >> char state_buf[120]; >> @@ -400,73 +389,58 @@ static int extcon_update_state(struct extcon_dev >> *edev, u32 mask, u32 state) >> int env_offset = 0; >> int length; >> int index; >> + int state; >> unsigned long flags; >> - bool attached; >> if (!edev) >> return -EINVAL; >> - spin_lock_irqsave(&edev->lock, flags); >> + index = find_cable_index_by_id(edev, id); >> + if (index < 0) >> + return index; >> - if (edev->state != ((edev->state & ~mask) | (state & mask))) { >> - u32 old_state; >> + state = !!(edev->state & (1 << index)); >> + raw_notifier_call_chain(&edev->nh[index], state, edev); >> - if (check_mutually_exclusive(edev, (edev->state & ~mask) | >> - (state & mask))) { >> - spin_unlock_irqrestore(&edev->lock, flags); >> - return -EPERM; >> - } >> + spin_lock_irqsave(&edev->lock, flags); >> - old_state = edev->state; >> - edev->state &= ~mask; >> - edev->state |= state & mask; >> + /* This could be in interrupt handler */ >> + prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); >> + if (!prop_buf) { >> + /* Unlock early before uevent */ >> + spin_unlock_irqrestore(&edev->lock, flags); >> - for (index = 0; index < edev->max_supported; index++) { >> - if (is_extcon_changed(old_state, edev->state, index, >> - &attached)) >> - raw_notifier_call_chain(&edev->nh[index], >> - attached, edev); >> - } >> + dev_err(&edev->dev, "out of memory in extcon_set_state\n"); >> + kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE); >> - /* This could be in interrupt handler */ >> - prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); >> - if (prop_buf) { >> - length = name_show(&edev->dev, NULL, prop_buf); >> - if (length > 0) { >> - if (prop_buf[length - 1] == '\n') >> - prop_buf[length - 1] = 0; >> - snprintf(name_buf, sizeof(name_buf), >> - "NAME=%s", prop_buf); >> - envp[env_offset++] = name_buf; >> - } >> - length = state_show(&edev->dev, NULL, prop_buf); >> - if (length > 0) { >> - if (prop_buf[length - 1] == '\n') >> - prop_buf[length - 1] = 0; >> - snprintf(state_buf, sizeof(state_buf), >> - "STATE=%s", prop_buf); >> - envp[env_offset++] = state_buf; >> - } >> - envp[env_offset] = NULL; >> - /* Unlock early before uevent */ >> - spin_unlock_irqrestore(&edev->lock, flags); >> + return 0; >> + } >> - kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp); >> - free_page((unsigned long)prop_buf); >> - } else { >> - /* Unlock early before uevent */ >> - spin_unlock_irqrestore(&edev->lock, flags); >> + length = name_show(&edev->dev, NULL, prop_buf); >> + if (length > 0) { >> + if (prop_buf[length - 1] == '\n') >> + prop_buf[length - 1] = 0; >> + snprintf(name_buf, sizeof(name_buf), "NAME=%s", prop_buf); >> + envp[env_offset++] = name_buf; >> + } >> - dev_err(&edev->dev, "out of memory in extcon_set_state\n"); >> - kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE); >> - } >> - } else { >> - /* No changes */ >> - spin_unlock_irqrestore(&edev->lock, flags); >> + length = state_show(&edev->dev, NULL, prop_buf); >> + if (length > 0) { >> + if (prop_buf[length - 1] == '\n') >> + prop_buf[length - 1] = 0; >> + snprintf(state_buf, sizeof(state_buf), "STATE=%s", prop_buf); >> + envp[env_offset++] = state_buf; >> } >> + envp[env_offset] = NULL; >> + >> + /* Unlock early before uevent */ >> + spin_unlock_irqrestore(&edev->lock, flags); >> + kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp); >> + free_page((unsigned long)prop_buf); >> return 0; >> } >> +EXPORT_SYMBOL_GPL(extcon_sync); >> /** >> * extcon_get_state() - Get the state of a external connector. >> @@ -493,17 +467,22 @@ EXPORT_SYMBOL_GPL(extcon_get_state); >> /** >> * extcon_set_state() - Set the state of a external connector. >> + * without a notification. >> * @edev: the extcon device that has the cable. >> * @id: the unique id of each external connector >> * in extcon enumeration. >> * @state: the new cable status. The default semantics is >> * true: attached / false: detached. >> + * >> + * This function only set the state of a external connector without >> + * a notification. To synchronize the data of a external connector, >> + * use extcon_set_state_sync() and extcon_sync(). >> */ >> int extcon_set_state(struct extcon_dev *edev, unsigned int id, >> bool cable_state) >> { >> - u32 state; >> - int index; >> + unsigned long flags; >> + int index, ret = 0; >> if (!edev) >> return -EINVAL; >> @@ -515,6 +494,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned >> int id, >> if (edev->max_supported && edev->max_supported <= index) >> return -EINVAL; >> + spin_lock_irqsave(&edev->lock, flags); >> + >> + /* Check whether the external connector's state is changed. */ >> + if (!is_extcon_changed(edev, index, cable_state)) >> + goto out; >> + >> + if (check_mutually_exclusive(edev, >> + (edev->state & ~(1 << index)) | (cable_state & (1 << index)))) { >> + ret = -EPERM; >> + goto out; >> + } >> + >> /* >> * Initialize the value of extcon property before setting >> * the detached state for an external connector. >> @@ -522,13 +513,44 @@ int extcon_set_state(struct extcon_dev *edev, unsigned >> int id, >> if (!cable_state) >> init_property(edev, id, index); >> - /* Set the state for external connector as the detached state. */ >> - state = cable_state ? (1 << index) : 0; >> - return extcon_update_state(edev, 1 << index, state); >> + /* Update the state for a external connector. */ >> + if (cable_state) >> + edev->state |= (1 << index); >> + else >> + edev->state &= ~(1 << index); >> +out: >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> } >> EXPORT_SYMBOL_GPL(extcon_set_state); >> /** >> + * extcon_set_state_sync() - Set the state of a external connector >> + * with a notification. >> + * @edev: the extcon device that has the cable. >> + * @id: the unique id of each external connector >> + * in extcon enumeration. >> + * @state: the new cable status. The default semantics is >> + * true: attached / false: detached. >> + * >> + * This function set the state of external connector and synchronize the >> data >> + * by usning a notification. >> + */ >> +int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id, >> + bool cable_state) >> +{ >> + int ret; >> + >> + ret = extcon_set_state(edev, id, cable_state); >> + if (ret < 0) >> + return ret; >> + >> + return extcon_sync(edev, id); > So, Regardless of whether the state change, the notifier chain will be called, > If this function can decide whether to call a notice, according to the state > change. I think it would be better. > The old extcon_set_cable_state_ was working like this.
OK. I'll modify it on next version. [snip] Regards, Chanwoo Choi
