[PATCH 0/4] extcon: Modify the device name for sysfs entry and add extcon_get_edev_name()

[PATCH 0/4] extcon: Modify the device name for sysfs entry and add extcon_get_edev_name()

[Chanwoo Choi]

This patchset update the extcon core to maintain the consistency of device name
on sysfs and add the extcon_get_edev_name() API to access the filed of 'struct
extcon_dev' by only extcon core API. And this patchset fix the checkpat warning
and minor coding style issue.

Chanwoo Choi (4):
  extcon: Modify the device name as extcon[X] for sysfs
  extcon: Add extcon_get_edev_name() API to get the extcon device name
  extcon: Fix the checkpatch warning and minor coding style issue
  extcon: arizona: Remove the setting of device name

 drivers/extcon/extcon-arizona.c |  1 -
 drivers/extcon/extcon.c         | 27 +++++++++++++++++++--------
 include/linux/extcon.h          | 11 ++++++++---
 3 files changed, 27 insertions(+), 12 deletions(-)

-- 
1.8.5.5

[PATCH 1/4] extcon: Modify the device name as extcon[X] for sysfs

[Chanwoo Choi]

This patch modify the device name as extcon[X] for sysfs by using the 'extcon'
prefix word instead of separate device name. On user-space aspect, user would
find the some extcon drvier with extcon[X] pattern. So, this patch modify the
device name as following:
- /sys/class/extcon/[device name] -> /sys/class/extcon/extcon[X]

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 4c9f165..1a93229 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -163,7 +163,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 			return ret;
 	}
 
-	return sprintf(buf, "%s\n", dev_name(&edev->dev));
+	return sprintf(buf, "%s\n", edev->name);
 }
 static DEVICE_ATTR_RO(name);
 
