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

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

Hi Guenter,

On 2016년 08월 02일 04:55, Guenter Roeck wrote:
> On Sun, Jul 31, 2016 at 10:50 PM, Chanwoo Choi <cw00.choi@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.choi@samsung.com>
>> Tested-by: Chris Zhong <zyw@rock-chips.com>
> 
> Reviewed-by: Guenter Roeck <groeck@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]