| Message ID | 1466165027-17917-5-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 17, 2016 at 01:03:38PM +0100, Jon Hunter wrote: > For Tegra210 the 'sor-safe' clock needs to be enabled when using DPAUX. > Add support to the DPAUX driver for enabling this clock on Tegra210. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/gpu/drm/tegra/dpaux.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c > index aa3a037fcd3b..d696a7e45935 100644 > --- a/drivers/gpu/drm/tegra/dpaux.c > +++ b/drivers/gpu/drm/tegra/dpaux.c > @@ -37,6 +37,7 @@ struct tegra_dpaux { > > struct reset_control *rst; > struct clk *clk_parent; > + struct clk *clk_sor; Can we call this "clk_safe", please? On one hand that mirrors the name of the clock in the binding and on the other hand it avoids confusion with the real SOR clock. > struct clk *clk; > > struct regulator *vdd; > @@ -340,18 +341,37 @@ static int tegra_dpaux_probe(struct platform_device *pdev) > return PTR_ERR(dpaux->rst); > } > > + if (of_device_is_compatible(pdev->dev.of_node, > + "nvidia,tegra210-dpaux")) { > + dpaux->clk_sor = devm_clk_get(&pdev->dev, "sor-safe"); > + if (IS_ERR(dpaux->clk_sor)) { > + dev_err(&pdev->dev, > + "failed to get sor-safe clock: %ld\n", > + PTR_ERR(dpaux->clk_sor)); > + return PTR_ERR(dpaux->clk_sor); > + } > + > + err = clk_prepare_enable(dpaux->clk_sor); > + if (err < 0) { > + dev_err(&pdev->dev, > + "failed to enable sor-safe clock: %d\n", err); > + return err; > + } > + } Please make this part of a struct tegra_dpaux_soc, so that we don't have to check the compatible string again here. This could look like: struct tegra_dpaux_soc { bool needs_safe_clock; }; static const struct tegra_dpaux_soc tegra124_dpaux_soc = { .needs_safe_clock = false, }; static const struct tegra_dpaux_soc tegra210_dpaux_soc = { .needs_safe_clock = true, }; ... static const struct of_device_id tegra_dpaux_of_match[] = { { .compatible = "nvidia,tegra210-dpaux", .data = &tegra210_dpaux_soc }, { .compatible = "nvidia,tegra124-dpaux", .data = &tegra124_dpaux_soc }, { }, }; > @@ -434,6 +454,9 @@ disable_parent_clk: > assert_reset: > reset_control_assert(dpaux->rst); > clk_disable_unprepare(dpaux->clk); > +disable_sor_clk: > + if (dpaux->clk_sor) > + clk_disable_unprepare(dpaux->clk_sor); You can drop the extra check here, since the common clock framework ignores NULL or ERR_PTR() pointers. > > return err; > } > @@ -456,6 +479,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) > clk_disable_unprepare(dpaux->clk_parent); > reset_control_assert(dpaux->rst); > clk_disable_unprepare(dpaux->clk); > + if (dpaux->clk_sor) > + clk_disable_unprepare(dpaux->clk_sor); Same here. Thierry
On 17/06/16 17:18, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, Jun 17, 2016 at 01:03:38PM +0100, Jon Hunter wrote: >> For Tegra210 the 'sor-safe' clock needs to be enabled when using DPAUX. >> Add support to the DPAUX driver for enabling this clock on Tegra210. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/gpu/drm/tegra/dpaux.c | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c >> index aa3a037fcd3b..d696a7e45935 100644 >> --- a/drivers/gpu/drm/tegra/dpaux.c >> +++ b/drivers/gpu/drm/tegra/dpaux.c >> @@ -37,6 +37,7 @@ struct tegra_dpaux { >> >> struct reset_control *rst; >> struct clk *clk_parent; >> + struct clk *clk_sor; > > Can we call this "clk_safe", please? On one hand that mirrors the name > of the clock in the binding and on the other hand it avoids confusion > with the real SOR clock. OK. >> struct clk *clk; >> >> struct regulator *vdd; >> @@ -340,18 +341,37 @@ static int tegra_dpaux_probe(struct platform_device *pdev) >> return PTR_ERR(dpaux->rst); >> } >> >> + if (of_device_is_compatible(pdev->dev.of_node, >> + "nvidia,tegra210-dpaux")) { >> + dpaux->clk_sor = devm_clk_get(&pdev->dev, "sor-safe"); >> + if (IS_ERR(dpaux->clk_sor)) { >> + dev_err(&pdev->dev, >> + "failed to get sor-safe clock: %ld\n", >> + PTR_ERR(dpaux->clk_sor)); >> + return PTR_ERR(dpaux->clk_sor); >> + } >> + >> + err = clk_prepare_enable(dpaux->clk_sor); >> + if (err < 0) { >> + dev_err(&pdev->dev, >> + "failed to enable sor-safe clock: %d\n", err); >> + return err; >> + } >> + } > > Please make this part of a struct tegra_dpaux_soc, so that we don't have > to check the compatible string again here. This could look like: > > struct tegra_dpaux_soc { > bool needs_safe_clock; > }; > > static const struct tegra_dpaux_soc tegra124_dpaux_soc = { > .needs_safe_clock = false, > }; > > static const struct tegra_dpaux_soc tegra210_dpaux_soc = { > .needs_safe_clock = true, > }; > > ... > > static const struct of_device_id tegra_dpaux_of_match[] = { > { .compatible = "nvidia,tegra210-dpaux", .data = &tegra210_dpaux_soc }, > { .compatible = "nvidia,tegra124-dpaux", .data = &tegra124_dpaux_soc }, > { }, > }; OK. I wonder if we should call it 'has_safe_clock' because this clock does not exist for tegra124 AFAICT. #bikeshed ;-) >> @@ -434,6 +454,9 @@ disable_parent_clk: >> assert_reset: >> reset_control_assert(dpaux->rst); >> clk_disable_unprepare(dpaux->clk); >> +disable_sor_clk: >> + if (dpaux->clk_sor) >> + clk_disable_unprepare(dpaux->clk_sor); > > You can drop the extra check here, since the common clock framework > ignores NULL or ERR_PTR() pointers. OK. >> >> return err; >> } >> @@ -456,6 +479,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) >> clk_disable_unprepare(dpaux->clk_parent); >> reset_control_assert(dpaux->rst); >> clk_disable_unprepare(dpaux->clk); >> + if (dpaux->clk_sor) >> + clk_disable_unprepare(dpaux->clk_sor); > > Same here. OK. Cheers Jon
On Mon, Jun 20, 2016 at 09:43:42AM +0100, Jon Hunter wrote: > On 17/06/16 17:18, Thierry Reding wrote: > > On Fri, Jun 17, 2016 at 01:03:38PM +0100, Jon Hunter wrote: > >> diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c [...] > >> + if (of_device_is_compatible(pdev->dev.of_node, > >> + "nvidia,tegra210-dpaux")) { > >> + dpaux->clk_sor = devm_clk_get(&pdev->dev, "sor-safe"); > >> + if (IS_ERR(dpaux->clk_sor)) { > >> + dev_err(&pdev->dev, > >> + "failed to get sor-safe clock: %ld\n", > >> + PTR_ERR(dpaux->clk_sor)); > >> + return PTR_ERR(dpaux->clk_sor); > >> + } > >> + > >> + err = clk_prepare_enable(dpaux->clk_sor); > >> + if (err < 0) { > >> + dev_err(&pdev->dev, > >> + "failed to enable sor-safe clock: %d\n", err); > >> + return err; > >> + } > >> + } > > > > Please make this part of a struct tegra_dpaux_soc, so that we don't have > > to check the compatible string again here. This could look like: > > > > struct tegra_dpaux_soc { > > bool needs_safe_clock; > > }; > > > > static const struct tegra_dpaux_soc tegra124_dpaux_soc = { > > .needs_safe_clock = false, > > }; > > > > static const struct tegra_dpaux_soc tegra210_dpaux_soc = { > > .needs_safe_clock = true, > > }; > > > > ... > > > > static const struct of_device_id tegra_dpaux_of_match[] = { > > { .compatible = "nvidia,tegra210-dpaux", .data = &tegra210_dpaux_soc }, > > { .compatible = "nvidia,tegra124-dpaux", .data = &tegra124_dpaux_soc }, > > { }, > > }; > > OK. I wonder if we should call it 'has_safe_clock' because this clock > does not exist for tegra124 AFAICT. #bikeshed ;-) has_safe_clock is fine with me, too. Thierry
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index aa3a037fcd3b..d696a7e45935 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -37,6 +37,7 @@ struct tegra_dpaux { struct reset_control *rst; struct clk *clk_parent; + struct clk *clk_sor; struct clk *clk; struct regulator *vdd; @@ -340,18 +341,37 @@ static int tegra_dpaux_probe(struct platform_device *pdev) return PTR_ERR(dpaux->rst); } + if (of_device_is_compatible(pdev->dev.of_node, + "nvidia,tegra210-dpaux")) { + dpaux->clk_sor = devm_clk_get(&pdev->dev, "sor-safe"); + if (IS_ERR(dpaux->clk_sor)) { + dev_err(&pdev->dev, + "failed to get sor-safe clock: %ld\n", + PTR_ERR(dpaux->clk_sor)); + return PTR_ERR(dpaux->clk_sor); + } + + err = clk_prepare_enable(dpaux->clk_sor); + if (err < 0) { + dev_err(&pdev->dev, + "failed to enable sor-safe clock: %d\n", err); + return err; + } + } + dpaux->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(dpaux->clk)) { dev_err(&pdev->dev, "failed to get module clock: %ld\n", PTR_ERR(dpaux->clk)); - return PTR_ERR(dpaux->clk); + err = PTR_ERR(dpaux->clk); + goto disable_sor_clk; } err = clk_prepare_enable(dpaux->clk); if (err < 0) { dev_err(&pdev->dev, "failed to enable module clock: %d\n", err); - return err; + goto disable_sor_clk; } reset_control_deassert(dpaux->rst); @@ -434,6 +454,9 @@ disable_parent_clk: assert_reset: reset_control_assert(dpaux->rst); clk_disable_unprepare(dpaux->clk); +disable_sor_clk: + if (dpaux->clk_sor) + clk_disable_unprepare(dpaux->clk_sor); return err; } @@ -456,6 +479,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) clk_disable_unprepare(dpaux->clk_parent); reset_control_assert(dpaux->rst); clk_disable_unprepare(dpaux->clk); + if (dpaux->clk_sor) + clk_disable_unprepare(dpaux->clk_sor); return 0; }
For Tegra210 the 'sor-safe' clock needs to be enabled when using DPAUX. Add support to the DPAUX driver for enabling this clock on Tegra210. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/gpu/drm/tegra/dpaux.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)