diff mbox

[4/4] ARM: tegra: roth: add display DT node

Message ID 1404303560-32209-5-git-send-email-acourbot@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alexandre Courbot July 2, 2014, 12:19 p.m. UTC
DSI support has been fixed to support continuous clock behavior that the
panel used on SHIELD requires, so finally add its device tree node since
it is functional.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra114-roth.dts | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Stephen Warren July 2, 2014, 3:55 p.m. UTC | #1
On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
> DSI support has been fixed to support continuous clock behavior that the
> panel used on SHIELD requires, so finally add its device tree node since
> it is functional.

> diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts

> +	host1x@50000000 {
> +		dsi@54300000 {

That node looks fine, but I wonder why we need to mark a bunch of
regulators as always-on? shouldn't the references to vdd-supply and
power-supply from this node be enough? If not, perhaps the tree
structure of the regulators isn't correct, or the DSI or panel bindings
don't allow enough regulators to be referenced?
Alexandre Courbot July 3, 2014, 3:10 a.m. UTC | #2
On 07/03/2014 12:55 AM, Stephen Warren wrote:
> On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
>> DSI support has been fixed to support continuous clock behavior that the
>> panel used on SHIELD requires, so finally add its device tree node since
>> it is functional.
>
>> diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts
>
>> +	host1x@50000000 {
>> +		dsi@54300000 {
>
> That node looks fine, but I wonder why we need to mark a bunch of
> regulators as always-on? shouldn't the references to vdd-supply and
> power-supply from this node be enough? If not, perhaps the tree
> structure of the regulators isn't correct, or the DSI or panel bindings
> don't allow enough regulators to be referenced?

regulator-always-on is indeed not needed for vdd_lcd. I actually had a 
patch in my tree removing this line that I forgot to squash. Will post a 
v2 for this patch that fixes this, thanks.

vdd-2v8-display needs to remain always-on however. Here we may hit a 
limitation of the simple-panel driver, where only one power supply can 
be provided.
Stephen Warren July 21, 2014, 3:35 p.m. UTC | #3
On 07/02/2014 09:10 PM, Alexandre Courbot wrote:
> On 07/03/2014 12:55 AM, Stephen Warren wrote:
>> On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
>>> DSI support has been fixed to support continuous clock behavior that the
>>> panel used on SHIELD requires, so finally add its device tree node since
>>> it is functional.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-roth.dts
>>> b/arch/arm/boot/dts/tegra114-roth.dts
>>
>>> +    host1x@50000000 {
>>> +        dsi@54300000 {
>>
>> That node looks fine, but I wonder why we need to mark a bunch of
>> regulators as always-on? shouldn't the references to vdd-supply and
>> power-supply from this node be enough? If not, perhaps the tree
>> structure of the regulators isn't correct, or the DSI or panel bindings
>> don't allow enough regulators to be referenced?
> 
> regulator-always-on is indeed not needed for vdd_lcd. I actually had a
> patch in my tree removing this line that I forgot to squash. Will post a
> v2 for this patch that fixes this, thanks.
> 
> vdd-2v8-display needs to remain always-on however. Here we may hit a
> limitation of the simple-panel driver, where only one power supply can
> be provided.

Can't we fix the simple-panel driver to allow a list of regulators in
the property?

I suppose that this technique is OK though; we can always simplify the
DT later if the simple-panel binding gets enhanced.
Thierry Reding July 21, 2014, 9:16 p.m. UTC | #4
On Mon, Jul 21, 2014 at 09:35:27AM -0600, Stephen Warren wrote:
> On 07/02/2014 09:10 PM, Alexandre Courbot wrote:
> > On 07/03/2014 12:55 AM, Stephen Warren wrote:
> >> On 07/02/2014 06:19 AM, Alexandre Courbot wrote:
> >>> DSI support has been fixed to support continuous clock behavior that the
> >>> panel used on SHIELD requires, so finally add its device tree node since
> >>> it is functional.
> >>
> >>> diff --git a/arch/arm/boot/dts/tegra114-roth.dts
> >>> b/arch/arm/boot/dts/tegra114-roth.dts
> >>
> >>> +    host1x@50000000 {
> >>> +        dsi@54300000 {
> >>
> >> That node looks fine, but I wonder why we need to mark a bunch of
> >> regulators as always-on? shouldn't the references to vdd-supply and
> >> power-supply from this node be enough? If not, perhaps the tree
> >> structure of the regulators isn't correct, or the DSI or panel bindings
> >> don't allow enough regulators to be referenced?
> > 
> > regulator-always-on is indeed not needed for vdd_lcd. I actually had a
> > patch in my tree removing this line that I forgot to squash. Will post a
> > v2 for this patch that fixes this, thanks.
> > 
> > vdd-2v8-display needs to remain always-on however. Here we may hit a
> > limitation of the simple-panel driver, where only one power supply can
> > be provided.
> 
> Can't we fix the simple-panel driver to allow a list of regulators in
> the property?

I have no objection to allowing that. But I don't think there's a way to
do that with the current regulator API. You can use the bulk API, but
that requires separate properties, not multiple regulators in one
property.

Perhaps one idea to solve this would be to make the regulator API return
a regulator handle that in fact controls an array of regulators? Adding
Mark.

Thierry
Mark Brown July 21, 2014, 10:51 p.m. UTC | #5
On Mon, Jul 21, 2014 at 11:16:32PM +0200, Thierry Reding wrote:
> On Mon, Jul 21, 2014 at 09:35:27AM -0600, Stephen Warren wrote:

> > > vdd-2v8-display needs to remain always-on however. Here we may hit a
> > > limitation of the simple-panel driver, where only one power supply can
> > > be provided.

> > Can't we fix the simple-panel driver to allow a list of regulators in
> > the property?

> I have no objection to allowing that. But I don't think there's a way to
> do that with the current regulator API. You can use the bulk API, but
> that requires separate properties, not multiple regulators in one
> property.

> Perhaps one idea to solve this would be to make the regulator API return
> a regulator handle that in fact controls an array of regulators? Adding
> Mark.

I'm really not comfortable with that idea, it seems like most of the
users would be abusing it - one of the biggest issues is always getting
people to understand that their driver may be used in other systems with
the supplies mapped differently.  If you were going to do something
along those lines you'd need to do something that enumerated all the
supply properties on the device.
Alexandre Courbot July 22, 2014, 4:39 a.m. UTC | #6
On Tue, Jul 22, 2014 at 7:51 AM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jul 21, 2014 at 11:16:32PM +0200, Thierry Reding wrote:
>> On Mon, Jul 21, 2014 at 09:35:27AM -0600, Stephen Warren wrote:
>
>> > > vdd-2v8-display needs to remain always-on however. Here we may hit a
>> > > limitation of the simple-panel driver, where only one power supply can
>> > > be provided.
>
>> > Can't we fix the simple-panel driver to allow a list of regulators in
>> > the property?
>
>> I have no objection to allowing that. But I don't think there's a way to
>> do that with the current regulator API. You can use the bulk API, but
>> that requires separate properties, not multiple regulators in one
>> property.
>
>> Perhaps one idea to solve this would be to make the regulator API return
>> a regulator handle that in fact controls an array of regulators? Adding
>> Mark.
>
> I'm really not comfortable with that idea, it seems like most of the
> users would be abusing it - one of the biggest issues is always getting
> people to understand that their driver may be used in other systems with
> the supplies mapped differently.  If you were going to do something
> along those lines you'd need to do something that enumerated all the
> supply properties on the device.

I also don't think that would do it - for many displays the power-on
sequence is not as simple as "switch all the regulators on".

Maybe what we would want is to have panel-simple allow panels to
provide a "probe" callback so they can request and manage any extra
resource they need. This would of course imply that custom
enable/disable and suspend/resume callbacks. Then the driver might not
be "simple" anymore, but IMHO it would not be that bad.
Mark Brown July 22, 2014, 9:45 a.m. UTC | #7
On Tue, Jul 22, 2014 at 01:39:51PM +0900, Alexandre Courbot wrote:

> I also don't think that would do it - for many displays the power-on
> sequence is not as simple as "switch all the regulators on".

> Maybe what we would want is to have panel-simple allow panels to
> provide a "probe" callback so they can request and manage any extra
> resource they need. This would of course imply that custom
> enable/disable and suspend/resume callbacks. Then the driver might not
> be "simple" anymore, but IMHO it would not be that bad.

This is starting to mirror the discussion about MMC/SDIO devices where
the talk was of having a property on the device saying which power up
scheme to use - added Ulf for that.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts
index ba210c6..c03c853 100644
--- a/arch/arm/boot/dts/tegra114-roth.dts
+++ b/arch/arm/boot/dts/tegra114-roth.dts
@@ -28,6 +28,22 @@ 
 		reg = <0x80000000 0x79600000>;
 	};
 
+	host1x@50000000 {
+		dsi@54300000 {
+			status = "okay";
+
+			vdd-supply = <&vdd_1v2_ap>;
+
+			panel@0 {
+				compatible = "lg,lh500wx1-sd03";
+				reg = <0>;
+
+				power-supply = <&vdd_lcd>;
+				backlight = <&backlight>;
+			};
+		};
+	};
+
 	pinmux@70000868 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&state_default>;
@@ -811,7 +827,6 @@ 
 						regulator-name = "vdd-1v8";
 						regulator-min-microvolt = <1800000>;
 						regulator-max-microvolt = <1800000>;
-						regulator-always-on;
 						regulator-boot-on;
 					};
 
@@ -858,10 +873,11 @@ 
 						regulator-name = "vdd-2v8-display";
 						regulator-min-microvolt = <2800000>;
 						regulator-max-microvolt = <2800000>;
+						regulator-always-on;
 						regulator-boot-on;
 					};
 
-					ldo3 {
+					vdd_1v2_ap: ldo3 {
 						regulator-name = "avdd-1v2";
 						regulator-min-microvolt = <1200000>;
 						regulator-max-microvolt = <1200000>;
@@ -1048,7 +1064,7 @@ 
 			regulator-boot-on;
 		};
 
-		regulator@1 {
+		vdd_lcd: regulator@1 {
 			compatible = "regulator-fixed";
 			reg = <1>;
 			regulator-name = "vdd_lcd_1v8";
@@ -1057,6 +1073,7 @@ 
 			vin-supply = <&vdd_1v8>;
 			enable-active-high;
 			gpio = <&gpio TEGRA_GPIO(U, 4) GPIO_ACTIVE_HIGH>;
+			regulator-always-on;
 			regulator-boot-on;
 		};