[PATCH v8 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[PATCH v8 0/2] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Jacopo Mondi]

Hello,
  new version with last bits hopefully fixed.

The vcc supply is now mandatory, as suggested by Mark Brown, and the driver
requires it to be described in device tree.

The "OE" GPIO is now described by 'oe' again, as I wrongly interpreted Rob's
suggestions on v6.

A few minor grammar fixes in bindings and in driver as suggested by Laurent.

As per v7, Eagle disaply enablement based on this series, is sent separately.

Thanks
   j

v7 -> b8:
- Make 'vcc' supply mandatory
- Use 'oe' property name to describe "OE" pin
- Minor fixes as suggested by Laurent on bindings and driver

v6 -> v7:
- Use semi-standard names for powerdown and output enable GPIOs as suggested
  by Rob and Vladimir
- Use 'regulator_get()' not the optional version, and list only 'vcc' as
  requested supply
- Addressed Laurent's review comments and removed Eagle display enablement patch
  to be sent separately

v5 -> v6:
- Drop check for CONFIG_OF as it is a Kconfig dependency
- Add Niklas Reviewed-by tags
- List [3/3] depenencies below commit message to ease integration

v4 -> v5:
- Fix punctuation in bindings documentation
- Add small statement to bindings document to clarify the chip has no
  control bus
- Print regulator name in enable/disable routines error path
- Add Andrzej Reviewed-by tag

v3 -> v4:
- Rename permutations of "pdwn" to just "pdwn" everywhere in the series
- Improve power enable/disable routines as suggested by Andrzej and Sergei
- Change "pdwn" gpio initialization to use the logical output level
- Change Kconfig description

v2 -> v3:
- Drop support for "lvds-decoder" and make the driver THC63LVD1024 specific
-- Rework bindings to describe multiple input/output ports
-- Rename driver and remove "lvds-decoder" references
-- Rework Eagle DTS to use new bindings

v1 -> v2:
- Drop support for THC63LVD1024

Jacopo Mondi (2):
  dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
  drm: bridge: Add thc63lvd1024 LVDS decoder driver

 .../bindings/display/bridge/thine,thc63lvd1024.txt |  60 ++++++
 drivers/gpu/drm/bridge/Kconfig                     |   6 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c              | 206 +++++++++++++++++++++
 4 files changed, 273 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

--
2.7.4

[PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Jacopo Mondi]

Document Thine THC63LVD1024 LVDS decoder device tree bindings.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
new file mode 100644
index 0000000..0b23e70
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -0,0 +1,60 @@
+Thine Electronics THC63LVD1024 LVDS decoder
+-------------------------------------------
+
+The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
+to parallel data outputs. The chip supports single/dual input/output modes,
+handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
+
+Single or dual operation mode, output data mapping and DDR output modes are
+configured through input signals and the chip does not expose any control bus.
+
+Required properties:
+- compatible: Shall be "thine,thc63lvd1024"
+- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
+  PPL and digital circuitry
+
+Optional properties:
+- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
+- oe-gpios: Output enable GPIO signal, pin name "OE". Active high
+
+The THC63LVD1024 video port connections are modeled according
+to OF graph bindings specified by Documentation/devicetree/bindings/graph.txt
+
+Required video port nodes:
+- port@0: First LVDS input port
+- port@2: First digital CMOS/TTL parallel output
+
+Optional video port nodes:
+- port@1: Second LVDS input port
+- port@3: Second digital CMOS/TTL parallel output
+
+Example:
+--------
+
+	thc63lvd1024: lvds-decoder {
+		compatible = "thine,thc63lvd1024";
+
+		vcc-supply = <&reg_lvds_vcc>;
+		powerdown-gpios = <&gpio4 15 GPIO_ACTIVE_LOW>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				lvds_dec_in_0: endpoint {
+					remote-endpoint = <&lvds_out>;
+				};
+			};
+
+			port@2{
+				reg = <2>;
+
+				lvds_dec_out_2: endpoint {
+					remote-endpoint = <&adv7511_in>;
+				};
+			};
+		};
+	};
--
2.7.4

[PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Jacopo Mondi]

Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
output converter.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/gpu/drm/bridge/Kconfig        |   6 +
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c | 206 ++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3aa65bd..42c9c2d 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -93,6 +93,12 @@ config DRM_SII9234
 	  It is an I2C driver, that detects connection of MHL bridge
 	  and starts encapsulation of HDMI signal.
 
