diff mbox series

[RFC,01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

Message ID 20190723122016.30279-2-a.swigon@partner.samsung.com (mailing list archive)
State Not Applicable
Headers show
Series Simple QoS for exynos-bus driver using interconnect | expand

Commit Message

Artur Świgoń July 23, 2019, 12:20 p.m. UTC
This patch adds a new static function, exynos_bus_profile_init(), extracted
from exynos_bus_probe().

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

Comments

Krzysztof Kozlowski July 24, 2019, 7:07 p.m. UTC | #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ń <a.swigon@partner.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 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
Chanwoo Choi July 25, 2019, 12:43 p.m. UTC | #2
2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.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 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.
Krzysztof Kozlowski July 26, 2019, 10:42 a.m. UTC | #3
On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
> >
> > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > from exynos_bus_probe().
> >
> > Signed-off-by: Artur Świgoń <a.swigon@partner.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 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
Chanwoo Choi July 26, 2019, 10:52 a.m. UTC | #4
On 19. 7. 26. 오후 7:42, Krzysztof Kozlowski wrote:
> On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>
>> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>>>
>>> This patch adds a new static function, exynos_bus_profile_init(), extracted
>>> from exynos_bus_probe().
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@partner.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 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.
Artur Świgoń July 31, 2019, 1 p.m. UTC | #5
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ń <a.swigon@partner.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 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,
Krzysztof Kozlowski Aug. 5, 2019, 9:56 a.m. UTC | #6
On Wed, 31 Jul 2019 at 15:00, Artur Świgoń <a.swigon@partner.samsung.com> 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ń <a.swigon@partner.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 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
diff mbox series

Patch

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: