[PATCH] EXTCON: Get and set cable properties

[PATCH] EXTCON: Get and set cable properties

[Jenny TC]

Existing EXTCON implementation doesn't give a mechanim to read the cable
properties and extra states a cable needs to support. There are scenarios
where a cable can have more than two states(CONNECT/DISCONNECT/SUSPEND/RESUME etc)
and can have some properties associated with cables(mA)

This patch introduces interface to get and set cable properties
from EXTCON framework. The cable property can be set either by
the extcon cable provider or by other subsystems who know the cable
properties using eth API extcon_cable_set_data()

When the consumer gets a notification from the extcon, it can use the
extcon_cable_get_data() to get the cable properties irrespective of who
provides the cable data.

This gives a single interface for setting and getting the cable properties.

Signed-off-by: Jenny TC <jenny.tc@intel.com>
---
 drivers/extcon/extcon-class.c |   30 ++++++++++++++++++++++++++++++
 include/linux/extcon.h        |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index d398821..304f343 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -545,6 +545,36 @@ int extcon_unregister_notifier(struct extcon_dev *edev,
 }
 EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
 
+/**
+ * extcon_cable_set_data() - Set the data structure for a cable
+ * @edev:	the extcon device
+ * @cable_index:	the cable index of the correspondant
+ * @type:	type of the data structure
+ * @data:
+ */
+void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
+			   enum extcon_cable_name type,
+			   union extcon_cable_data data)
+{
+	edev->cables[cable_index].type = type;
+	edev->cables[cable_index].data = data;
+}
+
+/**
+ * extcon_cable_get_data() - Get the data structure for a cable
+ * @edev:	the extcon device
+ * @cable_index:	the cable index of the correspondant
+ * @type:	type of the data structure
+ * @data:	the corresponding data structure (e.g., regulator)
+ */
+void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
+			   enum extcon_cable_name *type,
+			   union extcon_cable_data *data)
+{
+	*type = edev->cables[cable_index].type;
+	*data = edev->cables[cable_index].data;
+}
+
 static struct device_attribute extcon_attrs[] = {
 	__ATTR(state, S_IRUGO | S_IWUSR, state_show, state_store),
 	__ATTR_RO(name),
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 2c26c14..4556cc5 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -135,6 +135,19 @@ struct extcon_dev {
 	struct device_attribute *d_attrs_muex;
 };
 
+/* FIXME: Is this the right place for this structure definition?
+ * Do we need to move it to power_supply.h?
+ */
+struct extcon_chrgr_cable_props {
+	unsigned long state;
+	int mA;
+};
+
+union extcon_cable_data {
+	struct extcon_chrgr_cable_props chrgr_cbl_props;
+	/* Please add accordingly*/
+};
+
 /**
  * struct extcon_cable	- An internal data for each cable of extcon device.
  * @edev	The extcon device
@@ -143,6 +156,8 @@ struct extcon_dev {
  * @attr_name	"name" sysfs entry
  * @attr_state	"state" sysfs entry
  * @attrs	Array pointing to attr_name and attr_state for attr_g
+ * @type:	The type of @data.
+ * @data:	The data structure representing the status and states of this cable.
  */
 struct extcon_cable {
 	struct extcon_dev *edev;
@@ -153,6 +168,11 @@ struct extcon_cable {
 	struct device_attribute attr_state;
 
 	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
+
+	union extcon_cable_data data;
+
+	/* extcon cable type */
+	enum extcon_cable_name type;
 };
 
 /**
@@ -183,6 +203,17 @@ extern void extcon_dev_unregister(struct extcon_dev *edev);
 extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
 
 /*
+ * Following APIs are for managing the status and states of each cable.
+ * For example, if a cable is represented as a regulator, then the cable
+ * may have struct regulator as its data.
+ */
+extern void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
+			enum extcon_cable_name type,
+			union extcon_cable_data data);
+extern void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
+			enum extcon_cable_name *type,
+			union extcon_cable_data *data);
+/*
  * get/set/update_state access the 32b encoded state value, which represents
  * states of all possible cables of the multistate port. For example, if one
  * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
@@ -244,6 +275,14 @@ static inline int extcon_dev_register(struct extcon_dev *edev,
 
 static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
 
+static void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
+				enum extcon_cable_name type,
+				union extcon_cable_data data) { }
+
+static void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
+				enum extcon_cable_name *type,
+				union extcon_cable_data *data) { }
+
 static inline u32 extcon_get_state(struct extcon_dev *edev)
 {
 	return 0;
-- 
1.7.9.5

RE: [PATCH] EXTCON: Get and set cable properties

[Tc, Jenny]

Hi Myungjoo/Chanwoo/Aneesh/Anton,

Could you please review this. This is a follow up patch for "PATCH] extcon : callback function to read cable property"

-jtc

> Subject: [PATCH] EXTCON: Get and set cable properties
> 
> Existing EXTCON implementation doesn't give a mechanim to read the cable
> properties and extra states a cable needs to support. There are scenarios
> where a cable can have more than two
> states(CONNECT/DISCONNECT/SUSPEND/RESUME etc) and can have some
> properties associated with cables(mA)
> 
> This patch introduces interface to get and set cable properties from EXTCON
> framework. The cable property can be set either by the extcon cable
> provider or by other subsystems who know the cable properties using eth
> API extcon_cable_set_data()
> 
> When the consumer gets a notification from the extcon, it can use the
> extcon_cable_get_data() to get the cable properties irrespective of who
> provides the cable data.
> 
> This gives a single interface for setting and getting the cable properties.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  drivers/extcon/extcon-class.c |   30 ++++++++++++++++++++++++++++++
>  include/linux/extcon.h        |   39
> +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index d398821..304f343 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -545,6 +545,36 @@ int extcon_unregister_notifier(struct extcon_dev
> *edev,  }  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
> 
> +/**
> + * extcon_cable_set_data() - Set the data structure for a cable
> + * @edev:	the extcon device
> + * @cable_index:	the cable index of the correspondant
> + * @type:	type of the data structure
> + * @data:
> + */
> +void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
> +			   enum extcon_cable_name type,
> +			   union extcon_cable_data data)
> +{
> +	edev->cables[cable_index].type = type;
> +	edev->cables[cable_index].data = data; }
> +
> +/**
> + * extcon_cable_get_data() - Get the data structure for a cable
> + * @edev:	the extcon device
> + * @cable_index:	the cable index of the correspondant
> + * @type:	type of the data structure
> + * @data:	the corresponding data structure (e.g., regulator)
> + */
> +void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
> +			   enum extcon_cable_name *type,
> +			   union extcon_cable_data *data)
> +{
> +	*type = edev->cables[cable_index].type;
> +	*data = edev->cables[cable_index].data; }
> +
>  static struct device_attribute extcon_attrs[] = {
>  	__ATTR(state, S_IRUGO | S_IWUSR, state_show, state_store),
>  	__ATTR_RO(name),
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h index
> 2c26c14..4556cc5 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -135,6 +135,19 @@ struct extcon_dev {
>  	struct device_attribute *d_attrs_muex;  };
> 
> +/* FIXME: Is this the right place for this structure definition?
> + * Do we need to move it to power_supply.h?
> + */
> +struct extcon_chrgr_cable_props {
> +	unsigned long state;
> +	int mA;
> +};
> +
> +union extcon_cable_data {
> +	struct extcon_chrgr_cable_props chrgr_cbl_props;
> +	/* Please add accordingly*/
> +};
> +
>  /**
>   * struct extcon_cable	- An internal data for each cable of extcon device.
>   * @edev	The extcon device
> @@ -143,6 +156,8 @@ struct extcon_dev {
>   * @attr_name	"name" sysfs entry
>   * @attr_state	"state" sysfs entry
>   * @attrs	Array pointing to attr_name and attr_state for attr_g
> + * @type:	The type of @data.
> + * @data:	The data structure representing the status and states of this
> cable.
>   */
>  struct extcon_cable {
>  	struct extcon_dev *edev;
> @@ -153,6 +168,11 @@ struct extcon_cable {
>  	struct device_attribute attr_state;
> 
>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> +
> +	union extcon_cable_data data;
> +
> +	/* extcon cable type */
> +	enum extcon_cable_name type;
>  };
> 
>  /**
> @@ -183,6 +203,17 @@ extern void extcon_dev_unregister(struct
> extcon_dev *edev);  extern struct extcon_dev
> *extcon_get_extcon_dev(const char *extcon_name);
> 
>  /*
> + * Following APIs are for managing the status and states of each cable.
> + * For example, if a cable is represented as a regulator, then the
> +cable
> + * may have struct regulator as its data.
> + */
> +extern void extcon_cable_set_data(struct extcon_dev *edev, int
> cable_index,
> +			enum extcon_cable_name type,
> +			union extcon_cable_data data);
> +extern void extcon_cable_get_data(struct extcon_dev *edev, int
> cable_index,
> +			enum extcon_cable_name *type,
> +			union extcon_cable_data *data);
> +/*
>   * get/set/update_state access the 32b encoded state value, which
> represents
>   * states of all possible cables of the multistate port. For example, if one
>   * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
> @@ -244,6 +275,14 @@ static inline int extcon_dev_register(struct
> extcon_dev *edev,
> 
>  static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
> 
> +static void extcon_cable_set_data(struct extcon_dev *edev, int
> cable_index,
> +				enum extcon_cable_name type,
> +				union extcon_cable_data data) { }
> +
> +static void extcon_cable_get_data(struct extcon_dev *edev, int
> cable_index,
> +				enum extcon_cable_name *type,
> +				union extcon_cable_data *data) { }
> +
>  static inline u32 extcon_get_state(struct extcon_dev *edev)  {
>  	return 0;
> --
> 1.7.9.5

Re: [PATCH] EXTCON: Get and set cable properties

[Chanwoo Choi]

We discussed about this patch and then suggest some method to resolve
this issue by Myungjoo Ham. Why don't you write additional feature or
your opinion based on following patch by Myungjoo Ham?
-
http://git.kernel.org/?p=linux/kernel/git/mzx/extcon.git;a=commitdiff;h=7312b79d69a2b9f06af4f1254bc4644751e3e3ea


On 11/28/2012 06:49 AM, Jenny TC wrote:
> Existing EXTCON implementation doesn't give a mechanim to read the cable
> properties and extra states a cable needs to support. There are scenarios
> where a cable can have more than two states(CONNECT/DISCONNECT/SUSPEND/RESUME etc)
> and can have some properties associated with cables(mA)
> 
> This patch introduces interface to get and set cable properties
> from EXTCON framework. The cable property can be set either by
> the extcon cable provider or by other subsystems who know the cable
> properties using eth API extcon_cable_set_data()
> 
> When the consumer gets a notification from the extcon, it can use the
> extcon_cable_get_data() to get the cable properties irrespective of who
> provides the cable data.
> 
> This gives a single interface for setting and getting the cable properties.
> 
> Signed-off-by: Jenny TC <jenny.tc@intel.com>
> ---
>  drivers/extcon/extcon-class.c |   30 ++++++++++++++++++++++++++++++
>  include/linux/extcon.h        |   39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index d398821..304f343 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -545,6 +545,36 @@ int extcon_unregister_notifier(struct extcon_dev *edev,
>  }
>  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
>  
> +/**
> + * extcon_cable_set_data() - Set the data structure for a cable
> + * @edev:	the extcon device
> + * @cable_index:	the cable index of the correspondant
> + * @type:	type of the data structure
> + * @data:
> + */
> +void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
> +			   enum extcon_cable_name type,
> +			   union extcon_cable_data data)
> +{
> +	edev->cables[cable_index].type = type;
> +	edev->cables[cable_index].data = data;
> +}
> +
> +/**
> + * extcon_cable_get_data() - Get the data structure for a cable
> + * @edev:	the extcon device
> + * @cable_index:	the cable index of the correspondant
> + * @type:	type of the data structure
> + * @data:	the corresponding data structure (e.g., regulator)
> + */
> +void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
> +			   enum extcon_cable_name *type,
> +			   union extcon_cable_data *data)
> +{
> +	*type = edev->cables[cable_index].type;
> +	*data = edev->cables[cable_index].data;
> +}
> +
>  static struct device_attribute extcon_attrs[] = {
>  	__ATTR(state, S_IRUGO | S_IWUSR, state_show, state_store),
>  	__ATTR_RO(name),
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 2c26c14..4556cc5 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -135,6 +135,19 @@ struct extcon_dev {
>  	struct device_attribute *d_attrs_muex;
>  };
>  
> +/* FIXME: Is this the right place for this structure definition?
> + * Do we need to move it to power_supply.h?
> + */
> +struct extcon_chrgr_cable_props {
> +	unsigned long state;
> +	int mA;
> +};

As I said, extcon provider driver didn't provide directly charging current(int mA)
and some state(unsigned long state) because the extcon provider driver(Micro
USB interface controller device or other device related to external connector) haven't
mechanism which detect dynamically charging current immediately. If you wish to
provide charging current data to extcon consumer driver or framework, you should
use regulator/power_supply framework or other method in extcon provider driver.

The patch suggested by Myungjoo Ham define following struct:
	union extcon_cable_data {
		struct regualtor *reg;  /* EXTCON_CT_REGULATOR */
		struct power_supply *psy; /* EXTCON_CT_PSY */
		struct charger_cable *charger; /* EXTCON_CT_CHARGER_MANAGER */
		/* Please add accordingly with enum extcon_cable_type */
	};

Also if you want to define 'struct extcon_chrgr_cable_props', you should certainly show how
detect dynamically charging current or state.

> +
> +union extcon_cable_data {
> +	struct extcon_chrgr_cable_props chrgr_cbl_props;
> +	/* Please add accordingly*/
> +};
> +
>  /**
>   * struct extcon_cable	- An internal data for each cable of extcon device.
>   * @edev	The extcon device
> @@ -143,6 +156,8 @@ struct extcon_dev {
>   * @attr_name	"name" sysfs entry
>   * @attr_state	"state" sysfs entry
>   * @attrs	Array pointing to attr_name and attr_state for attr_g
> + * @type:	The type of @data.
> + * @data:	The data structure representing the status and states of this cable.
>   */
>  struct extcon_cable {
>  	struct extcon_dev *edev;
> @@ -153,6 +168,11 @@ struct extcon_cable {
>  	struct device_attribute attr_state;
>  
>  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> +
> +	union extcon_cable_data data;
> +
> +	/* extcon cable type */
> +	enum extcon_cable_name type;
It isn't correct. The 'enum extocn_cable_name' isn't type. For example,
the list of charger cable are TA/USB/Fast Charger/Slow Charger and so on.
The charger cables are charger type so, they has EXTCON_CT_CHARGER_MANAGER
as cable type.

The patch suggested by Myungjoo Ham define following struct:
	enum extcon_cable_type {
	       EXTCON_CT_NONE = 0,
	       EXTCON_CT_REGULATOR,
	       EXTCON_CT_PSY,
	       EXTCON_CT_CHARGER_MANAGER,
	       /* Please add other related standards when needed */
	};
	

Re: [PATCH] EXTCON: Get and set cable properties

[Anton Vorontsov]

On Sun, Dec 02, 2012 at 06:53:17AM +0000, Tc, Jenny wrote:
> Could you please review this. This is a follow up patch for "PATCH]
> extcon : callback function to read cable property"

While I see nothing wrong with the patch itself, I beg you to send some
users for the new calls. Don't be obsessed with the extcon internals too
much, think more about how things will interact (i.e. I really really want
to see how you use these calls from the power supply drivers).

Thanks,
Anton.

RE: [PATCH] EXTCON: Get and set cable properties

[Tc, Jenny]

> We discussed about this patch and then suggest some method to resolve this
> issue by Myungjoo Ham. Why don't you write additional feature or your
> opinion based on following patch by Myungjoo Ham?
> -
> http://git.kernel.org/?p=linux/kernel/git/mzx/extcon.git;a=commitdiff;h=73
> 12b79d69a2b9f06af4f1254bc4644751e3e3ea
> 

I replied on the thread and pointed out issues that I see with this solution. IMHO
It's not fair to register a cable with power_supply/regulator/charger manager just to
expose the cable properties. 

> 
> On 11/28/2012 06:49 AM, Jenny TC wrote:
> > Existing EXTCON implementation doesn't give a mechanim to read the
> > cable properties and extra states a cable needs to support. There are
> > scenarios where a cable can have more than two
> > states(CONNECT/DISCONNECT/SUSPEND/RESUME etc) and can have some
> > properties associated with cables(mA)
> >
> > This patch introduces interface to get and set cable properties from
> > EXTCON framework. The cable property can be set either by the extcon
> > cable provider or by other subsystems who know the cable properties
> > using eth API extcon_cable_set_data()
> >
> > When the consumer gets a notification from the extcon, it can use the
> > extcon_cable_get_data() to get the cable properties irrespective of
> > who provides the cable data.
> >
> > This gives a single interface for setting and getting the cable properties.
> >
> > Signed-off-by: Jenny TC <jenny.tc@intel.com>
> > ---
> >  drivers/extcon/extcon-class.c |   30
> ++++++++++++++++++++++++++++++
> >  include/linux/extcon.h        |   39
> +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 69 insertions(+)
> >
> > diff --git a/drivers/extcon/extcon-class.c
> > b/drivers/extcon/extcon-class.c index d398821..304f343 100644
> > --- a/drivers/extcon/extcon-class.c
> > +++ b/drivers/extcon/extcon-class.c
> > @@ -545,6 +545,36 @@ int extcon_unregister_notifier(struct extcon_dev
> > *edev,  }  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
> >
> > +/**
> > + * extcon_cable_set_data() - Set the data structure for a cable
> > + * @edev:	the extcon device
> > + * @cable_index:	the cable index of the correspondant
> > + * @type:	type of the data structure
> > + * @data:
> > + */
> > +void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
> > +			   enum extcon_cable_name type,
> > +			   union extcon_cable_data data)
> > +{
> > +	edev->cables[cable_index].type = type;
> > +	edev->cables[cable_index].data = data; }
> > +
> > +/**
> > + * extcon_cable_get_data() - Get the data structure for a cable
> > + * @edev:	the extcon device
> > + * @cable_index:	the cable index of the correspondant
> > + * @type:	type of the data structure
> > + * @data:	the corresponding data structure (e.g., regulator)
> > + */
> > +void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
> > +			   enum extcon_cable_name *type,
> > +			   union extcon_cable_data *data)
> > +{
> > +	*type = edev->cables[cable_index].type;
> > +	*data = edev->cables[cable_index].data; }
> > +
> >  static struct device_attribute extcon_attrs[] = {
> >  	__ATTR(state, S_IRUGO | S_IWUSR, state_show, state_store),
> >  	__ATTR_RO(name),
> > diff --git a/include/linux/extcon.h b/include/linux/extcon.h index
> > 2c26c14..4556cc5 100644
> > --- a/include/linux/extcon.h
> > +++ b/include/linux/extcon.h
> > @@ -135,6 +135,19 @@ struct extcon_dev {
> >  	struct device_attribute *d_attrs_muex;  };
> >
> > +/* FIXME: Is this the right place for this structure definition?
> > + * Do we need to move it to power_supply.h?
> > + */
> > +struct extcon_chrgr_cable_props {
> > +	unsigned long state;
> > +	int mA;
> > +};
> 
> As I said, extcon provider driver didn't provide directly charging current(int
> mA) and some state(unsigned long state) because the extcon provider
> driver(Micro USB interface controller device or other device related to
> external connector) haven't mechanism which detect dynamically charging
> current immediately. If you wish to provide charging current data to extcon
> consumer driver or framework, you should use regulator/power_supply
> framework or other method in extcon provider driver.
> 
> The patch suggested by Myungjoo Ham define following struct:
> 	union extcon_cable_data {
> 		struct regualtor *reg;  /* EXTCON_CT_REGULATOR */
> 		struct power_supply *psy; /* EXTCON_CT_PSY */
> 		struct charger_cable *charger; /*
> EXTCON_CT_CHARGER_MANAGER */
> 		/* Please add accordingly with enum extcon_cable_type */
> 	};
> 
> Also if you want to define 'struct extcon_chrgr_cable_props', you should
> certainly show how detect dynamically charging current or state.
> 
> > +
> > +union extcon_cable_data {
> > +	struct extcon_chrgr_cable_props chrgr_cbl_props;
> > +	/* Please add accordingly*/
> > +};
> > +
> >  /**
> >   * struct extcon_cable	- An internal data for each cable of extcon
> device.
> >   * @edev	The extcon device
> > @@ -143,6 +156,8 @@ struct extcon_dev {
> >   * @attr_name	"name" sysfs entry
> >   * @attr_state	"state" sysfs entry
> >   * @attrs	Array pointing to attr_name and attr_state for attr_g
> > + * @type:	The type of @data.
> > + * @data:	The data structure representing the status and states of this
> cable.
> >   */
> >  struct extcon_cable {
> >  	struct extcon_dev *edev;
> > @@ -153,6 +168,11 @@ struct extcon_cable {
> >  	struct device_attribute attr_state;
> >
> >  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> > +
> > +	union extcon_cable_data data;
> > +
> > +	/* extcon cable type */
> > +	enum extcon_cable_name type;
> It isn't correct. The 'enum extocn_cable_name' isn't type. For example, the
> list of charger cable are TA/USB/Fast Charger/Slow Charger and so on.
> The charger cables are charger type so, they has
> EXTCON_CT_CHARGER_MANAGER as cable type.
> 
> The patch suggested by Myungjoo Ham define following struct:
> 	enum extcon_cable_type {
> 	       EXTCON_CT_NONE = 0,
> 	       EXTCON_CT_REGULATOR,
> 	       EXTCON_CT_PSY,
> 	       EXTCON_CT_CHARGER_MANAGER,
> 	       /* Please add other related standards when needed */
> 	};
> 

RE: [PATCH] EXTCON: Get and set cable properties

[Tc, Jenny]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1613 bytes --]

> > Could you please review this. This is a follow up patch for "PATCH]
> > extcon : callback function to read cable property"
> 
> While I see nothing wrong with the patch itself, I beg you to send some users
> for the new calls. Don't be obsessed with the extcon internals too much,
> think more about how things will interact (i.e. I really really want to see how
> you use these calls from the power supply drivers).

The usage of extcon cable property is captured in patch https://lkml.org/lkml/2012/10/18/219
This patch uses a extcon_dev  callback function get_cable_properties() to get the
cable properties. As discussed in the previous mail thread, it may not be good to have a extcon call
back function since the extcon provider may not be aware of the cable properties. This patch replaces
the callback function with an API, so that whoever knows the cable property, can set the property
using the extcon API extcon_cable_set_data().

The usage flow would be
1)Consumer gets a notification from the extcon
2)consumer reads the property using the API extcon_cable_get_data

This way it doesn't mandatory for the extcon provider to give the cable property.
Anyone who is aware of the cable property can set the cable property using the API.
It makes the consumer and provider implementations very simple.

With this new API, the callback function in patch https://lkml.org/lkml/2012/10/18/219 can be
replaced by the API extcon_cable_set_data().
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] EXTCON: Get and set cable properties

[anish kumar]

On Mon, 2012-12-03 at 01:53 +0000, Tc, Jenny wrote:
> > We discussed about this patch and then suggest some method to resolve this
> > issue by Myungjoo Ham. Why don't you write additional feature or your
> > opinion based on following patch by Myungjoo Ham?
> > -
> > http://git.kernel.org/?p=linux/kernel/git/mzx/extcon.git;a=commitdiff;h=73
> > 12b79d69a2b9f06af4f1254bc4644751e3e3ea
> > 
> 
> I replied on the thread and pointed out issues that I see with this solution. IMHO
> It's not fair to register a cable with power_supply/regulator/charger manager just to
> expose the cable properties. 
Don't we do that with already in allmost all drivers?Like if I have a
keyboard driver and then I register with input framework and if the
keyboard driver needs clk services then I register with clk framework as
well and same with other services needed by keyboard driver.I think it
makes sense for a cable to register with different framework if it
supports that functionality as those services also would know that we
have a cable with so and so property.
> 
> > 
> > On 11/28/2012 06:49 AM, Jenny TC wrote:
> > > Existing EXTCON implementation doesn't give a mechanim to read the
> > > cable properties and extra states a cable needs to support. There are
> > > scenarios where a cable can have more than two
> > > states(CONNECT/DISCONNECT/SUSPEND/RESUME etc) and can have some
> > > properties associated with cables(mA)
> > >
> > > This patch introduces interface to get and set cable properties from
> > > EXTCON framework. The cable property can be set either by the extcon
> > > cable provider or by other subsystems who know the cable properties
> > > using eth API extcon_cable_set_data()
> > >
> > > When the consumer gets a notification from the extcon, it can use the
> > > extcon_cable_get_data() to get the cable properties irrespective of
> > > who provides the cable data.
> > >
> > > This gives a single interface for setting and getting the cable properties.
> > >
> > > Signed-off-by: Jenny TC <jenny.tc@intel.com>
> > > ---
> > >  drivers/extcon/extcon-class.c |   30
> > ++++++++++++++++++++++++++++++
> > >  include/linux/extcon.h        |   39
> > +++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 69 insertions(+)
> > >
> > > diff --git a/drivers/extcon/extcon-class.c
> > > b/drivers/extcon/extcon-class.c index d398821..304f343 100644
> > > --- a/drivers/extcon/extcon-class.c
> > > +++ b/drivers/extcon/extcon-class.c
> > > @@ -545,6 +545,36 @@ int extcon_unregister_notifier(struct extcon_dev
> > > *edev,  }  EXPORT_SYMBOL_GPL(extcon_unregister_notifier);
> > >
> > > +/**
> > > + * extcon_cable_set_data() - Set the data structure for a cable
> > > + * @edev:	the extcon device
> > > + * @cable_index:	the cable index of the correspondant
> > > + * @type:	type of the data structure
> > > + * @data:
> > > + */
> > > +void extcon_cable_set_data(struct extcon_dev *edev, int cable_index,
> > > +			   enum extcon_cable_name type,
> > > +			   union extcon_cable_data data)
> > > +{
> > > +	edev->cables[cable_index].type = type;
> > > +	edev->cables[cable_index].data = data; }
> > > +
> > > +/**
> > > + * extcon_cable_get_data() - Get the data structure for a cable
> > > + * @edev:	the extcon device
> > > + * @cable_index:	the cable index of the correspondant
> > > + * @type:	type of the data structure
> > > + * @data:	the corresponding data structure (e.g., regulator)
> > > + */
> > > +void extcon_cable_get_data(struct extcon_dev *edev, int cable_index,
> > > +			   enum extcon_cable_name *type,
> > > +			   union extcon_cable_data *data)
> > > +{
> > > +	*type = edev->cables[cable_index].type;
> > > +	*data = edev->cables[cable_index].data; }
> > > +
> > >  static struct device_attribute extcon_attrs[] = {
> > >  	__ATTR(state, S_IRUGO | S_IWUSR, state_show, state_store),
> > >  	__ATTR_RO(name),
> > > diff --git a/include/linux/extcon.h b/include/linux/extcon.h index
> > > 2c26c14..4556cc5 100644
> > > --- a/include/linux/extcon.h
> > > +++ b/include/linux/extcon.h
> > > @@ -135,6 +135,19 @@ struct extcon_dev {
> > >  	struct device_attribute *d_attrs_muex;  };
> > >
> > > +/* FIXME: Is this the right place for this structure definition?
> > > + * Do we need to move it to power_supply.h?
> > > + */
> > > +struct extcon_chrgr_cable_props {
> > > +	unsigned long state;
> > > +	int mA;
> > > +};
> > 
> > As I said, extcon provider driver didn't provide directly charging current(int
> > mA) and some state(unsigned long state) because the extcon provider
> > driver(Micro USB interface controller device or other device related to
> > external connector) haven't mechanism which detect dynamically charging
> > current immediately. If you wish to provide charging current data to extcon
> > consumer driver or framework, you should use regulator/power_supply
> > framework or other method in extcon provider driver.
> > 
> > The patch suggested by Myungjoo Ham define following struct:
> > 	union extcon_cable_data {
> > 		struct regualtor *reg;  /* EXTCON_CT_REGULATOR */
> > 		struct power_supply *psy; /* EXTCON_CT_PSY */
> > 		struct charger_cable *charger; /*
> > EXTCON_CT_CHARGER_MANAGER */
> > 		/* Please add accordingly with enum extcon_cable_type */
> > 	};
> > 
> > Also if you want to define 'struct extcon_chrgr_cable_props', you should
> > certainly show how detect dynamically charging current or state.
> > 
> > > +
> > > +union extcon_cable_data {
> > > +	struct extcon_chrgr_cable_props chrgr_cbl_props;
> > > +	/* Please add accordingly*/
> > > +};
> > > +
> > >  /**
> > >   * struct extcon_cable	- An internal data for each cable of extcon
> > device.
> > >   * @edev	The extcon device
> > > @@ -143,6 +156,8 @@ struct extcon_dev {
> > >   * @attr_name	"name" sysfs entry
> > >   * @attr_state	"state" sysfs entry
> > >   * @attrs	Array pointing to attr_name and attr_state for attr_g
> > > + * @type:	The type of @data.
> > > + * @data:	The data structure representing the status and states of this
> > cable.
> > >   */
> > >  struct extcon_cable {
> > >  	struct extcon_dev *edev;
> > > @@ -153,6 +168,11 @@ struct extcon_cable {
> > >  	struct device_attribute attr_state;
> > >
> > >  	struct attribute *attrs[3]; /* to be fed to attr_g.attrs */
> > > +
> > > +	union extcon_cable_data data;
> > > +
> > > +	/* extcon cable type */
> > > +	enum extcon_cable_name type;
> > It isn't correct. The 'enum extocn_cable_name' isn't type. For example, the
> > list of charger cable are TA/USB/Fast Charger/Slow Charger and so on.
> > The charger cables are charger type so, they has
> > EXTCON_CT_CHARGER_MANAGER as cable type.
> > 
> > The patch suggested by Myungjoo Ham define following struct:
> > 	enum extcon_cable_type {
> > 	       EXTCON_CT_NONE = 0,
> > 	       EXTCON_CT_REGULATOR,
> > 	       EXTCON_CT_PSY,
> > 	       EXTCON_CT_CHARGER_MANAGER,
> > 	       /* Please add other related standards when needed */
> > 	};
> > 

RE: [PATCH] EXTCON: Get and set cable properties

[Tc, Jenny]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2963 bytes --]


> > I replied on the thread and pointed out issues that I see with this
> > solution. IMHO It's not fair to register a cable with
> > power_supply/regulator/charger manager just to expose the cable
> properties.
> Don't we do that with already in allmost all drivers?Like if I have a keyboard
> driver and then I register with input framework and if the keyboard driver
> needs clk services then I register with clk framework as well and same with
> other services needed by keyboard driver.I think it makes sense for a cable
> to register with different framework if it supports that functionality as those
> services also would know that we have a cable with so and so property.

IMHO it's not a good choice to register a cable itself with any of the three subsystem
(power_supply/regulator/charger manager)

 For example it cannot register with power_supply subsystem since it's not a 
 power_supply. It's just source for a power_supply.We register either charger/battery with
 power supply. I couldn't find a way to register the cable with power supply subsystem. 

In previous discussion Anton also agreed to this.

Anton,
Could you please confirm?

I think the same case with regulator framework also. A cable doesn’t belong to a regulator framework.
A cable doesn’t expose any control attribute (current control/voltage control). It just have  properties (eg current)
 controlled by external agents (Host machine/wall charger)

And charger manager is a consumer and not a provider. It cannot decide the charger cable capabilities.


> > > As I said, extcon provider driver didn't provide directly charging
> > > current(int
> > > mA) and some state(unsigned long state) because the extcon provider
> > > driver(Micro USB interface controller device or other device related
> > > to external connector) haven't mechanism which detect dynamically
> > > charging current immediately. If you wish to provide charging
> > > current data to extcon consumer driver or framework, you should use
> > > regulator/power_supply framework or other method in extcon provider
> driver.

This information need not to come from the extcon provider. It can come from any driver
Who knows the state of a cable. It may be a platform driver or may be a driver belongs to any
of the three subsystem we discussed above (power_supply/regulator/charger_manager).
Just by having an API like this (extcon_cable_set_data), it's not mandatory to register the cable
with any of the subsystem.

> > > Also if you want to define 'struct extcon_chrgr_cable_props', you
> > > should certainly show how detect dynamically charging current or state.

For USB cables, it's determined by USB enumeration. For USB 3.0 it would be 900mA and
USB 2.0 it's 500mA. Also in un configured  state this would be 150 and 100mA respectively
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] EXTCON: Get and set cable properties

[Tc, Jenny]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1605 bytes --]

Anton,

Could you please have a look at my comments below?

-jtc

> > While I see nothing wrong with the patch itself, I beg you to send
> > some users for the new calls. Don't be obsessed with the extcon
> > internals too much, think more about how things will interact (i.e. I
> > really really want to see how you use these calls from the power supply
> drivers).
> 
> The usage of extcon cable property is captured in patch
> https://lkml.org/lkml/2012/10/18/219
> This patch uses a extcon_dev  callback function get_cable_properties() to get
> the cable properties. As discussed in the previous mail thread, it may not be
> good to have a extcon call back function since the extcon provider may not
> be aware of the cable properties. This patch replaces the callback function
> with an API, so that whoever knows the cable property, can set the property
> using the extcon API extcon_cable_set_data().
> 
> The usage flow would be
> 1)Consumer gets a notification from the extcon 2)consumer reads the
> property using the API extcon_cable_get_data
> 
> This way it doesn't mandatory for the extcon provider to give the cable
> property.
> Anyone who is aware of the cable property can set the cable property using
> the API.
> It makes the consumer and provider implementations very simple.
> 
> With this new API, the callback function in patch
> https://lkml.org/lkml/2012/10/18/219 can be replaced by the API
> extcon_cable_set_data().
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] EXTCON: Get and set cable properties