+config DRM_THINE_THC63LVD1024
+	tristate "Thine THC63LVD1024 LVDS decoder bridge"
+	depends on OF
+	---help---
+	  Thine THC63LVD1024 LVDS/parallel converter driver.
+
 config DRM_TOSHIBA_TC358767
 	tristate "Toshiba TC358767 eDP bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..fd90b16 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
+obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
new file mode 100644
index 0000000..aa1d650
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THC63LVD1024 LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+
+enum thc63_ports {
+	THC63_LVDS_IN0,
+	THC63_LVDS_IN1,
+	THC63_RGB_OUT0,
+	THC63_RGB_OUT1,
+};
+
+struct thc63_dev {
+	struct device *dev;
+
+	struct regulator *vcc;
+
+	struct gpio_desc *pdwn;
+	struct gpio_desc *oe;
+
+	struct drm_bridge bridge;
+	struct drm_bridge *next;
+};
+
+static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct thc63_dev, bridge);
+}
+
+static int thc63_attach(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+
+	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
+}
+
+static void thc63_enable(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	int ret;
+
+	ret = regulator_enable(thc63->vcc);
+	if (ret) {
+		dev_err(thc63->dev,
+			"Failed to enable regulator \"vcc\": %d\n", ret);
+		return;
+	}
+
+	gpiod_set_value(thc63->pdwn, 0);
+	gpiod_set_value(thc63->oe, 1);
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	int ret;
+
+	gpiod_set_value(thc63->oe, 0);
+	gpiod_set_value(thc63->pdwn, 1);
+
+	ret = regulator_disable(thc63->vcc);
+	if (ret)
+		dev_err(thc63->dev,
+			"Failed to disable regulator \"vcc\": %d\n", ret);
+}
+
+static const struct drm_bridge_funcs thc63_bridge_func = {
+	.attach	= thc63_attach,
+	.enable = thc63_enable,
+	.disable = thc63_disable,
+};
+
+static int thc63_parse_dt(struct thc63_dev *thc63)
+{
+	struct device_node *thc63_out;
+	struct device_node *remote;
+
+	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
+						  THC63_RGB_OUT0, -1);
+	if (!thc63_out) {
+		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
+			THC63_RGB_OUT0);
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(thc63_out);
+	of_node_put(thc63_out);
+	if (!remote) {
+		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
+			THC63_RGB_OUT0);
+		return -ENODEV;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
+			THC63_RGB_OUT0);
+		of_node_put(remote);
+		return -ENODEV;
+	}
+
+	thc63->next = of_drm_find_bridge(remote);
+	if (!thc63->next) {
+		of_node_put(remote);
+		return -EPROBE_DEFER;
+	}
+
+	return 0;
+}
+
+static int thc63_gpio_init(struct thc63_dev *thc63)
+{
+	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
+	if (IS_ERR(thc63->oe)) {
+		dev_err(thc63->dev, "Unable to get \"oe-gpios\": %ld\n",
+			PTR_ERR(thc63->oe));
+		return PTR_ERR(thc63->oe);
+	}
+
+	thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "powerdown",
+					      GPIOD_OUT_HIGH);
+	if (IS_ERR(thc63->pdwn)) {
+		dev_err(thc63->dev, "Unable to get \"powerdown-gpios\": %ld\n",
+			PTR_ERR(thc63->pdwn));
+		return PTR_ERR(thc63->pdwn);
+	}
+
+	return 0;
+}
+
+static int thc63_probe(struct platform_device *pdev)
+{
+	struct thc63_dev *thc63;
+	int ret;
+
+	thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
+	if (!thc63)
+		return -ENOMEM;
+
+	thc63->dev = &pdev->dev;
+	platform_set_drvdata(pdev, thc63);
+
+	thc63->vcc = devm_regulator_get_optional(thc63->dev, "vcc");
+	if (IS_ERR(thc63->vcc)) {
+		if (PTR_ERR(thc63->vcc) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+
+		dev_err(thc63->dev, "Unable to get \"vcc\" supply: %ld\n",
+			PTR_ERR(thc63->vcc));
+		return PTR_ERR(thc63->vcc);
+	}
+
+	ret = thc63_gpio_init(thc63);
+	if (ret)
+		return ret;
+
+	ret = thc63_parse_dt(thc63);
+	if (ret)
+		return ret;
+
+	thc63->bridge.driver_private = thc63;
+	thc63->bridge.of_node = pdev->dev.of_node;
+	thc63->bridge.funcs = &thc63_bridge_func;
+
+	drm_bridge_add(&thc63->bridge);
+
+	return 0;
+}
+
+static int thc63_remove(struct platform_device *pdev)
+{
+	struct thc63_dev *thc63 = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&thc63->bridge);
+
+	return 0;
+}
+
+static const struct of_device_id thc63_match[] = {
+	{ .compatible = "thine,thc63lvd1024", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, thc63_match);
+
+static struct platform_driver thc63_driver = {
+	.probe	= thc63_probe,
+	.remove	= thc63_remove,
+	.driver	= {
+		.name		= "thc63lvd1024",
+		.of_match_table	= thc63_match,
+	},
+};
+module_platform_driver(thc63_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
+MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[jacopo mondi]

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

Hello,
   small self review, as I've just noticed a trivial error.

On Tue, Apr 10, 2018 at 12:53:10PM +0200, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 206 ++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3aa65bd..42c9c2d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -93,6 +93,12 @@ config DRM_SII9234
>  	  It is an I2C driver, that detects connection of MHL bridge
>  	  and starts encapsulation of HDMI signal.
>
> +config DRM_THINE_THC63LVD1024
> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
> +	depends on OF
> +	---help---
> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>  	tristate "Toshiba TC358767 eDP bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 0000000..aa1d650
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum thc63_ports {
> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_RGB_OUT0,
> +	THC63_RGB_OUT1,
> +};
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vcc;
> +
> +	struct gpio_desc *pdwn;
> +	struct gpio_desc *oe;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	int ret;
> +
> +	ret = regulator_enable(thc63->vcc);
> +	if (ret) {
> +		dev_err(thc63->dev,
> +			"Failed to enable regulator \"vcc\": %d\n", ret);
> +		return;
> +	}
> +
> +	gpiod_set_value(thc63->pdwn, 0);
> +	gpiod_set_value(thc63->oe, 1);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	int ret;
> +
> +	gpiod_set_value(thc63->oe, 0);
> +	gpiod_set_value(thc63->pdwn, 1);
> +
> +	ret = regulator_disable(thc63->vcc);
> +	if (ret)
> +		dev_err(thc63->dev,
> +			"Failed to disable regulator \"vcc\": %d\n", ret);
> +}
> +
> +static const struct drm_bridge_funcs thc63_bridge_func = {
> +	.attach	= thc63_attach,
> +	.enable = thc63_enable,
> +	.disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> +	struct device_node *thc63_out;
> +	struct device_node *remote;
> +
> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						  THC63_RGB_OUT0, -1);
> +	if (!thc63_out) {
> +		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> +			THC63_RGB_OUT0);
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(thc63_out);
> +	of_node_put(thc63_out);
> +	if (!remote) {
> +		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> +			THC63_RGB_OUT0);
> +		return -ENODEV;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> +			THC63_RGB_OUT0);
> +		of_node_put(remote);
> +		return -ENODEV;
> +	}
> +
> +	thc63->next = of_drm_find_bridge(remote);
> +	if (!thc63->next) {
> +		of_node_put(remote);
> +		return -EPROBE_DEFER;
> +	}

