From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754050AbdC3JUw (ORCPT ); Thu, 30 Mar 2017 05:20:52 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:59971 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753060AbdC3JUu (ORCPT ); Thu, 30 Mar 2017 05:20:50 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: b6c32a2d-f792a6d0000055a1-ed-58dccdefc1f1 Content-transfer-encoding: 8BIT Message-id: <58DCCDED.6040605@samsung.com> Date: Thu, 30 Mar 2017 18:20:45 +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: Hans de Goede , linux-kernel@vger.kernel.org Cc: chanwoo@kernel.org, myungjoo.ham@samsung.com Subject: Re: [PATCH 2/2] extcon: Add new extcon_register_notifier_all() to monitor all external connectors In-reply-to: <5ec5c655-2254-696d-cfca-029d5eb5f17c@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrDKsWRmVeSWpSXmKPExsWy7bCmhu77s3ciDGZ3CllMvHGFxeLN8elM Fpd3zWGzuN24gs2BxWPTqk42j/f7rrJ59G1ZxejxeZNcAEtUqk1GamJKapFCal5yfkpmXrqt kndwvHO8qZmBoa6hpYW5kkJeYm6qrZKLT4CuW2YO0EolhbLEnFKgUEBicbGSvp1NUX5pSapC Rn5xia1StKGhkZ6hgbmekZGRnolxrJWRKVBJQmrGqcfH2Qtex1Uc+W7UwNjj2cXIySEhYCLx e+9CZghbTOLCvfVsILaQwFJGid4lhV2MXEB2O5NE5+qbTDANPRfOMUIkljNKzN66nREkwSsg KPFj8j2WLkYODmYBeYkjl7JBwswCmhJbd69nh6i/xyhxpPM4K0S9lsSpM/PZQOpZBFQl1s/O BwmzAYX3v7gBdgS/gKLE1R+PwcaLCkRI7Jz/jR3EFhFwlZi/bTc7xHwDiUdX94GNFBbIkViz 7ztYL6eAncTnxp1gd0oITGeXmPl0ISvILgkBWYlNB6AedpHYt+83lC0s8er4FnYIW1ri79Jb UL3tjBKb54D8BeJ0MErcX9nIClFlLHH/wT1miCv4JHp/P2GCWMAr0dEmBGF6SEw6mQBR7Sjx /NwXNkg4dAIDdO0t1gmMCrOQgm4WIuhmIQXdAkbmVYxiqQXFuempxaYFRnrFibnFpXnpesn5 uZsYwSlPS3cH45cF3ocYBTgYlXh4K9bejhBiTSwrrsw9xCjBwawkwjvjyJ0IId6UxMqq1KL8 +KLSnNTiQ4ymwJCfyCwlmpwPTMd5JfGGJpYGJqZmxgbGhhaWSuK86iuvRQgJpCeWpGanphak FsH0MXFwSjUwBp5+ef571MwHWW82Po99/iZWLlLMre+I3UG1yTye+fv4Avw5/NsMdsyqyefO W1TAY6S+4F/JJt/E07OKYjnTJI32H33Qpbv4wPfD/9T2vZh4XvryixWbTqckbdkXPztA5626 ZUPk5iVTym8uFI856na68vjLLtX3eeH9fk+9hC9V+Rnzbp5gpsRSnJFoqMVcVJwIAFBq5kaP AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrJIsWRmVeSWpSXmKPExsVy+t9jAd13Z+9EGHSstLaYeOMKi8Wb49OZ LC7vmsNmcbtxBZsDi8emVZ1sHu/3XWXz6NuyitHj8ya5AJYoN5uM1MSU1CKF1Lzk/JTMvHRb pdAQN10LJYW8xNxUW6UIXd+QICWFssScUiDPyAANODgHuAcr6dsluGWcenycveB1XMWR70YN jD2eXYycHBICJhI9F84xQthiEhfurWfrYuTiEBJYyiix5uYhsASvgKDEj8n3WLoYOTiYBeQl jlzKBgkzC6hLTJq3iBmi/gGjxJRJn1kg6rUkTp2ZzwZSzyKgKrF+dj5ImA0ovP/FDTYQm19A UeLqj8eMICWiAhES3ScqQcIiAq4S87ftZocYbyDx6Oo+VhBbWCBHYs2+71CndTJJtM74CnYa p4CdxOfGnYwTGAVnIbl0FsKls5BcuoCReRWjRGpBckFxUnquUV5quV5xYm5xaV66XnJ+7iZG cDQ9k97BeHiX+yFGAQ5GJR7eirW3I4RYE8uKK3MPMUpwMCuJ8M44cidCiDclsbIqtSg/vqg0 J7X4EKMp0KsTmaVEk/OBkZ5XEm9oYm5ibmxgYW5paWKkJM7bOPtZuJBAemJJanZqakFqEUwf EwenVAMje8kZaf3y1UxPX56N0gko0NGPqf8xsXvmElGdy6Ezv95NOpY+Z7P7AYnXIQymx71m rbjo8vzHVSWJfRcyWh4mH+r8EhH5xc5f8fHBW4XpLJz5NocLNr4/mxl7mPfT+YU8b6fsWqQr 6XviqvCmQkaNzBMvd3+QubBbo9AnvlJCgnnhd2b51wH6SizFGYmGWsxFxYkARva3oLwCAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170330092047epcas5p48e569d12efde6efe545746e0c00f231d X-Msg-Generator: CA X-Sender-IP: 203.254.230.27 X-Local-Sender: =?UTF-8?B?7LWc7LCs7JqwG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLw=?= =?UTF-8?B?7YSwKRvsgrzshLHsoITsnpAbU2VuaW9yIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?Q2hhbndvbyBDaG9pG1RpemVuIFBsYXRmb3JtIExhYi4bU2Ft?= =?UTF-8?B?c3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG1NUQUYbQzEwVjgxMTE=?= CMS-TYPE: 105P DLP-Filter: Pass X-CFilter-Loop: Reflected X-HopCount: 7 X-CMS-RootMailID: 20170330083943epcas1p4c5559cab13ef732b6bf149f810aa2f46 X-RootMTR: 20170330083943epcas1p4c5559cab13ef732b6bf149f810aa2f46 References: <1490863178-12782-1-git-send-email-cw00.choi@samsung.com> <1490863178-12782-2-git-send-email-cw00.choi@samsung.com> <5ec5c655-2254-696d-cfca-029d5eb5f17c@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017년 03월 30일 18:04, Hans de Goede wrote: > Hi Chanwoo, > > On 30-03-17 10:39, Chanwoo Choi wrote: >> The extcon core already provides the extcon_register_notifier() function >> in order to register the notifier block which is used to monitor >> the status change for the specific external connector such as EXTCON_USB, >> EXTCON_USB_HOST and so on. The extcon consumer uses the this function. >> >> The extcon consumer may need to monitor the all supported external >> connectors from the extcon device. In this case, The extcon consumer >> should have each notifier_block structure for each external connector. >> >> This patch adds the new extcon_register_notifier_all() function >> that extcon consumer is able to monitor the status change of all >> supported external connectors by using only one notifier_block structure. >> >> - List of new added functions: >> int extcon_register_notifier_all(struct extcon_dev *edev, >> struct notifier_block *nb); >> int extcon_unregister_notifier_all(struct extcon_dev *edev, >> struct notifier_block *nb); >> int devm_extcon_register_notifier_all(struct device *dev, >> struct extcon_dev *edev, struct notifier_block *nb); >> void devm_extcon_unregister_notifier_all(struct device *dev, >> struct extcon_dev *edev, struct notifier_block *nb); >> >> Suggested-by: Hans de Goede >> Signed-off-by: Chanwoo Choi >> --- >> drivers/extcon/devres.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/extcon/extcon.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++ >> drivers/extcon/extcon.h | 3 +++ >> include/linux/extcon.h | 21 ++++++++++++---- >> 4 files changed, 145 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/extcon/devres.c b/drivers/extcon/devres.c >> index b40eb1805927..3fc8eea52f1d 100644 >> --- a/drivers/extcon/devres.c >> +++ b/drivers/extcon/devres.c >> @@ -50,6 +50,13 @@ static void devm_extcon_dev_notifier_unreg(struct device *dev, void *res) >> extcon_unregister_notifier(this->edev, this->id, this->nb); >> } >> >> +static void devm_extcon_dev_notifier_all_unreg(struct device *dev, void *res) >> +{ >> + struct extcon_dev_notifier_devres *this = res; >> + >> + extcon_unregister_notifier_all(this->edev, this->nb); >> +} >> + >> /** >> * devm_extcon_dev_allocate - Allocate managed extcon device >> * @dev: device owning the extcon device being created >> @@ -214,3 +221,60 @@ void devm_extcon_unregister_notifier(struct device *dev, >> devm_extcon_dev_match, edev)); >> } >> EXPORT_SYMBOL(devm_extcon_unregister_notifier); >> + >> +/** >> + * devm_extcon_register_notifier_all() >> + * - Resource-managed extcon_register_notifier_all() >> + * @dev: device to allocate extcon device >> + * @edev: the extcon device that has the external connecotr. >> + * @nb: a notifier block to be registered. >> + * >> + * This function manages automatically the notifier of extcon device using >> + * device resource management and simplify the control of unregistering >> + * the notifier of extcon device. >> + * >> + * Note that the second parameter given to the callback of nb (val) is >> + * the current state and third parameter is the edev pointer. >> + * >> + * Returns 0 if success or negaive error number if failure. >> + */ >> +int devm_extcon_register_notifier_all(struct device *dev, struct extcon_dev *edev, >> + struct notifier_block *nb) >> +{ >> + struct extcon_dev_notifier_devres *ptr; >> + int ret; >> + >> + ptr = devres_alloc(devm_extcon_dev_notifier_all_unreg, sizeof(*ptr), >> + GFP_KERNEL); >> + if (!ptr) >> + return -ENOMEM; >> + >> + ret = extcon_register_notifier_all(edev, nb); >> + if (ret) { >> + devres_free(ptr); >> + return ret; >> + } >> + >> + ptr->edev = edev; >> + ptr->nb = nb; >> + devres_add(dev, ptr); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(devm_extcon_register_notifier_all); >> + >> +/** >> + * devm_extcon_unregister_notifier_all() >> + - Resource-managed extcon_unregister_notifier_all() >> + * @dev: device to allocate extcon device >> + * @edev: the extcon device that has the external connecotr. >> + * @nb: a notifier block to be registered. >> + */ >> +void devm_extcon_unregister_notifier_all(struct device *dev, >> + struct extcon_dev *edev, >> + struct notifier_block *nb) >> +{ >> + WARN_ON(devres_release(dev, devm_extcon_dev_notifier_all_unreg, >> + devm_extcon_dev_match, edev)); >> +} >> +EXPORT_SYMBOL(devm_extcon_unregister_notifier_all); >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >> index 193a3a673d10..4873fe8d5cd8 100644 >> --- a/drivers/extcon/extcon.c >> +++ b/drivers/extcon/extcon.c >> @@ -445,8 +445,19 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id) >> spin_lock_irqsave(&edev->lock, flags); >> >> state = !!(edev->state & BIT(index)); >> + >> + /* >> + * Call functions in a raw notifier chain for the specific one >> + * external connector. >> + */ >> raw_notifier_call_chain(&edev->nh[index], state, edev); >> >> + /* >> + * Call functions in a raw notifier chain for the all supported >> + * external connectors. >> + */ >> + raw_notifier_call_chain(&edev->nh_all, state, edev); >> + > > Passing state here is not useful since the receiver of > the notifier call will not know for which cable the state is. No, the receiver can use the 'state' value whether connector is attached or detached. Also, the extcon have to keep the same value for both extcon_register_notifier() and extcon_register_notfier_all(). > > Also this should be moved outside of the area of the > function holding the edev->lock spinlock, since we don't > pass state we do not need the lock and the called > notifier function may very well want to call extcon_get_state > to find out the state of one or more of the cables, > which takes the lock. The extcon uses the spinlock for the short delay. Extcon should update the status of external connector to the extcon consumer as soon as possible. > > Otherwise looks good. > > Regards, > > Hans > > > >> /* This could be in interrupt handler */ >> prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); >> if (!prop_buf) { >> @@ -951,6 +962,55 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id, >> } >> EXPORT_SYMBOL_GPL(extcon_unregister_notifier); >> >> +/** >> + * extcon_register_notifier_all() - Register a notifier block to get the noti >> + * of the status change for all supported external >> + * connectors from extcon. >> + * @edev: the extcon device that has the external connecotr. >> + * @nb: a notifier block to be registered. >> + * >> + * Note that the second parameter given to the callback of nb (val) is >> + * the current state and third parameter is the edev pointer. >> + */ >> +int extcon_register_notifier_all(struct extcon_dev *edev, >> + struct notifier_block *nb) >> +{ >> + unsigned long flags; >> + int ret; >> + >> + if (!edev || !nb) >> + return -EINVAL; >> + >> + spin_lock_irqsave(&edev->lock, flags); >> + ret = raw_notifier_chain_register(&edev->nh_all, nb); >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_register_notifier_all); >> + >> +/** >> + * extcon_unregister_notifier_all() - Unregister a notifiee from the extcon device. >> + * @edev: the extcon device that has the external connecotr. >> + * @nb: a notifier block to be registered. >> + */ >> +int extcon_unregister_notifier_all(struct extcon_dev *edev, >> + struct notifier_block *nb) >> +{ >> + unsigned long flags; >> + int ret; >> + >> + if (!edev || !nb) >> + return -EINVAL; >> + >> + spin_lock_irqsave(&edev->lock, flags); >> + ret = raw_notifier_chain_unregister(&edev->nh_all, nb); >> + spin_unlock_irqrestore(&edev->lock, flags); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(extcon_unregister_notifier_all); >> + >> static struct attribute *extcon_attrs[] = { >> &dev_attr_state.attr, >> &dev_attr_name.attr, >> @@ -1199,6 +1259,8 @@ int extcon_dev_register(struct extcon_dev *edev) >> for (index = 0; index < edev->max_supported; index++) >> RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]); >> >> + RAW_INIT_NOTIFIER_HEAD(&edev->nh_all); >> + >> dev_set_drvdata(&edev->dev, edev); >> edev->state = 0; >> >> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h >> index 993ddccafe11..dddddcfa0587 100644 >> --- a/drivers/extcon/extcon.h >> +++ b/drivers/extcon/extcon.h >> @@ -21,6 +21,8 @@ >> * @dev: Device of this extcon. >> * @state: Attach/detach state of this extcon. Do not provide at >> * register-time. >> + * @nh_all: Notifier for the state change events for all supported >> + * external connectors from this extcon. >> * @nh: Notifier for the state change events from this extcon >> * @entry: To support list of extcon devices so that users can >> * search for extcon devices based on the extcon name. >> @@ -43,6 +45,7 @@ struct extcon_dev { >> >> /* Internal data. Please do not set. */ >> struct device dev; >> + struct raw_notifier_head nh_all; >> struct raw_notifier_head *nh; >> struct list_head entry; >> int max_supported; >> diff --git a/include/linux/extcon.h b/include/linux/extcon.h >> index 3929d2e8a3c7..61e8d8c591a4 100644 >> --- a/include/linux/extcon.h >> +++ b/include/linux/extcon.h >> @@ -270,11 +270,11 @@ extern int extcon_set_property_capability(struct extcon_dev *edev, >> unsigned int id, unsigned int prop); >> >> /* >> - * Following APIs are to monitor every action of a notifier. >> - * Registrar gets notified for every external port of a connection device. >> - * Probably this could be used to debug an action of notifier; however, >> - * we do not recommend to use this for normal 'notifiee' device drivers who >> - * want to be notified by a specific external port of the notifier. >> + * Following APIs are to monitor the status change of the external connectors. >> + * extcon_register_notifier(*edev, id, *nb) : Register a notifier >> + * for specific external connector from the extcon. >> + * extcon_register_notifier_all(*edev, *nb) : Register a notifier >> + * for all supported external connectors from the extcon. >> */ >> extern int extcon_register_notifier(struct extcon_dev *edev, unsigned int id, >> struct notifier_block *nb); >> @@ -287,6 +287,17 @@ extern void devm_extcon_unregister_notifier(struct device *dev, >> struct extcon_dev *edev, unsigned int id, >> struct notifier_block *nb); >> >> +extern int extcon_register_notifier_all(struct extcon_dev *edev, >> + struct notifier_block *nb); >> +extern int extcon_unregister_notifier_all(struct extcon_dev *edev, >> + struct notifier_block *nb); >> +extern int devm_extcon_register_notifier_all(struct device *dev, >> + struct extcon_dev *edev, >> + struct notifier_block *nb); >> +extern void devm_extcon_unregister_notifier_all(struct device *dev, >> + struct extcon_dev *edev, >> + struct notifier_block *nb); >> + >> /* >> * Following API get the extcon device from devicetree. >> * This function use phandle of devicetree to get extcon device directly. >> > > > -- Best Regards, Chanwoo Choi Samsung Electronics