Message-ID: <ad1c1942-3a82-d728-8a6c-d89eb28e31e5@redhat.com>
Date:   Mon, 20 Mar 2017 10:48:21 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] extcon: Allow registering a single notifier for all
 cables on an extcon_dev

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!

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@...hat.com>
>> ---
>> 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 */
>>
>
>

Powered by blists - more mailing lists