the 'remote' OF node is only put in the error path.

If there are no other comments on this driver I can send a follow up
patch, or it's easier I can resend the series being it just two small
patches.

Thanks
   j


> +
> +	return 0;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get \"oe-gpios\": %ld\n",
> +			PTR_ERR(thc63->oe));
> +		return PTR_ERR(thc63->oe);
> +	}
> +
> +	thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "powerdown",
> +					      GPIOD_OUT_HIGH);
> +	if (IS_ERR(thc63->pdwn)) {
> +		dev_err(thc63->dev, "Unable to get \"powerdown-gpios\": %ld\n",
> +			PTR_ERR(thc63->pdwn));
> +		return PTR_ERR(thc63->pdwn);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63;
> +	int ret;
> +
> +	thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
> +	if (!thc63)
> +		return -ENOMEM;
> +
> +	thc63->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, thc63);
> +
> +	thc63->vcc = devm_regulator_get_optional(thc63->dev, "vcc");
> +	if (IS_ERR(thc63->vcc)) {
> +		if (PTR_ERR(thc63->vcc) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		dev_err(thc63->dev, "Unable to get \"vcc\" supply: %ld\n",
> +			PTR_ERR(thc63->vcc));
> +		return PTR_ERR(thc63->vcc);
> +	}
> +
> +	ret = thc63_gpio_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc63_parse_dt(thc63);
> +	if (ret)
> +		return ret;
> +
> +	thc63->bridge.driver_private = thc63;
> +	thc63->bridge.of_node = pdev->dev.of_node;
> +	thc63->bridge.funcs = &thc63_bridge_func;
> +
> +	drm_bridge_add(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thc63_match[] = {
> +	{ .compatible = "thine,thc63lvd1024", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +
> +static struct platform_driver thc63_driver = {
> +	.probe	= thc63_probe,
> +	.remove	= thc63_remove,
> +	.driver	= {
> +		.name		= "thc63lvd1024",
> +		.of_match_table	= thc63_match,
> +	},
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

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

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 7744 bytes --]

Hello,
   small self review, as I've just noticed a trivial error.

On Tue, Apr 10, 2018 at 12:53:10PM +0200, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   6 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 206 ++++++++++++++++++++++++++++++++++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3aa65bd..42c9c2d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -93,6 +93,12 @@ config DRM_SII9234
>  	  It is an I2C driver, that detects connection of MHL bridge
>  	  and starts encapsulation of HDMI signal.
>
> +config DRM_THINE_THC63LVD1024
> +	tristate "Thine THC63LVD1024 LVDS decoder bridge"
> +	depends on OF
> +	---help---
> +	  Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>  	tristate "Toshiba TC358767 eDP bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 0000000..aa1d650
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of_graph.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum thc63_ports {
> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_RGB_OUT0,
> +	THC63_RGB_OUT1,
> +};
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vcc;
> +
> +	struct gpio_desc *pdwn;
> +	struct gpio_desc *oe;
> +
> +	struct drm_bridge bridge;
> +	struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +
> +	return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	int ret;
> +
> +	ret = regulator_enable(thc63->vcc);
> +	if (ret) {
> +		dev_err(thc63->dev,
> +			"Failed to enable regulator \"vcc\": %d\n", ret);
> +		return;
> +	}
> +
> +	gpiod_set_value(thc63->pdwn, 0);
> +	gpiod_set_value(thc63->oe, 1);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	int ret;
> +
> +	gpiod_set_value(thc63->oe, 0);
> +	gpiod_set_value(thc63->pdwn, 1);
> +
> +	ret = regulator_disable(thc63->vcc);
> +	if (ret)
> +		dev_err(thc63->dev,
> +			"Failed to disable regulator \"vcc\": %d\n", ret);
> +}
> +
> +static const struct drm_bridge_funcs thc63_bridge_func = {
> +	.attach	= thc63_attach,
> +	.enable = thc63_enable,
> +	.disable = thc63_disable,
> +};
> +
> +static int thc63_parse_dt(struct thc63_dev *thc63)
> +{
> +	struct device_node *thc63_out;
> +	struct device_node *remote;
> +
> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node,
> +						  THC63_RGB_OUT0, -1);
> +	if (!thc63_out) {
> +		dev_err(thc63->dev, "Missing endpoint in port@%u\n",
> +			THC63_RGB_OUT0);
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(thc63_out);
> +	of_node_put(thc63_out);
> +	if (!remote) {
> +		dev_err(thc63->dev, "Endpoint in port@%u unconnected\n",
> +			THC63_RGB_OUT0);
> +		return -ENODEV;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(thc63->dev, "port@%u remote endpoint is disabled\n",
> +			THC63_RGB_OUT0);
> +		of_node_put(remote);
> +		return -ENODEV;
> +	}
> +
> +	thc63->next = of_drm_find_bridge(remote);
> +	if (!thc63->next) {
> +		of_node_put(remote);
> +		return -EPROBE_DEFER;
> +	}

the 'remote' OF node is only put in the error path.

If there are no other comments on this driver I can send a follow up
patch, or it's easier I can resend the series being it just two small
patches.

Thanks
   j


> +
> +	return 0;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get \"oe-gpios\": %ld\n",
> +			PTR_ERR(thc63->oe));
> +		return PTR_ERR(thc63->oe);
> +	}
> +
> +	thc63->pdwn = devm_gpiod_get_optional(thc63->dev, "powerdown",
> +					      GPIOD_OUT_HIGH);
> +	if (IS_ERR(thc63->pdwn)) {
> +		dev_err(thc63->dev, "Unable to get \"powerdown-gpios\": %ld\n",
> +			PTR_ERR(thc63->pdwn));
> +		return PTR_ERR(thc63->pdwn);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thc63_probe(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63;
> +	int ret;
> +
> +	thc63 = devm_kzalloc(&pdev->dev, sizeof(*thc63), GFP_KERNEL);
> +	if (!thc63)
> +		return -ENOMEM;
> +
> +	thc63->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, thc63);
> +
> +	thc63->vcc = devm_regulator_get_optional(thc63->dev, "vcc");
> +	if (IS_ERR(thc63->vcc)) {
> +		if (PTR_ERR(thc63->vcc) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		dev_err(thc63->dev, "Unable to get \"vcc\" supply: %ld\n",
> +			PTR_ERR(thc63->vcc));
> +		return PTR_ERR(thc63->vcc);
> +	}
> +
> +	ret = thc63_gpio_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	ret = thc63_parse_dt(thc63);
> +	if (ret)
> +		return ret;
> +
> +	thc63->bridge.driver_private = thc63;
> +	thc63->bridge.of_node = pdev->dev.of_node;
> +	thc63->bridge.funcs = &thc63_bridge_func;
> +
> +	drm_bridge_add(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static int thc63_remove(struct platform_device *pdev)
> +{
> +	struct thc63_dev *thc63 = platform_get_drvdata(pdev);
> +
> +	drm_bridge_remove(&thc63->bridge);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id thc63_match[] = {
> +	{ .compatible = "thine,thc63lvd1024", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +
> +static struct platform_driver thc63_driver = {
> +	.probe	= thc63_probe,
> +	.remove	= thc63_remove,
> +	.driver	= {
> +		.name		= "thc63lvd1024",
> +		.of_match_table	= thc63_match,
> +	},
> +};
> +module_platform_driver(thc63_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo@jmondi.org>");
> +MODULE_DESCRIPTION("Thine THC63LVD1024 LVDS decoder DRM bridge driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Rob Herring]

On Tue, Apr 10, 2018 at 12:53:09PM +0200, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt

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

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Rob Herring]

On Tue, Apr 10, 2018 at 12:53:09PM +0200, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt

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

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo, Laurent,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 0000000..0b23e70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +-------------------------------------------
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> +
> +Single or dual operation mode, output data mapping and DDR output modes are
> +configured through input signals and the chip does not expose any control bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +
> +Optional properties:
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low

Thank you for the change.

I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
as far as I understand it is only described in DT bindings documentation,
and the property is unused in the driver or board DTS files at the moment.

> +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high

Okay :)

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo, Laurent,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 0000000..0b23e70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +-------------------------------------------
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> +
> +Single or dual operation mode, output data mapping and DDR output modes are
> +configured through input signals and the chip does not expose any control bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +
> +Optional properties:
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low

Thank you for the change.

I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
as far as I understand it is only described in DT bindings documentation,
and the property is unused in the driver or board DTS files at the moment.

> +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high

Okay :)

--
With best wishes,
Vladimir
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo, Laurent,
> 
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
>> ---
>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> new file mode 100644
>> index 0000000..0b23e70
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> @@ -0,0 +1,60 @@
>> +Thine Electronics THC63LVD1024 LVDS decoder
>> +-------------------------------------------
>> +
>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
>> +to parallel data outputs. The chip supports single/dual input/output modes,
>> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
>> +
>> +Single or dual operation mode, output data mapping and DDR output modes are
>> +configured through input signals and the chip does not expose any control bus.
>> +
>> +Required properties:
>> +- compatible: Shall be "thine,thc63lvd1024"
>> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>> +  PPL and digital circuitry
>> +
>> +Optional properties:
>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> 

sorry for a follow-up, I've just noticed it, could you please double check
spelling PDWN vs PWDN? Thank you in advance.

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo, Laurent,
> 
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
>> ---
>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> new file mode 100644
>> index 0000000..0b23e70
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> @@ -0,0 +1,60 @@
>> +Thine Electronics THC63LVD1024 LVDS decoder
>> +-------------------------------------------
>> +
>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
>> +to parallel data outputs. The chip supports single/dual input/output modes,
>> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
>> +
>> +Single or dual operation mode, output data mapping and DDR output modes are
>> +configured through input signals and the chip does not expose any control bus.
>> +
>> +Required properties:
>> +- compatible: Shall be "thine,thc63lvd1024"
>> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>> +  PPL and digital circuitry
>> +
>> +Optional properties:
>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> 

sorry for a follow-up, I've just noticed it, could you please double check
spelling PDWN vs PWDN? Thank you in advance.

--
With best wishes,
Vladimir
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:48 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo,
> 
> On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
>> Hi Jacopo, Laurent,
>>
>> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>
>>> ---

[snip]

>>> +Optional properties:
>>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>>
> 
> sorry for a follow-up, I've just noticed it, could you please double check
> spelling PDWN vs PWDN? Thank you in advance.
> 

please ignore it, I did it myself and the datasheet describes pin as /PDWN,
I won't exclude a typo in the datasheet though...

--
With best wishes,
Vladimir

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:48 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo,
> 
> On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
>> Hi Jacopo, Laurent,
>>
>> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>
>>> ---

[snip]

>>> +Optional properties:
>>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>>
> 
> sorry for a follow-up, I've just noticed it, could you please double check
> spelling PDWN vs PWDN? Thank you in advance.
> 

please ignore it, I did it myself and the datasheet describes pin as /PDWN,
I won't exclude a typo in the datasheet though...

--
With best wishes,
Vladimir
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[jacopo mondi]

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

On Thu, Apr 19, 2018 at 12:44:32PM +0300, Vladimir Zapolskiy wrote:
Hi Vladimir,

> Hi Jacopo, Laurent,
>
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > new file mode 100644
> > index 0000000..0b23e70
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,60 @@
> > +Thine Electronics THC63LVD1024 LVDS decoder
> > +-------------------------------------------
> > +
> > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
> > +to parallel data outputs. The chip supports single/dual input/output modes,
> > +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> > +
> > +Single or dual operation mode, output data mapping and DDR output modes are
> > +configured through input signals and the chip does not expose any control bus.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024"
> > +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> > +  PPL and digital circuitry
> > +
> > +Optional properties:
> > +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>
> Thank you for the change.
>
> I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
> as far as I understand it is only described in DT bindings documentation,
> and the property is unused in the driver or board DTS files at the moment.

Thanks for the suggestion, I'll do that!

>
> > +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high
>
> Okay :)

Yeah, please see Rob's and Laurent's reply to v7, where I renamed this
to 'enable'.

Thanks
   j
>
> --
> With best wishes,
> Vladimir

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

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 2534 bytes --]

On Thu, Apr 19, 2018 at 12:44:32PM +0300, Vladimir Zapolskiy wrote:
Hi Vladimir,

> Hi Jacopo, Laurent,
>
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 ++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > new file mode 100644
> > index 0000000..0b23e70
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,60 @@
> > +Thine Electronics THC63LVD1024 LVDS decoder
> > +-------------------------------------------
> > +
> > +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS streams
> > +to parallel data outputs. The chip supports single/dual input/output modes,
> > +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> > +
> > +Single or dual operation mode, output data mapping and DDR output modes are
> > +configured through input signals and the chip does not expose any control bus.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024"
> > +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> > +  PPL and digital circuitry
> > +
> > +Optional properties:
> > +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>
> Thank you for the change.
>
> I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
> as far as I understand it is only described in DT bindings documentation,
> and the property is unused in the driver or board DTS files at the moment.

Thanks for the suggestion, I'll do that!

>
> > +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high
>
> Okay :)

Yeah, please see Rob's and Laurent's reply to v7, where I renamed this
to 'enable'.

Thanks
   j
>
> --
> With best wishes,
> Vladimir

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Generally I have only one pretty ignorable comment.

> +
> +enum thc63_ports {
> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_RGB_OUT0,
> +	THC63_RGB_OUT1,
> +};
> +

The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
configuration is ignored.

I don't know if right from the beginning it would be better to support
dual-out modes, preferably both single-in and dual-in ones. Will it
impact port enumeration?

I do understand that the extension is possible, and likely only hardware
accessibility postpones it.

--
With best wishes,
Vladimir

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Generally I have only one pretty ignorable comment.

> +
> +enum thc63_ports {
> +	THC63_LVDS_IN0,
> +	THC63_LVDS_IN1,
> +	THC63_RGB_OUT0,
> +	THC63_RGB_OUT1,
> +};
> +

The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
configuration is ignored.

I don't know if right from the beginning it would be better to support
dual-out modes, preferably both single-in and dual-in ones. Will it
impact port enumeration?

I do understand that the extension is possible, and likely only hardware
accessibility postpones it.

--
With best wishes,
Vladimir

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[jacopo mondi]

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

Hi Vladimir,

On Thu, Apr 19, 2018 at 02:18:30PM +0300, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output converter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>

Thanks. FYI I sent v9 yesterday with a minimal change compared to v8.

> Generally I have only one pretty ignorable comment.
>
> > +
> > +enum thc63_ports {
> > +	THC63_LVDS_IN0,
> > +	THC63_LVDS_IN1,
> > +	THC63_RGB_OUT0,
> > +	THC63_RGB_OUT1,
> > +};
> > +
>
> The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
> configuration is ignored.
>
> I don't know if right from the beginning it would be better to support
> dual-out modes, preferably both single-in and dual-in ones. Will it
> impact port enumeration?

The bindings have been designed to support dual in/out modes, as you
can see there are 4 possible ports described there:

Required video port nodes:
- port@0: First LVDS input port
- port@2: First digital CMOS/TTL parallel output

Optional video port nodes:
- port@1: Second LVDS input port
- port@3: Second digital CMOS/TTL parallel output

Future extension should not require changing the port enumeration,
just add a property to specify the selected mode.

>
> I do understand that the extension is possible, and likely only hardware
> accessibility postpones it.

Yes, hardware on one side, but also what I think is a shortcoming of
DRM (which exists in other sub-systems, say v4l2) that matches devices
on their OF device nodes and makes cumbersome handling drivers wanting
to register on 'port' nodes instead, as it would happen if you have 2
input endpoints.

See my [1] note here: https://lkml.org/lkml/2018/3/9/422
And this reply to Archit's comment which has been left floating as it
is not a real issue (yet): https://lkml.org/lkml/2018/3/10/214

Thanks
   j

>
> --
> With best wishes,
> Vladimir

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

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[jacopo mondi]

[-- Attachment #1.1: Type: text/plain, Size: 2219 bytes --]

Hi Vladimir,

On Thu, Apr 19, 2018 at 02:18:30PM +0300, Vladimir Zapolskiy wrote:
> Hi Jacopo,
>
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output converter.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>

Thanks. FYI I sent v9 yesterday with a minimal change compared to v8.

> Generally I have only one pretty ignorable comment.
>
> > +
> > +enum thc63_ports {
> > +	THC63_LVDS_IN0,
> > +	THC63_LVDS_IN1,
> > +	THC63_RGB_OUT0,
> > +	THC63_RGB_OUT1,
> > +};
> > +
>
> The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
> configuration is ignored.
>
> I don't know if right from the beginning it would be better to support
> dual-out modes, preferably both single-in and dual-in ones. Will it
> impact port enumeration?

The bindings have been designed to support dual in/out modes, as you
can see there are 4 possible ports described there:

Required video port nodes:
- port@0: First LVDS input port
- port@2: First digital CMOS/TTL parallel output

Optional video port nodes:
- port@1: Second LVDS input port
- port@3: Second digital CMOS/TTL parallel output

Future extension should not require changing the port enumeration,
just add a property to specify the selected mode.

>
> I do understand that the extension is possible, and likely only hardware
> accessibility postpones it.

Yes, hardware on one side, but also what I think is a shortcoming of
DRM (which exists in other sub-systems, say v4l2) that matches devices
on their OF device nodes and makes cumbersome handling drivers wanting
to register on 'port' nodes instead, as it would happen if you have 2
input endpoints.

See my [1] note here: https://lkml.org/lkml/2018/3/9/422
And this reply to Archit's comment which has been left floating as it
is not a real issue (yet): https://lkml.org/lkml/2018/3/10/214

Thanks
   j

>
> --
> With best wishes,
> Vladimir

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel