| Message ID | 20201111011456.7875-11-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Chanwoo Choi |
| Headers | show |
| Series | Introduce memory interconnect for NVIDIA Tegra SoCs | expand |
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@gmail.com> > --- > 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
On Wed, Nov 11, 2020 at 09:51:15AM +0100, Krzysztof Kozlowski wrote: > 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@gmail.com> > > --- > > 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 Correction: you added this in patch #9. Best regards, Krzysztof > 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. >
11.11.2020 11:52, Krzysztof Kozlowski пишет: >> You added this code in patch #8, so adding-and-removing a piece of code > Correction: you added this in patch #9. > > Best regards, > Krzysztof > > >> 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. >> Indeed, I squashed a similar change in the T124 patch #13, but forgot to squash it for the T30.
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); -unset_cb: - tegra20_clk_set_emc_round_callback(NULL, NULL); - - return err; } static int tegra_emc_suspend(struct device *dev)
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@gmail.com> --- drivers/memory/tegra/tegra30-emc.c | 70 ++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 23 deletions(-)