Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev
From: Hans de Goede
Date: Wed Mar 29 2017 - 05:51:50 EST
HI,
On 29-03-17 11:50, Chanwoo Choi wrote:
Hi,
On 2017ë 03ì 29ì 16:38, Hans de Goede wrote:
Hi,
On 29-03-17 09:06, Chanwoo Choi wrote:
Hi,
On 2017ë 03ì 25ì 23:03, Hans de Goede wrote:
Hi,
On 20-03-17 10:48, Hans de Goede wrote:
Hi,
On 20-03-17 01:55, Chanwoo Choi wrote:
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)
...
}
}
No that will not work, you cannot add the same notifier_block to 3
different notifier_heads, the list_head can only be part of one list!
Also see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
Which actually fixes an issue I encountered exactly because of doing
the above.
So we really need something like my patch to make this simpler
and to remove the ugliness we currently have in axp288_charger.c
because of this problem (and are about to have in more drivers
I'm upstreaming). Also note that my patch is quite simple, the
amount of lines it adds is way less then the amount of lines
it will allow to remove in a single driver!
Ping ?
As explained this is a real problem and we really need a proper solution
for this. I'm about to upstream 2 more drivers which need to monitor
an antire extcon rather then a single cable on it, and I do not want
to have to add hacks like this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f
To both drivers. My patch is IMHO a nice and clean solution for this,
can you please merge this or provide an alternative solution ?
As you said, when using one notifier_block for multiple external connector,
it has a problem. But, it is not only for extcon framework. It depends
on the notifier_block code. If each external connector uses the
each notifier_block (it is a not hack), there is no any problem.
As I already said on my reply[1] as following,
---------------------------------------------
The extcon_register_notifier() function must support the notification
for only one external connector.
[1] https://www.spinics.net/lists/kernel/msg2468906.html
---------------------------------------------
I agree that the EXTCON needs to support the notification
for all supported external connector of extcon device
by using only one notifier_chain.
I don't prefer to use the '-1' as the unique id for getting the
notification for all external connectors. Each argument of function
must have the correct meaning.
I think that thanks for your report and suggestion.
I'm considering to add the new function as following:
Following function will support the all external connector for notification
and the function argument don't include the unique id for external connector.
- extcon_regiter_all_notifier(struct extcon_dev *edev, struct notifier_block *nb);
(The function name is not fixed)
Adding a new extcon_register_all_notifier function for this is fine with
me, shall I send a new patch for this ?
I'll develop the new following functions with your reported-by.
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);
Looks good to me, thank you for looking into this. When you've a patch ready
please Cc me and I'll test it.
Regards,
Hans
Regards,
Hans
Regards,
Hans
Regards,
Hans
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 <hdegoede@xxxxxxxxxx>
---
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 */