[Chanwoo Choi]

On 12/15/2012 09:16 AM, Tc, Jenny wrote:
> 
>>> I replied on the thread and pointed out issues that I see with this
>>> solution. IMHO It's not fair to register a cable with
>>> power_supply/regulator/charger manager just to expose the cable
>> properties.
>> Don't we do that with already in allmost all drivers?Like if I have a keyboard
>> driver and then I register with input framework and if the keyboard driver
>> needs clk services then I register with clk framework as well and same with
>> other services needed by keyboard driver.I think it makes sense for a cable
>> to register with different framework if it supports that functionality as those
>> services also would know that we have a cable with so and so property.
> 
> IMHO it's not a good choice to register a cable itself with any of the three subsystem
> (power_supply/regulator/charger manager)
> 
>  For example it cannot register with power_supply subsystem since it's not a 
>  power_supply. It's just source for a power_supply.We register either charger/battery with
>  power supply. I couldn't find a way to register the cable with power supply subsystem. 
> 
> In previous discussion Anton also agreed to this.
> 
> Anton,
> Could you please confirm?
> 
> I think the same case with regulator framework also. A cable doesn’t belong to a regulator framework.
> A cable doesn’t expose any control attribute (current control/voltage control). It just have  properties (eg current)
>  controlled by external agents (Host machine/wall charger)
> 
> And charger manager is a consumer and not a provider. It cannot decide the charger cable capabilities.
No, charger-manager include information for battery/charger/charger cable dependency on target H/W.
Each target has different h/w constraint for charging, so charger-manager can determine whether
specific charger cable is used on target or not.

> 
> 
>>>> As I said, extcon provider driver didn't provide directly charging
>>>> current(int
>>>> mA) and some state(unsigned long state) because the extcon provider
>>>> driver(Micro USB interface controller device or other device related
>>>> to external connector) haven't mechanism which detect dynamically
>>>> charging current immediately. If you wish to provide charging
>>>> current data to extcon consumer driver or framework, you should use
>>>> regulator/power_supply framework or other method in extcon provider
>> driver.
> 
> This information need not to come from the extcon provider. It can come from any driver
> Who knows the state of a cable. It may be a platform driver or may be a driver belongs to any
> of the three subsystem we discussed above (power_supply/regulator/charger_manager).
> Just by having an API like this (extcon_cable_set_data), it's not mandatory to register the cable
> with any of the subsystem.
> 
>>>> Also if you want to define 'struct extcon_chrgr_cable_props', you
>>>> should certainly show how detect dynamically charging current or state.
> 
> For USB cables, it's determined by USB enumeration. For USB 3.0 it would be 900mA and
> USB 2.0 it's 500mA. Also in un configured  state this would be 150 and 100mA respectively
I think only USB enumeration determine standard charging current according to version of USB.
But, Extcon subsystem always need detection mechanism. (e.g, Jack/Microphone detection device
of Wolfson sound codec and MUIC(Micro USB Interface Controller) device of MAXIM)

You didn't show any detection mechanism which detect changed state of host system.

What can some device for detecting of following host state except of CONNECT/DISCONNECT state?
I don't know. Please let me know it.

- SUSPEND(Host suspend for SDP cable),
- RESUME(Host wakeup)
- UPDATE (to increase the charge current after USB enumaeration).
- CONNECT
- DISCOONECT

Re: [PATCH] EXTCON: Get and set cable properties

[Anton Vorontsov]

On Mon, Dec 03, 2012 at 02:09:02AM +0000, Tc, Jenny wrote:
> > > Could you please review this. This is a follow up patch for "PATCH]
> > > extcon : callback function to read cable property"
> > 
> > While I see nothing wrong with the patch itself, I beg you to send some users
> > for the new calls. Don't be obsessed with the extcon internals too much,
> > think more about how things will interact (i.e. I really really want to see how
> > you use these calls from the power supply drivers).
> 
> The usage of extcon cable property is captured in patch https://lkml.org/lkml/2012/10/18/219
> This patch uses a extcon_dev  callback function get_cable_properties() to get the
> cable properties. As discussed in the previous mail thread, it may not be good to have a extcon call
> back function since the extcon provider may not be aware of the cable properties. This patch replaces
> the callback function with an API, so that whoever knows the cable property, can set the property
> using the extcon API extcon_cable_set_data().
> 
> The usage flow would be
> 1)Consumer gets a notification from the extcon
> 2)consumer reads the property using the API extcon_cable_get_data
> 
> This way it doesn't mandatory for the extcon provider to give the cable property.
> Anyone who is aware of the cable property can set the cable property using the API.
> It makes the consumer and provider implementations very simple.
> 
> With this new API, the callback function in patch https://lkml.org/lkml/2012/10/18/219 can be
> replaced by the API extcon_cable_set_data().

Looking at this, the whole idea of hiding power source behind the "extcon"
seems dubious. Why don't you use USB device to get the current?

"extcon" subsystem, as I see it, should only be used to get notified about
external connectors events. And that's all. And chargers probably should
not even care about extcon (well, with the exception of the direct AC/gpio
power source).

For USB, it would make more sense if for you'd get plug/unplug
notifications *and* properties from the USB device (or OTG transceiver)
directly, not from the extcon. And I guess we have this mechanism already,
see drivers/power/pda_power.c.

Thanks,
Anton