diff mbox series

[v2,2/5] extcon: Get rid of not really used name field in struct extcon_dev

Message ID 20230405152745.24959-3-andriy.shevchenko@linux.intel.com
State New
Headers
Series extcon: Core cleanups and documentation fixes |

Commit Message

Andy Shevchenko April 5, 2023, 3:27 p.m. UTC 
  The name field is always set to the parent device name and never
altered. No need to keep it inside the struct extcon_dev as we
always may derive it from the dev_name(edev->dev.parent) call.

Moreover, the parent device pointer won't ever be NULL, otherwise
we may not allocate the extcon device at all.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/extcon/extcon.c | 12 +++---------
 drivers/extcon/extcon.h |  3 ---
 2 files changed, 3 insertions(+), 12 deletions(-)

Comments

Chanwoo Choi April 6, 2023, 7:26 p.m. UTC | #1 
Hi,

On 23. 4. 6. 00:27, Andy Shevchenko wrote:
> The name field is always set to the parent device name and never
> altered. No need to keep it inside the struct extcon_dev as we
> always may derive it from the dev_name(edev->dev.parent) call.
> 
> Moreover, the parent device pointer won't ever be NULL, otherwise
> we may not allocate the extcon device at all.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/extcon/extcon.c | 12 +++---------
>  drivers/extcon/extcon.h |  3 ---
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 47819c5144d5..75a0147703c0 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -387,7 +387,7 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>  {
>  	struct extcon_dev *edev = dev_get_drvdata(dev);
>  
> -	return sysfs_emit(buf, "%s\n", edev->name);
> +	return sysfs_emit(buf, "%s\n", dev_name(edev->dev.parent));
>  }
>  static DEVICE_ATTR_RO(name);
>  
> @@ -885,7 +885,7 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  
>  	mutex_lock(&extcon_dev_list_lock);
>  	list_for_each_entry(sd, &extcon_dev_list, entry) {
> -		if (!strcmp(sd->name, extcon_name))
> +		if (device_match_name(sd->dev.parent, extcon_name))
>  			goto out;
>  	}
>  	sd = ERR_PTR(-EPROBE_DEFER);
> @@ -1269,12 +1269,6 @@ int extcon_dev_register(struct extcon_dev *edev)
>  	edev->dev.class = extcon_class;
>  	edev->dev.release = extcon_dev_release;
>  
> -	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, "extcon%lu",
>  			(unsigned long)atomic_inc_return(&edev_no));
>  
> @@ -1465,7 +1459,7 @@ EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
>   */
>  const char *extcon_get_edev_name(struct extcon_dev *edev)
>  {
> -	return !edev ? NULL : edev->name;
> +	return edev ? dev_name(edev->dev.parent) : NULL;
>  }
>  EXPORT_SYMBOL_GPL(extcon_get_edev_name);
>  
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 49e4ed9f6450..9ce7042606d7 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -6,8 +6,6 @@
>  
>  /**
>   * struct extcon_dev - An extcon device represents one external connector.
> - * @name:		The name of this extcon device. Parent device name is
> - *			used if NULL.
>   * @supported_cable:	Array of supported cable names ending with EXTCON_NONE.
>   *			If supported_cable is NULL, cable name related APIs
>   *			are disabled.
> @@ -40,7 +38,6 @@
>   */
>  struct extcon_dev {
>  	/* Optional user initializing data */
> -	const char *name;

No I don't want to remove the name even if the edev->name is equal
with the parent's name. I might reduce the readability and understaning
of the code user and I think that it is not good to use 'dev.parent' directly
at multiple point. I think that it just better to edit the wrong description
as following:


diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 15616446140d..f03d596d148f 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -6,8 +6,7 @@
 
 /**
  * struct extcon_dev - An extcon device represents one external connector.
- * @name:              The name of this extcon device. Parent device name is
- *                     used if NULL.
+ * @name:              The name of this extcon device.
  * @supported_cable:   Array of supported cable names ending with EXTCON_NONE.
  *                     If supported_cable is NULL, cable name related APIs
  *                     are disabled.
@@ -39,7 +38,6 @@
  * are overwritten by register function.
  */
 struct extcon_dev {
-       /* Optional user initializing data */
        const char *name;
        const unsigned int *supported_cable;
        const u32 *mutually_exclusive;



>  	const unsigned int *supported_cable;
>  	const u32 *mutually_exclusive;
Andy Shevchenko April 11, 2023, 11:37 a.m. UTC | #2 
On Fri, Apr 07, 2023 at 04:26:35AM +0900, Chanwoo Choi wrote:
> On 23. 4. 6. 00:27, Andy Shevchenko wrote:

...

> > -	const char *name;
> 
> No I don't want to remove the name even if the edev->name is equal
> with the parent's name. I might reduce the readability and understaning
> of the code user and I think that it is not good to use 'dev.parent' directly
> at multiple point.

Obviously I think otherwise. But you are the extcon maintainer, so I assume
that I have got NAK for these patches.
Andy Shevchenko April 11, 2023, 11:42 a.m. UTC | #3 
On Fri, Apr 07, 2023 at 04:26:35AM +0900, Chanwoo Choi wrote:
> On 23. 4. 6. 00:27, Andy Shevchenko wrote:

...

> > -	if (IS_ERR_OR_NULL(edev->name)) {
> > -		dev_err(&edev->dev,
> > -			"extcon device name is null\n");
> > -		return -EINVAL;
> > -	}

JFYI: this is a dead code in case you want to clean it up.
Since the patch is NAKed, I'm not going to split it. Hence
just for your information.
diff mbox series

Patch

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 47819c5144d5..75a0147703c0 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -387,7 +387,7 @@  static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 {
 	struct extcon_dev *edev = dev_get_drvdata(dev);
 
-	return sysfs_emit(buf, "%s\n", edev->name);
+	return sysfs_emit(buf, "%s\n", dev_name(edev->dev.parent));
 }
 static DEVICE_ATTR_RO(name);
 
@@ -885,7 +885,7 @@  struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 
 	mutex_lock(&extcon_dev_list_lock);
 	list_for_each_entry(sd, &extcon_dev_list, entry) {
-		if (!strcmp(sd->name, extcon_name))
+		if (device_match_name(sd->dev.parent, extcon_name))
 			goto out;
 	}
 	sd = ERR_PTR(-EPROBE_DEFER);
@@ -1269,12 +1269,6 @@  int extcon_dev_register(struct extcon_dev *edev)
 	edev->dev.class = extcon_class;
 	edev->dev.release = extcon_dev_release;
 
-	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, "extcon%lu",
 			(unsigned long)atomic_inc_return(&edev_no));
 
@@ -1465,7 +1459,7 @@  EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
  */
 const char *extcon_get_edev_name(struct extcon_dev *edev)
 {
-	return !edev ? NULL : edev->name;
+	return edev ? dev_name(edev->dev.parent) : NULL;
 }
 EXPORT_SYMBOL_GPL(extcon_get_edev_name);
 
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 49e4ed9f6450..9ce7042606d7 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -6,8 +6,6 @@ 
 
 /**
  * struct extcon_dev - An extcon device represents one external connector.
- * @name:		The name of this extcon device. Parent device name is
- *			used if NULL.
  * @supported_cable:	Array of supported cable names ending with EXTCON_NONE.
  *			If supported_cable is NULL, cable name related APIs
  *			are disabled.
@@ -40,7 +38,6 @@ 
  */
 struct extcon_dev {
 	/* Optional user initializing data */
-	const char *name;
 	const unsigned int *supported_cable;
 	const u32 *mutually_exclusive;