[PATCH v3 0/4] devfreq: Clean up exynos-bus driver

[PATCH v3 0/4] devfreq: Clean up exynos-bus driver

[Artur Świgoń]

The following patchset incorporates the first four patches from a bigger
RFC[1]. The purpose of these patches is to improve readability of the code,
with the main focus on the exynos_bus_probe() function.

The original exynos_bus_probe() function has 13 local variables, over 140
lines of code, and multiple goto statements. Patches 01 and 02 from this
series extract two mutually exclusive code paths into separate functions,
exynos_bus_profile_init[_passive](). Furthermore, patch 03 reduces the
number of goto statements by introducing an if-else construct.

The last patch adds other minor improvements, including cleaning up header
includes, variables, and return paths. This also applies to functions
introduced by patches 01 & 02 -- to avoid moving and changing code in the
same patch.

---
Changes since RFCv2[1] (patches 01..04):
* Rebase on next-20191209.
* Drop some unnecessary changes, cf. [2].

---
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

---
References:
[1] https://patchwork.kernel.org/cover/11152595/
[2] https://patchwork.kernel.org/patch/11152637/

Artur Świgoń (4):
  devfreq: exynos-bus: Extract exynos_bus_profile_init()
  devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
  devfreq: exynos-bus: Change goto-based logic to if-else logic
  devfreq: exynos-bus: Clean up code

 drivers/devfreq/exynos-bus.c | 156 +++++++++++++++++++----------------
 1 file changed, 84 insertions(+), 72 deletions(-)

--
2.17.1

[PATCH v3 1/4] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Artur Świgoń]

This patch adds a new static function, exynos_bus_profile_init(), extracted
from exynos_bus_probe().

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
---
 drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index c832673273a2..b8ca6b9f4b82 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -287,12 +287,69 @@ static int exynos_bus_parse_of(struct device_node *np,
 	return ret;
 }
 
+static int exynos_bus_profile_init(struct exynos_bus *bus,
+				   struct devfreq_dev_profile *profile)
+{
+	struct device *dev = bus->dev;
+	struct devfreq_simple_ondemand_data *ondemand_data;
+	int ret;
+
+	/* Initialize the struct profile and governor data for parent device */
+	profile->polling_ms = 50;
+	profile->target = exynos_bus_target;
+	profile->get_dev_status = exynos_bus_get_dev_status;
+	profile->exit = exynos_bus_exit;
+
+	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
+	if (!ondemand_data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	ondemand_data->upthreshold = 40;
+	ondemand_data->downdifferential = 5;
+
+	/* Add devfreq device to monitor and handle the exynos bus */
+	bus->devfreq = devm_devfreq_add_device(dev, profile,
+						DEVFREQ_GOV_SIMPLE_ONDEMAND,
+						ondemand_data);
+	if (IS_ERR(bus->devfreq)) {
+		dev_err(dev, "failed to add devfreq device\n");
+		ret = PTR_ERR(bus->devfreq);
+		goto err;
+	}
+
+	/* 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;
+	}
+
+	/*
+	 * Enable devfreq-event to get raw data which is used to determine
+	 * current bus load.
+	 */
+	ret = exynos_bus_enable_edev(bus);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable devfreq-event devices\n");
+		goto err;
+	}
+
+	ret = exynos_bus_set_event(bus);
+	if (ret < 0) {
+		dev_err(dev, "failed to set event to devfreq-event devices\n");
+		goto err;
+	}
+
+err:
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node, *node;
 	struct devfreq_dev_profile *profile;
-	struct devfreq_simple_ondemand_data *ondemand_data;
 	struct devfreq_passive_data *passive_data;
 	struct devfreq *parent_devfreq;
 	struct exynos_bus *bus;
@@ -334,52 +391,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	if (passive)
 		goto passive;
 
-	/* Initialize the struct profile and governor data for parent device */
-	profile->polling_ms = 50;
-	profile->target = exynos_bus_target;
-	profile->get_dev_status = exynos_bus_get_dev_status;
-	profile->exit = exynos_bus_exit;
-
-	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
-	if (!ondemand_data) {
-		ret = -ENOMEM;
+	ret = exynos_bus_profile_init(bus, profile);
+	if (ret < 0)
 		goto err;
-	}
-	ondemand_data->upthreshold = 40;
-	ondemand_data->downdifferential = 5;
-
-	/* Add devfreq device to monitor and handle the exynos bus */
-	bus->devfreq = devm_devfreq_add_device(dev, profile,
-						DEVFREQ_GOV_SIMPLE_ONDEMAND,
-						ondemand_data);
-	if (IS_ERR(bus->devfreq)) {
-		dev_err(dev, "failed to add devfreq device\n");
-		ret = PTR_ERR(bus->devfreq);
-		goto err;
-	}
-
-	/* 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;
-	}
-
-	/*
-	 * Enable devfreq-event to get raw data which is used to determine
-	 * current bus load.
-	 */
-	ret = exynos_bus_enable_edev(bus);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable devfreq-event devices\n");
-		goto err;
-	}
-
-	ret = exynos_bus_set_event(bus);
-	if (ret < 0) {
-		dev_err(dev, "failed to set event to devfreq-event devices\n");
-		goto err;
-	}
 
 	goto out;
 passive:
-- 
2.17.1

Re: [PATCH v3 1/4] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Chanwoo Choi]

On 12/9/19 7:48 PM, Artur Świgoń wrote:
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index c832673273a2..b8ca6b9f4b82 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -287,12 +287,69 @@ static int exynos_bus_parse_of(struct device_node *np,
>  	return ret;
>  }
>  
> +static int exynos_bus_profile_init(struct exynos_bus *bus,
> +				   struct devfreq_dev_profile *profile)
> +{
> +	struct device *dev = bus->dev;
> +	struct devfreq_simple_ondemand_data *ondemand_data;
> +	int ret;
> +
> +	/* Initialize the struct profile and governor data for parent device */
> +	profile->polling_ms = 50;
> +	profile->target = exynos_bus_target;
> +	profile->get_dev_status = exynos_bus_get_dev_status;
> +	profile->exit = exynos_bus_exit;
> +
> +	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> +	if (!ondemand_data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	ondemand_data->upthreshold = 40;
> +	ondemand_data->downdifferential = 5;
> +
> +	/* Add devfreq device to monitor and handle the exynos bus */
> +	bus->devfreq = devm_devfreq_add_device(dev, profile,
> +						DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +						ondemand_data);
> +	if (IS_ERR(bus->devfreq)) {
> +		dev_err(dev, "failed to add devfreq device\n");
> +		ret = PTR_ERR(bus->devfreq);
> +		goto err;
> +	}
> +
> +	/* 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;
> +	}
> +
> +	/*
> +	 * Enable devfreq-event to get raw data which is used to determine
> +	 * current bus load.
> +	 */
> +	ret = exynos_bus_enable_edev(bus);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable devfreq-event devices\n");
> +		goto err;
> +	}
> +
> +	ret = exynos_bus_set_event(bus);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set event to devfreq-event devices\n");
> +		goto err;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node, *node;
>  	struct devfreq_dev_profile *profile;
> -	struct devfreq_simple_ondemand_data *ondemand_data;
>  	struct devfreq_passive_data *passive_data;
>  	struct devfreq *parent_devfreq;
>  	struct exynos_bus *bus;
> @@ -334,52 +391,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  	if (passive)
>  		goto passive;
>  
> -	/* Initialize the struct profile and governor data for parent device */
> -	profile->polling_ms = 50;
> -	profile->target = exynos_bus_target;
> -	profile->get_dev_status = exynos_bus_get_dev_status;
> -	profile->exit = exynos_bus_exit;
> -
> -	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> -	if (!ondemand_data) {
> -		ret = -ENOMEM;
> +	ret = exynos_bus_profile_init(bus, profile);
> +	if (ret < 0)
>  		goto err;
> -	}
> -	ondemand_data->upthreshold = 40;
> -	ondemand_data->downdifferential = 5;
> -
> -	/* Add devfreq device to monitor and handle the exynos bus */
> -	bus->devfreq = devm_devfreq_add_device(dev, profile,
> -						DEVFREQ_GOV_SIMPLE_ONDEMAND,
> -						ondemand_data);
> -	if (IS_ERR(bus->devfreq)) {
> -		dev_err(dev, "failed to add devfreq device\n");
> -		ret = PTR_ERR(bus->devfreq);
> -		goto err;
> -	}
> -
> -	/* 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;
> -	}
> -
> -	/*
> -	 * Enable devfreq-event to get raw data which is used to determine
> -	 * current bus load.
> -	 */
> -	ret = exynos_bus_enable_edev(bus);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable devfreq-event devices\n");
> -		goto err;
> -	}
> -
> -	ret = exynos_bus_set_event(bus);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to set event to devfreq-event devices\n");
> -		goto err;
> -	}
>  
>  	goto out;
>  passive:
> 

Applied it. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v3 2/4] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()

[Artur Świgoń]

This patch adds a new static function, exynos_bus_profile_init_passive(),
extracted from exynos_bus_probe().

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
---
 drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index b8ca6b9f4b82..19d9f9f8ced2 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -345,13 +345,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
 	return ret;
 }
 
