diff mbox

[RFC,04/13] drm/tegra: Add sor-safe clock for DPAUX on Tegra210

Message ID 1466165027-17917-5-git-send-email-jonathanh@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jon Hunter June 17, 2016, 12:03 p.m. UTC
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(-)

Comments

Thierry Reding June 17, 2016, 4:18 p.m. UTC | #1
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
Jon Hunter June 20, 2016, 8:43 a.m. UTC | #2
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
Thierry Reding June 20, 2016, 4:33 p.m. UTC | #3
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 mbox

Patch

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;
 }