It was modified to increase readability.
Bumwoo Lee (4):
extcon: remove redundant null checking for class
extcon: added extcon_alloc_cables to simplify extcon register function
extcon: added extcon_alloc_muex to simplify extcon register function
extcon: added extcon_alloc_groups to simplify extcon register function
drivers/extcon/extcon.c | 286 ++++++++++++++++++++++------------------
1 file changed, 160 insertions(+), 126 deletions(-)
--
2.35.1
Create_extcon_class() is already Null checking.
Signed-off-by: Bumwoo Lee <[email protected]>
---
drivers/extcon/extcon.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index e1c71359b605..adcf01132f70 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1012,12 +1012,13 @@ ATTRIBUTE_GROUPS(extcon);
static int create_extcon_class(void)
{
- if (!extcon_class) {
- extcon_class = class_create(THIS_MODULE, "extcon");
- if (IS_ERR(extcon_class))
- return PTR_ERR(extcon_class);
- extcon_class->dev_groups = extcon_groups;
- }
+ if (extcon_class)
+ return 0;
+
+ extcon_class = class_create(THIS_MODULE, "extcon");
+ if (IS_ERR(extcon_class))
+ return PTR_ERR(extcon_class);
+ extcon_class->dev_groups = extcon_groups;
return 0;
}
@@ -1088,11 +1089,9 @@ 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();
- if (ret < 0)
- return ret;
- }
+ ret = create_extcon_class();
+ if (ret < 0)
+ return ret;
if (!edev || !edev->supported_cable)
return -EINVAL;
--
2.35.1
The alloc groups is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
drivers/extcon/extcon.c | 57 +++++++++++++++++++++++++----------------
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 93cab4fe178e..9336d0d7589a 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1179,6 +1179,39 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
return 0;
}
+/**
+ * extcon_alloc_groups() - alloc the groups for extcon device
+ * @edev: extcon device
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_groups(struct extcon_dev *edev)
+{
+ int index;
+
+ if (!edev->max_supported)
+ return 0;
+
+ edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
+ sizeof(struct attribute_group *),
+ GFP_KERNEL);
+ if (!edev->extcon_dev_type.groups)
+ return -ENOMEM;
+
+ edev->extcon_dev_type.name = dev_name(&edev->dev);
+ edev->extcon_dev_type.release = dummy_sysfs_dev_release;
+
+ for (index = 0; index < edev->max_supported; index++)
+ edev->extcon_dev_type.groups[index] = &edev->cables[index].attr_g;
+
+ if (edev->mutually_exclusive)
+ edev->extcon_dev_type.groups[index] = &edev->attr_g_muex;
+
+ edev->dev.type = &edev->extcon_dev_type;
+
+ return 0;
+}
+
/**
* extcon_dev_register() - Register an new extcon device
* @edev: the extcon device to be registered
@@ -1232,28 +1265,8 @@ int extcon_dev_register(struct extcon_dev *edev)
if (extcon_alloc_muex(edev))
goto err_alloc_muex;
- if (edev->max_supported) {
- edev->extcon_dev_type.groups =
- kcalloc(edev->max_supported + 2,
- sizeof(struct attribute_group *),
- GFP_KERNEL);
- if (!edev->extcon_dev_type.groups) {
- ret = -ENOMEM;
- goto err_alloc_groups;
- }
-
- edev->extcon_dev_type.name = dev_name(&edev->dev);
- edev->extcon_dev_type.release = dummy_sysfs_dev_release;
-
- for (index = 0; index < edev->max_supported; index++)
- edev->extcon_dev_type.groups[index] =
- &edev->cables[index].attr_g;
- if (edev->mutually_exclusive)
- edev->extcon_dev_type.groups[index] =
- &edev->attr_g_muex;
-
- edev->dev.type = &edev->extcon_dev_type;
- }
+ if (extcon_alloc_groups(edev))
+ goto err_alloc_groups;
spin_lock_init(&edev->lock);
if (edev->max_supported) {
--
2.35.1
The mutual exclusive part is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
drivers/extcon/extcon.c | 105 ++++++++++++++++++++++------------------
1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 28058a1d2745..93cab4fe178e 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1125,6 +1125,60 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
return 0;
}
+/**
+ * extcon_alloc_muex() - alloc the mutual exclusive for extcon device
+ * @edev: extcon device
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_muex(struct extcon_dev *edev)
+{
+ char *name;
+ int index;
+
+ if (!(edev->max_supported && edev->mutually_exclusive))
+ return 0;
+
+ /* Count the size of mutually_exclusive array */
+ for (index = 0; edev->mutually_exclusive[index]; index++)
+ ;
+
+ edev->attrs_muex = kcalloc(index + 1,
+ sizeof(struct attribute *),
+ GFP_KERNEL);
+ if (!edev->attrs_muex)
+ return -ENOMEM;
+
+ edev->d_attrs_muex = kcalloc(index,
+ sizeof(struct device_attribute),
+ GFP_KERNEL);
+ if (!edev->d_attrs_muex) {
+ kfree(edev->attrs_muex);
+ return -ENOMEM;
+ }
+
+ for (index = 0; edev->mutually_exclusive[index]; index++) {
+ name = kasprintf(GFP_KERNEL, "0x%x",
+ edev->mutually_exclusive[index]);
+ if (!name) {
+ for (index--; index >= 0; index--)
+ kfree(edev->d_attrs_muex[index].attr.name);
+
+ kfree(edev->d_attrs_muex);
+ kfree(edev->attrs_muex);
+ return -ENOMEM;
+ }
+ sysfs_attr_init(&edev->d_attrs_muex[index].attr);
+ edev->d_attrs_muex[index].attr.name = name;
+ edev->d_attrs_muex[index].attr.mode = 0000;
+ edev->attrs_muex[index] = &edev->d_attrs_muex[index].attr;
+ }
+ edev->attr_g_muex.name = muex_name;
+ edev->attr_g_muex.attrs = edev->attrs_muex;
+
+ return 0;
+}
+
/**
* extcon_dev_register() - Register an new extcon device
* @edev: the extcon device to be registered
@@ -1175,53 +1229,8 @@ int extcon_dev_register(struct extcon_dev *edev)
if (extcon_alloc_cables(edev))
goto err_alloc_cables;
- if (edev->max_supported && edev->mutually_exclusive) {
- char *name;
-
- /* Count the size of mutually_exclusive array */
- for (index = 0; edev->mutually_exclusive[index]; index++)
- ;
-
- edev->attrs_muex = kcalloc(index + 1,
- sizeof(struct attribute *),
- GFP_KERNEL);
- if (!edev->attrs_muex) {
- ret = -ENOMEM;
- goto err_muex;
- }
-
- edev->d_attrs_muex = kcalloc(index,
- sizeof(struct device_attribute),
- GFP_KERNEL);
- if (!edev->d_attrs_muex) {
- ret = -ENOMEM;
- kfree(edev->attrs_muex);
- goto err_muex;
- }
-
- for (index = 0; edev->mutually_exclusive[index]; index++) {
- name = kasprintf(GFP_KERNEL, "0x%x",
- edev->mutually_exclusive[index]);
- if (!name) {
- for (index--; index >= 0; index--) {
- kfree(edev->d_attrs_muex[index].attr.
- name);
- }
- kfree(edev->d_attrs_muex);
- kfree(edev->attrs_muex);
- ret = -ENOMEM;
- goto err_muex;
- }
- sysfs_attr_init(&edev->d_attrs_muex[index].attr);
- edev->d_attrs_muex[index].attr.name = name;
- edev->d_attrs_muex[index].attr.mode = 0000;
- edev->attrs_muex[index] = &edev->d_attrs_muex[index]
- .attr;
- }
- edev->attr_g_muex.name = muex_name;
- edev->attr_g_muex.attrs = edev->attrs_muex;
-
- }
+ if (extcon_alloc_muex(edev))
+ goto err_alloc_muex;
if (edev->max_supported) {
edev->extcon_dev_type.groups =
@@ -1289,7 +1298,7 @@ int extcon_dev_register(struct extcon_dev *edev)
kfree(edev->d_attrs_muex);
kfree(edev->attrs_muex);
}
-err_muex:
+err_alloc_muex:
for (index = 0; index < edev->max_supported; index++)
kfree(edev->cables[index].attr_g.name);
err_alloc_cables:
--
2.35.1
The cable allocation part is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
drivers/extcon/extcon.c | 103 ++++++++++++++++++++++------------------
1 file changed, 58 insertions(+), 45 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index adcf01132f70..28058a1d2745 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)
}
EXPORT_SYMBOL_GPL(extcon_dev_free);
+/**
+ * extcon_alloc_cables() - alloc the cables for extcon device
+ * @edev: extcon device which has cables
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_cables(struct extcon_dev *edev)
+{
+ int index;
+ char *str;
+ struct extcon_cable *cable;
+
+ if (!edev->max_supported)
+ return 0;
+
+ edev->cables = kcalloc(edev->max_supported,
+ sizeof(struct extcon_cable),
+ GFP_KERNEL);
+ if (!edev->cables)
+ return -ENOMEM;
+
+ for (index = 0; index < edev->max_supported; index++) {
+ cable = &edev->cables[index];
+
+ str = kasprintf(GFP_KERNEL, "cable.%d", index);
+ if (!str) {
+ for (index--; index >= 0; index--) {
+ cable = &edev->cables[index];
+ kfree(cable->attr_g.name);
+ }
+ return -ENOMEM;
+ }
+
+ cable->edev = edev;
+ cable->cable_index = index;
+ cable->attrs[0] = &cable->attr_name.attr;
+ cable->attrs[1] = &cable->attr_state.attr;
+ cable->attrs[2] = NULL;
+ cable->attr_g.name = str;
+ cable->attr_g.attrs = cable->attrs;
+
+ sysfs_attr_init(&cable->attr_name.attr);
+ cable->attr_name.attr.name = "name";
+ cable->attr_name.attr.mode = 0444;
+ cable->attr_name.show = cable_name_show;
+
+ sysfs_attr_init(&cable->attr_state.attr);
+ cable->attr_state.attr.name = "state";
+ cable->attr_state.attr.mode = 0444;
+ cable->attr_state.show = cable_state_show;
+ }
+
+ return 0;
+}
+
/**
* extcon_dev_register() - Register an new extcon device
* @edev: the extcon device to be registered
@@ -1117,50 +1172,8 @@ int extcon_dev_register(struct extcon_dev *edev)
dev_set_name(&edev->dev, "extcon%lu",
(unsigned long)atomic_inc_return(&edev_no));
- if (edev->max_supported) {
- char *str;
- struct extcon_cable *cable;
-
- edev->cables = kcalloc(edev->max_supported,
- sizeof(struct extcon_cable),
- GFP_KERNEL);
- if (!edev->cables) {
- ret = -ENOMEM;
- goto err_sysfs_alloc;
- }
- for (index = 0; index < edev->max_supported; index++) {
- cable = &edev->cables[index];
-
- str = kasprintf(GFP_KERNEL, "cable.%d", index);
- if (!str) {
- for (index--; index >= 0; index--) {
- cable = &edev->cables[index];
- kfree(cable->attr_g.name);
- }
- ret = -ENOMEM;
-
- goto err_alloc_cables;
- }
-
- cable->edev = edev;
- cable->cable_index = index;
- cable->attrs[0] = &cable->attr_name.attr;
- cable->attrs[1] = &cable->attr_state.attr;
- cable->attrs[2] = NULL;
- cable->attr_g.name = str;
- cable->attr_g.attrs = cable->attrs;
-
- sysfs_attr_init(&cable->attr_name.attr);
- cable->attr_name.attr.name = "name";
- cable->attr_name.attr.mode = 0444;
- cable->attr_name.show = cable_name_show;
-
- sysfs_attr_init(&cable->attr_state.attr);
- cable->attr_state.attr.name = "state";
- cable->attr_state.attr.mode = 0444;
- cable->attr_state.show = cable_state_show;
- }
- }
+ if (extcon_alloc_cables(edev))
+ goto err_alloc_cables;
if (edev->max_supported && edev->mutually_exclusive) {
char *name;
@@ -1282,7 +1295,7 @@ int extcon_dev_register(struct extcon_dev *edev)
err_alloc_cables:
if (edev->max_supported)
kfree(edev->cables);
-err_sysfs_alloc:
+
return ret;
}
EXPORT_SYMBOL_GPL(extcon_dev_register);
--
2.35.1
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>Product S/W Lab(VD)/삼성전자
>Date : 2023-02-17 17:48 (GMT+9)
>Title : [PATCH 3/4] extcon: added extcon_alloc_muex to simplify extcon register function
>
>The mutual exclusive part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <[email protected]>
>---
> drivers/extcon/extcon.c | 105 ++++++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 48 deletions(-)
>
>diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>index 28058a1d2745..93cab4fe178e 100644
>--- a/drivers/extcon/extcon.c
>+++ b/drivers/extcon/extcon.c
>@@ -1125,6 +1125,60 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
> return 0;
> }
>
...
>- if (!edev->attrs_muex) {
>- ret = -ENOMEM;
>- goto err_muex;
>- }
...
>+ if (extcon_alloc_muex(edev))
>+ goto err_alloc_muex;
It's not good.
You may return an uninitialized value or 0 when your new function has an error.
Cheers,
MyungJoo
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>Product S/W Lab(VD)/삼성전자
>Date : 2023-02-17 17:48 (GMT+9)
>Title : [PATCH 2/4] extcon: added extcon_alloc_cables to simplify extcon register function
>
>The cable allocation part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <[email protected]>
...
>+ if (extcon_alloc_cables(edev))
>+ goto err_alloc_cables;
>
You are abandoning error return values, here.
Please return appropriate error codes.
Cheers,
MyungJoo
I Got it.
I will prepare new patch for return value.
Thank for your comment.
Regards,
Bumwoo.
-----Original Message-----
From: MyungJoo Ham <[email protected]>
Sent: Monday, February 20, 2023 1:39 PM
To: Bumwoo Lee <[email protected]>; Chanwoo Choi <[email protected]>; [email protected]
Subject: RE: [PATCH 2/4] extcon: added extcon_alloc_cables to simplify extcon register function
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>Product S/W Lab(VD)/삼성전자 Date :
>2023-02-17 17:48 (GMT+9) Title : [PATCH 2/4] extcon: added
>extcon_alloc_cables to simplify extcon register function
>
>The cable allocation part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <[email protected]>
...
>+ if (extcon_alloc_cables(edev))
>+ goto err_alloc_cables;
>
You are abandoning error return values, here.
Please return appropriate error codes.
Cheers,
MyungJoo