This patch adds a new static function, exynos_bus_profile_init(), extracted
from exynos_bus_probe().
Signed-off-by: Artur Świgoń <[email protected]>
---
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 d9f377912c10..d8f1efaf2d49 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -372,12 +372,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;
@@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
- /* 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
On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> 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 d9f377912c10..d8f1efaf2d49 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -372,12 +372,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;
Just return proper error code. Less lines, obvious code since you do not
have any cleanup in error path.
> + }
> + 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;
The same - return err.
Best regards,
Krzysztof
2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <[email protected]>님이 작성:
>
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
>
> Signed-off-by: Artur Świgoń <[email protected]>
> ---
> 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 d9f377912c10..d8f1efaf2d49 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -372,12 +372,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;
> @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err;
>
> - /* 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
>
NACK.
It has not any benefit and I don't understand reason why it is necessary.
I don't agree. Please drop it.
--
Best Regards,
Chanwoo Choi
On 19. 7. 26. 오후 7:42, Krzysztof Kozlowski wrote:
> On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <[email protected]> wrote:
>>
>> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <[email protected]>님이 작성:
>>>
>>> This patch adds a new static function, exynos_bus_profile_init(), extracted
>>> from exynos_bus_probe().
>>>
>>> Signed-off-by: Artur Świgoń <[email protected]>
>>> ---
>>> 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 d9f377912c10..d8f1efaf2d49 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -372,12 +372,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;
>>> @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>> if (ret < 0)
>>> goto err;
>>>
>>> - /* 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
>>>
>>
>> NACK.
>>
>> It has not any benefit and I don't understand reason why it is necessary.
>> I don't agree. Please drop it.
>
> The probe has 12 local variables and around 140 lines of code (so much
> more than coding style recommendations). Therefore splitting some
> logical part out of probe to make code better organized and more
> readable is pretty obvious benefit.
After checked the patch3, I changed my opinion. It seems more simple than before
and I replied on patch3. But, I think that can merge patch1/2/2 to one patch.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <[email protected]> wrote:
>
> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <[email protected]>님이 작성:
> >
> > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > from exynos_bus_probe().
> >
> > Signed-off-by: Artur Świgoń <[email protected]>
> > ---
> > 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 d9f377912c10..d8f1efaf2d49 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -372,12 +372,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;
> > @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
> > if (ret < 0)
> > goto err;
> >
> > - /* 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
> >
>
> NACK.
>
> It has not any benefit and I don't understand reason why it is necessary.
> I don't agree. Please drop it.
The probe has 12 local variables and around 140 lines of code (so much
more than coding style recommendations). Therefore splitting some
logical part out of probe to make code better organized and more
readable is pretty obvious benefit.
Best regards,
Krzysztof
Hi,
On Wed, 2019-07-24 at 21:07 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > from exynos_bus_probe().
> >
> > Signed-off-by: Artur Świgoń <[email protected]>
> > ---
> > 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 d9f377912c10..d8f1efaf2d49 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -372,12 +372,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;
>
> Just return proper error code. Less lines, obvious code since you do not
> have any cleanup in error path.
I was advised to avoid modifying code being moved (in one patch). I do make
changes in these places in patch 04/11, i.e. change the original label 'err' to
'out'. What's your opinion on making the proposed changes to patches 01 and 02
(s/goto err/return ret/) in patch 04 instead?
> > +
> > + /* 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;
>
> The same - return err.
>
> Best regards,
> Krzysztof
Best regards,
--
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics
On Wed, 31 Jul 2019 at 15:00, Artur Świgoń <[email protected]> wrote:
>
> Hi,
>
> On Wed, 2019-07-24 at 21:07 +0200, Krzysztof Kozlowski wrote:
> > On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> > > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > > from exynos_bus_probe().
> > >
> > > Signed-off-by: Artur Świgoń <[email protected]>
> > > ---
> > > 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 d9f377912c10..d8f1efaf2d49 100644
> > > --- a/drivers/devfreq/exynos-bus.c
> > > +++ b/drivers/devfreq/exynos-bus.c
> > > @@ -372,12 +372,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;
> >
> > Just return proper error code. Less lines, obvious code since you do not
> > have any cleanup in error path.
>
> I was advised to avoid modifying code being moved (in one patch). I do make
> changes in these places in patch 04/11, i.e. change the original label 'err' to
> 'out'. What's your opinion on making the proposed changes to patches 01 and 02
> (s/goto err/return ret/) in patch 04 instead?
Yes, you're right. I also prefer not to touch moved code.
Best regards,
Krzysztof