2014-07-02 12:22:31

by Alexandre Courbot

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

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


2014-07-02 12:22:32

by Alexandre Courbot

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

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 <[email protected]>
---
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

2014-07-02 12:22:34

by Alexandre Courbot

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

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 <[email protected]>
---
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

2014-07-02 12:23:13

by Alexandre Courbot

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

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 <[email protected]>
---
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

2014-07-02 12:22:30

by Alexandre Courbot

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

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 <[email protected]>
---
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

2014-07-02 15:55:17

by Stephen Warren

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

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?

2014-07-03 03:11:01

by Alexandre Courbot

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

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.

2014-07-03 03:15:14

by Alexandre Courbot

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

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 <[email protected]>
---
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

2014-07-03 08:24:13

by Andrzej Hajda

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


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 <[email protected]>
> ---
> 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

2014-07-04 05:57:44

by Alexandre Courbot

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

Hi Andrejz,

On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <[email protected]> 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 <[email protected]>
>> ---
>> 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.

2014-07-04 09:53:43

by Andrzej Hajda

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

On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
> Hi Andrejz,
>
> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
>

2014-07-06 09:31:20

by Alexandre Courbot

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

On Fri, Jul 4, 2014 at 6:53 PM, Andrzej Hajda <[email protected]> wrote:
> On 07/04/2014 07:57 AM, Alexandre Courbot wrote:
>> Hi Andrejz,
>>
>> On Thu, Jul 3, 2014 at 5:23 PM, Andrzej Hajda <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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.

2014-07-21 15:35:32

by Stephen Warren

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

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.

2014-07-21 21:16:39

by Thierry Reding

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

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


Attachments:
(No filename) (1.72 kB)
(No filename) (836.00 B)
Download all attachments

2014-07-21 22:52:07

by Mark Brown

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

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.


Attachments:
(No filename) (1.14 kB)
signature.asc (819.00 B)
Digital signature
Download all attachments

2014-07-22 04:40:14

by Alexandre Courbot

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

On Tue, Jul 22, 2014 at 7:51 AM, Mark Brown <[email protected]> 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.

2014-07-22 09:46:16

by Mark Brown

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

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.


Attachments:
(No filename) (710.00 B)
signature.asc (819.00 B)
Digital signature
Download all attachments