It was modified to increase readability.
Chages from v3:
removed possibility of kfree(NULL).
Chages from v2:
resolved possible memory leak of dev->cables.
Changes from v1:
added return value handling.
Bumwoo Lee (4):
extcon: Removed 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 | 293 +++++++++++++++++++++++-----------------
1 file changed, 166 insertions(+), 127 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 | 58 +++++++++++++++++++++++++----------------
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 321988231806..c366a7988daf 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1181,6 +1181,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
@@ -1236,28 +1269,9 @@ int extcon_dev_register(struct extcon_dev *edev)
if (ret < 0)
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;
- }
+ ret = extcon_alloc_groups(edev);
+ if (ret < 0)
+ 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 | 106 ++++++++++++++++++++++------------------
1 file changed, 58 insertions(+), 48 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 49605e96bedd..321988231806 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1127,6 +1127,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
@@ -1178,53 +1232,9 @@ int extcon_dev_register(struct extcon_dev *edev)
if (ret < 0)
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;
-
- }
+ ret = extcon_alloc_muex(edev);
+ if (ret < 0)
+ goto err_alloc_muex;
if (edev->max_supported) {
edev->extcon_dev_type.groups =
@@ -1292,7 +1302,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);
if (edev->max_supported)
--
2.35.1
The cable allocation part is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
drivers/extcon/extcon.c | 108 +++++++++++++++++++++++-----------------
1 file changed, 62 insertions(+), 46 deletions(-)
diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index adcf01132f70..49605e96bedd 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1070,6 +1070,63 @@ 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);
+ }
+
+ kfree(edev->cables);
+ 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 +1174,9 @@ 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;
- }
- }
+ ret = extcon_alloc_cables(edev);
+ if (ret < 0)
+ goto err_alloc_cables;
if (edev->max_supported && edev->mutually_exclusive) {
char *name;
@@ -1279,10 +1295,10 @@ int extcon_dev_register(struct extcon_dev *edev)
err_muex:
for (index = 0; index < edev->max_supported; index++)
kfree(edev->cables[index].attr_g.name);
-err_alloc_cables:
if (edev->max_supported)
kfree(edev->cables);
-err_sysfs_alloc:
+err_alloc_cables:
+
return ret;
}
EXPORT_SYMBOL_GPL(extcon_dev_register);
--
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);
>
Acked-by: MyungJoo Ham <[email protected]>
Chanwoo, please take a look at this series.
On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> 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;
Applied it with following edited title:
On later, please write the patch title in command form.
extcon: Remove redundant null checking for class
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> The cable allocation part is functionalized from extcon_dev_register.
>
> Signed-off-by: Bumwoo Lee <[email protected]>
> ---
> drivers/extcon/extcon.c | 108 +++++++++++++++++++++++-----------------
> 1 file changed, 62 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index adcf01132f70..49605e96bedd 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1070,6 +1070,63 @@ 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;
Need to check whether edev is NULL or not
because make the separate function.
if (!edev && !edev->max_supported)
> +
> + 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);
> + }
> +
> + kfree(edev->cables);
> + 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 +1174,9 @@ 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;
> - }
> - }
> + ret = extcon_alloc_cables(edev);
> + if (ret < 0)
> + goto err_alloc_cables;
>
> if (edev->max_supported && edev->mutually_exclusive) {
> char *name;
> @@ -1279,10 +1295,10 @@ int extcon_dev_register(struct extcon_dev *edev)
> err_muex:
> for (index = 0; index < edev->max_supported; index++)
> kfree(edev->cables[index].attr_g.name);
> -err_alloc_cables:
> if (edev->max_supported)
> kfree(edev->cables);
> -err_sysfs_alloc:
> +err_alloc_cables:
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(extcon_dev_register);
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 13. 20:06, Chanwoo Choi wrote:
> On 23. 3. 2. 18:01, Bumwoo Lee wrote:
>> The cable allocation part is functionalized from extcon_dev_register.
>>
>> Signed-off-by: Bumwoo Lee <[email protected]>
>> ---
>> drivers/extcon/extcon.c | 108 +++++++++++++++++++++++-----------------
>> 1 file changed, 62 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index adcf01132f70..49605e96bedd 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -1070,6 +1070,63 @@ 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;
>
>
> Need to check whether edev is NULL or not
> because make the separate function.
>
> if (!edev && !edev->max_supported)
Instead of previous my comment, need to check it as following:
if (!edev)
return -EINVAL;
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);
>> + }
>> +
>> + kfree(edev->cables);
>> + 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 +1174,9 @@ 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;
>> - }
>> - }
>> + ret = extcon_alloc_cables(edev);
>> + if (ret < 0)
>> + goto err_alloc_cables;
>>
>> if (edev->max_supported && edev->mutually_exclusive) {
>> char *name;
>> @@ -1279,10 +1295,10 @@ int extcon_dev_register(struct extcon_dev *edev)
>> err_muex:
>> for (index = 0; index < edev->max_supported; index++)
>> kfree(edev->cables[index].attr_g.name);
>> -err_alloc_cables:
>> if (edev->max_supported)
>> kfree(edev->cables);
>> -err_sysfs_alloc:
>> +err_alloc_cables:
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(extcon_dev_register);
>
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> The mutual exclusive part is functionalized from extcon_dev_register.
>
> Signed-off-by: Bumwoo Lee <[email protected]>
> ---
> drivers/extcon/extcon.c | 106 ++++++++++++++++++++++------------------
> 1 file changed, 58 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 49605e96bedd..321988231806 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1127,6 +1127,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;
> +
ditto.
if (!edev)
return -EINVAL;
> + 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
> @@ -1178,53 +1232,9 @@ int extcon_dev_register(struct extcon_dev *edev)
> if (ret < 0)
> 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;
> -
> - }
> + ret = extcon_alloc_muex(edev);
> + if (ret < 0)
> + goto err_alloc_muex;
>
> if (edev->max_supported) {
> edev->extcon_dev_type.groups =
> @@ -1292,7 +1302,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);
> if (edev->max_supported)
--
Best Regards,
Samsung Electronics
Chanwoo Choi
On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> The alloc groups is functionalized from extcon_dev_register.
>
> Signed-off-by: Bumwoo Lee <[email protected]>
> ---
> drivers/extcon/extcon.c | 58 +++++++++++++++++++++++++----------------
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 321988231806..c366a7988daf 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1181,6 +1181,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;
> +
ditto.
if (!edev)
return -EINVAL;
> + 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
> @@ -1236,28 +1269,9 @@ int extcon_dev_register(struct extcon_dev *edev)
> if (ret < 0)
> 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;
> - }
> + ret = extcon_alloc_groups(edev);
> + if (ret < 0)
> + goto err_alloc_groups;
>
> spin_lock_init(&edev->lock);
> if (edev->max_supported) {
--
Best Regards,
Samsung Electronics
Chanwoo Choi
> -----Original Message-----
> From: Chanwoo Choi <[email protected]>
> Sent: Monday, March 13, 2023 8:08 PM
> To: Bumwoo Lee <[email protected]>; MyungJoo Ham
> <[email protected]>; Chanwoo Choi <[email protected]>; linux-
> [email protected]
> Subject: Re: [PATCH v4 4/4] extcon: Added extcon_alloc_groups to simplify
> extcon register function
>
> On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> > The alloc groups is functionalized from extcon_dev_register.
> >
> > Signed-off-by: Bumwoo Lee <[email protected]>
> > ---
> > drivers/extcon/extcon.c | 58
> > +++++++++++++++++++++++++----------------
> > 1 file changed, 36 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index
> > 321988231806..c366a7988daf 100644
> > --- a/drivers/extcon/extcon.c
> > +++ b/drivers/extcon/extcon.c
> > @@ -1181,6 +1181,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;
> > +
>
> ditto.
>
> if (!edev)
> return -EINVAL;
>
> > + 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
> > @@ -1236,28 +1269,9 @@ int extcon_dev_register(struct extcon_dev *edev)
> > if (ret < 0)
> > 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;
> > - }
> > + ret = extcon_alloc_groups(edev);
> > + if (ret < 0)
> > + goto err_alloc_groups;
> >
> > spin_lock_init(&edev->lock);
> > if (edev->max_supported) {
>
> --
Thank you for the review and I will send a new patch including what you
mentioned soon
Thanks,
> Best Regards,
> Samsung Electronics
> Chanwoo Choi