Date:	Tue, 02 Aug 2016 17:08:33 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	Roger Quadros <rogerq@...com>, linux-kernel@...r.kernel.org,
	Felipe Balbi <balbi@...nel.org>
Cc:	myungjoo.ham@...sung.com, zyw@...k-chips.com, groeck@...omium.org,
	chanwoo@...nel.org, Krzysztof Kozlowski <k.kozlowski@...sung.com>
Subject: Re: [PATCH v3 2/6] extcon: Add the support for extcon property
 according to extcon type

Hi,

On 2016년 08월 02일 16:43, Roger Quadros wrote:
> +Felipe
> 
> Hi,
> 
> On 02/08/16 04:58, Chanwoo Choi wrote:
>> This patch support the extcon property for the external connector
>> because each external connector might have the property according to
>> the H/W design and the specific characteristics.
>>
>> - EXTCON_PROP_USB_[property name]
>> - EXTCON_PROP_CHG_[property name]
>> - EXTCON_PROP_JACK_[property name]
>> - EXTCON_PROP_DISP_[property name]
>>
>> Add the new extcon APIs to get/set the property value as following:
>> - int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> 			unsigned int prop,
>> 			union extcon_property_value *prop_val)
>> - int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> 			unsigned int prop,
>> 			union extcon_property_value prop_val)
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
>> Tested-by: Chris Zhong <zyw@...k-chips.com>
>> Tested-by: Guenter Roeck <groeck@...omium.org>
>> Reviewed-by: Guenter Roeck <groeck@...omium.org>
>> ---
>>  drivers/extcon/extcon.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/extcon.h  |  86 +++++++++++++++++++++
>>  2 files changed, 286 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 129afc87313e..bb6e99fe84c8 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -196,6 +196,11 @@ struct extcon_cable {
>>  	struct device_attribute attr_state;
>>  
>>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
>> +
>> +	union extcon_property_value usb_propval[EXTCON_PROP_USB_CNT];
>> +	union extcon_property_value chg_propval[EXTCON_PROP_CHG_CNT];
>> +	union extcon_property_value jack_propval[EXTCON_PROP_JACK_CNT];
>> +	union extcon_property_value disp_propval[EXTCON_PROP_DISP_CNT];
>>  };
>>  
>>  static struct class *extcon_class;
>> @@ -248,6 +253,27 @@ static int find_cable_index_by_id(struct extcon_dev *edev, const unsigned int id
>>  	return -EINVAL;
>>  }
>>  
>> +static int get_extcon_type(unsigned int prop)
>> +{
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		return EXTCON_TYPE_USB;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		return EXTCON_TYPE_CHG;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		return EXTCON_TYPE_JACK;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		return EXTCON_TYPE_DISP;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static bool is_extcon_attached(struct extcon_dev *edev, unsigned int index)
>> +{
>> +	return !!(edev->state & BIT(index));
>> +}
>> +
>>  static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>  {
>>  	if (((prev >> idx) & 0x1) != ((new >> idx) & 0x1)) {
>> @@ -258,6 +284,34 @@ static bool is_extcon_changed(u32 prev, u32 new, int idx, bool *attached)
>>  	return false;
>>  }
>>  
>> +static bool is_extcon_property_supported(unsigned int id, unsigned int prop)
>> +{
>> +	int type;
>> +
>> +	/* Check whether the property is supported or not. */
>> +	type = get_extcon_type(prop);
>> +	if (type < 0)
>> +		return false;
>> +
>> +	/* Check whether a specific extcon id supports the property or not. */
>> +	return !!(extcon_info[id].type & type);
>> +}
>> +
>> +static void init_property(struct extcon_dev *edev, unsigned int id, int index)
>> +{
>> +	unsigned int type = extcon_info[id].type;
>> +	struct extcon_cable *cable = &edev->cables[index];
>> +
>> +	if (EXTCON_TYPE_USB & type)
>> +		memset(cable->usb_propval, 0, sizeof(cable->usb_propval));
>> +	if (EXTCON_TYPE_CHG & type)
>> +		memset(cable->chg_propval, 0, sizeof(cable->chg_propval));
>> +	if (EXTCON_TYPE_JACK & type)
>> +		memset(cable->jack_propval, 0, sizeof(cable->jack_propval));
>> +	if (EXTCON_TYPE_DISP & type)
>> +		memset(cable->disp_propval, 0, sizeof(cable->disp_propval));
>> +}
>> +
>>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
>>  			  char *buf)
>>  {
>> @@ -421,7 +475,7 @@ int extcon_get_cable_state_(struct extcon_dev *edev, const unsigned int id)
>>  	if (edev->max_supported && edev->max_supported <= index)
>>  		return -EINVAL;
>>  
>> -	return !!(edev->state & (1 << index));
>> +	return is_extcon_attached(edev, index);
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_get_cable_state_);
>>  
>> @@ -449,12 +503,157 @@ int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>  	if (edev->max_supported && edev->max_supported <= index)
>>  		return -EINVAL;
>>  
>> +	/*
>> +	 * Initialize the value of extcon property before setting
>> +	 * the detached state for an external connector.
>> +	 */
>> +	if (!cable_state)
>> +		init_property(edev, id, index);
>> +
> 
> I'm a bit concerned about this for USB case. See below why.
> 
>>  	state = cable_state ? (1 << index) : 0;
>>  	return extcon_update_state(edev, 1 << index, state);
>>  }
>>  EXPORT_SYMBOL_GPL(extcon_set_cable_state_);
>>  
>>  /**
>> + * 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
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + * @prop_val:		the pointer which store the value of property.
>> + *
>> + * When getting the property value of external connector, the external connector
>> + * should be attached. If detached state, function just return 0 without
>> + * property value. Also, 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_get_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value *prop_val)
>> +{
>> +	struct extcon_cable *cable;
>> +	unsigned long flags;
>> +	int index, ret = 0;
>> +
>> +	*prop_val = (union extcon_property_value)(0);
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	spin_lock_irqsave(&edev->lock, flags);
>> +
>> +	/*
>> +	 * Check whether the external connector is attached.
>> +	 * If external connector is detached, the user can not
>> +	 * get the property value.
>> +	 */
> 
> How will this work for USB case? We need to know VBUS and ID states
> even if the USB cable is detached.

