Message-ID: <5fb2c4a3-71c1-5ccc-a928-258d31b8af8e@gmail.com>
Date:   Mon, 13 Mar 2023 20:07:36 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v4 2/4] extcon: Added extcon_alloc_cables to simplify
 extcon register function
Content-Language: en-US
From:   Chanwoo Choi <cwchoi00@gmail.com>
To:     Bumwoo Lee <bw365.lee@samsung.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        linux-kernel@vger.kernel.org
References: <20230302090143.46678-1-bw365.lee@samsung.com>
 <CGME20230302090149epcas1p2bad39de9aa367644cda3ffcb4dd4a612@epcas1p2.samsung.com>
 <20230302090143.46678-3-bw365.lee@samsung.com>
 <c2e6aae5-e980-112d-e75f-187d27c3af5d@gmail.com>
In-Reply-To: <c2e6aae5-e980-112d-e75f-187d27c3af5d@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <bw365.lee@samsung.com>
>> ---
>>  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