| Message ID | 1404303560-32209-5-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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.
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.
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
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.
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.
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 --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; };
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(-)