Re: [PATCH] regulator: reorder initialization steps in regulator_register()

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 12 Apr 2016 12:31:00 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> device_register() is calling ->get_voltage() as part of is sysfs attribute

							  ^ its

> initialization process, and this functions might need to know the regulator
> constraints to return a valid value.
> This is at least true for the pwm regulator driver (when operating in
> continuous mode) which needs to know the minimum and maximum voltage values
> to calculate the current voltage:
> 
> min_uV + (((max_uV - min_uV) * dutycycle) / 100);
> 
> Move device_register() after set_machine_constraints() to make sure those
> constraints are correctly initialized when ->get_voltage() is called.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Reported-by: Stephen Barber <smbarber@chromium.org>
> ---
>  drivers/regulator/core.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e0b7642..8258568 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3950,13 +3950,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  	rdev->dev.parent = dev;
>  	dev_set_name(&rdev->dev, "regulator.%lu",
>  		    (unsigned long) atomic_inc_return(&regulator_no));
> -	ret = device_register(&rdev->dev);
> -	if (ret != 0) {
> -		put_device(&rdev->dev);
> -		goto wash;
> -	}
> -
> -	dev_set_drvdata(&rdev->dev, rdev);
>  
>  	/* set regulator constraints */
>  	if (init_data)
> @@ -3964,7 +3957,15 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  
>  	ret = set_machine_constraints(rdev, constraints);
>  	if (ret < 0)
> -		goto scrub;
> +		goto wash;
> +
> +	ret = device_register(&rdev->dev);
> +	if (ret != 0) {
> +		put_device(&rdev->dev);
> +		goto wash;
> +	}
> +
> +	dev_set_drvdata(&rdev->dev, rdev);
>  
>  	if (init_data && init_data->supply_regulator)
>  		rdev->supply_name = init_data->supply_regulator;
> @@ -3993,8 +3994,6 @@ out:
>  
>  unset_supplies:
>  	unset_regulator_supplies(rdev);
> -
> -scrub:
>  	regulator_ena_gpio_free(rdev);
>  	device_unregister(&rdev->dev);
>  	/* device core frees rdev */
> @@ -4002,6 +4001,7 @@ scrub:
>  	goto out;
>  
>  wash:
> +	kfree(rdev->constraints);
>  	regulator_ena_gpio_free(rdev);
>  clean:
>  	kfree(rdev);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com