[PATCH 0/3] drm/bridge: various small lvds-encoder things

[PATCH 0/3] drm/bridge: various small lvds-encoder things

[Peter Rosin]

Hi!

These patches are useful for me. Please consider merging.

Cheers,
Peter

Peter Rosin (3):
  dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c187
  dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios
  drm/bridge: add pwdn-gpios support to the lvds-encoder

 .../bindings/display/bridge/lvds-transmitter.txt   |  4 +++
 drivers/gpu/drm/bridge/lvds-encoder.c              | 34 ++++++++++++++++++++++
 2 files changed, 38 insertions(+)

-- 
2.11.0

[PATCH 1/3] dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c187

[Peter Rosin]

This chip is compatible with "lvds-encoder".

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..f9e7dd666f58 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -24,6 +24,7 @@ Required properties:
 
 - compatible: Must be one or more of the following
   - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
+  - "ti,ds90c187" for the TI DS90C187 Dual Pixel FPD-Link Serializer
   - "lvds-encoder" for a generic LVDS encoder device
 
   When compatible with the generic version, nodes must list the
-- 
2.11.0

[PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios

[Peter Rosin]

Add optional property to specify a power-down GPIO.
The pwdn-gpios name is already in use by the thine,thc63lvdm83d
binding, so go with that.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index f9e7dd666f58..47941d39f92f 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -31,6 +31,9 @@ Required properties:
   device-specific version corresponding to the device first
   followed by the generic version.
 
+Optional properties:
+- pwdn-gpios: Power-down control GPIO
+
 Required nodes:
 
 This device has two video ports. Their connections are modeled using the OF
-- 
2.11.0

Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios

[Andrzej Hajda]

On 19.12.2018 00:19, Peter Rosin wrote:
> Add optional property to specify a power-down GPIO.
> The pwdn-gpios name is already in use by the thine,thc63lvdm83d
> binding, so go with that.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index f9e7dd666f58..47941d39f92f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -31,6 +31,9 @@ Required properties:
>    device-specific version corresponding to the device first
>    followed by the generic version.
>  
> +Optional properties:
> +- pwdn-gpios: Power-down control GPIO
> +


Since naming is not enforced by any datasheet I would propose something
more popular with less twisted logic. Maybe:

- enable-gpios: ... (active high).


Regards

Andrzej


>  Required nodes:
>  
>  This device has two video ports. Their connections are modeled using the OF

Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios

[Peter Rosin]

On 2018-12-19 10:12, Andrzej Hajda wrote:
> On 19.12.2018 00:19, Peter Rosin wrote:
>> Add optional property to specify a power-down GPIO.
>> The pwdn-gpios name is already in use by the thine,thc63lvdm83d
>> binding, so go with that.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> index f9e7dd666f58..47941d39f92f 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>> @@ -31,6 +31,9 @@ Required properties:
>>    device-specific version corresponding to the device first
>>    followed by the generic version.
>>  
>> +Optional properties:
>> +- pwdn-gpios: Power-down control GPIO
>> +
> 
> 
> Since naming is not enforced by any datasheet I would propose something
> more popular with less twisted logic. Maybe:
> 
> - enable-gpios: ... (active high).

That was my original thought too, but the driver implementing the
lvds-encoder bindings also handles the mentioned thine,thc63lvdm83d
lvds encoder, and that binding has the "pwdn" naming. So, for driver
implementation simplicity I went with what was already there, thus
allowing adding support for both bindings with one implementation
(in patch 3/3).

Adding code just to handle multiple names for the same thing does
not sounds too appealing.

Cheers,
Peter

Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios

[Laurent Pinchart]

Hello,

On Wednesday, 19 December 2018 11:57:32 EET Peter Rosin wrote:
> On 2018-12-19 10:12, Andrzej Hajda wrote:
> > On 19.12.2018 00:19, Peter Rosin wrote:
> >> Add optional property to specify a power-down GPIO.
> >> The pwdn-gpios name is already in use by the thine,thc63lvdm83d
> >> binding, so go with that.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >> 
> >>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt |
> >>  3 +++ 
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >> index f9e7dd666f58..47941d39f92f 100644
> >> ---
> >> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >> +++
> >> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> >> @@ -31,6 +31,9 @@ Required properties:
> >>    device-specific version corresponding to the device first
> >>    followed by the generic version.
> >> 
> >> +Optional properties:
> >> +- pwdn-gpios: Power-down control GPIO
> >> +
> > 
> > Since naming is not enforced by any datasheet I would propose something
> > more popular with less twisted logic. Maybe:
> > 
> > - enable-gpios: ... (active high).
> 
> That was my original thought too, but the driver implementing the
> lvds-encoder bindings also handles the mentioned thine,thc63lvdm83d
> lvds encoder, and that binding has the "pwdn" naming. So, for driver
> implementation simplicity I went with what was already there, thus
> allowing adding support for both bindings with one implementation
> (in patch 3/3).
> 
> Adding code just to handle multiple names for the same thing does
> not sounds too appealing.

I'm afraid I think we shouldn't add pwdn-gpios support to the lvds encoder DT 
bindings. The reason is that control GPIOs (and regulators) come with device-
specific semantics. If we add pwdn-gpios now, we'll then add reset-gpios, and 
power supplies, and all of a sudden we'll end up having to encode sequencing 
of GPIOs and power supplies in DT. That path has been tried in the past, with 
no good results.

I would instead create device-specific bindings, like done for 
thine,thc63lvdm83d. It's fine to then add support for the pwdn-gpios property 
in the lvds-encoder driver, as long as the meaning of the property comes from 
specific DT bindings, not from the generic ones.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] dt-bindings: display: bridge: lvds-transmitter: add pwdn-gpios

[Peter Rosin]

On 2018-12-19 12:38, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday, 19 December 2018 11:57:32 EET Peter Rosin wrote:
>> On 2018-12-19 10:12, Andrzej Hajda wrote:
>>> On 19.12.2018 00:19, Peter Rosin wrote:
>>>> Add optional property to specify a power-down GPIO.
>>>> The pwdn-gpios name is already in use by the thine,thc63lvdm83d
>>>> binding, so go with that.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>
>>>>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt |
>>>>  3 +++ 
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>>>> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>>>> index f9e7dd666f58..47941d39f92f 100644
>>>> ---
>>>> a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>>>> +++
>>>> b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
>>>> @@ -31,6 +31,9 @@ Required properties:
>>>>    device-specific version corresponding to the device first
>>>>    followed by the generic version.
>>>>
>>>> +Optional properties:
>>>> +- pwdn-gpios: Power-down control GPIO
>>>> +
>>>
>>> Since naming is not enforced by any datasheet I would propose something
>>> more popular with less twisted logic. Maybe:
>>>
>>> - enable-gpios: ... (active high).
>>
>> That was my original thought too, but the driver implementing the
>> lvds-encoder bindings also handles the mentioned thine,thc63lvdm83d
>> lvds encoder, and that binding has the "pwdn" naming. So, for driver
>> implementation simplicity I went with what was already there, thus
>> allowing adding support for both bindings with one implementation
>> (in patch 3/3).
>>
>> Adding code just to handle multiple names for the same thing does
>> not sounds too appealing.
> 
> I'm afraid I think we shouldn't add pwdn-gpios support to the lvds encoder DT 
> bindings. The reason is that control GPIOs (and regulators) come with device-
> specific semantics. If we add pwdn-gpios now, we'll then add reset-gpios, and 
> power supplies, and all of a sudden we'll end up having to encode sequencing 
> of GPIOs and power supplies in DT. That path has been tried in the past, with 
> no good results.
> 
> I would instead create device-specific bindings, like done for 
> thine,thc63lvdm83d. It's fine to then add support for the pwdn-gpios property 
> in the lvds-encoder driver, as long as the meaning of the property comes from 
> specific DT bindings, not from the generic ones.

Right, I'll fork out the bindings for the texas chips. v2 coming up.

Cheers,
Peter

[PATCH 3/3] drm/bridge: add pwdn-gpios support to the lvds-encoder

[Peter Rosin]

Optionally power down the LVDS-encoder when it is not in use.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..43d584a6a6b9 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -11,11 +11,13 @@
 #include <drm/drm_bridge.h>
 #include <drm/drm_panel.h>
 
+#include <linux/gpio/consumer.h>
 #include <linux/of_graph.h>
 
 struct lvds_encoder {
 	struct drm_bridge bridge;
 	struct drm_bridge *panel_bridge;
+	struct gpio_desc *pwdn_gpio;
 };
 
 static int lvds_encoder_attach(struct drm_bridge *bridge)
@@ -28,8 +30,30 @@ static int lvds_encoder_attach(struct drm_bridge *bridge)
 				 bridge);
 }
 
+static void lvds_encoder_enable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds_encoder = container_of(bridge,
+							 struct lvds_encoder,
+							 bridge);
+
+	if (lvds_encoder->pwdn_gpio)
+		gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 0);
+}
+
+static void lvds_encoder_disable(struct drm_bridge *bridge)
+{
+	struct lvds_encoder *lvds_encoder = container_of(bridge,
+							 struct lvds_encoder,
+							 bridge);
+
+	if (lvds_encoder->pwdn_gpio)
+		gpiod_set_value_cansleep(lvds_encoder->pwdn_gpio, 1);
+}
+
 static struct drm_bridge_funcs funcs = {
 	.attach = lvds_encoder_attach,
+	.enable = lvds_encoder_enable,
+	.disable = lvds_encoder_disable,
 };
 
 static int lvds_encoder_probe(struct platform_device *pdev)
@@ -45,6 +69,16 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	if (!lvds_encoder)
 		return -ENOMEM;
 
+	lvds_encoder->pwdn_gpio = devm_gpiod_get_optional(&pdev->dev, "pwdn",
+							  GPIOD_OUT_HIGH);
+	if (IS_ERR(lvds_encoder->pwdn_gpio)) {
+		int err = PTR_ERR(lvds_encoder->pwdn_gpio);
+
+		if (err != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "pwdn GPIO failure: %d\n", err);
+		return err;
+	}
+
 	/* Locate the panel DT node. */
 	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
 	if (!port) {
-- 
2.11.0