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 v3:
- retained a (modified) note in lvds-transmitter.txt about encoders with
additional device-specific properties
- added tag from Rob on patch 3/5
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 | 12 ++---
.../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, 103 insertions(+), 19 deletions(-)
create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
--
2.11.0
Drop #address-cells and #size-cells from the root node in the
example, they are unused.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Peter Rosin <[email protected]>
---
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 bc6960741cb5..60091db5dfa5 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -42,8 +42,6 @@ Example
lvds-encoder {
compatible = "lvds-encoder";
- #address-cells = <1>;
- #size-cells = <0>;
ports {
#address-cells = <1>;
--
2.11.0
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.
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: Peter Rosin <[email protected]>
---
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
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 <[email protected]>
---
.../bindings/display/bridge/lvds-transmitter.txt | 10 ++--
.../bindings/display/bridge/ti,ds90c185.txt | 55 ++++++++++++++++++++++
2 files changed, 59 insertions(+), 6 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..bc6960741cb5 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-transmitter.txt
@@ -22,13 +22,11 @@ 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
+- compatible: Must be "lvds-encoder"
- When compatible with the generic version, nodes must list the
- device-specific version corresponding to the device first
- followed by the generic version.
+ Any encoder compatible with this generic binding, but with additional
+ properties not listed here, must list a device specific compatible first
+ followed by this generic compatible.
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
Make the code easier to read and modify.
Signed-off-by: Peter Rosin <[email protected]>
---
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
Optionally power down the LVDS-encoder when it is not in use.
Signed-off-by: Peter Rosin <[email protected]>
---
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
On Fri, 11 Jan 2019 15:18:55 +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 <[email protected]>
> ---
> .../bindings/display/bridge/lvds-transmitter.txt | 10 ++--
> .../bindings/display/bridge/ti,ds90c185.txt | 55 ++++++++++++++++++++++
> 2 files changed, 59 insertions(+), 6 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>
Reviewed-by: Rob Herring <[email protected]>
On 11.01.2019 16:19, Peter Rosin wrote:
> Make the code easier to read and modify.
>
> Signed-off-by: Peter Rosin <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
--
Regards
Andrzej
On 11.01.2019 16:19, Peter Rosin wrote:
> Optionally power down the LVDS-encoder when it is not in use.
>
> Signed-off-by: Peter Rosin <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
--
Regards
Andrzej
On 11.01.2019 16:18, Peter Rosin wrote:
> 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 v3:
> - retained a (modified) note in lvds-transmitter.txt about encoders with
> additional device-specific properties
> - added tag from Rob on patch 3/5
>
> 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
Queued to drm-misc-next.
One thing I have spotted after merging is that there no need to
null-check before gpiod_set_value_cansleep, no big deal, I guess.
Regards
Andrzej
>
> 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 | 12 ++---
> .../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, 103 insertions(+), 19 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,ds90c185.txt
>
The gpio API explicitly allows skipping the NULL check, precisely to
allow for neat support for optional gpios. Which is exactly what is at
play here.
Reported-by: Andrzej Hajda <[email protected]>
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/gpu/drm/bridge/lvds-encoder.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index 36d8557bc097..584007eaf6e1 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -36,8 +36,7 @@ static void lvds_encoder_enable(struct drm_bridge *bridge)
struct lvds_encoder,
bridge);
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
}
static void lvds_encoder_disable(struct drm_bridge *bridge)
@@ -46,8 +45,7 @@ static void lvds_encoder_disable(struct drm_bridge *bridge)
struct lvds_encoder,
bridge);
- if (lvds_encoder->powerdown_gpio)
- gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
+ gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
}
static struct drm_bridge_funcs funcs = {
--
2.11.0
On Fri, Jan 18, 2019 at 11:11:38PM +0000, Peter Rosin wrote:
> The gpio API explicitly allows skipping the NULL check, precisely to
> allow for neat support for optional gpios. Which is exactly what is at
> play here.
>
> Reported-by: Andrzej Hajda <[email protected]>
> Signed-off-by: Peter Rosin <[email protected]>
lgtm, Reviewed-by: Daniel Vetter <[email protected]>
btw plan to stick around a bit in drm land, i.e. want commit rights for
drm-misc?
-Daniel
> ---
> drivers/gpu/drm/bridge/lvds-encoder.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 36d8557bc097..584007eaf6e1 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -36,8 +36,7 @@ static void lvds_encoder_enable(struct drm_bridge *bridge)
> struct lvds_encoder,
> bridge);
>
> - if (lvds_encoder->powerdown_gpio)
> - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> + gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> }
>
> static void lvds_encoder_disable(struct drm_bridge *bridge)
> @@ -46,8 +45,7 @@ static void lvds_encoder_disable(struct drm_bridge *bridge)
> struct lvds_encoder,
> bridge);
>
> - if (lvds_encoder->powerdown_gpio)
> - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> + gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> }
>
> static struct drm_bridge_funcs funcs = {
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Peter,
Thank you for the patch.
On Fri, Jan 18, 2019 at 11:11:38PM +0000, Peter Rosin wrote:
> The gpio API explicitly allows skipping the NULL check, precisely to
> allow for neat support for optional gpios. Which is exactly what is at
> play here.
>
> Reported-by: Andrzej Hajda <[email protected]>
> Signed-off-by: Peter Rosin <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
> ---
> drivers/gpu/drm/bridge/lvds-encoder.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
> index 36d8557bc097..584007eaf6e1 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -36,8 +36,7 @@ static void lvds_encoder_enable(struct drm_bridge *bridge)
> struct lvds_encoder,
> bridge);
>
> - if (lvds_encoder->powerdown_gpio)
> - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> + gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 0);
> }
>
> static void lvds_encoder_disable(struct drm_bridge *bridge)
> @@ -46,8 +45,7 @@ static void lvds_encoder_disable(struct drm_bridge *bridge)
> struct lvds_encoder,
> bridge);
>
> - if (lvds_encoder->powerdown_gpio)
> - gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> + gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
> }
>
> static struct drm_bridge_funcs funcs = {
--
Regards,
Laurent Pinchart