Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev

[Chanwoo Choi]

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 <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 */
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics