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

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

[Peter Rosin]

Hi!

I'm not sure if I should have added the texas chips to the lvds_encoder_match
list in the driver, right next to the thine,thc63lvdm83d entry, but ended
up not doing that. That can always be added later, if needed...

Changes since v2:
- changed from pwdn-gpios to powerdown-gpios after discussion with Rob with
  new patch 3/5 updating the thine,thc63lvdm83d binding as well as a
  consequence
- added patch 4/5 which helps keep lines shorter in the lvds-encoder driver
- added tag from Rob on patch 2/5

Changes since v1:
- fork out the bindings for the texas chips into their own file in order
  to avoid clutter in the generic lvds-transmitter binding.
- added a patch to remove some surplus stuff in the generic lvds-transmitter
  binding.

Cheers,
Peter

Peter Rosin (5):
  dt-bindings: display: bridge: fork out ti,ds90c185 from
    lvds-transmitter
  dt-bindings: display: bridge: lvds-transmitter: cleanup example
  dt-bindings: display: bridge: thc63lvdm83d: use standard
    powerdown-gpios
  drm/bridge: lvds-encoder: add dev helper variable in .probe()
  drm/bridge: lvds-encoder: add powerdown-gpios support

 .../bindings/display/bridge/lvds-transmitter.txt   | 10 +---
 .../bindings/display/bridge/thine,thc63lvdm83d.txt |  2 +-
 .../bindings/display/bridge/ti,ds90c185.txt        | 55 ++++++++++++++++++++++
 drivers/gpu/drm/bridge/lvds-encoder.c              | 53 +++++++++++++++++----
 4 files changed, 100 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt

-- 
2.11.0

[PATCH v3 1/5] dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter

[Peter Rosin]

DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
binding, which is meant to be generic.

The sister chip DS90C187 is similar to DS90C185, describe it here as well.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../bindings/display/bridge/lvds-transmitter.txt   |  8 +---
 .../bindings/display/bridge/ti,ds90c185.txt        | 55 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index 50220190c203..fd39ad34c383 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,13 +22,7 @@ among others.
 
 Required properties:
 
-- compatible: Must be one or more of the following
-  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
-  - "lvds-encoder" for a generic LVDS encoder device
-
-  When compatible with the generic version, nodes must list the
-  device-specific version corresponding to the device first
-  followed by the generic version.
+- compatible: Must be "lvds-encoder"
 
 Required nodes:
 
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
new file mode 100644
index 000000000000..e575f996959a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
@@ -0,0 +1,55 @@
+Texas Instruments FPD-Link (LVDS) Serializer
+--------------------------------------------
+
+The DS90C185 and DS90C187 are low-power serializers for portable
+battery-powered applications that reduces the size of the RGB
+interface between the host GPU and the display.
+
+Required properties:
+
+- compatible: Should be
+  "ti,ds90c185", "lvds-encoder"  for the TI DS90C185 FPD-Link Serializer
+  "ti,ds90c187", "lvds-encoder"  for the TI DS90C187 FPD-Link Serializer
+
+Optional properties:
+
+- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
+
+Required nodes:
+
+The devices have two video ports. Their connections are modeled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for parallel input
+- Video port 1 for LVDS output
+
+
+Example
+-------
+
+lvds-encoder {
+	compatible = "ti,ds90c185", "lvds-encoder";
+
+	powerdown-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			lvds_enc_in: endpoint {
+				remote-endpoint = <&lcdc_out_rgb>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			lvds_enc_out: endpoint {
+				remote-endpoint = <&lvds_panel_in>;
+			};
+		};
+	};
+};
-- 
2.11.0

[PATCH v3 2/5] dt-bindings: display: bridge: lvds-transmitter: cleanup example

[Peter Rosin]

Drop #address-cells and #size-cells from the root node in the
example, they are unused.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
index fd39ad34c383..36ce07c8d8e6 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -38,8 +38,6 @@ Example
 
 lvds-encoder {
 	compatible = "lvds-encoder";
-	#address-cells = <1>;
-	#size-cells = <0>;
 
 	ports {
 		#address-cells = <1>;
-- 
2.11.0

[PATCH v3 3/5] dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios

[Peter Rosin]

The name powerdown-gpios is the standard property name for the
functionality covered by the previous pwdn-gpios name. This rename
should be safe to do since the linux driver supporting the binding
(lvds-encoder.c) never implemented the property, and no dts file
names it. At least not upstream.

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

diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
index 527e236e9a2a..fee3c88e1a17 100644
--- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
@@ -10,7 +10,7 @@ Required properties:
 
 Optional properties:
 
-- pwdn-gpios: Power down control GPIO
+- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low).
 
 Required nodes:
 
-- 
2.11.0

[PATCH v3 4/5] drm/bridge: lvds-encoder: add dev helper variable in .probe()

[Peter Rosin]

Make the code easier to read and modify.

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

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 75b0d3f6e4de..f6770e83d49d 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -34,48 +34,47 @@ static struct drm_bridge_funcs funcs = {
 
 static int lvds_encoder_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct device_node *port;
 	struct device_node *endpoint;
 	struct device_node *panel_node;
 	struct drm_panel *panel;
 	struct lvds_encoder *lvds_encoder;
 
-	lvds_encoder = devm_kzalloc(&pdev->dev, sizeof(*lvds_encoder),
-				    GFP_KERNEL);
+	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
 	if (!lvds_encoder)
 		return -ENOMEM;
 
 	/* Locate the panel DT node. */
-	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
+	port = of_graph_get_port_by_id(dev->of_node, 1);
 	if (!port) {
-		dev_dbg(&pdev->dev, "port 1 not found\n");
+		dev_dbg(dev, "port 1 not found\n");
 		return -ENXIO;
 	}
 
 	endpoint = of_get_child_by_name(port, "endpoint");
 	of_node_put(port);
 	if (!endpoint) {
-		dev_dbg(&pdev->dev, "no endpoint for port 1\n");
+		dev_dbg(dev, "no endpoint for port 1\n");
 		return -ENXIO;
 	}
 
 	panel_node = of_graph_get_remote_port_parent(endpoint);
 	of_node_put(endpoint);
 	if (!panel_node) {
-		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
+		dev_dbg(dev, "no remote endpoint for port 1\n");
 		return -ENXIO;
 	}
 
 	panel = of_drm_find_panel(panel_node);
 	of_node_put(panel_node);
 	if (!panel) {
-		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
+		dev_dbg(dev, "panel not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
 
 	lvds_encoder->panel_bridge =
-		devm_drm_panel_bridge_add(&pdev->dev,
-					  panel, DRM_MODE_CONNECTOR_LVDS);
+		devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_LVDS);
 	if (IS_ERR(lvds_encoder->panel_bridge))
 		return PTR_ERR(lvds_encoder->panel_bridge);
 
@@ -83,7 +82,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 	 * but we need a bridge attached to our of_node for our user
 	 * to look up.
 	 */
-	lvds_encoder->bridge.of_node = pdev->dev.of_node;
+	lvds_encoder->bridge.of_node = dev->of_node;
 	lvds_encoder->bridge.funcs = &funcs;
 	drm_bridge_add(&lvds_encoder->bridge);
 
-- 
2.11.0

[PATCH v3 5/5] drm/bridge: lvds-encoder: add powerdown-gpios support

[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 f6770e83d49d..36d8557bc097 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 *powerdown_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->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_encoder->powerdown_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->powerdown_gpio)
+		gpiod_set_value_cansleep(lvds_encoder->powerdown_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->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+							       GPIOD_OUT_HIGH);
+	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
+		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
+
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "powerdown GPIO failure: %d\n", err);
+		return err;
+	}
+
 	/* Locate the panel DT node. */
 	port = of_graph_get_port_by_id(dev->of_node, 1);
 	if (!port) {
-- 
2.11.0

Re: [PATCH v3 1/5] dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter

[Rob Herring]

On Sat, Dec 29, 2018 at 07:07:19AM +0000, Peter Rosin wrote:
> DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
> binding, which is meant to be generic.
> 
> The sister chip DS90C187 is similar to DS90C185, describe it here as well.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  .../bindings/display/bridge/lvds-transmitter.txt   |  8 +---
>  .../bindings/display/bridge/ti,ds90c185.txt        | 55 ++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index 50220190c203..fd39ad34c383 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -22,13 +22,7 @@ among others.
>  
>  Required properties:
>  
> -- compatible: Must be one or more of the following
> -  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
> -  - "lvds-encoder" for a generic LVDS encoder device
> -
> -  When compatible with the generic version, nodes must list the
> -  device-specific version corresponding to the device first
> -  followed by the generic version.

I think you should keep something here. I'd reword it to say any encoder 
with additional properties must have a device specific compatible in 
addition.

> +- compatible: Must be "lvds-encoder"
>  
>  Required nodes:
>  
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> new file mode 100644
> index 000000000000..e575f996959a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> @@ -0,0 +1,55 @@
> +Texas Instruments FPD-Link (LVDS) Serializer
> +--------------------------------------------
> +
> +The DS90C185 and DS90C187 are low-power serializers for portable
> +battery-powered applications that reduces the size of the RGB
> +interface between the host GPU and the display.
> +
> +Required properties:
> +
> +- compatible: Should be
> +  "ti,ds90c185", "lvds-encoder"  for the TI DS90C185 FPD-Link Serializer
> +  "ti,ds90c187", "lvds-encoder"  for the TI DS90C187 FPD-Link Serializer
> +
> +Optional properties:
> +
> +- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
> +
> +Required nodes:
> +
> +The devices have two video ports. Their connections are modeled using the OF
> +graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> +
> +- Video port 0 for parallel input
> +- Video port 1 for LVDS output
> +
> +
> +Example
> +-------
> +
> +lvds-encoder {
> +	compatible = "ti,ds90c185", "lvds-encoder";
> +
> +	powerdown-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@0 {
> +			reg = <0>;
> +
> +			lvds_enc_in: endpoint {
> +				remote-endpoint = <&lcdc_out_rgb>;
> +			};
> +		};
> +
> +		port@1 {
> +			reg = <1>;
> +
> +			lvds_enc_out: endpoint {
> +				remote-endpoint = <&lvds_panel_in>;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.11.0
> 

Re: [PATCH v3 3/5] dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios

[Rob Herring]

On Sat, 29 Dec 2018 07:07:31 +0000, Peter Rosin wrote:
> The name powerdown-gpios is the standard property name for the
> functionality covered by the previous pwdn-gpios name. This rename
> should be safe to do since the linux driver supporting the binding
> (lvds-encoder.c) never implemented the property, and no dts file
> names it. At least not upstream.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/5] dt-bindings: display: bridge: lvds-transmitter: cleanup example

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Sat, Dec 29, 2018 at 07:07:25AM +0000, Peter Rosin wrote:
> Drop #address-cells and #size-cells from the root node in the
> example, they are unused.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> index fd39ad34c383..36ce07c8d8e6 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> @@ -38,8 +38,6 @@ Example
>  
>  lvds-encoder {
>  	compatible = "lvds-encoder";
> -	#address-cells = <1>;
> -	#size-cells = <0>;
>  
>  	ports {
>  		#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/5] dt-bindings: display: bridge: fork out ti,ds90c185 from lvds-transmitter

[Laurent Pinchart]

Hello,

On Fri, Jan 11, 2019 at 08:49:04AM -0600, Rob Herring wrote:
> On Sat, Dec 29, 2018 at 07:07:19AM +0000, Peter Rosin wrote:
> > DS90C185 has a shutdown pin which does not fit in the lvds-transmitter
> > binding, which is meant to be generic.
> > 
> > The sister chip DS90C187 is similar to DS90C185, describe it here as well.
> > 
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  .../bindings/display/bridge/lvds-transmitter.txt   |  8 +---
> >  .../bindings/display/bridge/ti,ds90c185.txt        | 55 ++++++++++++++++++++++
> >  2 files changed, 56 insertions(+), 7 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > index 50220190c203..fd39ad34c383 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
> > @@ -22,13 +22,7 @@ among others.
> >  
> >  Required properties:
> >  
> > -- compatible: Must be one or more of the following
> > -  - "ti,ds90c185" for the TI DS90C185 FPD-Link Serializer
> > -  - "lvds-encoder" for a generic LVDS encoder device
> > -
> > -  When compatible with the generic version, nodes must list the
> > -  device-specific version corresponding to the device first
> > -  followed by the generic version.
> 
> I think you should keep something here. I'd reword it to say any encoder 
> with additional properties must have a device specific compatible in 
> addition.

I was going to mention this too. Maybe

- compatible: Shall contain a device-specific compatible string follow
  by "lvds-encoder".

That's stronger than Rob's suggestion, as I don't think we should allow
omitting the device-specific compatible string in any case to make sure
we can preserve backward compatibility in the future.

> > +- compatible: Must be "lvds-encoder"
> >  
> >  Required nodes:
> >  
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> > new file mode 100644
> > index 000000000000..e575f996959a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
> > @@ -0,0 +1,55 @@
> > +Texas Instruments FPD-Link (LVDS) Serializer
> > +--------------------------------------------
> > +
> > +The DS90C185 and DS90C187 are low-power serializers for portable
> > +battery-powered applications that reduces the size of the RGB
> > +interface between the host GPU and the display.
> > +
> > +Required properties:
> > +
> > +- compatible: Should be
> > +  "ti,ds90c185", "lvds-encoder"  for the TI DS90C185 FPD-Link Serializer
> > +  "ti,ds90c187", "lvds-encoder"  for the TI DS90C187 FPD-Link Serializer

s/  for/ for/

With these fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +Optional properties:
> > +
> > +- powerdown-gpios: Power down control GPIO (the PDB pin, active-low)
> > +
> > +Required nodes:
> > +
> > +The devices have two video ports. Their connections are modeled using the OF
> > +graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> > +
> > +- Video port 0 for parallel input
> > +- Video port 1 for LVDS output
> > +
> > +
> > +Example
> > +-------
> > +
> > +lvds-encoder {
> > +	compatible = "ti,ds90c185", "lvds-encoder";
> > +
> > +	powerdown-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +
> > +			lvds_enc_in: endpoint {
> > +				remote-endpoint = <&lcdc_out_rgb>;
> > +			};
> > +		};
> > +
> > +		port@1 {
> > +			reg = <1>;
> > +
> > +			lvds_enc_out: endpoint {
> > +				remote-endpoint = <&lvds_panel_in>;
> > +			};
> > +		};
> > +	};
> > +};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/5] dt-bindings: display: bridge: thc63lvdm83d: use standard powerdown-gpios

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Sat, Dec 29, 2018 at 07:07:31AM +0000, Peter Rosin wrote:
> The name powerdown-gpios is the standard property name for the
> functionality covered by the previous pwdn-gpios name. This rename
> should be safe to do since the linux driver supporting the binding
> (lvds-encoder.c) never implemented the property, and no dts file
> names it. At least not upstream.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> index 527e236e9a2a..fee3c88e1a17 100644
> --- a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> @@ -10,7 +10,7 @@ Required properties:
>  
>  Optional properties:
>  
> -- pwdn-gpios: Power down control GPIO
> +- powerdown-gpios: Power down control GPIO (the /PWDN pin, active low).
>  
>  Required nodes:
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 4/5] drm/bridge: lvds-encoder: add dev helper variable in .probe()

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Sat, Dec 29, 2018 at 07:07:37AM +0000, Peter Rosin wrote:
> Make the code easier to read and modify.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/lvds-encoder.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 75b0d3f6e4de..f6770e83d49d 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -34,48 +34,47 @@ static struct drm_bridge_funcs funcs = {
>  
>  static int lvds_encoder_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct device_node *port;
>  	struct device_node *endpoint;
>  	struct device_node *panel_node;
>  	struct drm_panel *panel;
>  	struct lvds_encoder *lvds_encoder;
>  
> -	lvds_encoder = devm_kzalloc(&pdev->dev, sizeof(*lvds_encoder),
> -				    GFP_KERNEL);
> +	lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
>  	if (!lvds_encoder)
>  		return -ENOMEM;
>  
>  	/* Locate the panel DT node. */
> -	port = of_graph_get_port_by_id(pdev->dev.of_node, 1);
> +	port = of_graph_get_port_by_id(dev->of_node, 1);
>  	if (!port) {
> -		dev_dbg(&pdev->dev, "port 1 not found\n");
> +		dev_dbg(dev, "port 1 not found\n");
>  		return -ENXIO;
>  	}
>  
>  	endpoint = of_get_child_by_name(port, "endpoint");
>  	of_node_put(port);
>  	if (!endpoint) {
> -		dev_dbg(&pdev->dev, "no endpoint for port 1\n");
> +		dev_dbg(dev, "no endpoint for port 1\n");
>  		return -ENXIO;
>  	}
>  
>  	panel_node = of_graph_get_remote_port_parent(endpoint);
>  	of_node_put(endpoint);
>  	if (!panel_node) {
> -		dev_dbg(&pdev->dev, "no remote endpoint for port 1\n");
> +		dev_dbg(dev, "no remote endpoint for port 1\n");
>  		return -ENXIO;
>  	}
>  
>  	panel = of_drm_find_panel(panel_node);
>  	of_node_put(panel_node);
>  	if (!panel) {
> -		dev_dbg(&pdev->dev, "panel not found, deferring probe\n");
> +		dev_dbg(dev, "panel not found, deferring probe\n");
>  		return -EPROBE_DEFER;
>  	}
>  
>  	lvds_encoder->panel_bridge =
> -		devm_drm_panel_bridge_add(&pdev->dev,
> -					  panel, DRM_MODE_CONNECTOR_LVDS);
> +		devm_drm_panel_bridge_add(dev, panel, DRM_MODE_CONNECTOR_LVDS);
>  	if (IS_ERR(lvds_encoder->panel_bridge))
>  		return PTR_ERR(lvds_encoder->panel_bridge);
>  
> @@ -83,7 +82,7 @@ static int lvds_encoder_probe(struct platform_device *pdev)
>  	 * but we need a bridge attached to our of_node for our user
>  	 * to look up.
>  	 */
> -	lvds_encoder->bridge.of_node = pdev->dev.of_node;
> +	lvds_encoder->bridge.of_node = dev->of_node;
>  	lvds_encoder->bridge.funcs = &funcs;
>  	drm_bridge_add(&lvds_encoder->bridge);
>  
> -- 
> 2.11.0
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 5/5] drm/bridge: lvds-encoder: add powerdown-gpios support

[Laurent Pinchart]

Hi Peter,

Thank you for the patch.

On Sat, Dec 29, 2018 at 07:07:43AM +0000, Peter Rosin wrote:
> Optionally power down the LVDS-encoder when it is not in use.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

As the powerdown-gpios property is only defined for two encoders, and
both of them should work well with this code, I think the change is
fine. In the future if different power-up/down sequences need to be
implemented for different devices this will need to be made
device-specific.

> ---
>  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 f6770e83d49d..36d8557bc097 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 *powerdown_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->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);

gpiod_set_value_cansleep() already contains a NULL check, so you can
drop it from here.

> +}
> +
> +static void lvds_encoder_disable(struct drm_bridge *bridge)
> +{
> +	struct lvds_encoder *lvds_encoder = container_of(bridge,
> +							 struct lvds_encoder,
> +							 bridge);
> +
> +	if (lvds_encoder->powerdown_gpio)
> +		gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);

Same here.

> +}
> +
>  static struct drm_bridge_funcs funcs = {

While at it, could you make this const ?

With these fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	.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->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +							       GPIOD_OUT_HIGH);
> +	if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> +		int err = PTR_ERR(lvds_encoder->powerdown_gpio);
> +
> +		if (err != -EPROBE_DEFER)
> +			dev_err(dev, "powerdown GPIO failure: %d\n", err);
> +		return err;
> +	}
> +
>  	/* Locate the panel DT node. */
>  	port = of_graph_get_port_by_id(dev->of_node, 1);
>  	if (!port) {

-- 
Regards,

Laurent Pinchart