[PATCH 0/4] drm/dsi/tegra: continuous clock support

[PATCH 0/4] drm/dsi/tegra: continuous clock support

[Alexandre Courbot]

This small series adds support for DSI continuous clock as required by the
DSI specification and as needed for NVIDIA SHIELD's panel. Continuous clock
mode can be specified as the flags of a DSI device, and host drivers can check
this flag to adapt their behavior.

The flag is then added to the panel embedded in NVIDIA SHIELD, which allows
us to finally enable the panel in its device tree.

Alexandre Courbot (4):
  drm/dsi: Add flag for continuous clock behavior
  drm/tegra: dsi - Handle continuous clock flag
  drm/panel: LG LH500WX1-SD03 uses continuous clock
  ARM: tegra: roth: add display DT node

 arch/arm/boot/dts/tegra114-roth.dts  | 23 ++++++++++++++++++++---
 drivers/gpu/drm/panel/panel-simple.c |  2 +-
 drivers/gpu/drm/tegra/dsi.c          |  3 ++-
 include/drm/drm_mipi_dsi.h           |  2 ++
 4 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.0.0

[PATCH 0/4] drm/dsi/tegra: continuous clock support

[Alexandre Courbot]

This small series adds support for DSI continuous clock as required by the
DSI specification and as needed for NVIDIA SHIELD's panel. Continuous clock
mode can be specified as the flags of a DSI device, and host drivers can check
this flag to adapt their behavior.

The flag is then added to the panel embedded in NVIDIA SHIELD, which allows
us to finally enable the panel in its device tree.

Alexandre Courbot (4):
  drm/dsi: Add flag for continuous clock behavior
  drm/tegra: dsi - Handle continuous clock flag
  drm/panel: LG LH500WX1-SD03 uses continuous clock
  ARM: tegra: roth: add display DT node

 arch/arm/boot/dts/tegra114-roth.dts  | 23 ++++++++++++++++++++---
 drivers/gpu/drm/panel/panel-simple.c |  2 +-
 drivers/gpu/drm/tegra/dsi.c          |  3 ++-
 include/drm/drm_mipi_dsi.h           |  2 ++
 4 files changed, 25 insertions(+), 5 deletions(-)

-- 
2.0.0

[PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

As per section 5.6.1 of the DSI specification, all DSI transmitters must
support continuous clock behavior on the clock lane, while non-continuous
mode support is only optional. Add a flag that allows devices to indicate
that they require continuous clock mode to operate properly.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 include/drm/drm_mipi_dsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 944f33f..5913ef4 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
 #define MIPI_DSI_MODE_VSYNC_FLUSH	BIT(8)
 /* disable EoT packets in HS mode */
 #define MIPI_DSI_MODE_EOT_PACKET	BIT(9)
+/* use continuous clock behavior on the clock lane */
+#define MIPI_DSI_MODE_CLOCK_CONTINUOUS	BIT(10)
 
 enum mipi_dsi_pixel_format {
 	MIPI_DSI_FMT_RGB888,
-- 
2.0.0

[PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

As per section 5.6.1 of the DSI specification, all DSI transmitters must
support continuous clock behavior on the clock lane, while non-continuous
mode support is only optional. Add a flag that allows devices to indicate
that they require continuous clock mode to operate properly.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 include/drm/drm_mipi_dsi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 944f33f..5913ef4 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
 #define MIPI_DSI_MODE_VSYNC_FLUSH	BIT(8)
 /* disable EoT packets in HS mode */
 #define MIPI_DSI_MODE_EOT_PACKET	BIT(9)
+/* use continuous clock behavior on the clock lane */
+#define MIPI_DSI_MODE_CLOCK_CONTINUOUS	BIT(10)
 
 enum mipi_dsi_pixel_format {
 	MIPI_DSI_FMT_RGB888,
-- 
2.0.0

[PATCH 2/4] drm/tegra: dsi - Handle continuous clock flag

[Alexandre Courbot]

Handle the MIPI_DSI_MODE_CLOCK_CONTINUOUS flag and only set TX-only
clock behavior when this flag is not present for the DSI device.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/drm/tegra/dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index bd56f2a..79777f8 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -474,7 +474,8 @@ static int tegra_output_dsi_enable(struct tegra_output *output)
 	tegra_dsi_writel(dsi, value, DSI_HOST_CONTROL);
 
 	value = tegra_dsi_readl(dsi, DSI_CONTROL);
-	value |= DSI_CONTROL_HS_CLK_CTRL;
+	if (!(dsi->flags & MIPI_DSI_MODE_CLOCK_CONTINUOUS))
+		value |= DSI_CONTROL_HS_CLK_CTRL;
 	value &= ~DSI_CONTROL_TX_TRIG(3);
 	value &= ~DSI_CONTROL_DCS_ENABLE;
 	value |= DSI_CONTROL_VIDEO_ENABLE;
-- 
2.0.0

[PATCH 2/4] drm/tegra: dsi - Handle continuous clock flag

[Alexandre Courbot]

Handle the MIPI_DSI_MODE_CLOCK_CONTINUOUS flag and only set TX-only
clock behavior when this flag is not present for the DSI device.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/drm/tegra/dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index bd56f2a..79777f8 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -474,7 +474,8 @@ static int tegra_output_dsi_enable(struct tegra_output *output)
 	tegra_dsi_writel(dsi, value, DSI_HOST_CONTROL);
 
 	value = tegra_dsi_readl(dsi, DSI_CONTROL);
-	value |= DSI_CONTROL_HS_CLK_CTRL;
+	if (!(dsi->flags & MIPI_DSI_MODE_CLOCK_CONTINUOUS))
+		value |= DSI_CONTROL_HS_CLK_CTRL;
 	value &= ~DSI_CONTROL_TX_TRIG(3);
 	value &= ~DSI_CONTROL_DCS_ENABLE;
 	value |= DSI_CONTROL_VIDEO_ENABLE;
-- 
2.0.0

[PATCH 3/4] drm/panel: LG LH500WX1-SD03 uses continuous clock

[Alexandre Courbot]

This panel can only operate in continuous clock behavior, so set the
appropriate flag to inform host drivers of this fact.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a251361..428cdf3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -572,7 +572,7 @@ static const struct panel_desc_dsi lg_lh500wx1_sd03 = {
 			.height = 110,
 		},
 	},
-	.flags = MIPI_DSI_MODE_VIDEO,
+	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_CLOCK_CONTINUOUS,
 	.format = MIPI_DSI_FMT_RGB888,
 	.lanes = 4,
 };
-- 
2.0.0

[PATCH 3/4] drm/panel: LG LH500WX1-SD03 uses continuous clock

[Alexandre Courbot]

This panel can only operate in continuous clock behavior, so set the
appropriate flag to inform host drivers of this fact.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/drm/panel/panel-simple.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a251361..428cdf3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -572,7 +572,7 @@ static const struct panel_desc_dsi lg_lh500wx1_sd03 = {
 			.height = 110,
 		},
 	},
-	.flags = MIPI_DSI_MODE_VIDEO,
+	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_CLOCK_CONTINUOUS,
 	.format = MIPI_DSI_FMT_RGB888,
 	.lanes = 4,
 };
-- 
2.0.0

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

[Alexandre Courbot]

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(-)

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

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

[Alexandre Courbot]

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(-)

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

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

[Stephen Warren]

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?

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

[Stephen Warren]

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?

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

[Alexandre Courbot]

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.

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

[Alexandre Courbot]

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.

[PATCH v2] ARM: tegra: roth: add display DT node

[Alexandre Courbot]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes since v1:
- Removed unneeded regulator-always-on property for vdd_lcd regulator
Only patch 4/4 of the original series has been resent for this v2.

 arch/arm/boot/dts/tegra114-roth.dts | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts
index ba210c6e189f..c7c6825f11fb 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";
-- 
2.0.0

[PATCH v2] ARM: tegra: roth: add display DT node

[Alexandre Courbot]

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>
---
Changes since v1:
- Removed unneeded regulator-always-on property for vdd_lcd regulator
Only patch 4/4 of the original series has been resent for this v2.

 arch/arm/boot/dts/tegra114-roth.dts | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114-roth.dts b/arch/arm/boot/dts/tegra114-roth.dts
index ba210c6e189f..c7c6825f11fb 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";
-- 
2.0.0

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Andrzej Hajda]

Hi Alexandre,

Thanks for the patch.

On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
> As per section 5.6.1 of the DSI specification, all DSI transmitters must
> support continuous clock behavior on the clock lane, while non-continuous
> mode support is only optional. Add a flag that allows devices to indicate
> that they require continuous clock mode to operate properly.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  include/drm/drm_mipi_dsi.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 944f33f..5913ef4 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>  #define MIPI_DSI_MODE_VSYNC_FLUSH	BIT(8)
>  /* disable EoT packets in HS mode */
>  #define MIPI_DSI_MODE_EOT_PACKET	BIT(9)
> +/* use continuous clock behavior on the clock lane */
> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS	BIT(10)
>  

According to MIPI DSI specification "All DSI transmitters and receivers
shall support continuous clock behavior on the Clock Lane, and
optionally may support non-continuous clock behavior". It suggests that
continuous clock should be default behavior. So maybe better is to
introduce sth like:
+#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS	BIT(10)


Regards
Andrzej

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

Hi Andrejz,

On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>
> Hi Alexandre,
>
> Thanks for the patch.
>
> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>> support continuous clock behavior on the clock lane, while non-continuous
>> mode support is only optional. Add a flag that allows devices to indicate
>> that they require continuous clock mode to operate properly.
>>
>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  include/drm/drm_mipi_dsi.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> index 944f33f..5913ef4 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>  /* disable EoT packets in HS mode */
>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>> +/* use continuous clock behavior on the clock lane */
>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>
>
> According to MIPI DSI specification "All DSI transmitters and receivers
> shall support continuous clock behavior on the Clock Lane, and
> optionally may support non-continuous clock behavior". It suggests that
> continuous clock should be default behavior. So maybe better is to
> introduce sth like:
> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)

I started under the assumption that current host drivers assumed
non-continuous clock (as the Tegra driver currently does). In that
light, it seemed to make sense (and to be less intrusive) to introduce
that flag as a restriction rather than a capability. But if you think
this should be a capability I am not strongly against it - either way,
host drivers need to be changed to take that flag into account.

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

Hi Andrejz,

On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> Hi Alexandre,
>
> Thanks for the patch.
>
> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>> support continuous clock behavior on the clock lane, while non-continuous
>> mode support is only optional. Add a flag that allows devices to indicate
>> that they require continuous clock mode to operate properly.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  include/drm/drm_mipi_dsi.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> index 944f33f..5913ef4 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>  /* disable EoT packets in HS mode */
>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>> +/* use continuous clock behavior on the clock lane */
>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>
>
> According to MIPI DSI specification "All DSI transmitters and receivers
> shall support continuous clock behavior on the Clock Lane, and
> optionally may support non-continuous clock behavior". It suggests that
> continuous clock should be default behavior. So maybe better is to
> introduce sth like:
> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)

I started under the assumption that current host drivers assumed
non-continuous clock (as the Tegra driver currently does). In that
light, it seemed to make sense (and to be less intrusive) to introduce
that flag as a restriction rather than a capability. But if you think
this should be a capability I am not strongly against it - either way,
host drivers need to be changed to take that flag into account.

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Andrzej Hajda]

On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
> Hi Andrejz,
>
> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> Hi Alexandre,
>>
>> Thanks for the patch.
>>
>> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>>> support continuous clock behavior on the clock lane, while non-continuous
>>> mode support is only optional. Add a flag that allows devices to indicate
>>> that they require continuous clock mode to operate properly.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  include/drm/drm_mipi_dsi.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index 944f33f..5913ef4 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>>  /* disable EoT packets in HS mode */
>>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>>> +/* use continuous clock behavior on the clock lane */
>>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>>
>> According to MIPI DSI specification "All DSI transmitters and receivers
>> shall support continuous clock behavior on the Clock Lane, and
>> optionally may support non-continuous clock behavior". It suggests that
>> continuous clock should be default behavior. So maybe better is to
>> introduce sth like:
>> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)
> I started under the assumption that current host drivers assumed
> non-continuous clock (as the Tegra driver currently does).

Exynos DSI driver uses continuous clock.
Currently, in mainline,  there are no more dsi hosts using drm_mipi_dsi.h.
As I stated before I prefer to follow dsi specification and it states
clearly that
continuous behavior is required, non-continouous is optional.
Moreover for tegra chip continuous behavior is also the default one.

Regards
Andrzej

>  In that
> light, it seemed to make sense (and to be less intrusive) to introduce
> that flag as a restriction rather than a capability. But if you think
> this should be a capability I am not strongly against it - either way,
> host drivers need to be changed to take that flag into account.
>

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Andrzej Hajda]

On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
> Hi Andrejz,
>
> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Hi Alexandre,
>>
>> Thanks for the patch.
>>
>> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>>> support continuous clock behavior on the clock lane, while non-continuous
>>> mode support is only optional. Add a flag that allows devices to indicate
>>> that they require continuous clock mode to operate properly.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> ---
>>>  include/drm/drm_mipi_dsi.h | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index 944f33f..5913ef4 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>>  /* disable EoT packets in HS mode */
>>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>>> +/* use continuous clock behavior on the clock lane */
>>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>>
>> According to MIPI DSI specification "All DSI transmitters and receivers
>> shall support continuous clock behavior on the Clock Lane, and
>> optionally may support non-continuous clock behavior". It suggests that
>> continuous clock should be default behavior. So maybe better is to
>> introduce sth like:
>> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)
> I started under the assumption that current host drivers assumed
> non-continuous clock (as the Tegra driver currently does).

Exynos DSI driver uses continuous clock.
Currently, in mainline,  there are no more dsi hosts using drm_mipi_dsi.h.
As I stated before I prefer to follow dsi specification and it states
clearly that
continuous behavior is required, non-continouous is optional.
Moreover for tegra chip continuous behavior is also the default one.

Regards
Andrzej

>  In that
> light, it seemed to make sense (and to be less intrusive) to introduce
> that flag as a restriction rather than a capability. But if you think
> this should be a capability I am not strongly against it - either way,
> host drivers need to be changed to take that flag into account.
>

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

On Fri, Jul 4, 2014 at 6:53 PM, Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
>> Hi Andrejz,
>>
>> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>> Hi Alexandre,
>>>
>>> Thanks for the patch.
>>>
>>> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>>>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>>>> support continuous clock behavior on the clock lane, while non-continuous
>>>> mode support is only optional. Add a flag that allows devices to indicate
>>>> that they require continuous clock mode to operate properly.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>  include/drm/drm_mipi_dsi.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>>> index 944f33f..5913ef4 100644
>>>> --- a/include/drm/drm_mipi_dsi.h
>>>> +++ b/include/drm/drm_mipi_dsi.h
>>>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>>>  /* disable EoT packets in HS mode */
>>>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>>>> +/* use continuous clock behavior on the clock lane */
>>>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>>>
>>> According to MIPI DSI specification "All DSI transmitters and receivers
>>> shall support continuous clock behavior on the Clock Lane, and
>>> optionally may support non-continuous clock behavior". It suggests that
>>> continuous clock should be default behavior. So maybe better is to
>>> introduce sth like:
>>> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)
>> I started under the assumption that current host drivers assumed
>> non-continuous clock (as the Tegra driver currently does).
>
> Exynos DSI driver uses continuous clock.
> Currently, in mainline,  there are no more dsi hosts using drm_mipi_dsi.h.
> As I stated before I prefer to follow dsi specification and it states
> clearly that
> continuous behavior is required, non-continouous is optional.
> Moreover for tegra chip continuous behavior is also the default one.

Makes perfect sense indeed, especially if we have only two users of
this interface for now. Will resubmit this series to make the Tegra
driver use continuous clock by default, and update the panels it used
to far to make use of the new flag.

Thanks,
Alex.

Re: [PATCH 1/4] drm/dsi: Add flag for continuous clock behavior

[Alexandre Courbot]

On Fri, Jul 4, 2014 at 6:53 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
>> Hi Andrejz,
>>
>> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> Hi Alexandre,
>>>
>>> Thanks for the patch.
>>>
>>> On 07/02/2014 02:19 PM, Alexandre Courbot wrote:
>>>> As per section 5.6.1 of the DSI specification, all DSI transmitters must
>>>> support continuous clock behavior on the clock lane, while non-continuous
>>>> mode support is only optional. Add a flag that allows devices to indicate
>>>> that they require continuous clock mode to operate properly.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>> ---
>>>>  include/drm/drm_mipi_dsi.h | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>>> index 944f33f..5913ef4 100644
>>>> --- a/include/drm/drm_mipi_dsi.h
>>>> +++ b/include/drm/drm_mipi_dsi.h
>>>> @@ -94,6 +94,8 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>>>>  #define MIPI_DSI_MODE_VSYNC_FLUSH    BIT(8)
>>>>  /* disable EoT packets in HS mode */
>>>>  #define MIPI_DSI_MODE_EOT_PACKET     BIT(9)
>>>> +/* use continuous clock behavior on the clock lane */
>>>> +#define MIPI_DSI_MODE_CLOCK_CONTINUOUS       BIT(10)
>>>>
>>> According to MIPI DSI specification "All DSI transmitters and receivers
>>> shall support continuous clock behavior on the Clock Lane, and
>>> optionally may support non-continuous clock behavior". It suggests that
>>> continuous clock should be default behavior. So maybe better is to
>>> introduce sth like:
>>> +#define MIPI_DSI_MODE_CLOCK_NON_CONTINUOUS     BIT(10)
>> I started under the assumption that current host drivers assumed
>> non-continuous clock (as the Tegra driver currently does).
>
> Exynos DSI driver uses continuous clock.
> Currently, in mainline,  there are no more dsi hosts using drm_mipi_dsi.h.
> As I stated before I prefer to follow dsi specification and it states
> clearly that
> continuous behavior is required, non-continouous is optional.
> Moreover for tegra chip continuous behavior is also the default one.

Makes perfect sense indeed, especially if we have only two users of
this interface for now. Will resubmit this series to make the Tegra
driver use continuous clock by default, and update the panels it used
to far to make use of the new flag.

Thanks,
Alex.

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

[Stephen Warren]

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.

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

[Stephen Warren]

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.

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Alexandre Courbot]

On Tue, Jul 22, 2014 at 7:51 AM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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.

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

[Alexandre Courbot]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]