Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification

From: Chanwoo Choi
Date: Wed Jul 27 2016 - 03:41:20 EST

Next message: Maxime Ripard: "Re: [PATCH 7/9] clk: sunxi-ng: mux: Add clk notifier functions"
Previous message: Maxime Ripard: "Re: [PATCH 4/9] clk: sunxi-ng: mux: Add support for mux tables"
In reply to: Chris Zhong: "Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification"
Next in thread: Chanwoo Choi: "[PATCH 1/6] extcon: Add the extcon_type to group each connector into five category"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Chris,

On 2016ë 07ì 27ì 16:30, Chris Zhong wrote:
> Hi Chanwoo
>
> On 07/26/2016 08:09 PM, Chanwoo Choi wrote:
>> This patch adds the synchronization extcon APIs to support the notifications
>> for both state and property. When extcon_*_sync() functions is called,
>> the extcon informs the information from extcon provider to extcon client.
>>
>> The extcon driver may need to change the both state and multiple properties
>> at the same time. After setting the data of a external connector,
>> the extcon send the notification to client driver with the extcon_*_sync().
>>
>> The list of new extcon APIs as following:
>> - extcon_sync() : Send the notification for each external connector to
>> synchronize the information between extcon provider driver
>> and extcon client driver.
>> - extcon_set_state_sync() : Set the state of external connector with noti.
>> - extcon_set_property_sync() : Set the property of external connector with noti.
>>
>> For example,
>> case 1, change the state of external connector and synchronized the data.
>> extcon_set_state_sync(edev, EXTCON_USB, 1);
>>
>> case 2, change both the state and property of external connector
>> and synchronized the data.
>> extcon_set_state(edev, EXTCON_USB, 1);
>> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
>> extcon_sync(edev, EXTCON_USB);
>>
>> case 3, change the property of external connector and synchronized the data.
>> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
>> extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
>> extcon_sync(edev, EXTCON_USB);
>>
>> case 4, change the property of external connector and synchronized the data.
>> extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>> ---
>> 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

Next message: Maxime Ripard: "Re: [PATCH 7/9] clk: sunxi-ng: mux: Add clk notifier functions"
Previous message: Maxime Ripard: "Re: [PATCH 4/9] clk: sunxi-ng: mux: Add support for mux tables"
In reply to: Chris Zhong: "Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to support the notification"
Next in thread: Chanwoo Choi: "[PATCH 1/6] extcon: Add the extcon_type to group each connector into five category"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]