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

[Chanwoo Choi <cw00.choi@samsung.com>]

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@samsung.com>
>> ---
>>   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