From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752715AbdCTAzp (ORCPT ); Sun, 19 Mar 2017 20:55:45 -0400 Received: from mailout1.samsung.com ([203.254.224.24]:34041 "EHLO mailout1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752151AbdCTAzm (ORCPT ); Sun, 19 Mar 2017 20:55:42 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8 X-AuditID: b6c32a2e-f79d66d0000012ad-1c-58cf288b15cf Content-transfer-encoding: 8BIT Message-id: <58CF288A.9090507@samsung.com> Date: Mon, 20 Mar 2017 09:55:38 +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 , MyungJoo Ham Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev In-reply-to: <20170319182334.6456-1-hdegoede@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrMKsWRmVeSWpSXmKPExsWy7bCmhm63xvkIgzO/rSzeHJ/OZHF51xw2 i9uNK9gcmD3e77vK5tG3ZRWjx+dNcgHMUak2GamJKalFCql5yfkpmXnptkrewfHO8aZmBoa6 hpYW5koKeYm5qbZKLj4Bum6ZOUCLlBTKEnNKgUIBicXFSvp2NkX5pSWpChn5xSW2StGGhkZ6 hgbmekZGRnomxrFWRqZAJQmpGf82PmYr6LatmPxnE2MDY7NhFyMHh4SAicTE6+JdjJxAppjE hXvr2boYuTiEBJYyShz7cJsJJCEk0M4kcfOyFkSRicSUKe+giuYwSvzccooFJMErICjxY/I9 FpChzALyEkcuZYOEmQU0JbbuXs8OMeceo0TfPyOIci2J+etuMIPYLAKqEpOvHQKrYQOK739x gw3E5hdQlLj64zEjiC0qECGxc/43sBoRgSCJG99XsELMV5D4dW8TmC0skCrRu3gHWC+ngIXE yQ/XWEDulBDoZpdoPwoyCORhWYlNB5ghfnGRmLn0B5QtLPHq+BZ2CFta4u/SW4wQve2MEpvn 3IMa1MEocX9lIytElbHE/Qf3mCGu4JPo/f2ECWIBr0RHmxBEiYfE/I/32CBsR4npfSuYIAHX zShx4/4mtgmMCrOQwm4WIuxmIYXdAkbmVYxiqQXFuempxaYFxnrFibnFpXnpesn5uZsYwQlO S28H478F3ocYBTgYlXh4b1w6FyHEmlhWXJl7iFGCg1lJhFeI53yEEG9KYmVValF+fFFpTmrx IUZTYNhPZJYSTc4HJt+8knhDEzNDEyNLIDQ3NFcS540ymBghJJCeWJKanZpakFoE08fEwSnV wCj5+tHy9B1RARlL9a131MnNmp6/6P35uM8sX/o5f7+u6PjL/XnbQzm/B2ujJX6sYYiOkFeS yfGd4RB33qA8VWHdZpvaV+IyoqGhD+vexScY7lpp8Ue/3M1Ck6N+s22P/OYm4ddHG+Wcr/DU uHLV//TqU/3nLBLwcu3BJYGXWPJvtiu2n9C+o8RSnJFoqMVcVJwIAJbdFnSGAwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrIIsWRmVeSWpSXmKPExsVy+t9jQd1ujfMRBjtu81i8OT6dyeLyrjls FrcbV7A5MHu833eVzaNvyypGj8+b5AKYo9xsMlITU1KLFFLzkvNTMvPSbZVCQ9x0LZQU8hJz U22VInR9Q4KUFMoSc0qBPCMDNODgHOAerKRvl+CW8W/jY7aCbtuKyX82MTYwNht2MXJySAiY SEyZ8o4NwhaTuHBvPZDNxSEkMItR4t2TjUwgCV4BQYkfk++xdDFycDALyEscuZQNEmYWUJeY NG8RM0T9A0aJw//3Q9VrScxfd4MZxGYRUJWYfO0QO4jNBhTf/+IG2DJ+AUWJqz8eM4LMFBWI kOg+UQkSFhEIkPh5qp8dYr6CxK97m1hBbGGBVInexTugbutllJh2ajbYfE4BC4mTH66xTGAU nIXk1FkIp85CcuoCRuZVjBKpBckFxUnpuUZ5qeV6xYm5xaV56XrJ+bmbGMHR80x6B+PhXe6H GAU4GJV4eG9cOhchxJpYVlyZe4hRgoNZSYRXiOd8hBBvSmJlVWpRfnxRaU5q8SFGU6BfJzJL iSbnAyM7ryTe0MTcxNzYwMLc0tLESEmct3H2s3AhgfTEktTs1NSC1CKYPiYOTqkGxsbFs+oz l9s6S9ls2aSkpTLXdG9oyc8Z/Q8Kj4hIL3rBuPXAl479S6Wu7WNi+Pidg+WooO2XzCsvZfpd PIudHcQeMl54YKT+yXTJx1M8R4zk18zM2HIs0GteRXfbdol/y6YsOL3z7efuxe7HDK10M4Pf b2fkqlmWdDno4NOTj0qehfO8Z9g4778SS3FGoqEWc1FxIgAHSP2+tAIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170320005539epcas5p3b83de511855dcdebb72bde2d591afe8c 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: 20170319182339epcas4p16545551b1d3a114b9426edc54c610ac1 X-RootMTR: 20170319182339epcas4p16545551b1d3a114b9426edc54c610ac1 References: <20170319182334.6456-1-hdegoede@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 2017년 03월 20일 03:23, Hans de Goede wrote: > In some cases a driver may want to monitor multiple cables on a single > extcon. For example a charger driver will typically want to monitor all > of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure > the max. current it can sink while charging. > > Due to the signature of the notifier_call function + how extcon passes > state and the extcon_dev as parameters this requires using one > notifier_block + one notifier_call function per cable, otherwise the > notifier_call function cannot get to its driver's data (using container_of > requires it to know which notifier block its first parameter is). > > For a driver wanting to monitor the above 3 cables that would result > in something like this: > > static const unsigned int vbus_cable_ids[] = { > EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP }; > > struct driver_data { > struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)]; > } > > /* > * We need 3 copies of this, because there is no way to find out for which > * cable id we are being called from the passed in arguments; and we must > * have a separate nb for each extcon_register_notifier call. > */ > static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p) > { > struct driver_data *data = > container_of(nb, struct driver_data, vbus_nb[0]); > ... > } > > static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p) > { > struct driver_data *data = > container_of(nb, struct driver_data, vbus_nb[1]); > ... > } > > static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p) > { > struct driver_data *data = > container_of(nb, struct driver_data, vbus_nb[2]); > ... > } > > int probe(...) > { > /* Register for vbus notification */ > data->vbus_nb[0].notifier_call = vbus_cable0_evt; > data->vbus_nb[1].notifier_call = vbus_cable1_evt; > data->vbus_nb[2].notifier_call = vbus_cable2_evt; > for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) { > ret = devm_extcon_register_notifier(dev, data->vbus_extcon, > vbus_cable_ids[i], &data->vbus_nb[i]); > if (ret) > ... > } > } You can get the notification for multiple external connector by using the only one notifier_block as following: static const unsigned int vbus_cable_ids[] = { EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP }; struct driver_data { struct notifier_block vbus_nb; } static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p) { struct driver_data *data = container_of(nb, struct driver_data, vbus_nb); int sdp_state, cdp_state, dcp_state; sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP); cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP); dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP); ... } int probe(...) { /* Register for vbus notification */ data->vbus_nb.notifier_call = vbus_cable_evt; for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) { ret = devm_extcon_register_notifier(dev, data->vbus_extcon, vbus_cable_ids[i], &data->vbus_nb); if (ret) ... } } I don't prefer this patch because the extcon_register_notifier() function must support the notification for only one external connector. If you want to know the notifications for multiple external connectors, you can make the additional helper function without modifying the inside of the extcon_register_notifier(). I added the example already. > > And then in the event handling the driver often checks the state of > all cables explicitly using extcon_get_state, rather then using the > event argument to the notifier_call. > > This commit makes extcon_[un]register_notifier accept -1 as cable-id, > which will cause the notifier to get called for changes on any cable > on the extcon_dev. Compared to the above example code this allows much > simpler code in drivers which want to monitor multiple cable types. > > Signed-off-by: Hans de Goede > --- > Changes in v2: > -Fix false-positive "warning: 'idx' may be used uninitialized" warning > seen with some compiler versions > --- > drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++--------- > drivers/extcon/extcon.h | 1 + > 2 files changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c > index 09ac5e7..2a042ee 100644 > --- a/drivers/extcon/extcon.c > +++ b/drivers/extcon/extcon.c > @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id) > > state = !!(edev->state & BIT(index)); > raw_notifier_call_chain(&edev->nh[index], state, edev); > + raw_notifier_call_chain(&edev->nh_all, 0, edev); > > /* This could be in interrupt handler */ > prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); > @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev); > * any attach status changes from the extcon. > * @edev: the extcon device that has the external connecotr. > * @id: the unique id of each external connector in extcon enumeration. > + * or -1 to get notififications for all cables on edev, in this > + * case no state info will get passed to the notifier_call. > * @nb: a notifier block to be registered. > * > * Note that the second parameter given to the callback of nb (val) is > @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id, > if (!edev || !nb) > return -EINVAL; > > - idx = find_cable_index_by_id(edev, id); > - if (idx < 0) > - return idx; > + if ((int)id != -1) { > + idx = find_cable_index_by_id(edev, id); > + if (idx < 0) > + return idx; > + } > > spin_lock_irqsave(&edev->lock, flags); > - ret = raw_notifier_chain_register(&edev->nh[idx], nb); > + if ((int)id != -1) > + ret = raw_notifier_chain_register(&edev->nh[idx], nb); > + else > + ret = raw_notifier_chain_register(&edev->nh_all, nb); > + > spin_unlock_irqrestore(&edev->lock, flags); > > return ret; > @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id, > struct notifier_block *nb) > { > unsigned long flags; > - int ret, idx; > + int ret, idx = 0; > > if (!edev || !nb) > return -EINVAL; > > - idx = find_cable_index_by_id(edev, id); > - if (idx < 0) > - return idx; > + if ((int)id != -1) { > + idx = find_cable_index_by_id(edev, id); > + if (idx < 0) > + return idx; > + } > > spin_lock_irqsave(&edev->lock, flags); > - ret = raw_notifier_chain_unregister(&edev->nh[idx], nb); > + if ((int)id != -1) > + ret = raw_notifier_chain_unregister(&edev->nh[idx], nb); > + else > + ret = raw_notifier_chain_unregister(&edev->nh_all, nb); > + > spin_unlock_irqrestore(&edev->lock, flags); > > return ret; > @@ -1212,6 +1227,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 993ddcc..2e6c09d 100644 > --- a/drivers/extcon/extcon.h > +++ b/drivers/extcon/extcon.h > @@ -44,6 +44,7 @@ struct extcon_dev { > /* Internal data. Please do not set. */ > struct device dev; > struct raw_notifier_head *nh; > + struct raw_notifier_head nh_all; > struct list_head entry; > int max_supported; > spinlock_t lock; /* could be called by irq handler */ > -- Best Regards, Chanwoo Choi Samsung Electronics