Re: [PATCH v4 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

From: Chanwoo Choi
Date: Mon Mar 13 2023 - 07:06:22 EST


On 23. 3. 2. 18:01, Bumwoo Lee wrote:
> The cable allocation part is functionalized from extcon_dev_register.
>
> Signed-off-by: Bumwoo Lee <bw365.lee@xxxxxxxxxxx>
> ---
> 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