Re: [PATCH v3 4/4] devfreq: exynos-bus: Clean up code

[Chanwoo Choi]

Hi,

On 12/11/19 11:39 PM, Artur ÅwigoÅ wrote:
> Hi,
> 
> On Tue, 2019-12-10 at 13:20 +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> This patch contains the clean-up code related to 'goto' style.
>> Please merge the the clean-up code of 'goto' to one patch with patch3/patch4.
>> - patch3 related to 'goto' clean-up code
>> - patch4 related to remaining clean-up code. 
>>
>> And I added the comment below. Please check them.
> 
> OK, I can merge these patches. Please also see my comments below regarding
> the issues you highlighted: kzalloc vs. kcalloc, fitting in 80 columns and
> changing repeated expressions to variables.
> 
>>
>> On 12/9/19 7:49 PM, Artur ÅwigoÅ wrote:
>>> This patch adds minor improvements to the exynos-bus driver, including
>>> cleaning up header includes, variables, and return paths.
>>>
>>> Signed-off-by: Artur ÅwigoÅ <a.swigon@xxxxxxxxxxx>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 56 +++++++++++++++---------------------
>>>  1 file changed, 23 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 0b557df63666..3eb6a043284a 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -15,11 +15,10 @@
>>>  #include <linux/device.h>
>>>  #include <linux/export.h>
>>>  #include <linux/module.h>
>>> -#include <linux/of_device.h>
>>> +#include <linux/of.h>
>>>  #include <linux/pm_opp.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regulator/consumer.h>
>>> -#include <linux/slab.h>
>>>  
>>>  #define DEFAULT_SATURATION_RATIO	40
>>>  
>>> @@ -178,7 +177,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  	struct device *dev = bus->dev;
>>>  	struct opp_table *opp_table;
>>>  	const char *vdd = "vdd";
>>> -	int i, ret, count, size;
>>> +	int i, ret, count;
>>>  
>>>  	opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>  	if (IS_ERR(opp_table)) {
>>> @@ -201,8 +200,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  	}
>>>  	bus->edev_count = count;
>>>  
>>> -	size = sizeof(*bus->edev) * count;
>>> -	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>> +	bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
>>
>> ditto.
>> It depends on personal style. Don't change it because we cannot
>> modify them at the all device driver. If is not wrong,
>> just keep the original code.
> 
> Of course, this is a matter of style, but I think that Coccinelle reports
> such code, compare with e.g., https://protect2.fireeye.com/url?k=9d96be53-c05a72f6-9d97351c-0cc47a30d446-615469bafe78ddb7&u=https://lkml.org/lkml/2019/5/8/927

OK. If it is result from Coccinelle reports, you need to add
the 'Coccinelle reports' in the patch description.

When you send v2, you can contain this clean-up
with 'Coccinelle reports'. If there are reasonable reason,
always I'll agree.

> 
> Anyway, I can drop it since the purpose of this patchset as a whole was to
> untangle all the goto's and I agree this is kind of unrelated.
> 
>>
>>>  	if (!bus->edev) {
>>>  		ret = -ENOMEM;
>>>  		goto err_regulator;
>>> @@ -301,10 +299,9 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>  	profile->exit = exynos_bus_exit;
>>>  
>>>  	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>>> -	if (!ondemand_data) {
>>> -		ret = -ENOMEM;
>>> -		goto err;
>>> -	}
>>> +	if (!ondemand_data)
>>> +		return -ENOMEM;
>>> +
>>>  	ondemand_data->upthreshold = 40;
>>>  	ondemand_data->downdifferential = 5;
>>>  
>>> @@ -314,15 +311,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>  						ondemand_data);
>>>  	if (IS_ERR(bus->devfreq)) {
>>>  		dev_err(dev, "failed to add devfreq device\n");
>>> -		ret = PTR_ERR(bus->devfreq);
>>> -		goto err;
>>> +		return PTR_ERR(bus->devfreq);
>>>  	}
>>>  
>>>  	/* Register opp_notifier to catch the change of OPP  */
>>>  	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>>>  	if (ret < 0) {
>>>  		dev_err(dev, "failed to register opp notifier\n");
>>> -		goto err;
>>> +		return ret;
>>>  	}
>>>  
>>>  	/*
>>> @@ -332,17 +328,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>  	ret = exynos_bus_enable_edev(bus);
>>>  	if (ret < 0) {
>>>  		dev_err(dev, "failed to enable devfreq-event devices\n");
>>> -		goto err;
>>> +		return ret;
>>>  	}
>>>  
>>>  	ret = exynos_bus_set_event(bus);
>>>  	if (ret < 0) {
>>>  		dev_err(dev, "failed to set event to devfreq-event devices\n");
>>> -		goto err;
>>> +		return ret;
>>>  	}
>>>  
>>> -err:
>>> -	return ret;
>>> +	return 0;
>>>  }
>>>  
>>>  static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>> @@ -351,7 +346,6 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>>  	struct device *dev = bus->dev;
>>>  	struct devfreq_passive_data *passive_data;
>>>  	struct devfreq *parent_devfreq;
>>> -	int ret = 0;
>>>  
>>>  	/* Initialize the struct profile and governor data for passive device */
>>>  	profile->target = exynos_bus_target;
>>> @@ -359,30 +353,26 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>>  
>>>  	/* Get the instance of parent devfreq device */
>>>  	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>> -	if (IS_ERR(parent_devfreq)) {
>>> -		ret = -EPROBE_DEFER;
>>> -		goto err;
>>> -	}
>>> +	if (IS_ERR(parent_devfreq))
>>> +		return -EPROBE_DEFER;
>>>  
>>>  	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
>>> -	if (!passive_data) {
>>> -		ret = -ENOMEM;
>>> -		goto err;
>>> -	}
>>> +	if (!passive_data)
>>> +		return -ENOMEM;
>>> +
>>>  	passive_data->parent = parent_devfreq;
>>>  
>>>  	/* Add devfreq device for exynos bus with passive governor */
>>> -	bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
>>> +	bus->devfreq = devm_devfreq_add_device(dev, profile,
>>> +						DEVFREQ_GOV_PASSIVE,
>>
>> It is not clean-up. It depends on personal style. Don't change it
>> because we cannot modify them at the all device driver. If is not wrong,
>> just keep the original code.
> 
> I wanted to make the code fit in 80 columns (issue reported by
> scripts/checkpatch.pl). For the reasons stated in my previous comment,
> I am happy to drop this change if you don't like it.

ditto. Please mention the reason from checkpatch.pl on patch4 description.

> 
>>
>>>  						passive_data);
>>>  	if (IS_ERR(bus->devfreq)) {
>>>  		dev_err(dev,
>>>  			"failed to add devfreq dev with passive governor\n");
>>> -		ret = PTR_ERR(bus->devfreq);
>>> -		goto err;
>>> +		return PTR_ERR(bus->devfreq);
>>>  	}
>>>  
>>> -err:
>>> -	return ret;
>>> +	return 0;
>>>  }
>>>  
>>>  static int exynos_bus_probe(struct platform_device *pdev)
>>> @@ -400,18 +390,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>> +	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>>
>> ditto.
>> It depends on personal style. Don't change it because we cannot
>> modify them at the all device driver. If is not wrong,
>> just keep the original code.
> 
> Please note that there exists this variable in exynos_bus_probe():
> 
> struct device *dev = &pdev->dev;
> 
> but the expression '&pdev->dev' is reused twice more ('dev' itself
> is also used). Is there any reason for such inconsistency?

Frankly, it is not any reason. Mabye, we could find same thing like thin
in the linux kernel. As you commented, we better to keep the only one
style without any specific reason.

But, in my experience, these minor issues will happen continuously
in linux kernel. I think tha it is not much valuable to change
the these minor issues except for result rom checkpatch.pl/Coccinelle script.
I think that sometimes it make the difficult to keep the patch history.

If there are any special reason, In my case, just keep the origin code
for the patch history.

> 
>>
>>>  	if (!bus)
>>>  		return -ENOMEM;
>>>  	mutex_init(&bus->lock);
>>> -	bus->dev = &pdev->dev;
>>> +	bus->dev = dev;
>>
>> ditto.
>> It depends on personal style. Don't change it because we cannot
>> modify them at the all device driver. If is not wrong,
>> just keep the original code.
> 
> (See above)
> 
>>
>>>  	platform_set_drvdata(pdev, bus);
>>>  
>>>  	profile = devm_kzalloc(dev, sizeof(*profile), GFP_KERNEL);
>>>  	if (!profile)
>>>  		return -ENOMEM;
>>>  
>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> +	node = of_parse_phandle(np, "devfreq", 0);
>>>  	if (node) {
>>>  		of_node_put(node);
>>>  		passive = true;
>>>
>>
> 
> Best regards,
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics