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 at 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
--
nvpublic
