Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4632ce69-2a94-0c33-9a76-b97596436e24@gmail.com>
Date:   Fri, 7 Apr 2023 04:26:35 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2 2/5] extcon: Get rid of not really used name field in
 struct extcon_dev
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        linux-kernel@vger.kernel.org
Cc:     MyungJoo Ham <myungjoo.ham@samsung.com>
References: <20230405152745.24959-1-andriy.shevchenko@linux.intel.com>
 <20230405152745.24959-3-andriy.shevchenko@linux.intel.com>
From:   Chanwoo Choi <cwchoi00@gmail.com>
In-Reply-To: <20230405152745.24959-3-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi