[PATCH v2 0/5] extcon: Core cleanups and documentation fixes

[PATCH v2 0/5] extcon: Core cleanups and documentation fixes

[Andy Shevchenko]

A few fixes and some cleanups against extcon core module.

Changelog v2:
- dropped applied patches
- completely rewrote the patch to handle name field
- dropped kasprintf_strarray() patch for now (Chanwoo)
- used new IDA APIs (Chanwoo)
- added tag (Bumwoo) to the patches that haven't changed

Cc: Chanwoo Choi <cwchoi00@gmail.com>

Note, MAINTAINERS shows what it has and hence the above Cc is manually
added. If the database has issues it should be updated, but it's out of
scope of this series.

Andy Shevchenko (5):
  extcon: Make the allocation and freeing to be private calls
  extcon: Get rid of not really used name field in struct extcon_dev
  extcon: Use unique number for the extcon device ID
  extcon: Use sizeof(*pointer) instead of sizeof(type)
  extcon: Drop unneeded assignments

 drivers/extcon/extcon.c         | 48 ++++++++++++++++-----------------
 drivers/extcon/extcon.h         |  9 ++++---
 include/linux/extcon-provider.h |  9 -------
 3 files changed, 30 insertions(+), 36 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 1/5] extcon: Make the allocation and freeing to be private calls

[Andy Shevchenko]

The extcon_dev_allocate() and extcon_dev_free() are not used
outside of the extcon framework. Moreover, the struct extcon_dev
can't be filled outside of the framework either after allocation.
The registration part, for instance, requires a parent device to
be set and that's done in the devm_extcon_dev_allocate() wrapper.

Taking the above into account, sumply move the mentioned APIs to
the private headers.

Alternatively, the pointer to the parent device can be added to
the extcon_dev_allocate(), but since there are no users and magnitude
of the change it makes a little sense to go this way.

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

diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 15616446140d..49e4ed9f6450 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -63,4 +63,8 @@ struct extcon_dev {
 	struct device_attribute *d_attrs_muex;
 };
 
+/* Following APIs allocate/free the memory of the extcon device. */
+struct extcon_dev *extcon_dev_allocate(const unsigned int *cable);
+void extcon_dev_free(struct extcon_dev *edev);
+
 #endif /* __LINUX_EXTCON_INTERNAL_H__ */
diff --git a/include/linux/extcon-provider.h b/include/linux/extcon-provider.h
index fa70945f4e6b..db474ae3c711 100644
--- a/include/linux/extcon-provider.h
+++ b/include/linux/extcon-provider.h
@@ -25,8 +25,6 @@ void devm_extcon_dev_unregister(struct device *dev,
 				struct extcon_dev *edev);
 
 /* Following APIs allocate/free the memory of the extcon device. */
-struct extcon_dev *extcon_dev_allocate(const unsigned int *cable);
-void extcon_dev_free(struct extcon_dev *edev);
 struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
 				const unsigned int *cable);
 void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
@@ -78,13 +76,6 @@ static inline int devm_extcon_dev_register(struct device *dev,
 static inline void devm_extcon_dev_unregister(struct device *dev,
 				struct extcon_dev *edev) { }
 
-static inline struct extcon_dev *extcon_dev_allocate(const unsigned int *cable)
-{
-	return ERR_PTR(-ENOSYS);
-}
-
-static inline void extcon_dev_free(struct extcon_dev *edev) { }
-
 static inline struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
 				const unsigned int *cable)
 {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 1/5] extcon: Make the allocation and freeing to be private calls

[Chanwoo Choi]

Hi,

On 23. 4. 6. 00:27, Andy Shevchenko wrote:
> The extcon_dev_allocate() and extcon_dev_free() are not used
> outside of the extcon framework. Moreover, the struct extcon_dev
> can't be filled outside of the framework either after allocation.
> The registration part, for instance, requires a parent device to
> be set and that's done in the devm_extcon_dev_allocate() wrapper.
> 
> Taking the above into account, sumply move the mentioned APIs to
> the private headers.
> 
> Alternatively, the pointer to the parent device can be added to
> the extcon_dev_allocate(), but since there are no users and magnitude
> of the change it makes a little sense to go this way.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The role of extcon_dev_allocate is not that supporting to touch
the internal data of struct extcon_dev. Some driver needs
the removing sequence between extcon device and their used resource
like irq/opp/regulator or others.

When they need some sequence of resource freeing, it should be supported.
I think that there is no benefit to move into driver/extcon. It might
limit the various use-case of extcon_dev_allocate.


> ---
>  drivers/extcon/extcon.h         | 4 ++++
>  include/linux/extcon-provider.h | 9 ---------
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 15616446140d..49e4ed9f6450 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -63,4 +63,8 @@ struct extcon_dev {
>  	struct device_attribute *d_attrs_muex;
>  };
>  
> +/* Following APIs allocate/free the memory of the extcon device. */
> +struct extcon_dev *extcon_dev_allocate(const unsigned int *cable);
> +void extcon_dev_free(struct extcon_dev *edev);
> +
>  #endif /* __LINUX_EXTCON_INTERNAL_H__ */
> diff --git a/include/linux/extcon-provider.h b/include/linux/extcon-provider.h
> index fa70945f4e6b..db474ae3c711 100644
> --- a/include/linux/extcon-provider.h
> +++ b/include/linux/extcon-provider.h
> @@ -25,8 +25,6 @@ void devm_extcon_dev_unregister(struct device *dev,
>  				struct extcon_dev *edev);
>  
>  /* Following APIs allocate/free the memory of the extcon device. */
> -struct extcon_dev *extcon_dev_allocate(const unsigned int *cable);
> -void extcon_dev_free(struct extcon_dev *edev);
>  struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
>  				const unsigned int *cable);
>  void devm_extcon_dev_free(struct device *dev, struct extcon_dev *edev);
> @@ -78,13 +76,6 @@ static inline int devm_extcon_dev_register(struct device *dev,
>  static inline void devm_extcon_dev_unregister(struct device *dev,
>  				struct extcon_dev *edev) { }
>  
> -static inline struct extcon_dev *extcon_dev_allocate(const unsigned int *cable)
> -{
> -	return ERR_PTR(-ENOSYS);
> -}
> -
> -static inline void extcon_dev_free(struct extcon_dev *edev) { }
> -
>  static inline struct extcon_dev *devm_extcon_dev_allocate(struct device *dev,
>  				const unsigned int *cable)
>  {

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

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

[Andy Shevchenko]

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;
 	const unsigned int *supported_cable;
 	const u32 *mutually_exclusive;
 
-- 
2.40.0.1.gaa8946217a0b

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

[Chanwoo Choi]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/5] extcon: Use unique number for the extcon device ID

[Andy Shevchenko]

The use of atomic variable is still racy when we do not control which
device has been unregistered and there is a (theoretical) possibility
of the overflow that may cause a duplicate extcon device ID number
to be allocated next time a device is registered.

Replace above mentioned approach by using IDA framework.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 15 ++++++++++++---
 drivers/extcon/extcon.h |  2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 75a0147703c0..daaded92cf80 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -16,6 +16,7 @@
 
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/fs.h>
@@ -238,6 +239,7 @@ struct extcon_cable {
 
 static struct class *extcon_class;
 
+static DEFINE_IDA(extcon_dev_ids);
 static LIST_HEAD(extcon_dev_list);
 static DEFINE_MUTEX(extcon_dev_list_lock);
 
@@ -1248,7 +1250,6 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
 int extcon_dev_register(struct extcon_dev *edev)
 {
 	int ret, index = 0;
-	static atomic_t edev_no = ATOMIC_INIT(-1);
 
 	ret = create_extcon_class();
 	if (ret < 0)
@@ -1269,8 +1270,13 @@ int extcon_dev_register(struct extcon_dev *edev)
 	edev->dev.class = extcon_class;
 	edev->dev.release = extcon_dev_release;
 
-	dev_set_name(&edev->dev, "extcon%lu",
-			(unsigned long)atomic_inc_return(&edev_no));
+	ret = ida_alloc(&extcon_dev_ids, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	edev->id = ret;
+
+	dev_set_name(&edev->dev, "extcon%d", edev->id);
 
 	ret = extcon_alloc_cables(edev);
 	if (ret < 0)
@@ -1333,6 +1339,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 	if (edev->max_supported)
 		kfree(edev->cables);
 err_alloc_cables:
+	ida_free(&extcon_dev_ids, edev->id);
 
 	return ret;
 }
@@ -1361,6 +1368,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
 		return;
 	}
 
+	ida_free(&extcon_dev_ids, edev->id);
+
 	device_unregister(&edev->dev);
 
 	if (edev->mutually_exclusive && edev->max_supported) {
diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
index 9ce7042606d7..5744c325e226 100644
--- a/drivers/extcon/extcon.h
+++ b/drivers/extcon/extcon.h
@@ -18,6 +18,7 @@
  *			{0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
  *			can be no simultaneous connections.
  * @dev:		Device of this extcon.
+ * @id:			Unique device ID of this extcon.
  * @state:		Attach/detach state of this extcon. Do not provide at
  *			register-time.
  * @nh_all:		Notifier for the state change events for all supported
@@ -43,6 +44,7 @@ struct extcon_dev {
 
 	/* Internal data. Please do not set. */
 	struct device dev;
+	unsigned int id;
 	struct raw_notifier_head nh_all;
 	struct raw_notifier_head *nh;
 	struct list_head entry;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 4/5] extcon: Use sizeof(*pointer) instead of sizeof(type)

[Andy Shevchenko]

It is preferred to use sizeof(*pointer) instead of sizeof(type).
The type of the variable can change and one needs not change
the former (unlike the latter). No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index daaded92cf80..50c5fd454488 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1098,8 +1098,7 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
 	if (!edev->max_supported)
 		return 0;
 
-	edev->cables = kcalloc(edev->max_supported,
-			       sizeof(struct extcon_cable),
+	edev->cables = kcalloc(edev->max_supported, sizeof(*edev->cables),
 			       GFP_KERNEL);
 	if (!edev->cables)
 		return -ENOMEM;
@@ -1161,14 +1160,12 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
 	for (index = 0; edev->mutually_exclusive[index]; index++)
 		;
 
-	edev->attrs_muex = kcalloc(index + 1,
-				   sizeof(struct attribute *),
+	edev->attrs_muex = kcalloc(index + 1, sizeof(*edev->attrs_muex),
 				   GFP_KERNEL);
 	if (!edev->attrs_muex)
 		return -ENOMEM;
 
-	edev->d_attrs_muex = kcalloc(index,
-				     sizeof(struct device_attribute),
+	edev->d_attrs_muex = kcalloc(index, sizeof(*edev->d_attrs_muex),
 				     GFP_KERNEL);
 	if (!edev->d_attrs_muex) {
 		kfree(edev->attrs_muex);
@@ -1214,8 +1211,8 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
 		return 0;
 
 	edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
-			sizeof(struct attribute_group *),
-			GFP_KERNEL);
+					  sizeof(*edev->extcon_dev_type.groups),
+					  GFP_KERNEL);
 	if (!edev->extcon_dev_type.groups)
 		return -ENOMEM;
 
@@ -1293,7 +1290,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 	spin_lock_init(&edev->lock);
 	if (edev->max_supported) {
 		edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh),
-				GFP_KERNEL);
+				   GFP_KERNEL);
 		if (!edev->nh) {
 			ret = -ENOMEM;
 			goto err_alloc_nh;
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 4/5] extcon: Use sizeof(*pointer) instead of sizeof(type)

[Chanwoo Choi]

On 23. 4. 6. 00:27, Andy Shevchenko wrote:
> It is preferred to use sizeof(*pointer) instead of sizeof(type).
> The type of the variable can change and one needs not change
> the former (unlike the latter). No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Bumwoo Lee <bw365.lee@samsung.com>
> ---
>  drivers/extcon/extcon.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index daaded92cf80..50c5fd454488 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1098,8 +1098,7 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
>  	if (!edev->max_supported)
>  		return 0;
>  
> -	edev->cables = kcalloc(edev->max_supported,
> -			       sizeof(struct extcon_cable),
> +	edev->cables = kcalloc(edev->max_supported, sizeof(*edev->cables),
>  			       GFP_KERNEL);
>  	if (!edev->cables)
>  		return -ENOMEM;
> @@ -1161,14 +1160,12 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
>  	for (index = 0; edev->mutually_exclusive[index]; index++)
>  		;
>  
> -	edev->attrs_muex = kcalloc(index + 1,
> -				   sizeof(struct attribute *),
> +	edev->attrs_muex = kcalloc(index + 1, sizeof(*edev->attrs_muex),
>  				   GFP_KERNEL);
>  	if (!edev->attrs_muex)
>  		return -ENOMEM;
>  
> -	edev->d_attrs_muex = kcalloc(index,
> -				     sizeof(struct device_attribute),
> +	edev->d_attrs_muex = kcalloc(index, sizeof(*edev->d_attrs_muex),
>  				     GFP_KERNEL);
>  	if (!edev->d_attrs_muex) {
>  		kfree(edev->attrs_muex);
> @@ -1214,8 +1211,8 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
>  		return 0;
>  
>  	edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
> -			sizeof(struct attribute_group *),
> -			GFP_KERNEL);
> +					  sizeof(*edev->extcon_dev_type.groups),
> +					  GFP_KERNEL);
>  	if (!edev->extcon_dev_type.groups)
>  		return -ENOMEM;
>  
> @@ -1293,7 +1290,7 @@ int extcon_dev_register(struct extcon_dev *edev)
>  	spin_lock_init(&edev->lock);
>  	if (edev->max_supported) {
>  		edev->nh = kcalloc(edev->max_supported, sizeof(*edev->nh),
> -				GFP_KERNEL);
> +				   GFP_KERNEL);

Looks good to me. 

But, actually, this line change is not related to role of this patch.
If possible, we have to keep the only one role on one patch. Don't touch it.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

[PATCH v2 5/5] extcon: Drop unneeded assignments

[Andy Shevchenko]

In one case the assignment is duplicative, in the other,
it's better to move it into the loop — the user of it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 50c5fd454488..88ce0656d23c 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -245,7 +245,7 @@ static DEFINE_MUTEX(extcon_dev_list_lock);
 
 static int check_mutually_exclusive(struct extcon_dev *edev, u32 new_state)
 {
-	int i = 0;
+	int i;
 
 	if (!edev->mutually_exclusive)
 		return 0;
@@ -1246,7 +1246,7 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
  */
 int extcon_dev_register(struct extcon_dev *edev)
 {
-	int ret, index = 0;
+	int ret, index;
 
 	ret = create_extcon_class();
 	if (ret < 0)
@@ -1255,7 +1255,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 	if (!edev || !edev->supported_cable)
 		return -EINVAL;
 
-	for (; edev->supported_cable[index] != EXTCON_NONE; index++);
+	for (index = 0; edev->supported_cable[index] != EXTCON_NONE; index++);
 
 	edev->max_supported = index;
 	if (index > SUPPORTED_CABLE_MAX) {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 5/5] extcon: Drop unneeded assignments

[Chanwoo Choi]

On 23. 4. 6. 00:27, Andy Shevchenko wrote:
> In one case the assignment is duplicative, in the other,
> it's better to move it into the loop — the user of it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Bumwoo Lee <bw365.lee@samsung.com>
> ---
>  drivers/extcon/extcon.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 50c5fd454488..88ce0656d23c 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -245,7 +245,7 @@ static DEFINE_MUTEX(extcon_dev_list_lock);
>  
>  static int check_mutually_exclusive(struct extcon_dev *edev, u32 new_state)
>  {
> -	int i = 0;
> +	int i;
>  
>  	if (!edev->mutually_exclusive)
>  		return 0;
> @@ -1246,7 +1246,7 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
>   */
>  int extcon_dev_register(struct extcon_dev *edev)
>  {
> -	int ret, index = 0;
> +	int ret, index;
>  
>  	ret = create_extcon_class();
>  	if (ret < 0)
> @@ -1255,7 +1255,7 @@ int extcon_dev_register(struct extcon_dev *edev)
>  	if (!edev || !edev->supported_cable)
>  		return -EINVAL;
>  
> -	for (; edev->supported_cable[index] != EXTCON_NONE; index++);
> +	for (index = 0; edev->supported_cable[index] != EXTCON_NONE; index++);
>  
>  	edev->max_supported = index;
>  	if (index > SUPPORTED_CABLE_MAX) {


On previous version, I already replied the my ack.
But this version doesn't contain my ack tag.

So that 
Acked-by: Chanwoo Choi <cw00.choi@samsung.com> again.

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v2 5/5] extcon: Drop unneeded assignments

[Andy Shevchenko]

On Fri, Apr 07, 2023 at 04:35:53AM +0900, Chanwoo Choi wrote:
> On 23. 4. 6. 00:27, Andy Shevchenko wrote:

...

> But this version doesn't contain my ack tag.

Oh, sorry, I somehow missed that.

> So that 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com> again.

Thank you and thanks for the review, I'll issue a v3 shortly.

-- 
With Best Regards,
Andy Shevchenko