@@ -701,6 +701,7 @@ EXPORT_SYMBOL_GPL(devm_extcon_dev_free);
 int extcon_dev_register(struct extcon_dev *edev)
 {
 	int ret, index = 0;
+	static atomic_t edev_no = ATOMIC_INIT(-1);
 
 	if (!extcon_class) {
 		ret = create_extcon_class();
@@ -725,13 +726,14 @@ int extcon_dev_register(struct extcon_dev *edev)
 	edev->dev.class = extcon_class;
 	edev->dev.release = extcon_dev_release;
 
-	edev->name = edev->name ? edev->name : dev_name(edev->dev.parent);
+	edev->name = dev_name(edev->dev.parent);
 	if (IS_ERR_OR_NULL(edev->name)) {
 		dev_err(&edev->dev,
 			"extcon device name is null\n");
 		return -EINVAL;
 	}
-	dev_set_name(&edev->dev, "%s", edev->name);
+	dev_set_name(&edev->dev, "extcon%lu",
+			(unsigned long)atomic_inc_return(&edev_no));
 
 	if (edev->max_supported) {
 		char buf[10];
-- 
1.8.5.5

Re: [PATCH 1/4] extcon: Modify the device name as extcon[X] for sysfs

[Charles Keepax]

On Mon, Apr 27, 2015 at 09:13:35PM +0900, Chanwoo Choi wrote:
> This patch modify the device name as extcon[X] for sysfs by using the 'extcon'
> prefix word instead of separate device name. On user-space aspect, user would
> find the some extcon drvier with extcon[X] pattern. So, this patch modify the
> device name as following:
> - /sys/class/extcon/[device name] -> /sys/class/extcon/extcon[X]
> 
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
>  drivers/extcon/extcon.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 4c9f165..1a93229 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -163,7 +163,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>  			return ret;
>  	}
>  
> -	return sprintf(buf, "%s\n", dev_name(&edev->dev));
> +	return sprintf(buf, "%s\n", edev->name);
>  }
>  static DEVICE_ATTR_RO(name);
>  
> @@ -701,6 +701,7 @@ EXPORT_SYMBOL_GPL(devm_extcon_dev_free);
>  int extcon_dev_register(struct extcon_dev *edev)
>  {
>  	int ret, index = 0;
> +	static atomic_t edev_no = ATOMIC_INIT(-1);
>  
>  	if (!extcon_class) {
>  		ret = create_extcon_class();
> @@ -725,13 +726,14 @@ int extcon_dev_register(struct extcon_dev *edev)
>  	edev->dev.class = extcon_class;
>  	edev->dev.release = extcon_dev_release;
>  
> -	edev->name = edev->name ? edev->name : dev_name(edev->dev.parent);
> +	edev->name = dev_name(edev->dev.parent);
>  	if (IS_ERR_OR_NULL(edev->name)) {
>  		dev_err(&edev->dev,
>  			"extcon device name is null\n");
>  		return -EINVAL;
>  	}
> -	dev_set_name(&edev->dev, "%s", edev->name);
> +	dev_set_name(&edev->dev, "extcon%lu",
> +			(unsigned long)atomic_inc_return(&edev_no));
>  
>  	if (edev->max_supported) {
>  		char buf[10];
> -- 
> 1.8.5.5
> 

I am not quite sure I see the advantage of this. Why is naming
the node extcon[X] better than the old system? Seems like the
older more descriptive names are better on the face of it, unless
there is some problem with them I am missing.

It also looks problematic, it changes the ABI for the Arizona
extcon driver for a start, admittedly there arn't many users for
the extcon driver at the moment but still is far from ideal to
break the ABI. Secondly, the order of the extcon[X]'s will not be
guaranteed, add some new hardware to the system probe order
changes and now what was extcon0 is extcon1. So it somewhat
complicates the user-space code as it now has to work out if the
device is the one it wants whereas before it could just hard-code
the name.

Thanks,
Charles

Re: [PATCH 1/4] extcon: Modify the device name as extcon[X] for sysfs

[Chanwoo Choi]

Hi Charles,

On Tue, Apr 28, 2015 at 11:17 PM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 27, 2015 at 09:13:35PM +0900, Chanwoo Choi wrote:
>> This patch modify the device name as extcon[X] for sysfs by using the 'extcon'
>> prefix word instead of separate device name. On user-space aspect, user would
>> find the some extcon drvier with extcon[X] pattern. So, this patch modify the
>> device name as following:
>> - /sys/class/extcon/[device name] -> /sys/class/extcon/extcon[X]
>>
>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/extcon/extcon.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 4c9f165..1a93229 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -163,7 +163,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>>                       return ret;
>>       }
>>
>> -     return sprintf(buf, "%s\n", dev_name(&edev->dev));
>> +     return sprintf(buf, "%s\n", edev->name);
>>  }
>>  static DEVICE_ATTR_RO(name);
>>
>> @@ -701,6 +701,7 @@ EXPORT_SYMBOL_GPL(devm_extcon_dev_free);
>>  int extcon_dev_register(struct extcon_dev *edev)
>>  {
>>       int ret, index = 0;
>> +     static atomic_t edev_no = ATOMIC_INIT(-1);
>>
>>       if (!extcon_class) {
>>               ret = create_extcon_class();
>> @@ -725,13 +726,14 @@ int extcon_dev_register(struct extcon_dev *edev)
>>       edev->dev.class = extcon_class;
>>       edev->dev.release = extcon_dev_release;
>>
>> -     edev->name = edev->name ? edev->name : dev_name(edev->dev.parent);
>> +     edev->name = dev_name(edev->dev.parent);
>>       if (IS_ERR_OR_NULL(edev->name)) {
>>               dev_err(&edev->dev,
>>                       "extcon device name is null\n");
>>               return -EINVAL;
>>       }
>> -     dev_set_name(&edev->dev, "%s", edev->name);
>> +     dev_set_name(&edev->dev, "extcon%lu",
>> +                     (unsigned long)atomic_inc_return(&edev_no));
>>
>>       if (edev->max_supported) {
>>               char buf[10];
>> --
>> 1.8.5.5
>>
>
> I am not quite sure I see the advantage of this. Why is naming
> the node extcon[X] better than the old system? Seems like the
> older more descriptive names are better on the face of it, unless
> there is some problem with them I am missing.

In the older method for device name, if some board have the
two more same extcon h/w, extcon driver will fail to probe because
of same device name. There is definitely critical problem with older method.

>
> It also looks problematic, it changes the ABI for the Arizona
> extcon driver for a start, admittedly there arn't many users for
> the extcon driver at the moment but still is far from ideal to
> break the ABI. Secondly, the order of the extcon[X]'s will not be
> guaranteed, add some new hardware to the system probe order
> changes and now what was extcon0 is extcon1. So it somewhat
> complicates the user-space code as it now has to work out if the
> device is the one it wants whereas before it could just hard-code
> the name.

If extcon use the device name as 'extcon[X]' pattern, user-space should
find the appropriate extcon device by using the 'extcon[X]' pattern.
This method is more flexible instead of static hard coded device name.

If extcon use the hard coded device name, user-space platform or
process have to modify the sysfs path of extcon device when changing
the user-space platform or process. It is the cause of strong dependency
of between hardware and user-space platform/process. The user-space
don't consider the h/w name.

So, I'll have the plan to add 'type' field which means the type of h/w
port of extcon device as following and add the 'type' sysfs entry.

#define EXTCON_PORT_USB 0x1
#define EXTCON_PORT_HEADSET 0x2
#define EXTCON_PORT_HDMI 0x3
#define EXTCON_PORT_MHL 0x4
...

For example, some board have the two headset jack port:
The user-space can find the jack device by 'type' entry.

/sys/class/extcon/extcon0/name : arizona-extcon
/sys/class/extcon/extcon0/type   : 0x2
/sys/class/extcon/extcon1/name : arizona-extcon
/sys/class/extcon/extcon1/type   : 0x2

Thanks,
Chanwoo Choi

Re: [PATCH 1/4] extcon: Modify the device name as extcon[X] for sysfs

[Charles Keepax]

On Thu, Apr 30, 2015 at 12:31:06AM +0900, Chanwoo Choi wrote:
> Hi Charles,
> 
> On Tue, Apr 28, 2015 at 11:17 PM, Charles Keepax
> <ckeepax@opensource.wolfsonmicro.com> wrote:
> > On Mon, Apr 27, 2015 at 09:13:35PM +0900, Chanwoo Choi wrote:
> >> This patch modify the device name as extcon[X] for sysfs by using the 'extcon'
> >> prefix word instead of separate device name. On user-space aspect, user would
> >> find the some extcon drvier with extcon[X] pattern. So, this patch modify the
> >> device name as following:
> >> - /sys/class/extcon/[device name] -> /sys/class/extcon/extcon[X]
> >>
> >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> >> ---
> >>  drivers/extcon/extcon.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> >> index 4c9f165..1a93229 100644
> >> --- a/drivers/extcon/extcon.c
> >> +++ b/drivers/extcon/extcon.c
> >> @@ -163,7 +163,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> >>                       return ret;
> >>       }
> >>
> >> -     return sprintf(buf, "%s\n", dev_name(&edev->dev));
> >> +     return sprintf(buf, "%s\n", edev->name);
> >>  }
> >>  static DEVICE_ATTR_RO(name);
> >>
> >> @@ -701,6 +701,7 @@ EXPORT_SYMBOL_GPL(devm_extcon_dev_free);
> >>  int extcon_dev_register(struct extcon_dev *edev)
> >>  {
> >>       int ret, index = 0;
> >> +     static atomic_t edev_no = ATOMIC_INIT(-1);
> >>
> >>       if (!extcon_class) {
> >>               ret = create_extcon_class();
> >> @@ -725,13 +726,14 @@ int extcon_dev_register(struct extcon_dev *edev)
> >>       edev->dev.class = extcon_class;
> >>       edev->dev.release = extcon_dev_release;
> >>
> >> -     edev->name = edev->name ? edev->name : dev_name(edev->dev.parent);
> >> +     edev->name = dev_name(edev->dev.parent);
> >>       if (IS_ERR_OR_NULL(edev->name)) {
> >>               dev_err(&edev->dev,
> >>                       "extcon device name is null\n");
> >>               return -EINVAL;
> >>       }
> >> -     dev_set_name(&edev->dev, "%s", edev->name);
> >> +     dev_set_name(&edev->dev, "extcon%lu",
> >> +                     (unsigned long)atomic_inc_return(&edev_no));
> >>
> >>       if (edev->max_supported) {
> >>               char buf[10];
> >> --
> >> 1.8.5.5
> >>
> >
> > I am not quite sure I see the advantage of this. Why is naming
> > the node extcon[X] better than the old system? Seems like the
> > older more descriptive names are better on the face of it, unless
> > there is some problem with them I am missing.
> 
> In the older method for device name, if some board have the
> two more same extcon h/w, extcon driver will fail to probe because
> of same device name. There is definitely critical problem with older method.

Hmm... ok that is probably reasonable enough reason to change the
ABI. I will add my ack to the Arizona patch.

Thanks,
Charles

[PATCH 2/4] extcon: Add extcon_get_edev_name() API to get the extcon device name

[Chanwoo Choi]

This patch adds the extcon_get_edev_name() API to get the name of extcon device
because all information inclued in the structure extcon_dev should be accessed
by extcon core API instead of directly accessing the data.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon.c | 9 +++++++++
 include/linux/extcon.h  | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 1a93229..c5e9ebf 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1046,6 +1046,15 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
 #endif /* CONFIG_OF */
 EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
 
+/**
+ * extcon_get_edev_name() - Get the name of the extcon device.
+ * @edev:	the extcon device
+ */
+const char *extcon_get_edev_name(struct extcon_dev *edev)
+{
+	return !edev ? NULL : edev->name;
+}
+
 static int __init extcon_class_init(void)
 {
 	return create_extcon_class();
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 36f49c4..e2cf625 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -259,6 +259,10 @@ extern int extcon_unregister_notifier(struct extcon_dev *edev,
  * This function use phandle of devicetree to get extcon device directly.
  */
 extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index);
+
+/* Following API to get information of extcon device */
+extern const char *extcon_get_edev_name(struct extcon_dev *edev);
+
 #else /* CONFIG_EXTCON */
 static inline int extcon_dev_register(struct extcon_dev *edev)
 {
-- 
1.8.5.5

[PATCH 3/4] extcon: Fix the checkpatch warning and minor coding style issue

[Chanwoo Choi]

This patch clean up the extcon core driver by fixing the checkpatch warning
and minor coding style issue.

Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon.c | 10 +++++-----
 include/linux/extcon.h  |  7 ++++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index c5e9ebf..2fb5f75 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1,5 +1,5 @@
 /*
- *  drivers/extcon/extcon_class.c
+ *  drivers/extcon/extcon.c - External Connector (extcon) framework.
  *
  *  External connector (extcon) class driver
  *
@@ -19,8 +19,7 @@
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
- *
-*/
+ */
 
 #include <linux/module.h>
 #include <linux/types.h>
@@ -469,7 +468,6 @@ int extcon_register_interest(struct extcon_specific_cable_nb *obj,
 		ret = raw_notifier_chain_register(&obj->edev->nh,
 						  &obj->internal_nb);
 		spin_unlock_irqrestore(&obj->edev->lock, flags);
-		return ret;
 	} else {
 		struct class_dev_iter iter;
 		struct extcon_dev *extd;
@@ -489,8 +487,10 @@ int extcon_register_interest(struct extcon_specific_cable_nb *obj,
 						cable_name, nb);
 		}
 
-		return -ENODEV;
+		ret = -ENODEV;
 	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(extcon_register_interest);
 
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index e2cf625..799474d9d 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -97,8 +97,8 @@ struct extcon_cable;
  * @state:		Attach/detach state of this extcon. Do not provide at
  *			register-time.
  * @nh:			Notifier for the state change events from this extcon
- * @entry:		To support list of extcon devices so that users can search
- *			for extcon devices based on the extcon name.
+ * @entry:		To support list of extcon devices so that users can
+ *			search for extcon devices based on the extcon name.
  * @lock:
  * @max_supported:	Internal value to store the number of cables.
  * @extcon_dev_type:	Device_type struct to provide attribute_groups
@@ -258,7 +258,8 @@ extern int extcon_unregister_notifier(struct extcon_dev *edev,
  * Following API get the extcon device from devicetree.
  * This function use phandle of devicetree to get extcon device directly.
  */
-extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index);
+extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev,
+						     int index);
 
 /* Following API to get information of extcon device */
 extern const char *extcon_get_edev_name(struct extcon_dev *edev);
-- 
1.8.5.5

[PATCH 4/4] extcon: arizona: Remove the setting of device name

[Chanwoo Choi]

This patch removes the setting of device name. Instead, extcon_dev_register()
set the device name such as 'extcon[number]' naming method.
- /sys/class/extcon/Headset Jack -> /sys/class/extcon/extcon[number]

Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
---
 drivers/extcon/extcon-arizona.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index a0ed35b..363e6b8 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -1185,7 +1185,6 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to allocate extcon device\n");
 		return -ENOMEM;
 	}
-	info->edev->name = "Headset Jack";
 
 	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
 	if (ret < 0) {
-- 
1.8.5.5

Re: [PATCH 4/4] extcon: arizona: Remove the setting of device name

[Charles Keepax]

On Mon, Apr 27, 2015 at 09:13:38PM +0900, Chanwoo Choi wrote:
> This patch removes the setting of device name. Instead, extcon_dev_register()
> set the device name such as 'extcon[number]' naming method.
> - /sys/class/extcon/Headset Jack -> /sys/class/extcon/extcon[number]
> 
> Cc: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles