Re: [PATCH 2/5] PM / devfreq: Split device_register usage

From: Chanwoo Choi <cw00.choi@samsung.com>
To: Leonard Crestez <leonard.crestez@nxp.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: "Kyungmin Park" <kyungmin.park@samsung.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Matthias Kaehlcke" <mka@chromium.org>,
	"Artur Świgoń" <a.swigon@partner.samsung.com>,
	"Georgi Djakov" <georgi.djakov@linaro.org>,
	"Viresh Kumar" <viresh.kumar@linaro.org>,
	linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-imx@nxp.com
Subject: Re: [PATCH 2/5] PM / devfreq: Split device_register usage
Date: Mon, 2 Dec 2019 10:08:31 +0900	[thread overview]
Message-ID: <69d37137-8c7a-ddcc-f692-09546c79ff7d@samsung.com> (raw)
In-Reply-To: <85ccf6afe5db556c610ce2b47ccc38132b6671f6.1573686315.git.leonard.crestez@nxp.com>

On 11/14/19 8:21 AM, Leonard Crestez wrote:
> Splitting device_register into device_initialize and device_add allows
> devm-based allocations to be performed before device_add.
> 
> It also simplifies error paths in devfreq_add_device: just call
> put_device instead of duplicating parts of devfreq_dev_release.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 27af1b95fd23..b89a82382536 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -689,10 +689,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	mutex_init(&devfreq->lock);
>  	mutex_lock(&devfreq->lock);
>  	devfreq->dev.parent = dev;
>  	devfreq->dev.class = devfreq_class;
>  	devfreq->dev.release = devfreq_dev_release;
> +	device_initialize(&devfreq->dev);
>  	INIT_LIST_HEAD(&devfreq->node);
>  	devfreq->profile = profile;
>  	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->last_status.current_frequency = profile->initial_freq;
> @@ -726,15 +727,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	dev_set_name(&devfreq->dev, "devfreq%d",
>  				atomic_inc_return(&devfreq_no));
> -	err = device_register(&devfreq->dev);
> +	err = device_add(&devfreq->dev);
>  	if (err) {
>  		mutex_unlock(&devfreq->lock);
> -		put_device(&devfreq->dev);
> -		goto err_out;
> +		goto err_dev;
>  	}
>  
>  	devfreq->trans_table = devm_kzalloc(&devfreq->dev,
>  			array3_size(sizeof(unsigned int),
>  				    devfreq->profile->max_state,
> @@ -789,13 +789,13 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  
>  err_init:
>  	mutex_unlock(&devfreq_list_lock);
>  err_devfreq:
>  	devfreq_remove_device(devfreq);
> -	devfreq = NULL;
> +	return ERR_PTR(err);
>  err_dev:
> -	kfree(devfreq);
> +	put_device(&devfreq->dev);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);
>  
> 

As I previously commented, I don't prefer to split out of bodyf of device_register().
Instead, your first version is better without devm.

Regards,
Chanwoo Choi

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics