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@xxxxxxxxxx> >> --- >> 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 -- nvpublic -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html