When USB is detached, extcon_get_property return the default value without any operation.
The default value of supported property are 0 (zero). If new property need the differnt default
value, I'll support it.

> 
> Moreover there is no specific mechanism to detect if the USB cable is attached
> or not in the extcon-usb-gpio.c case.
> One solution could be to set EXTCON_USB as always attached on probe in extcon-usb-gpio.c.
> 
> Is this acceptable?

No. the extcon have to detect the correct state. When USB cable is detached, 
the extcon cannot set the attached state for EXTCON_USB.

> 
>> +	if (!is_extcon_attached(edev, index)) {
>> +		spin_unlock_irqrestore(&edev->lock, flags);
>> +		return 0;
>> +	}
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	/* Get the property value according to extcon type */
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		*prop_val = cable->usb_propval[prop - EXTCON_PROP_USB_MIN];
>> +		break;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		*prop_val = cable->chg_propval[prop - EXTCON_PROP_CHG_MIN];
>> +		break;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		*prop_val = cable->jack_propval[prop - EXTCON_PROP_JACK_MIN];
>> +		break;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		*prop_val = cable->disp_propval[prop - EXTCON_PROP_DISP_MIN];
>> +		break;
> 
> All these should be simplified to
> 		*prop_val = cable->foo_propval[prop];
> see below why.

You mean that the macro?
I already used the macro. But, as comment, I don't use it.

> 
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_get_property);
>> +
>> +/**
>> + * extcon_set_property() - Set the property value of a specific cable.
>> + * @edev:		the extcon device that has the cable.
>> + * @id:			the unique id of each external connector
>> + *			in extcon enumeration.
>> + * @prop:		the property id among enum extcon_property.
>> + * @prop_val:		the pointer including the new value of property.
>> + *
>> + * 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(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value prop_val)
>> +{
>> +	struct extcon_cable *cable;
>> +	unsigned long flags;
>> +	int index, ret = 0;
>> +
>> +	if (!edev)
>> +		return -EINVAL;
>> +
>> +	/* Check whether the property is supported or not */
>> +	if (!is_extcon_property_supported(id, prop))
>> +		return -EINVAL;
>> +
>> +	/* Find the cable index of external connector by using id */
>> +	index = find_cable_index_by_id(edev, id);
>> +	if (index < 0)
>> +		return index;
>> +
>> +	spin_lock_irqsave(&edev->lock, flags);
>> +
>> +	cable = &edev->cables[index];
>> +
>> +	/* Set the property value according to extcon type */
>> +	switch (prop) {
>> +	case EXTCON_PROP_USB_MIN ... EXTCON_PROP_USB_MAX:
>> +		cable->usb_propval[prop - EXTCON_PROP_USB_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_CHG_MIN ... EXTCON_PROP_CHG_MAX:
>> +		cable->chg_propval[prop - EXTCON_PROP_CHG_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_JACK_MIN ... EXTCON_PROP_JACK_MAX:
>> +		cable->jack_propval[prop - EXTCON_PROP_JACK_MIN] = prop_val;
>> +		break;
>> +	case EXTCON_PROP_DISP_MIN ... EXTCON_PROP_DISP_MAX:
>> +		cable->disp_propval[prop - EXTCON_PROP_DISP_MIN] = prop_val;
>> +		break;
> 
> All these should be simplified to
> 		*prop_val = cable->foo_propval[prop];
> see below why.

ditto.

> 
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	spin_unlock_irqrestore(&edev->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_set_property);
>> +
>> +/**
>>   * extcon_get_extcon_dev() - Get the extcon device instance from the name
>>   * @extcon_name:	The extcon name provided with extcon_dev_register()
>>   */
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index 46d802892c82..fe583fca7732 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -77,6 +77,68 @@
>>  
>>  #define EXTCON_NUM		63
>>  
>> +/*
>> + * Define the property of supported external connectors.
>> + *
>> + * When adding the new extcon property, they *must* have
>> + * the type/value/default information. Also, you *have to*
>> + * modify the EXTCON_PROP_[type]_START/END definitions
>> + * which mean the range of the supported properties
>> + * for each extcon type.
>> + *
>> + * The naming style of property
>> + * : EXTCON_PROP_[type]_[property name]
>> + *
>> + * EXTCON_PROP_USB_[property name]	: USB property
>> + * EXTCON_PROP_CHG_[property name]	: Charger property
>> + * EXTCON_PROP_JACK_[property name]	: Jack property
>> + * EXTCON_PROP_DISP_[property name]	: Display property
>> + */
>> +
>> +/*
>> + * Properties of EXTCON_TYPE_USB.
>> + *
>> + * - EXTCON_PROP_USB_ID
>> + * @type:	integer (intval)
>> + * @value:	0 (low) or 1 (high)
>> + * @default:	0 (low)
>> + * - EXTCON_PROP_USB_VBUS
>> + * @type:	integer (intval)
>> + * @value:	0 (low) or 1 (high)
>> + * @default:	0 (low)
>> + */
>> +#define EXTCON_PROP_USB_ID		0
>> +#define EXTCON_PROP_USB_VBUS		1
>> +
>> +#define EXTCON_PROP_USB_MIN		0
>> +#define EXTCON_PROP_USB_MAX		1
>> +#define EXTCON_PROP_USB_CNT	(EXTCON_PROP_USB_MAX - EXTCON_PROP_USB_MIN + 1)
> 
> As the properties themselves are separated by the cable type
> they can overlap.
> 
> So MIN is not needed as it is always 0

Yes. Just adding the EXTCON_PROP_USB_MIN because remaining the
consistent expression for other types (EXTCON_PROP_[type]_MIN).

> 
> #define EXTCON_PROP_USB_ID		0
> #define EXTCON_PROP_USB_VBUS		1
> #define EXTCON_PROP_USB_MAX		1
> 
> #define EXTCON_PROP_CHG_MAX		1
> #define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX + 1)
> 
> and so on.
> 
>> +
>> +/* Properties of EXTCON_TYPE_CHG. */
>> +#define EXTCON_PROP_CHG_MIN		50
>> +#define EXTCON_PROP_CHG_MAX		50
>> +#define EXTCON_PROP_CHG_CNT	(EXTCON_PROP_CHG_MAX - EXTCON_PROP_CHG_MIN + 1)
>> +
>> +/* Properties of EXTCON_TYPE_JACK. */
>> +#define EXTCON_PROP_JACK_MIN		100
>> +#define EXTCON_PROP_JACK_MAX		100
>> +#define EXTCON_PROP_JACK_CNT (EXTCON_PROP_JACK_MAX - EXTCON_PROP_JACK_MIN + 1)
>> +
>> +/* Properties of EXTCON_TYPE_DISP. */
>> +#define EXTCON_PROP_DISP_MIN		150
>> +#define EXTCON_PROP_DISP_MAX		150
>> +#define EXTCON_PROP_DISP_CNT (EXTCON_PROP_DISP_MAX - EXTCON_PROP_DISP_MIN + 1)
>> +
>> +/*
>> + * Define the type of property's value.
>> + *
>> + * Define the property's value as union type. Because each property
>> + * would need the different data type to store it.
>> + */
>> +union extcon_property_value {
>> +	int intval;	/* type : integer (intval) */
>> +};
>> +
> 
> Why is this a union? There is only one member.

No. The extcon support the various type of external connector.
This patch don't support the only USB type.

> 
>>  struct extcon_cable;
>>  
>>  /**
>> @@ -167,6 +229,17 @@ extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
>>  				   bool cable_state);
>>  
>>  /*
>> + * get/set_property access the property value of each external connector.
>> + * They are used to access the property of each cable based on the property id.
>> + */
>> +extern int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value *prop_val);
>> +extern int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> +				unsigned int prop,
>> +				union extcon_property_value prop_val);
>> +
>> +/*
>>   * Following APIs are to monitor every action of a notifier.
>>   * Registrar gets notified for every external port of a connection device.
>>   * Probably this could be used to debug an action of notifier; however,
>> @@ -239,6 +312,19 @@ static inline int extcon_set_cable_state_(struct extcon_dev *edev,
>>  	return 0;
>>  }
>>  
>> +static inline int extcon_get_property(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop,
>> +					union extcon_property_value *prop_val)
>> +{
>> +	return 0;
>> +}
>> +static inline int extcon_set_property(struct extcon_dev *edev, unsigned int id,
>> +					unsigned int prop,
>> +					union extcon_property_value prop_val)
>> +{
>> +	return 0;
>> +}
>> +
>>  static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>  {
>>  	return NULL;
>>
> 

Thanks,
Chanwoo Choi

Powered by blists - more mailing lists