+static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
+					   struct devfreq_dev_profile *profile)
+{
+	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;
+	profile->exit = exynos_bus_passive_exit;
+
+	/* 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;
+	}
+
+	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
+	if (!passive_data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	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,
+						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;
+	}
+
+err:
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node, *node;
 	struct devfreq_dev_profile *profile;
-	struct devfreq_passive_data *passive_data;
-	struct devfreq *parent_devfreq;
 	struct exynos_bus *bus;
 	int ret, max_state;
 	unsigned long min_freq, max_freq;
@@ -397,33 +435,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
 
 	goto out;
 passive:
-	/* Initialize the struct profile and governor data for passive device */
-	profile->target = exynos_bus_target;
-	profile->exit = exynos_bus_passive_exit;
-
-	/* Get the instance of parent devfreq device */
-	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
-	if (IS_ERR(parent_devfreq)) {
-		ret = -EPROBE_DEFER;
+	ret = exynos_bus_profile_init_passive(bus, profile);
+	if (ret < 0)
 		goto err;
-	}
-
-	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
-	if (!passive_data) {
-		ret = -ENOMEM;
-		goto err;
-	}
-	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,
-						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;
-	}
 
 out:
 	max_state = bus->devfreq->profile->max_state;
-- 
2.17.1

Re: [PATCH v3 2/4] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()

[Chanwoo Choi]

On 12/9/19 7:49 PM, Artur Świgoń wrote:
> This patch adds a new static function, exynos_bus_profile_init_passive(),
> extracted from exynos_bus_probe().
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index b8ca6b9f4b82..19d9f9f8ced2 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -345,13 +345,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>  	return ret;
>  }
>  
> +static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> +					   struct devfreq_dev_profile *profile)
> +{
> +	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;
> +	profile->exit = exynos_bus_passive_exit;
> +
> +	/* 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;
> +	}
> +
> +	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> +	if (!passive_data) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	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,
> +						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;
> +	}
> +
> +err:
> +	return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node, *node;
>  	struct devfreq_dev_profile *profile;
> -	struct devfreq_passive_data *passive_data;
> -	struct devfreq *parent_devfreq;
>  	struct exynos_bus *bus;
>  	int ret, max_state;
>  	unsigned long min_freq, max_freq;
> @@ -397,33 +435,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  
>  	goto out;
>  passive:
> -	/* Initialize the struct profile and governor data for passive device */
> -	profile->target = exynos_bus_target;
> -	profile->exit = exynos_bus_passive_exit;
> -
> -	/* Get the instance of parent devfreq device */
> -	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> -	if (IS_ERR(parent_devfreq)) {
> -		ret = -EPROBE_DEFER;
> +	ret = exynos_bus_profile_init_passive(bus, profile);
> +	if (ret < 0)
>  		goto err;
> -	}
> -
> -	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> -	if (!passive_data) {
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> -	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,
> -						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;
> -	}
>  
>  out:
>  	max_state = bus->devfreq->profile->max_state;
> 

Applied it. Thanks.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v3 3/4] devfreq: exynos-bus: Change goto-based logic to if-else logic

[Artur Świgoń]

This patch improves code readability by changing the following construct:

>    if (cond)
>        goto passive;
>    foo();
>    goto out;
> passive:
>    bar();
> out:

into this:

>    if (cond)
>        bar();
>    else
>        foo();

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
---
 drivers/devfreq/exynos-bus.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 19d9f9f8ced2..0b557df63666 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -427,19 +427,13 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		goto err_reg;
 
 	if (passive)
-		goto passive;
+		ret = exynos_bus_profile_init_passive(bus, profile);
+	else
+		ret = exynos_bus_profile_init(bus, profile);
 
-	ret = exynos_bus_profile_init(bus, profile);
 	if (ret < 0)
 		goto err;
 
-	goto out;
-passive:
-	ret = exynos_bus_profile_init_passive(bus, profile);
-	if (ret < 0)
-		goto err;
-
-out:
 	max_state = bus->devfreq->profile->max_state;
 	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
 	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
-- 
2.17.1

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

[Artur Świgoń]

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@samsung.com>
---
 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);
 	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,
 						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);
 	if (!bus)
 		return -ENOMEM;
 	mutex_init(&bus->lock);
-	bus->dev = &pdev->dev;
+	bus->dev = dev;
 	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;
-- 
2.17.1

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

[Chanwoo Choi]

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.

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


>  	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.

>  						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.

>  	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.


>  	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,
Chanwoo Choi
Samsung Electronics

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

[Artur Świgoń]

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@samsung.com>
> > ---
> >  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://lkml.org/lkml/2019/5/8/927

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.

> 
> >  						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?

> 
> >  	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,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

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