Hi Guenter,
On 2016년 08월 02일 04:55, Guenter Roeck wrote:
> On Sun, Jul 31, 2016 at 10:50 PM, Chanwoo Choi <cw00.c...@samsung.com> 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.c...@samsung.com>
>> Tested-by: Chris Zhong <z...@rock-chips.com>
>
> Reviewed-by: Guenter Roeck <gro...@chromium.org>
Thanks for the review.
>
> [ couple of nitpicks below ]
>
>> ---
>> drivers/extcon/extcon.c | 210
>> +++++++++++++++++++++++++++++++-----------------
>> include/linux/extcon.h | 30 ++++++-
>> 2 files changed, 164 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 207143347fb7..680246cceb62 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -279,14 +279,11 @@ static bool is_extcon_attached(struct extcon_dev
>> *edev, unsigned int index)
>> return !!(edev->state & BIT(index));
>> }
>>
>> -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;
>
> This is identical to
> return state != new_state;
OK. I'll modify it.
>
>> }
>>
>> static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>> @@ -402,21 +399,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];
>> @@ -425,73 +414,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;
>>
>> + index = find_cable_index_by_id(edev, id);
>> + if (index < 0)
>> + return index;
>> +
>> spin_lock_irqsave(&edev->lock, flags);
>>
>> - if (edev->state != ((edev->state & ~mask) | (state & mask))) {
>> - u32 old_state;
>> + state = !!(edev->state & (1 << index));
>
> At some point it might make sense to update the entire file to use
> BIT(index) instead of "1 << index".
OK. I'll update them on extcon.c.
Regards,
Chanwoo Choi
[snip]
