Re: [PATCH v8 10/26] memory: tegra30-emc: Factor out clk initialization

[Krzysztof Kozlowski]

On Wed, Nov 11, 2020 at 04:14:40AM +0300, Dmitry Osipenko wrote:
> Factor out clk initialization and make it resource-managed. This makes
> easier to follow code and will help to make further changes cleaner.
> 
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/memory/tegra/tegra30-emc.c | 70 ++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
> index d27df842a667..1df42e212d73 100644
> --- a/drivers/memory/tegra/tegra30-emc.c
> +++ b/drivers/memory/tegra/tegra30-emc.c
> @@ -1550,6 +1550,49 @@ static int tegra_emc_opp_table_init(struct tegra_emc *emc)
>  	return err;
>  }
>  
> +static void devm_tegra_emc_unset_callback(void *data)
> +{
> +	tegra20_clk_set_emc_round_callback(NULL, NULL);
> +}
> +
> +static void devm_tegra_emc_unreg_clk_notifier(void *data)
> +{
> +	struct tegra_emc *emc = data;
> +
> +	clk_notifier_unregister(emc->clk, &emc->clk_nb);
> +}
> +
> +static int tegra_emc_init_clk(struct tegra_emc *emc)
> +{
> +	int err;
> +
> +	tegra20_clk_set_emc_round_callback(emc_round_rate, emc);
> +
> +	err = devm_add_action_or_reset(emc->dev, devm_tegra_emc_unset_callback,
> +				       NULL);
> +	if (err)
> +		return err;
> +
> +	emc->clk = devm_clk_get(emc->dev, NULL);
> +	if (IS_ERR(emc->clk)) {
> +		dev_err(emc->dev, "failed to get EMC clock: %pe\n", emc->clk);
> +		return PTR_ERR(emc->clk);
> +	}
> +
> +	err = clk_notifier_register(emc->clk, &emc->clk_nb);
> +	if (err) {
> +		dev_err(emc->dev, "failed to register clk notifier: %d\n", err);
> +		return err;
> +	}
> +
> +	err = devm_add_action_or_reset(emc->dev,
> +				       devm_tegra_emc_unreg_clk_notifier, emc);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
>  static int tegra_emc_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np;
> @@ -1599,25 +1642,13 @@ static int tegra_emc_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	tegra20_clk_set_emc_round_callback(emc_round_rate, emc);
> -
> -	emc->clk = devm_clk_get(&pdev->dev, "emc");
> -	if (IS_ERR(emc->clk)) {
> -		err = PTR_ERR(emc->clk);
> -		dev_err(&pdev->dev, "failed to get emc clock: %d\n", err);
> -		goto unset_cb;
> -	}
> -
> -	err = clk_notifier_register(emc->clk, &emc->clk_nb);
> -	if (err) {
> -		dev_err(&pdev->dev, "failed to register clk notifier: %d\n",
> -			err);
> -		goto unset_cb;
> -	}
> +	err = tegra_emc_init_clk(emc);
> +	if (err)
> +		return err;
>  
>  	err = tegra_emc_opp_table_init(emc);
>  	if (err)
> -		goto unreg_notifier;
> +		return err;
>  
>  	platform_set_drvdata(pdev, emc);
>  	tegra_emc_rate_requests_init(emc);
> @@ -1632,13 +1663,6 @@ static int tegra_emc_probe(struct platform_device *pdev)
>  	try_module_get(THIS_MODULE);
>  
>  	return 0;
> -
> -unreg_notifier:
> -	clk_notifier_unregister(emc->clk, &emc->clk_nb);

You added this code in patch #8, so adding-and-removing a piece of code
is a nice hint that this patch should be before. Don't add new code
which later you simplify. Move all bugfixes and all simplifications to
beginning of patchset.

That's quite similar case to v6 where you put bugfixes in the middle
of patchset.

Best regards,
Krzysztof