Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev
From: Hans de Goede
Date: Sat Mar 25 2017 - 10:04:00 EST
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 ?
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 */