Date:	Wed, 27 Jul 2016 15:30:54 +0800
From:	Chris Zhong <zyw@...k-chips.com>
To:	Chanwoo Choi <cw00.choi@...sung.com>, linux-kernel@...r.kernel.org
Cc:	myungjoo.ham@...sung.com, groeck@...omium.org, cwchoi00@...il.com
Subject: Re: [PATCH 5/6] extcon: Add the synchronization extcon APIs to
 support the notification

Hi Chanwoo

On 07/26/2016 08:09 PM, Chanwoo Choi wrote:
> This patch adds the synchronization extcon APIs to support the notifications
> for both state and property. When extcon_*_sync() functions is called,
> the extcon informs the information from extcon provider to extcon client.
>
> The extcon driver may need to change the both state and multiple properties
> at the same time. After setting the data of a external connector,
> the extcon send the notification to client driver with the extcon_*_sync().
>
> The list of new extcon APIs as following:
> - extcon_sync() : Send the notification for each external connector to
> 		synchronize the information between extcon provider driver
> 		and extcon client driver.
> - extcon_set_state_sync() : Set the state of external connector with noti.
> - extcon_set_property_sync() : Set the property of external connector with noti.
>
> For example,
> case 1, change the state of external connector and synchronized the data.
> 	extcon_set_state_sync(edev, EXTCON_USB, 1);
>
> case 2, change both the state and property of external connector
> 	and synchronized the data.
> 	extcon_set_state(edev, EXTCON_USB, 1);
> 	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
> 	extcon_sync(edev, EXTCON_USB);
>
> case 3, change the property of external connector and synchronized the data.
> 	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
> 	extcon_set_property(edev, EXTCON_USB, EXTCON_PROP_USB_ID, 1);
> 	extcon_sync(edev, EXTCON_USB);
>
> case 4, change the property of external connector and synchronized the data.
> 	extcon_set_property_sync(edev, EXTCON_USB, EXTCON_PROP_USB_VBUS, 0);
>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> ---
>   drivers/extcon/extcon.c | 199 ++++++++++++++++++++++++++++++------------------
>   include/linux/extcon.h  |  30 +++++++-
>   2 files changed, 152 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 73c6981bf559..8572523bfd40 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -280,14 +280,11 @@ static bool is_extcon_attached(struct extcon_dev *edev, unsigned int id,
>   	return ((!!(edev->state & (1 << index))) == 1) ? true : false;
>   }
>   
> -static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
> +static bool is_extcon_changed(struct extcon_dev *edev, int index,
> +				bool new_state)
>   {
> -	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
> -		*attached = ((new >> idx) & 0x1) ? true : false;
> -		return true;
> -	}
> -
> -	return false;
> +	int state = !!(edev->state & (1 << index));
> +	return (state != new_state) ? true : false;
>   }
>   
>   static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
> @@ -377,21 +374,13 @@ static ssize_t cable_state_show(struct device *dev,
>   }
>   
>   /**
> - * extcon_update_state() - Update the cable attach states of the extcon device
> - *			   only for the masked bits.
> - * @edev:	the extcon device
> - * @mask:	the bit mask to designate updated bits.
> - * @state:	new cable attach status for @edev
> - *
> - * Changing the state sends uevent with environment variable containing
> - * the name of extcon device (envp[0]) and the state output (envp[1]).
> - * Tizen uses this format for extcon device to get events from ports.
> - * Android uses this format as well.
> + * extcon_sync()	- Synchronize the states for both the attached/detached
> + * @edev:		the extcon device that has the cable.
>    *
> - * Note that the notifier provides which bits are changed in the state
> - * variable with the val parameter (second) to the callback.
> + * This function send a notification to synchronize the all states of a
> + * specific external connector
>    */
> -static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
> +int extcon_sync(struct extcon_dev *edev, unsigned int id)
>   {
>   	char name_buf[120];
>   	char state_buf[120];
> @@ -400,73 +389,58 @@ static int extcon_update_state(struct extcon_dev *edev, u32 mask, u32 state)
>   	int env_offset = 0;
>   	int length;
>   	int index;
> +	int state;
>   	unsigned long flags;
> -	bool attached;
>   
>   	if (!edev)
>   		return -EINVAL;
>   
> -	spin_lock_irqsave(&edev->lock, flags);
> +	index = find_cable_index_by_id(edev, id);
> +	if (index < 0)
> +		return index;
>   
> -	if (edev->state != ((edev->state & ~mask) | (state & mask))) {
> -		u32 old_state;
> +	state = !!(edev->state & (1 << index));
> +	raw_notifier_call_chain(&edev->nh[index], state, edev);
>   
> -		if (check_mutually_exclusive(edev, (edev->state & ~mask) |
> -						   (state & mask))) {
> -			spin_unlock_irqrestore(&edev->lock, flags);
> -			return -EPERM;
> -		}
> +	spin_lock_irqsave(&edev->lock, flags);
>   
> -		old_state = edev->state;
> -		edev->state &= ~mask;
> -		edev->state |= state & mask;
> +	/* This could be in interrupt handler */
> +	prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
> +	if (!prop_buf) {
> +		/* Unlock early before uevent */
> +		spin_unlock_irqrestore(&edev->lock, flags);
>   
> -		for (index = 0; index < edev->max_supported; index++) {
> -			if (is_extcon_changed(old_state, edev->state, index,
> -					      &attached))
> -				raw_notifier_call_chain(&edev->nh[index],
> -							attached, edev);
> -		}
> +		dev_err(&edev->dev, "out of memory in extcon_set_state\n");
> +		kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
>   
> -		/* This could be in interrupt handler */
> -		prop_buf = (char *)get_zeroed_page(GFP_ATOMIC);
> -		if (prop_buf) {
> -			length = name_show(&edev->dev, NULL, prop_buf);
> -			if (length > 0) {
> -				if (prop_buf[length - 1] == '\n')
> -					prop_buf[length - 1] = 0;
> -				snprintf(name_buf, sizeof(name_buf),
> -					"NAME=%s", prop_buf);
> -				envp[env_offset++] = name_buf;
> -			}
> -			length = state_show(&edev->dev, NULL, prop_buf);
> -			if (length > 0) {
> -				if (prop_buf[length - 1] == '\n')
> -					prop_buf[length - 1] = 0;
> -				snprintf(state_buf, sizeof(state_buf),
> -					"STATE=%s", prop_buf);
> -				envp[env_offset++] = state_buf;
> -			}
> -			envp[env_offset] = NULL;
> -			/* Unlock early before uevent */
> -			spin_unlock_irqrestore(&edev->lock, flags);
> +		return 0;
> +	}
>   
> -			kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
> -			free_page((unsigned long)prop_buf);
> -		} else {
> -			/* Unlock early before uevent */
> -			spin_unlock_irqrestore(&edev->lock, flags);
> +	length = name_show(&edev->dev, NULL, prop_buf);
> +	if (length > 0) {
> +		if (prop_buf[length - 1] == '\n')
> +			prop_buf[length - 1] = 0;
> +		snprintf(name_buf, sizeof(name_buf), "NAME=%s", prop_buf);
> +		envp[env_offset++] = name_buf;
> +	}
>   
> -			dev_err(&edev->dev, "out of memory in extcon_set_state\n");
> -			kobject_uevent(&edev->dev.kobj, KOBJ_CHANGE);
> -		}
> -	} else {
> -		/* No changes */
> -		spin_unlock_irqrestore(&edev->lock, flags);
> +	length = state_show(&edev->dev, NULL, prop_buf);
> +	if (length > 0) {
> +		if (prop_buf[length - 1] == '\n')
> +			prop_buf[length - 1] = 0;
> +		snprintf(state_buf, sizeof(state_buf), "STATE=%s", prop_buf);
> +		envp[env_offset++] = state_buf;
>   	}
> +	envp[env_offset] = NULL;
> +
> +	/* Unlock early before uevent */
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +	kobject_uevent_env(&edev->dev.kobj, KOBJ_CHANGE, envp);
> +	free_page((unsigned long)prop_buf);
>   
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(extcon_sync);
>   
>   /**
>    * extcon_get_state() - Get the state of a external connector.
> @@ -493,17 +467,22 @@ EXPORT_SYMBOL_GPL(extcon_get_state);
>   
>   /**
>    * extcon_set_state() - Set the state of a external connector.
> + *			without a notification.
>    * @edev:		the extcon device that has the cable.
>    * @id:			the unique id of each external connector
>    *			in extcon enumeration.
>    * @state:		the new cable status. The default semantics is
>    *			true: attached / false: detached.
> + *
> + * This function only set the state of a external connector without
> + * a notification. To synchronize the data of a external connector,
> + * use extcon_set_state_sync() and extcon_sync().
>    */
>   int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   				bool cable_state)
>   {
> -	u32 state;
> -	int index;
> +	unsigned long flags;
> +	int index, ret = 0;
>   
>   	if (!edev)
>   		return -EINVAL;
> @@ -515,6 +494,18 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   	if (edev->max_supported && edev->max_supported <= index)
>   		return -EINVAL;
>   
> +	spin_lock_irqsave(&edev->lock, flags);
> +
> +	/* Check whether the external connector's state is changed. */
> +	if (!is_extcon_changed(edev, index, cable_state))
> +		goto out;
> +
> +	if (check_mutually_exclusive(edev,
> +		(edev->state & ~(1 << index)) | (cable_state & (1 << index)))) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
>   	/*
>   	 * Initialize the value of extcon property before setting
>   	 * the detached state for an external connector.
> @@ -522,13 +513,44 @@ int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   	if (!cable_state)
>   		init_property(edev, id, index);
>   
> -	/* Set the state for external connector as the detached state. */
> -	state = cable_state ? (1 << index) : 0;
> -	return extcon_update_state(edev, 1 << index, state);
> +	/* Update the state for a external connector. */
> +	if (cable_state)
> +		edev->state |= (1 << index);
> +	else
> +		edev->state &= ~(1 << index);
> +out:
> +	spin_unlock_irqrestore(&edev->lock, flags);
> +
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(extcon_set_state);
>   
>   /**
> + * extcon_set_state_sync() - Set the state of a external connector
> + *			with a notification.
> + * @edev:		the extcon device that has the cable.
> + * @id:			the unique id of each external connector
> + *			in extcon enumeration.
> + * @state:		the new cable status. The default semantics is
> + *			true: attached / false: detached.
> + *
> + * This function set the state of external connector and synchronize the data
> + * by usning a notification.
> + */
> +int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +				bool cable_state)
> +{
> +	int ret;
> +
> +	ret = extcon_set_state(edev, id, cable_state);
> +	if (ret < 0)
> +		return ret;
> +
> +	return extcon_sync(edev, id);
So, Regardless of whether the state change, the notifier chain will be 
called,
If this function can decide whether to call a notice, according to the 
state change. I think it would be better.
The old extcon_set_cable_state_ was working like this.

> +}
> +EXPORT_SYMBOL_GPL(extcon_set_state_sync);
> +
> +/**
>    * extcon_get_property() - Get the property value of a specific cable.
>    * @edev:		the extcon device that has the cable.
>    * @id:			the unique id of each external connector
> @@ -673,6 +695,31 @@ int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>   EXPORT_SYMBOL_GPL(extcon_set_property);
>   
>   /**
> + * extcon_set_property_sync() - Set the property value of a specific cable
> +			with a notification.
> + * @prop_val:		the pointer including the new value of property.
> + *
> + * When setting the property value of external connector, the external connector
> + * should be attached. The each property should be included in the list of
> + * supported properties according to the type of external connectors.
> + *
> + * Returns 0 if success or error number if fail
> + */
> +int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value prop_val)
> +{
> +	int ret;
> +
> +	ret = extcon_set_property(edev, id, prop, prop_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return extcon_sync(edev, id);
> +}
> +EXPORT_SYMBOL_GPL(extcon_set_property_sync);
> +
> +/**
>    * extcon_get_property_capability() - Get the capability of property
>    *			of an external connector.
>    * @edev:		the extcon device that has the cable.
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 26f4481b3788..00ab1180346c 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -227,6 +227,13 @@ extern void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
>   extern int extcon_get_state(struct extcon_dev *edev, unsigned int id);
>   extern int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   				   bool cable_state);
> +extern int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +				bool cable_state);
> +
> +/*
> + * Synchronize the state and property data for a specific external connector.
> + */
> +extern int extcon_sync(struct extcon_dev *edev, unsigned int id);
>   
>   /*
>    * get/set_property access the property value of each external connector.
> @@ -238,6 +245,9 @@ extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>   extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>   				unsigned int prop,
>   				union extcon_property_value prop_val);
> +extern int extcon_set_property_sync(struct extcon_dev *edev, unsigned int id,
> +				unsigned int prop,
> +				union extcon_property_value prop_val);
>   
>   /*
>    * get/set_property_capability set the capability of the property for each
> @@ -322,6 +332,17 @@ static inline int extcon_set_state(struct extcon_dev *edev, unsigned int id,
>   	return 0;
>   }
>   
> +static inline int extcon_set_state_sync(struct extcon_dev *edev, unsigned int id,
> +				bool cable_state)
> +{
> +	return 0;
> +}
> +
> +static inline int extcon_sync(struct extcon_dev *edev, unsigned int id)
> +{
> +	return 0;
> +}
> +
>   static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>   					unsigned int prop,
>   					union extcon_property_value *prop_val)
> @@ -335,6 +356,13 @@ static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>   	return 0;
>   }
>   
> +static inline int extcon_set_property_sync(struct extcon_dev *edev,
> +					unsigned int id, unsigned int prop,
> +					union extcon_property_value prop_val)
> +{
> +	return 0;
> +}
> +
>   static inline int extcon_get_property_capability(struct extcon_dev *edev,
>   					unsigned int id, unsigned int prop)
>   {
> @@ -416,6 +444,6 @@ static inline int extcon_get_cable_state_(struct extcon_dev *edev, unsigned int
>   static inline int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>   				   bool cable_state)
>   {
> -	return extcon_set_state(edev, id, cable_state);
> +	return extcon_set_state_sync(edev, id, cable_state);
>   }
>   #endif /* __LINUX_EXTCON_H__ */

Powered by blists - more mailing lists