From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932149AbcHBBuK (ORCPT ); Mon, 1 Aug 2016 21:50:10 -0400 Received: from mailout2.samsung.com ([203.254.224.25]:47237 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932090AbcHBBuE convert rfc822-to-8bit (ORCPT ); Mon, 1 Aug 2016 21:50:04 -0400 X-AuditID: cbfee68e-f79cb6d000006cfe-66-579ffc49f57a MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 8BIT Message-id: <579FFC49.1040005@samsung.com> Date: Tue, 02 Aug 2016 10:50:01 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Guenter Roeck Cc: linux-kernel , myungjoo.ham@samsung.com, Chris Zhong , Guenter Roeck , chanwoo@kernel.org, "cpgs (cpgs@samsung.com)" Subject: Re: [PATCH v2 5/6] extcon: Add the synchronization extcon APIs to support the notification References: <1470030654-6641-1-git-send-email-cw00.choi@samsung.com> <1470030654-6641-6-git-send-email-cw00.choi@samsung.com> In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrJIsWRmVeSWpSXmKPExsWyRsSkUNfzz/xwg023TC0m3rjCYvHykKbF qVfLmC16Fy1hsbi8aw6bxe3GFWwW8x/sYHZg95jdcJHFY8GmUo9NqzrZPP7O2s/i0bdlFaPH 501yAWxRXDYpqTmZZalF+nYJXBnbnn1hK3ijXbH+bQdrA2OzYhcjJ4eEgInEoz1z2CFsMYkL 99azdTFycQgJrGCUONLzjhWm6OntdiaIxCxGiVO/JrCAJHgFBCV+TL4HZHNwMAuoS0yZkgsS ZhYQkWhbMIEZwtaWWLbwNTNE7wNGiaOnrzFB9GpJzFz0ihWkl0VAVeLCDV+QMBtQeP+LG2wg Nr+AosTVH48ZQUpEBSIkuk9UgoRFgKpPPT0OdiezwDVGiWX3FoPtEhZIlXg+cSbUA7OYJI5M PAb2GadAsMS540sZQRISAo/YJVat+gOWYBEQkPg2+RDYAxICshKbDjBDPCwpcXDFDZYJjBKz kLw5C+HNWUjenIXkzQWMLKsYRVMLkguKk9KLjPSKE3OLS/PS9ZLzczcxAmP49L9nfTsYbx6w PsQowMGoxMOr8WB+uBBrYllxZe4hRlOggyYyS4km5wMTRV5JvKGxmZGFqYmpsZG5pZmSOG+C 1M9gIYH0xJLU7NTUgtSi+KLSnNTiQ4xMHJxSDYyBPM2vTXzv/wv62eKdF++fGT7rZREvz28f O8NpezemRZf8mCrGelyxerdU/qbdU1LzG643hZQblhXnLJr2Mzx/eWZc0vzt0usLLn6smP25 n/uk3ePW2I2zdxY735zxV+qcv6XMXJ6SX77v37zdax5nf2mnT8taJ9Xc4jtvW8pli/8vSecX fKHEUpyRaKjFXFScCACw7ZRY3AIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrDIsWRmVeSWpSXmKPExsVy+t9jQV3PP/PDDf6t0LeYeOMKi8XLQ5oW p14tY7boXbSExeLyrjlsFrcbV7BZzH+wg9mB3WN2w0UWjwWbSj02repk8/g7az+LR9+WVYwe nzfJBbBFNTDaZKQmpqQWKaTmJeenZOal2yp5B8c7x5uaGRjqGlpamCsp5CXmptoqufgE6Lpl 5gCdoqRQlphTChQKSCwuVtK3wzQhNMRN1wKmMULXNyQIrsfIAA0krGHMmLr5IFPBEe2KWy8f szYw3lboYuTkkBAwkXh6u50JwhaTuHBvPVsXIxeHkMAsRolTvyawgCR4BQQlfky+B2RzcDAL yEscuZQNYapLTJmSC1H+gFHi6OlrTBDlWhIzF71iBalhEVCVuHDDFyTMBhTe/+IGG4jNL6Ao cfXHY0aQElGBCInuE5UgYRGg6lNPj4NdwCxwjVFi2b3FzCAJYYFUiecTZ8KcxiRxZOIxdpAE p0CwxLnjSxknMArOQnLpLIRLZyFcuoCReRWjRGpBckFxUnquYV5quV5xYm5xaV66XnJ+7iZG cMw/k9rBeHCX+yFGAQ5GJR5ei3fzw4VYE8uKK3MPMUpwMCuJ8DJ8BgrxpiRWVqUW5ccXleak Fh9iNAV6dSKzlGhyPjAd5ZXEGxqbmBlZGpkbWhgZmyuJ8z7+vy5MSCA9sSQ1OzW1ILUIpo+J g1OqgdFWa8aVlaU/pni+rUj9USYR9TYlqrZFX/tx/3Z9lUMc3me/bOjt7j3c1Ox04slv6aI9 lwLLWXxWxn/SnCvxuJlzhoXhQe5rDBPj74pE8DOcyDT/xGq6KMTw7TqR6q2vpzy5Jlk9PWKT ttaKM9kKIQsvGLClvJ+mMfuZscyCn7nieb5uF30umSixFGckGmoxFxUnAgAA/khWDwMAAA== DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Guenter, On 2016년 08월 02일 04:55, Guenter Roeck wrote: > On Sun, Jul 31, 2016 at 10:50 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 >> Tested-by: Chris Zhong > > Reviewed-by: Guenter Roeck 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]