[PATCH v3 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[PATCH v3 0/3] drm: Add Thine THC63LVD1024 LVDS decoder bridge

[Jacopo Mondi]

Hello,
   opposed to v2, this version drops the "transparent LVDS decoder" and provides
support for the Thine THC63LVD1024 chip only.

I removed all references to "lvds-decoder" and made driver and bindings
specific for the above mentioned chip.

Andrzej: Bindings now describe 2 available inputs (Port@0 mandatory, Port@1
optional) and 2 possible outputs (Port@2 mandatory, Port@3 optional). The driver
still takes only Port@0 and Port@2 into account. This leaves out discussions
on how support bridges with multiple input streams within the DRM framework from
this series.

Simon: Please drop the previous bindings proposal you marked as "deferred" as
that one is superseded by this new one.

I still plan to use this series to propose an API to propagate formats through
bridges, which is not possible at the moment, if I got things right.

Thanks
   j

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 (3):
  dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
  drm: bridge: Add thc63lvd1024 LVDS decoder driver
  arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

 .../bindings/display/bridge/thine,thc63lvd1024.txt |  63 ++++++
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 ++-
 drivers/gpu/drm/bridge/Kconfig                     |  10 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c              | 239 +++++++++++++++++++++
 5 files changed, 342 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

--
2.7.4

[PATCH v3 1/3] 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>
---
 .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 ++++++++++++++++++++++
 1 file changed, 63 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..5b5afcd
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
@@ -0,0 +1,63 @@
+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 two input LVDS stream and up to two digital CMOS/TTL outputs.
+
+Required properties:
+- compatible: Shall be "thine,thc63lvd1024",
+
+Optional properties:
+- vcc-supply: Power supply for TTL output and digital circuitry
+- cvcc-supply: Power supply for TTL CLOCKOUT signal
+- lvcc-supply: Power supply for LVDS inputs
+- pvcc-supply: Power supply for PLL circuitry
+- pwnd-gpios: Power down GPIO signal. Active low.
+- oe-gpios: Output enable GPIO signal. 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>;
+		lvcc-supply = <&reg_lvds_lvcc>;
+
+		pwdn-gpio = <&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 v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Jacopo Mondi]

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

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/bridge/Kconfig        |   7 +
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
 3 files changed, 249 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 3b99d5a..facf6bd 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -92,6 +92,13 @@ 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
+	select DRM_PANEL_BRIDGE
+	---help---
+	  Thine THC63LVD1024 LVDS decoder bridge 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..4b059c0
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,241 @@
+// 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>
+
+static const char * const thc63_reg_names[] = {
+	"vcc", "lvcc", "pvcc", "cvcc", };
+
+struct thc63_dev {
+	struct device *dev;
+
+	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
+
+	struct gpio_desc *pwdn;
+	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);
+	struct regulator *vcc;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
+		vcc = thc63->vccs[i];
+		if (vcc) {
+			ret = regulator_enable(vcc);
+			if (ret)
+				goto error_vcc_enable;
+		}
+	}
+
+	if (thc63->pwdn)
+		gpiod_set_value(thc63->pwdn, 0);
+
+	if (thc63->oe)
+		gpiod_set_value(thc63->oe, 1);
+
+	return;
+
+error_vcc_enable:
+	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+	struct thc63_dev *thc63 = to_thc63(bridge);
+	struct regulator *vcc;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
+		vcc = thc63->vccs[i];
+		if (vcc) {
+			ret = regulator_disable(vcc);
+			if (ret)
+				goto error_vcc_disable;
+		}
+	}
+
+	if (thc63->pwdn)
+		gpiod_set_value(thc63->pwdn, 1);
+
+	if (thc63->oe)
+		gpiod_set_value(thc63->oe, 0);
+
+	return;
+
+error_vcc_disable:
+	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
+}
+
+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;
+	int ret = 0;
+
+	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
+	if (!thc63_out) {
+		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
+		return -ENODEV;
+	}
+
+	remote = of_graph_get_remote_port_parent(thc63_out);
+	if (!remote) {
+		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
+		ret = -ENODEV;
+		goto error_put_out_node;
+	}
+
+	if (!of_device_is_available(remote)) {
+		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
+		ret = -ENODEV;
+		goto error_put_remote_node;
+	}
+
+	thc63->next = of_drm_find_bridge(remote);
+	if (!thc63->next)
+		ret = -EPROBE_DEFER;
+
+error_put_remote_node:
+	of_node_put(remote);
+error_put_out_node:
+	of_node_put(thc63_out);
+
+	return ret;
+}
+
+static int thc63_gpio_init(struct thc63_dev *thc63)
+{
+	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
+					      GPIOD_OUT_LOW);
+	if (IS_ERR(thc63->pwdn)) {
+		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
+		return PTR_ERR(thc63->pwdn);
+	}
+
+	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
+	if (IS_ERR(thc63->oe)) {
+		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
+		return PTR_ERR(thc63->oe);
+	}
+
+	return 0;
+}
+
+static int thc63_regulator_init(struct thc63_dev *thc63)
+{
+	struct regulator **reg;
+	int i;
+
+	reg = &thc63->vccs[0];
+	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
+		*reg = devm_regulator_get_optional(thc63->dev,
+						   thc63_reg_names[i]);
+		if (IS_ERR(*reg)) {
+			if (PTR_ERR(*reg) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+			*reg = NULL;
+		}
+	}
+
+	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);
+
+	ret = thc63_regulator_init(thc63);
+	if (ret)
+		return ret;
+
+	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;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id thc63_match[] = {
+	{ .compatible = "thine,thc63lvd1024", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, thc63_match);
+#endif
+
+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

[PATCH v3 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Jacopo Mondi]

The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
decoder, connected to the on-chip LVDS encoder output on one side
and to HDMI encoder ADV7511w on the other one.

As the decoder does not need any configuration it has been so-far
omitted from DTS. Now that a driver is available, describe it in DT
as well.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 +++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
index c0fd144..69f43b8 100644
--- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
+++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
@@ -42,6 +42,33 @@
 			};
 		};
 	};
+
+	thc63lvd1024: lvds-decoder {
+		compatible = "thine,thc63lvd1024";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+
+				thc63lvd1024_in_0: endpoint {
+					remote-endpoint = <&lvds0_out>;
+				};
+			};
+
+			port@2{
+				reg = <2>;
+
+				thc63lvd1024_out_2: endpoint {
+					remote-endpoint = <&adv7511_in>;
+				};
+
+			};
+
+		};
+	};
 };
 
 &avb {
@@ -98,7 +125,7 @@
 			port@0 {
 				reg = <0>;
 				adv7511_in: endpoint {
-					remote-endpoint = <&lvds0_out>;
+					remote-endpoint = <&thc63lvd1024_out_2>;
 				};
 			};
 
@@ -152,8 +179,8 @@
 
 	ports {
 		port@1 {
-			endpoint {
-				remote-endpoint = <&adv7511_in>;
+			lvds0_out: endpoint {
+				remote-endpoint = <&thc63lvd1024_in_0>;
 			};
 		};
 	};
-- 
2.7.4

Re: [PATCH v3 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Simon Horman]

On Tue, Mar 13, 2018 at 03:30:25PM +0100, Jacopo Mondi wrote:
> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
> decoder, connected to the on-chip LVDS encoder output on one side
> and to HDMI encoder ADV7511w on the other one.
> 
> As the decoder does not need any configuration it has been so-far
> omitted from DTS. Now that a driver is available, describe it in DT
> as well.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

I have marked this as deferred pending acceptance of the new binding.
Please repost or ping me once that has happened.

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

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 ++++++++++++++++++++++
>  1 file changed, 63 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..5b5afcd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,63 @@
> +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 two input LVDS stream and up to two digital CMOS/TTL outputs.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024",
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output and digital circuitry
> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> +- lvcc-supply: Power supply for LVDS inputs
> +- pvcc-supply: Power supply for PLL circuitry

I wonder if it wouldn't be better to make them required (at least VCC) -
it is closer to reality.

> +- pwnd-gpios: Power down GPIO signal. Active low.

As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
different docs?
Moreover there are already bindings for THC63LVDM83D with the same
dichotomy [2].

Out of curiosity I have googled for "pwnd pin" and it looks like some
vendors use this form.
For me both forms are quite misleading: power down signal, active low,
why they couldn't call it just 'enable, active high'.

[1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
[2]:
https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt

> +- oe-gpios: Output enable GPIO signal. 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>;
> +		lvcc-supply = <&reg_lvds_lvcc>;
> +
> +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
And here another variation :), should be pdwn-gpios.

> +
> +		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>;
> +				};
> +
> +			};
> +
> +		};
> +	};

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

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 ++++++++++++++++++++++
>  1 file changed, 63 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..5b5afcd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,63 @@
> +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 two input LVDS stream and up to two digital CMOS/TTL outputs.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024",
> +
> +Optional properties:
> +- vcc-supply: Power supply for TTL output and digital circuitry
> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> +- lvcc-supply: Power supply for LVDS inputs
> +- pvcc-supply: Power supply for PLL circuitry

I wonder if it wouldn't be better to make them required (at least VCC) -
it is closer to reality.

> +- pwnd-gpios: Power down GPIO signal. Active low.

As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
different docs?
Moreover there are already bindings for THC63LVDM83D with the same
dichotomy [2].

Out of curiosity I have googled for "pwnd pin" and it looks like some
vendors use this form.
For me both forms are quite misleading: power down signal, active low,
why they couldn't call it just 'enable, active high'.

[1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
[2]:
https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt

> +- oe-gpios: Output enable GPIO signal. 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>;
> +		lvcc-supply = <&reg_lvds_lvcc>;
> +
> +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
And here another variation :), should be pdwn-gpios.

> +
> +		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>;
> +				};
> +
> +			};
> +
> +		};
> +	};


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

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

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.

IMO converter suits here better, but it is just suggestion.

>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   7 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
>  3 files changed, 249 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 3b99d5a..facf6bd 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,13 @@ 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
> +	select DRM_PANEL_BRIDGE

You don't use PANEL_BRIDGE, it should be possible to drop the select.

> +	---help---
> +	  Thine THC63LVD1024 LVDS decoder bridge driver.

Decoder to what?
Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
converter or bridge.

> +
>  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..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// 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>
> +
> +static const char * const thc63_reg_names[] = {
> +	"vcc", "lvcc", "pvcc", "cvcc", };
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> +	struct gpio_desc *pwdn;
> +	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);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_enable(vcc);
> +			if (ret)
> +				goto error_vcc_enable;
> +		}
> +	}
> +
> +	if (thc63->pwdn)
> +		gpiod_set_value(thc63->pwdn, 0);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 1);
> +
> +	return;
> +
> +error_vcc_enable:
> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_disable(vcc);
> +			if (ret)
> +				goto error_vcc_disable;

I think in disable path you can report an error and continue - should be
safer.

> +		}
> +	}
> +
> +	if (thc63->pwdn)
> +		gpiod_set_value(thc63->pwdn, 1);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 0);

Usually disable uses reverse order. Ie first disable oe, then, pwdn,
then regulators, also in reverse order.
Looks more reasonable.

> +
> +	return;
> +
> +error_vcc_disable:
> +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> +}
> +
> +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;
> +	int ret = 0;
> +
> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);

Please define and use enums for port number, it will be more clear.

> +	if (!thc63_out) {
> +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(thc63_out);
> +	if (!remote) {
> +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_out_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	thc63->next = of_drm_find_bridge(remote);
> +	if (!thc63->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_out_node:
> +	of_node_put(thc63_out);
> +
> +	return ret;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> +					      GPIOD_OUT_LOW);
Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?

Regards
Andrzej
> +	if (IS_ERR(thc63->pwdn)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> +		return PTR_ERR(thc63->pwdn);
> +	}
> +
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> +		return PTR_ERR(thc63->oe);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thc63_regulator_init(struct thc63_dev *thc63)
> +{
> +	struct regulator **reg;
> +	int i;
> +
> +	reg = &thc63->vccs[0];
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> +		*reg = devm_regulator_get_optional(thc63->dev,
> +						   thc63_reg_names[i]);
> +		if (IS_ERR(*reg)) {
> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			*reg = NULL;
> +		}
> +	}
> +
> +	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);
> +
> +	ret = thc63_regulator_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id thc63_match[] = {
> +	{ .compatible = "thine,thc63lvd1024", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +#endif
> +
> +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");

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

[Andrzej Hajda]

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.

IMO converter suits here better, but it is just suggestion.

>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   7 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
>  3 files changed, 249 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 3b99d5a..facf6bd 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,13 @@ 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
> +	select DRM_PANEL_BRIDGE

You don't use PANEL_BRIDGE, it should be possible to drop the select.

> +	---help---
> +	  Thine THC63LVD1024 LVDS decoder bridge driver.

Decoder to what?
Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
converter or bridge.

> +
>  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..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// 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>
> +
> +static const char * const thc63_reg_names[] = {
> +	"vcc", "lvcc", "pvcc", "cvcc", };
> +
> +struct thc63_dev {
> +	struct device *dev;
> +
> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> +	struct gpio_desc *pwdn;
> +	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);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_enable(vcc);
> +			if (ret)
> +				goto error_vcc_enable;
> +		}
> +	}
> +
> +	if (thc63->pwdn)
> +		gpiod_set_value(thc63->pwdn, 0);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 1);
> +
> +	return;
> +
> +error_vcc_enable:
> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_disable(vcc);
> +			if (ret)
> +				goto error_vcc_disable;

I think in disable path you can report an error and continue - should be
safer.

> +		}
> +	}
> +
> +	if (thc63->pwdn)
> +		gpiod_set_value(thc63->pwdn, 1);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 0);

Usually disable uses reverse order. Ie first disable oe, then, pwdn,
then regulators, also in reverse order.
Looks more reasonable.

> +
> +	return;
> +
> +error_vcc_disable:
> +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> +}
> +
> +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;
> +	int ret = 0;
> +
> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);

Please define and use enums for port number, it will be more clear.

> +	if (!thc63_out) {
> +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
> +		return -ENODEV;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(thc63_out);
> +	if (!remote) {
> +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
> +		ret = -ENODEV;
> +		goto error_put_out_node;
> +	}
> +
> +	if (!of_device_is_available(remote)) {
> +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
> +		ret = -ENODEV;
> +		goto error_put_remote_node;
> +	}
> +
> +	thc63->next = of_drm_find_bridge(remote);
> +	if (!thc63->next)
> +		ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> +	of_node_put(remote);
> +error_put_out_node:
> +	of_node_put(thc63_out);
> +
> +	return ret;
> +}
> +
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> +					      GPIOD_OUT_LOW);
Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?

Regards
Andrzej
> +	if (IS_ERR(thc63->pwdn)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> +		return PTR_ERR(thc63->pwdn);
> +	}
> +
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> +		return PTR_ERR(thc63->oe);
> +	}
> +
> +	return 0;
> +}
> +
> +static int thc63_regulator_init(struct thc63_dev *thc63)
> +{
> +	struct regulator **reg;
> +	int i;
> +
> +	reg = &thc63->vccs[0];
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> +		*reg = devm_regulator_get_optional(thc63->dev,
> +						   thc63_reg_names[i]);
> +		if (IS_ERR(*reg)) {
> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +			*reg = NULL;
> +		}
> +	}
> +
> +	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);
> +
> +	ret = thc63_regulator_init(thc63);
> +	if (ret)
> +		return ret;
> +
> +	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;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id thc63_match[] = {
> +	{ .compatible = "thine,thc63lvd1024", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, thc63_match);
> +#endif
> +
> +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");


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

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

[jacopo mondi]

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

Hi Andrzej,
    sorry for the mess :(

On Wed, Mar 14, 2018 at 09:15:42AM +0100, Andrzej Hajda wrote:
> On 13.03.2018 15:30, Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 ++++++++++++++++++++++
> >  1 file changed, 63 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..5b5afcd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,63 @@
> > +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 two input LVDS stream and up to two digital CMOS/TTL outputs.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024",
> > +
> > +Optional properties:
> > +- vcc-supply: Power supply for TTL output and digital circuitry
> > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> > +- lvcc-supply: Power supply for LVDS inputs
> > +- pvcc-supply: Power supply for PLL circuitry
>
> I wonder if it wouldn't be better to make them required (at least VCC) -
> it is closer to reality.

In cases like our Eagle board, where VCC is directly connected to the
powering rail and not through a controllable regulator, I feel like
making this mandatory requires more effort (not much, I agree, just a
"fixed-regulator" more) with no additional benefits.

But I understand your point, and I'm open to whatever fits better with
the already existing DRM bridges bindings

>
> > +- pwnd-gpios: Power down GPIO signal. Active low.
>
> As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
> different docs?

I didn't notice the typo in first review, and I thought you were referring to
the initial '/' which I found weird to be part of the gpio name... Then I now
realized I badly typed in "pwnd" in place of "pwdn", which is not even correct
because it has to be "pdwn"... Sorry about this mess, I will fix in v4

> Moreover there are already bindings for THC63LVDM83D with the same
> dichotomy [2].

Seems like this is 'wrong' as well.. The chip manual says the pin is
named "pdwn" here too..

>
> Out of curiosity I have googled for "pwnd pin" and it looks like some
> vendors use this form.
> For me both forms are quite misleading: power down signal, active low,
> why they couldn't call it just 'enable, active high'.
>

It's not much the actual physical active level that bugs me, but the fact
that the GPIO name defines if it has to be set to "active" or
"inactive" logical state in enable/disable routines that I don't
like..

> [1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
> [2]:
> https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
>
> > +- oe-gpios: Output enable GPIO signal. 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>;
> > +		lvcc-supply = <&reg_lvds_lvcc>;
> > +
> > +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
> And here another variation :), should be pdwn-gpios.

Next time it will be "pndw".. Is there a prize if I do insert all
permutations of the same name in a single bindings document? :)

Will fix this shortly.

Thanks
   j

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

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

[jacopo mondi]

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

Hi Andrzej,
    sorry for the mess :(

On Wed, Mar 14, 2018 at 09:15:42AM +0100, Andrzej Hajda wrote:
> On 13.03.2018 15:30, Jacopo Mondi wrote:
> > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 ++++++++++++++++++++++
> >  1 file changed, 63 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..5b5afcd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> > @@ -0,0 +1,63 @@
> > +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 two input LVDS stream and up to two digital CMOS/TTL outputs.
> > +
> > +Required properties:
> > +- compatible: Shall be "thine,thc63lvd1024",
> > +
> > +Optional properties:
> > +- vcc-supply: Power supply for TTL output and digital circuitry
> > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> > +- lvcc-supply: Power supply for LVDS inputs
> > +- pvcc-supply: Power supply for PLL circuitry
>
> I wonder if it wouldn't be better to make them required (at least VCC) -
> it is closer to reality.

In cases like our Eagle board, where VCC is directly connected to the
powering rail and not through a controllable regulator, I feel like
making this mandatory requires more effort (not much, I agree, just a
"fixed-regulator" more) with no additional benefits.

But I understand your point, and I'm open to whatever fits better with
the already existing DRM bridges bindings

>
> > +- pwnd-gpios: Power down GPIO signal. Active low.
>
> As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
> different docs?

I didn't notice the typo in first review, and I thought you were referring to
the initial '/' which I found weird to be part of the gpio name... Then I now
realized I badly typed in "pwnd" in place of "pwdn", which is not even correct
because it has to be "pdwn"... Sorry about this mess, I will fix in v4

> Moreover there are already bindings for THC63LVDM83D with the same
> dichotomy [2].

Seems like this is 'wrong' as well.. The chip manual says the pin is
named "pdwn" here too..

>
> Out of curiosity I have googled for "pwnd pin" and it looks like some
> vendors use this form.
> For me both forms are quite misleading: power down signal, active low,
> why they couldn't call it just 'enable, active high'.
>

It's not much the actual physical active level that bugs me, but the fact
that the GPIO name defines if it has to be set to "active" or
"inactive" logical state in enable/disable routines that I don't
like..

> [1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
> [2]:
> https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
>
> > +- oe-gpios: Output enable GPIO signal. 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>;
> > +		lvcc-supply = <&reg_lvds_lvcc>;
> > +
> > +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
> And here another variation :), should be pdwn-gpios.

Next time it will be "pndw".. Is there a prize if I do insert all
permutations of the same name in a single bindings document? :)

Will fix this shortly.

Thanks
   j

[-- 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 v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[jacopo mondi]

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

Hi Andrzej,
    thanks for review

On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> On 13.03.2018 15:30, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output decoder.
>
> IMO converter suits here better, but it is just suggestion.
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig        |   7 +
> >  drivers/gpu/drm/bridge/Makefile       |   1 +
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 249 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 3b99d5a..facf6bd 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -92,6 +92,13 @@ 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
> > +	select DRM_PANEL_BRIDGE
>
> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>

Ack

> > +	---help---
> > +	  Thine THC63LVD1024 LVDS decoder bridge driver.
>
> Decoder to what?
> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> converter or bridge.

"LVDS to digital CMOS/TTL parallel data converter" as the manual
describes the chip?

>
> > +
> >  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..4b059c0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,241 @@
> > +// 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>
> > +
> > +static const char * const thc63_reg_names[] = {
> > +	"vcc", "lvcc", "pvcc", "cvcc", };
> > +
> > +struct thc63_dev {
> > +	struct device *dev;
> > +
> > +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> > +
> > +	struct gpio_desc *pwdn;
> > +	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);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_enable(vcc);
> > +			if (ret)
> > +				goto error_vcc_enable;
> > +		}
> > +	}
> > +
> > +	if (thc63->pwdn)
> > +		gpiod_set_value(thc63->pwdn, 0);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 1);
> > +
> > +	return;
> > +
> > +error_vcc_enable:
> > +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> > +}
> > +
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_disable(vcc);
> > +			if (ret)
> > +				goto error_vcc_disable;
>
> I think in disable path you can report an error and continue - should be
> safer.
>

Ack

> > +		}
> > +	}
> > +
> > +	if (thc63->pwdn)
> > +		gpiod_set_value(thc63->pwdn, 1);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 0);
>
> Usually disable uses reverse order. Ie first disable oe, then, pwdn,
> then regulators, also in reverse order.
> Looks more reasonable.
>

Indeed! I will invert the order.

(I wonder if the regulator disabling order actually makes a difference
though).

> > +
> > +	return;
> > +
> > +error_vcc_disable:
> > +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> > +}
> > +
> > +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;
> > +	int ret = 0;
> > +
> > +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
>
> Please define and use enums for port number, it will be more clear.
>

yup

> > +	if (!thc63_out) {
> > +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	remote = of_graph_get_remote_port_parent(thc63_out);
> > +	if (!remote) {
> > +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
> > +		ret = -ENODEV;
> > +		goto error_put_out_node;
> > +	}
> > +
> > +	if (!of_device_is_available(remote)) {
> > +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
> > +		ret = -ENODEV;
> > +		goto error_put_remote_node;
> > +	}
> > +
> > +	thc63->next = of_drm_find_bridge(remote);
> > +	if (!thc63->next)
> > +		ret = -EPROBE_DEFER;
> > +
> > +error_put_remote_node:
> > +	of_node_put(remote);
> > +error_put_out_node:
> > +	of_node_put(thc63_out);
> > +
> > +	return ret;
> > +}
> > +
> > +static int thc63_gpio_init(struct thc63_dev *thc63)
> > +{
> > +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> > +					      GPIOD_OUT_LOW);
> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?

No, the PWDN gpio is active low, it puts the chip in power down mode
when the physical line level is set to 0.

That's another confusing thing, when requesting the GPIO, the actual
physical line value has to be provided, while when "set_value()" the
logical one is requested, iirc.

Being the GPIO defined as active low, in power enable we set it to
"logical inactive" == "physical 1", while during power disable it has
to be set to "logical active" == "physical 0"

Not the right place to complain here, but imo that's not consistent.
Or have I misinterpreted the APIs? I cannot do much test, as in my setup
the PWDN pin is hardwired to +vcc (through a physical switch, not
controlled through a GPIO though).

Thanks
   j

>
> Regards
> Andrzej
> > +	if (IS_ERR(thc63->pwdn)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> > +		return PTR_ERR(thc63->pwdn);
> > +	}
> > +
> > +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > +	if (IS_ERR(thc63->oe)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> > +		return PTR_ERR(thc63->oe);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int thc63_regulator_init(struct thc63_dev *thc63)
> > +{
> > +	struct regulator **reg;
> > +	int i;
> > +
> > +	reg = &thc63->vccs[0];
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> > +		*reg = devm_regulator_get_optional(thc63->dev,
> > +						   thc63_reg_names[i]);
> > +		if (IS_ERR(*reg)) {
> > +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			*reg = NULL;
> > +		}
> > +	}
> > +
> > +	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);
> > +
> > +	ret = thc63_regulator_init(thc63);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id thc63_match[] = {
> > +	{ .compatible = "thine,thc63lvd1024", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, thc63_match);
> > +#endif
> > +
> > +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");
>
>

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

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

[jacopo mondi]

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

Hi Andrzej,
    thanks for review

On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> On 13.03.2018 15:30, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output decoder.
>
> IMO converter suits here better, but it is just suggestion.
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/bridge/Kconfig        |   7 +
> >  drivers/gpu/drm/bridge/Makefile       |   1 +
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 249 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 3b99d5a..facf6bd 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -92,6 +92,13 @@ 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
> > +	select DRM_PANEL_BRIDGE
>
> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>

Ack

> > +	---help---
> > +	  Thine THC63LVD1024 LVDS decoder bridge driver.
>
> Decoder to what?
> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> converter or bridge.

"LVDS to digital CMOS/TTL parallel data converter" as the manual
describes the chip?

>
> > +
> >  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..4b059c0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,241 @@
> > +// 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>
> > +
> > +static const char * const thc63_reg_names[] = {
> > +	"vcc", "lvcc", "pvcc", "cvcc", };
> > +
> > +struct thc63_dev {
> > +	struct device *dev;
> > +
> > +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> > +
> > +	struct gpio_desc *pwdn;
> > +	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);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_enable(vcc);
> > +			if (ret)
> > +				goto error_vcc_enable;
> > +		}
> > +	}
> > +
> > +	if (thc63->pwdn)
> > +		gpiod_set_value(thc63->pwdn, 0);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 1);
> > +
> > +	return;
> > +
> > +error_vcc_enable:
> > +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> > +}
> > +
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_disable(vcc);
> > +			if (ret)
> > +				goto error_vcc_disable;
>
> I think in disable path you can report an error and continue - should be
> safer.
>

Ack

> > +		}
> > +	}
> > +
> > +	if (thc63->pwdn)
> > +		gpiod_set_value(thc63->pwdn, 1);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 0);
>
> Usually disable uses reverse order. Ie first disable oe, then, pwdn,
> then regulators, also in reverse order.
> Looks more reasonable.
>

Indeed! I will invert the order.

(I wonder if the regulator disabling order actually makes a difference
though).

> > +
> > +	return;
> > +
> > +error_vcc_disable:
> > +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> > +}
> > +
> > +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;
> > +	int ret = 0;
> > +
> > +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
>
> Please define and use enums for port number, it will be more clear.
>

yup

> > +	if (!thc63_out) {
> > +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	remote = of_graph_get_remote_port_parent(thc63_out);
> > +	if (!remote) {
> > +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
> > +		ret = -ENODEV;
> > +		goto error_put_out_node;
> > +	}
> > +
> > +	if (!of_device_is_available(remote)) {
> > +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
> > +		ret = -ENODEV;
> > +		goto error_put_remote_node;
> > +	}
> > +
> > +	thc63->next = of_drm_find_bridge(remote);
> > +	if (!thc63->next)
> > +		ret = -EPROBE_DEFER;
> > +
> > +error_put_remote_node:
> > +	of_node_put(remote);
> > +error_put_out_node:
> > +	of_node_put(thc63_out);
> > +
> > +	return ret;
> > +}
> > +
> > +static int thc63_gpio_init(struct thc63_dev *thc63)
> > +{
> > +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> > +					      GPIOD_OUT_LOW);
> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?

No, the PWDN gpio is active low, it puts the chip in power down mode
when the physical line level is set to 0.

That's another confusing thing, when requesting the GPIO, the actual
physical line value has to be provided, while when "set_value()" the
logical one is requested, iirc.

Being the GPIO defined as active low, in power enable we set it to
"logical inactive" == "physical 1", while during power disable it has
to be set to "logical active" == "physical 0"

Not the right place to complain here, but imo that's not consistent.
Or have I misinterpreted the APIs? I cannot do much test, as in my setup
the PWDN pin is hardwired to +vcc (through a physical switch, not
controlled through a GPIO though).

Thanks
   j

>
> Regards
> Andrzej
> > +	if (IS_ERR(thc63->pwdn)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> > +		return PTR_ERR(thc63->pwdn);
> > +	}
> > +
> > +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > +	if (IS_ERR(thc63->oe)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> > +		return PTR_ERR(thc63->oe);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int thc63_regulator_init(struct thc63_dev *thc63)
> > +{
> > +	struct regulator **reg;
> > +	int i;
> > +
> > +	reg = &thc63->vccs[0];
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> > +		*reg = devm_regulator_get_optional(thc63->dev,
> > +						   thc63_reg_names[i]);
> > +		if (IS_ERR(*reg)) {
> > +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +			*reg = NULL;
> > +		}
> > +	}
> > +
> > +	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);
> > +
> > +	ret = thc63_regulator_init(thc63);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id thc63_match[] = {
> > +	{ .compatible = "thine,thc63lvd1024", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, thc63_match);
> > +#endif
> > +
> > +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");
>
>

[-- 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 v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Sergei Shtylyov]

On 03/13/2018 05:30 PM, Jacopo Mondi wrote:

> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
[...]
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 0000000..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// 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>
> +
> +static const char * const thc63_reg_names[] = {
> +	"vcc", "lvcc", "pvcc", "cvcc", };

   Your bracing style is pretty strange -- neither here nor there. Please place };
on the next line...

[...]
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_enable(vcc);
> +			if (ret)

   You hardly need this variable, could do a call right in this *if*.

[...]
> +error_vcc_enable:
> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +

   Why not do this instead of *goto* before?
 
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> +	struct thc63_dev *thc63 = to_thc63(bridge);
> +	struct regulator *vcc;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> +		vcc = thc63->vccs[i];
> +		if (vcc) {
> +			ret = regulator_disable(vcc);
> +			if (ret)

   Again, no need for 'ret' whatsoever...

> +				goto error_vcc_disable;
> +		}
> +	}
> +
> +	if (thc63->pwdn)
> +		gpiod_set_value(thc63->pwdn, 1);
> +
> +	if (thc63->oe)
> +		gpiod_set_value(thc63->oe, 0);
> +
> +	return;
> +
> +error_vcc_disable:
> +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);

   Again, why not do it instead of *goto*?

[...]
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> +					      GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->pwdn)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");

   "pwdn-gpios" maybe?

> +		return PTR_ERR(thc63->pwdn);
> +	}
> +
> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> +	if (IS_ERR(thc63->oe)) {
> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");

   "oe-gpios" maybe?

[...]

MBR, Sergei

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

[jacopo mondi]

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

Hi Sergei,
   thanks for review

On Wed, Mar 14, 2018 at 08:09:52PM +0300, Sergei Shtylyov wrote:
> On 03/13/2018 05:30 PM, Jacopo Mondi wrote:
>
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output decoder.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> [...]
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > new file mode 100644
> > index 0000000..4b059c0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,241 @@
> > +// 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>
> > +
> > +static const char * const thc63_reg_names[] = {
> > +	"vcc", "lvcc", "pvcc", "cvcc", };
>
>    Your bracing style is pretty strange -- neither here nor there. Please place };
> on the next line...

Yeah, I had doubt about this.. The most common style I found around is

static const char * const foo[] = {
        "bar",
        "baz",
        "...",
};

But seems really too many lines for a bunch of 4 character strings...

>
> [...]
> > +static void thc63_enable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_enable(vcc);
> > +			if (ret)
>
>    You hardly need this variable, could do a call right in this *if*.
>
> [...]
> > +error_vcc_enable:
> > +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> > +}
> > +
>
>    Why not do this instead of *goto* before?

Well, goto breaks the loop, if I only print out the error message, the
enable sequence will go on and enable the other regulators.

I can print out and break, but I don't see that much benefit

One thing I could do instead, is not only print out the error message,
but disable the already enabled regulators if one fails to start.

>
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +	struct thc63_dev *thc63 = to_thc63(bridge);
> > +	struct regulator *vcc;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +		vcc = thc63->vccs[i];
> > +		if (vcc) {
> > +			ret = regulator_disable(vcc);
> > +			if (ret)
>
>    Again, no need for 'ret' whatsoever...
>
> > +				goto error_vcc_disable;
> > +		}
> > +	}
> > +
> > +	if (thc63->pwdn)
> > +		gpiod_set_value(thc63->pwdn, 1);
> > +
> > +	if (thc63->oe)
> > +		gpiod_set_value(thc63->oe, 0);
> > +
> > +	return;
> > +
> > +error_vcc_disable:
> > +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
>
>    Again, why not do it instead of *goto*?

ditto

>
> [...]
> > +static int thc63_gpio_init(struct thc63_dev *thc63)
> > +{
> > +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> > +					      GPIOD_OUT_LOW);
> > +	if (IS_ERR(thc63->pwdn)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>
>    "pwdn-gpios" maybe?
>
> > +		return PTR_ERR(thc63->pwdn);
> > +	}
> > +
> > +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > +	if (IS_ERR(thc63->oe)) {
> > +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>
>    "oe-gpios" maybe?

Are you referring to the error message? I can change this, but again, I
see no standards around.

Thanks
   j

>
> [...]
>
> MBR, Sergei

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

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

[Sergei Shtylyov]

On 03/14/2018 09:04 PM, jacopo mondi wrote:

>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> [...]
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 0000000..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
[...]
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_enable(vcc);
>>> +			if (ret)
>>
>>    You hardly need this variable, could do a call right in this *if*.
>>
>> [...]
>>> +error_vcc_enable:
>>> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>
>>    Why not do this instead of *goto* before?
> 
> Well, goto breaks the loop, if I only print out the error message, the
> enable sequence will go on and enable the other regulators.

> I can print out and break, but I don't see that much benefit

   Sorry, I meant that you should *return* there instead of *goto*.

> One thing I could do instead, is not only print out the error message,
> but disable the already enabled regulators if one fails to start.

   Yeah, probably...

[...]
>>> +static int thc63_gpio_init(struct thc63_dev *thc63)
>>> +{
>>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
>>> +					      GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->pwdn)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>>
>>    "pwdn-gpios" maybe?
>>
>>> +		return PTR_ERR(thc63->pwdn);
>>> +	}
>>> +
>>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->oe)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>>
>>    "oe-gpios" maybe?
> 
> Are you referring to the error message?

   Yes, seems more clear what to look for this way, IMHO...

[...]

MBR, Sergei

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

[Sergei Shtylyov]

On 03/14/2018 09:04 PM, jacopo mondi wrote:

>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>> [...]
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 0000000..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
[...]
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_enable(vcc);
>>> +			if (ret)
>>
>>    You hardly need this variable, could do a call right in this *if*.
>>
>> [...]
>>> +error_vcc_enable:
>>> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>
>>    Why not do this instead of *goto* before?
> 
> Well, goto breaks the loop, if I only print out the error message, the
> enable sequence will go on and enable the other regulators.

> I can print out and break, but I don't see that much benefit

   Sorry, I meant that you should *return* there instead of *goto*.

> One thing I could do instead, is not only print out the error message,
> but disable the already enabled regulators if one fails to start.

   Yeah, probably...

[...]
>>> +static int thc63_gpio_init(struct thc63_dev *thc63)
>>> +{
>>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
>>> +					      GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->pwdn)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>>
>>    "pwdn-gpios" maybe?
>>
>>> +		return PTR_ERR(thc63->pwdn);
>>> +	}
>>> +
>>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->oe)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>>
>>    "oe-gpios" maybe?
> 
> Are you referring to the error message?

   Yes, seems more clear what to look for this way, IMHO...

[...]

MBR, Sergei

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

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

[Andrzej Hajda]

On 14.03.2018 11:09, jacopo mondi wrote:
> Hi Andrzej,
>     thanks for review
>
> On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
>> On 13.03.2018 15:30, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>> IMO converter suits here better, but it is just suggestion.
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig        |   7 +
>>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 249 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 3b99d5a..facf6bd 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,13 @@ 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
>>> +	select DRM_PANEL_BRIDGE
>> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>>
> Ack
>
>>> +	---help---
>>> +	  Thine THC63LVD1024 LVDS decoder bridge driver.
>> Decoder to what?
>> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
>> converter or bridge.
> "LVDS to digital CMOS/TTL parallel data converter" as the manual
> describes the chip?

Should work, unless we want some consistency in interface names, we have
already: parallel / DPI / RGB. I am little bit confused about relations
between them so I do not want to enforce any.


>
>>> +
>>>  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..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
>>> +// 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>
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +	"vcc", "lvcc", "pvcc", "cvcc", };
>>> +
>>> +struct thc63_dev {
>>> +	struct device *dev;
>>> +
>>> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +	struct gpio_desc *pwdn;
>>> +	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);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_enable(vcc);
>>> +			if (ret)
>>> +				goto error_vcc_enable;
>>> +		}
>>> +	}
>>> +
>>> +	if (thc63->pwdn)
>>> +		gpiod_set_value(thc63->pwdn, 0);
>>> +
>>> +	if (thc63->oe)
>>> +		gpiod_set_value(thc63->oe, 1);
>>> +
>>> +	return;
>>> +
>>> +error_vcc_enable:
>>> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>> +static void thc63_disable(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_disable(vcc);
>>> +			if (ret)
>>> +				goto error_vcc_disable;
>> I think in disable path you can report an error and continue - should be
>> safer.
>>
> Ack
>
>>> +		}
>>> +	}
>>> +
>>> +	if (thc63->pwdn)
>>> +		gpiod_set_value(thc63->pwdn, 1);
>>> +
>>> +	if (thc63->oe)
>>> +		gpiod_set_value(thc63->oe, 0);
>> Usually disable uses reverse order. Ie first disable oe, then, pwdn,
>> then regulators, also in reverse order.
>> Looks more reasonable.
>>
> Indeed! I will invert the order.
>
> (I wonder if the regulator disabling order actually makes a difference
> though).
>>> +
>>> +	return;
>>> +
>>> +error_vcc_disable:
>>> +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
>>> +}
>>> +
>>> +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;
>>> +	int ret = 0;
>>> +
>>> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
>> Please define and use enums for port number, it will be more clear.
>>
> yup
>
>>> +	if (!thc63_out) {
>>> +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	remote = of_graph_get_remote_port_parent(thc63_out);
>>> +	if (!remote) {
>>> +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
>>> +		ret = -ENODEV;
>>> +		goto error_put_out_node;
>>> +	}
>>> +
>>> +	if (!of_device_is_available(remote)) {
>>> +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
>>> +		ret = -ENODEV;
>>> +		goto error_put_remote_node;
>>> +	}
>>> +
>>> +	thc63->next = of_drm_find_bridge(remote);
>>> +	if (!thc63->next)
>>> +		ret = -EPROBE_DEFER;
>>> +
>>> +error_put_remote_node:
>>> +	of_node_put(remote);
>>> +error_put_out_node:
>>> +	of_node_put(thc63_out);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int thc63_gpio_init(struct thc63_dev *thc63)
>>> +{
>>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
>>> +					      GPIOD_OUT_LOW);
>> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?
> No, the PWDN gpio is active low, it puts the chip in power down mode
> when the physical line level is set to 0.
>
> That's another confusing thing, when requesting the GPIO, the actual
> physical line value has to be provided, while when "set_value()" the
> logical one is requested, iirc.

I think it is opposite, only *raw* functions uses physical level. Other
uses logical level.

>
> Being the GPIO defined as active low, in power enable we set it to
> "logical inactive" == "physical 1", while during power disable it has
> to be set to "logical active" == "physical 0"
>
> Not the right place to complain here, but imo that's not consistent.
> Or have I misinterpreted the APIs? I cannot do much test, as in my setup
> the PWDN pin is hardwired to +vcc (through a physical switch, not
> controlled through a GPIO though).

devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which
calls gpiod_direction_output which uses logical levels.

Regards
Andrzej

>
> Thanks
>    j
>
>> Regards
>> Andrzej
>>> +	if (IS_ERR(thc63->pwdn)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>>> +		return PTR_ERR(thc63->pwdn);
>>> +	}
>>> +
>>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->oe)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>>> +		return PTR_ERR(thc63->oe);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int thc63_regulator_init(struct thc63_dev *thc63)
>>> +{
>>> +	struct regulator **reg;
>>> +	int i;
>>> +
>>> +	reg = &thc63->vccs[0];
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
>>> +		*reg = devm_regulator_get_optional(thc63->dev,
>>> +						   thc63_reg_names[i]);
>>> +		if (IS_ERR(*reg)) {
>>> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
>>> +				return -EPROBE_DEFER;
>>> +			*reg = NULL;
>>> +		}
>>> +	}
>>> +
>>> +	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);
>>> +
>>> +	ret = thc63_regulator_init(thc63);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	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;
>>> +}
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id thc63_match[] = {
>>> +	{ .compatible = "thine,thc63lvd1024", },
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, thc63_match);
>>> +#endif
>>> +
>>> +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");
>>

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

[Andrzej Hajda]

On 14.03.2018 11:09, jacopo mondi wrote:
> Hi Andrzej,
>     thanks for review
>
> On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
>> On 13.03.2018 15:30, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>> IMO converter suits here better, but it is just suggestion.
>>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig        |   7 +
>>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 249 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 3b99d5a..facf6bd 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,13 @@ 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
>>> +	select DRM_PANEL_BRIDGE
>> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>>
> Ack
>
>>> +	---help---
>>> +	  Thine THC63LVD1024 LVDS decoder bridge driver.
>> Decoder to what?
>> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
>> converter or bridge.
> "LVDS to digital CMOS/TTL parallel data converter" as the manual
> describes the chip?

Should work, unless we want some consistency in interface names, we have
already: parallel / DPI / RGB. I am little bit confused about relations
between them so I do not want to enforce any.


>
>>> +
>>>  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..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
>>> +// 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>
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +	"vcc", "lvcc", "pvcc", "cvcc", };
>>> +
>>> +struct thc63_dev {
>>> +	struct device *dev;
>>> +
>>> +	struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +	struct gpio_desc *pwdn;
>>> +	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);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_enable(vcc);
>>> +			if (ret)
>>> +				goto error_vcc_enable;
>>> +		}
>>> +	}
>>> +
>>> +	if (thc63->pwdn)
>>> +		gpiod_set_value(thc63->pwdn, 0);
>>> +
>>> +	if (thc63->oe)
>>> +		gpiod_set_value(thc63->oe, 1);
>>> +
>>> +	return;
>>> +
>>> +error_vcc_enable:
>>> +	dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>> +static void thc63_disable(struct drm_bridge *bridge)
>>> +{
>>> +	struct thc63_dev *thc63 = to_thc63(bridge);
>>> +	struct regulator *vcc;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +		vcc = thc63->vccs[i];
>>> +		if (vcc) {
>>> +			ret = regulator_disable(vcc);
>>> +			if (ret)
>>> +				goto error_vcc_disable;
>> I think in disable path you can report an error and continue - should be
>> safer.
>>
> Ack
>
>>> +		}
>>> +	}
>>> +
>>> +	if (thc63->pwdn)
>>> +		gpiod_set_value(thc63->pwdn, 1);
>>> +
>>> +	if (thc63->oe)
>>> +		gpiod_set_value(thc63->oe, 0);
>> Usually disable uses reverse order. Ie first disable oe, then, pwdn,
>> then regulators, also in reverse order.
>> Looks more reasonable.
>>
> Indeed! I will invert the order.
>
> (I wonder if the regulator disabling order actually makes a difference
> though).
>>> +
>>> +	return;
>>> +
>>> +error_vcc_disable:
>>> +	dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
>>> +}
>>> +
>>> +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;
>>> +	int ret = 0;
>>> +
>>> +	thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
>> Please define and use enums for port number, it will be more clear.
>>
> yup
>
>>> +	if (!thc63_out) {
>>> +		dev_err(thc63->dev, "Missing endpoint in Port@2\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	remote = of_graph_get_remote_port_parent(thc63_out);
>>> +	if (!remote) {
>>> +		dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
>>> +		ret = -ENODEV;
>>> +		goto error_put_out_node;
>>> +	}
>>> +
>>> +	if (!of_device_is_available(remote)) {
>>> +		dev_err(thc63->dev, "Port@2 remote endpoint is disabled\n");
>>> +		ret = -ENODEV;
>>> +		goto error_put_remote_node;
>>> +	}
>>> +
>>> +	thc63->next = of_drm_find_bridge(remote);
>>> +	if (!thc63->next)
>>> +		ret = -EPROBE_DEFER;
>>> +
>>> +error_put_remote_node:
>>> +	of_node_put(remote);
>>> +error_put_out_node:
>>> +	of_node_put(thc63_out);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int thc63_gpio_init(struct thc63_dev *thc63)
>>> +{
>>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
>>> +					      GPIOD_OUT_LOW);
>> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?
> No, the PWDN gpio is active low, it puts the chip in power down mode
> when the physical line level is set to 0.
>
> That's another confusing thing, when requesting the GPIO, the actual
> physical line value has to be provided, while when "set_value()" the
> logical one is requested, iirc.

I think it is opposite, only *raw* functions uses physical level. Other
uses logical level.

>
> Being the GPIO defined as active low, in power enable we set it to
> "logical inactive" == "physical 1", while during power disable it has
> to be set to "logical active" == "physical 0"
>
> Not the right place to complain here, but imo that's not consistent.
> Or have I misinterpreted the APIs? I cannot do much test, as in my setup
> the PWDN pin is hardwired to +vcc (through a physical switch, not
> controlled through a GPIO though).

devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which
calls gpiod_direction_output which uses logical levels.

Regards
Andrzej

>
> Thanks
>    j
>
>> Regards
>> Andrzej
>>> +	if (IS_ERR(thc63->pwdn)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>>> +		return PTR_ERR(thc63->pwdn);
>>> +	}
>>> +
>>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
>>> +	if (IS_ERR(thc63->oe)) {
>>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>>> +		return PTR_ERR(thc63->oe);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int thc63_regulator_init(struct thc63_dev *thc63)
>>> +{
>>> +	struct regulator **reg;
>>> +	int i;
>>> +
>>> +	reg = &thc63->vccs[0];
>>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
>>> +		*reg = devm_regulator_get_optional(thc63->dev,
>>> +						   thc63_reg_names[i]);
>>> +		if (IS_ERR(*reg)) {
>>> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
>>> +				return -EPROBE_DEFER;
>>> +			*reg = NULL;
>>> +		}
>>> +	}
>>> +
>>> +	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);
>>> +
>>> +	ret = thc63_regulator_init(thc63);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	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;
>>> +}
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id thc63_match[] = {
>>> +	{ .compatible = "thine,thc63lvd1024", },
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, thc63_match);
>>> +#endif
>>> +
>>> +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");
>>

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

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

[jacopo mondi]

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

HI Andrezej,

On Thu, Mar 15, 2018 at 10:44:42AM +0100, Andrzej Hajda wrote:
> On 14.03.2018 11:09, jacopo mondi wrote:
> > Hi Andrzej,
> >     thanks for review
> >
> > On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> >> On 13.03.2018 15:30, Jacopo Mondi wrote:
> >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> >>> output decoder.
> >> IMO converter suits here better, but it is just suggestion.
> >>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/gpu/drm/bridge/Kconfig        |   7 +
> >>>  drivers/gpu/drm/bridge/Makefile       |   1 +
> >>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 249 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 3b99d5a..facf6bd 100644
> >>> --- a/drivers/gpu/drm/bridge/Kconfig
> >>> +++ b/drivers/gpu/drm/bridge/Kconfig
> >>> @@ -92,6 +92,13 @@ 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
> >>> +	select DRM_PANEL_BRIDGE
> >> You don't use PANEL_BRIDGE, it should be possible to drop the select.
> >>
> > Ack
> >
> >>> +	---help---
> >>> +	  Thine THC63LVD1024 LVDS decoder bridge driver.
> >> Decoder to what?
> >> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> >> converter or bridge.
> > "LVDS to digital CMOS/TTL parallel data converter" as the manual
> > describes the chip?
>
> Should work, unless we want some consistency in interface names, we have
> already: parallel / DPI / RGB. I am little bit confused about relations
> between them so I do not want to enforce any.

config DRM_THINE_THC63LVD1024
	tristate "Thine THC63LVD1024 LVDS decoder bridge"
	depends on OF
	---help---
	  Thine THC63LVD1024 LVDS/parallel converter driver.

I guess this is consistent with other symbol descriptions I found

>

[snip]
>
> >>> +
> >>> +static int thc63_gpio_init(struct thc63_dev *thc63)
> >>> +{
> >>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> >>> +					      GPIOD_OUT_LOW);
> >> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?
> > No, the PWDN gpio is active low, it puts the chip in power down mode
> > when the physical line level is set to 0.
> >
> > That's another confusing thing, when requesting the GPIO, the actual
> > physical line value has to be provided, while when "set_value()" the
> > logical one is requested, iirc.
>
> I think it is opposite, only *raw* functions uses physical level. Other
> uses logical level.
>
> >
> > Being the GPIO defined as active low, in power enable we set it to
> > "logical inactive" == "physical 1", while during power disable it has
> > to be set to "logical active" == "physical 0"
> >
> > Not the right place to complain here, but imo that's not consistent.
> > Or have I misinterpreted the APIs? I cannot do much test, as in my setup
> > the PWDN pin is hardwired to +vcc (through a physical switch, not
> > controlled through a GPIO though).
>
> devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which
> calls gpiod_direction_output which uses logical levels.

I clearly mis-interpreted the APIs.

I'll fix and I'm sending v4 out shortly.

Thanks
   j

>
> Regards
> Andrzej
>
> >
> > Thanks
> >    j
> >
> >> Regards
> >> Andrzej
> >>> +	if (IS_ERR(thc63->pwdn)) {
> >>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> >>> +		return PTR_ERR(thc63->pwdn);
> >>> +	}
> >>> +
> >>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> >>> +	if (IS_ERR(thc63->oe)) {
> >>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> >>> +		return PTR_ERR(thc63->oe);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int thc63_regulator_init(struct thc63_dev *thc63)
> >>> +{
> >>> +	struct regulator **reg;
> >>> +	int i;
> >>> +
> >>> +	reg = &thc63->vccs[0];
> >>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> >>> +		*reg = devm_regulator_get_optional(thc63->dev,
> >>> +						   thc63_reg_names[i]);
> >>> +		if (IS_ERR(*reg)) {
> >>> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> >>> +				return -EPROBE_DEFER;
> >>> +			*reg = NULL;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	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);
> >>> +
> >>> +	ret = thc63_regulator_init(thc63);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	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;
> >>> +}
> >>> +
> >>> +#ifdef CONFIG_OF
> >>> +static const struct of_device_id thc63_match[] = {
> >>> +	{ .compatible = "thine,thc63lvd1024", },
> >>> +	{ },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, thc63_match);
> >>> +#endif
> >>> +
> >>> +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");
> >>
>

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

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

[jacopo mondi]

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

HI Andrezej,

On Thu, Mar 15, 2018 at 10:44:42AM +0100, Andrzej Hajda wrote:
> On 14.03.2018 11:09, jacopo mondi wrote:
> > Hi Andrzej,
> >     thanks for review
> >
> > On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> >> On 13.03.2018 15:30, Jacopo Mondi wrote:
> >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> >>> output decoder.
> >> IMO converter suits here better, but it is just suggestion.
> >>
> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >>> ---
> >>>  drivers/gpu/drm/bridge/Kconfig        |   7 +
> >>>  drivers/gpu/drm/bridge/Makefile       |   1 +
> >>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 249 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 3b99d5a..facf6bd 100644
> >>> --- a/drivers/gpu/drm/bridge/Kconfig
> >>> +++ b/drivers/gpu/drm/bridge/Kconfig
> >>> @@ -92,6 +92,13 @@ 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
> >>> +	select DRM_PANEL_BRIDGE
> >> You don't use PANEL_BRIDGE, it should be possible to drop the select.
> >>
> > Ack
> >
> >>> +	---help---
> >>> +	  Thine THC63LVD1024 LVDS decoder bridge driver.
> >> Decoder to what?
> >> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> >> converter or bridge.
> > "LVDS to digital CMOS/TTL parallel data converter" as the manual
> > describes the chip?
>
> Should work, unless we want some consistency in interface names, we have
> already: parallel / DPI / RGB. I am little bit confused about relations
> between them so I do not want to enforce any.

config DRM_THINE_THC63LVD1024
	tristate "Thine THC63LVD1024 LVDS decoder bridge"
	depends on OF
	---help---
	  Thine THC63LVD1024 LVDS/parallel converter driver.

I guess this is consistent with other symbol descriptions I found

>

[snip]
>
> >>> +
> >>> +static int thc63_gpio_init(struct thc63_dev *thc63)
> >>> +{
> >>> +	thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> >>> +					      GPIOD_OUT_LOW);
> >> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?
> > No, the PWDN gpio is active low, it puts the chip in power down mode
> > when the physical line level is set to 0.
> >
> > That's another confusing thing, when requesting the GPIO, the actual
> > physical line value has to be provided, while when "set_value()" the
> > logical one is requested, iirc.
>
> I think it is opposite, only *raw* functions uses physical level. Other
> uses logical level.
>
> >
> > Being the GPIO defined as active low, in power enable we set it to
> > "logical inactive" == "physical 1", while during power disable it has
> > to be set to "logical active" == "physical 0"
> >
> > Not the right place to complain here, but imo that's not consistent.
> > Or have I misinterpreted the APIs? I cannot do much test, as in my setup
> > the PWDN pin is hardwired to +vcc (through a physical switch, not
> > controlled through a GPIO though).
>
> devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which
> calls gpiod_direction_output which uses logical levels.

I clearly mis-interpreted the APIs.

I'll fix and I'm sending v4 out shortly.

Thanks
   j

>
> Regards
> Andrzej
>
> >
> > Thanks
> >    j
> >
> >> Regards
> >> Andrzej
> >>> +	if (IS_ERR(thc63->pwdn)) {
> >>> +		dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> >>> +		return PTR_ERR(thc63->pwdn);
> >>> +	}
> >>> +
> >>> +	thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> >>> +	if (IS_ERR(thc63->oe)) {
> >>> +		dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> >>> +		return PTR_ERR(thc63->oe);
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int thc63_regulator_init(struct thc63_dev *thc63)
> >>> +{
> >>> +	struct regulator **reg;
> >>> +	int i;
> >>> +
> >>> +	reg = &thc63->vccs[0];
> >>> +	for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> >>> +		*reg = devm_regulator_get_optional(thc63->dev,
> >>> +						   thc63_reg_names[i]);
> >>> +		if (IS_ERR(*reg)) {
> >>> +			if (PTR_ERR(*reg) == -EPROBE_DEFER)
> >>> +				return -EPROBE_DEFER;
> >>> +			*reg = NULL;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	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);
> >>> +
> >>> +	ret = thc63_regulator_init(thc63);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	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;
> >>> +}
> >>> +
> >>> +#ifdef CONFIG_OF
> >>> +static const struct of_device_id thc63_match[] = {
> >>> +	{ .compatible = "thine,thc63lvd1024", },
> >>> +	{ },
> >>> +};
> >>> +MODULE_DEVICE_TABLE(of, thc63_match);
> >>> +#endif
> >>> +
> >>> +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");
> >>
>

[-- 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 v3 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Laurent Pinchart]

Hi Jacopo,

On Wednesday, 14 March 2018 11:06:51 EET jacopo mondi wrote:
> Hi Andrzej,
>     sorry for the mess :(
> 
> On Wed, Mar 14, 2018 at 09:15:42AM +0100, Andrzej Hajda wrote:
> > On 13.03.2018 15:30, Jacopo Mondi wrote:
> > > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > > 
> > >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 +++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> > >  t
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt
> > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt new file mode 100644
> > > index 0000000..5b5afcd
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt @@ -0,0 +1,63 @@
> > > +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 two input LVDS stream and up to
> > > two digital CMOS/TTL outputs. +
> > > +Required properties:
> > > +- compatible: Shall be "thine,thc63lvd1024",
> > > +
> > > +Optional properties:
> > > +- vcc-supply: Power supply for TTL output and digital circuitry
> > > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> > > +- lvcc-supply: Power supply for LVDS inputs
> > > +- pvcc-supply: Power supply for PLL circuitry
> > 
> > I wonder if it wouldn't be better to make them required (at least VCC) -
> > it is closer to reality.
> 
> In cases like our Eagle board, where VCC is directly connected to the
> powering rail and not through a controllable regulator, I feel like
> making this mandatory requires more effort (not much, I agree, just a
> "fixed-regulator" more) with no additional benefits.
> 
> But I understand your point, and I'm open to whatever fits better with
> the already existing DRM bridges bindings

I still haven't made up my mind on this topic. I like making the VCC power 
supply mandatory as it is mandatory, but you're right that it creates 
additional complexity in DT without much added benefit (although it might 
simplify the driver as bit). In that case I'd make the other supplies 
mandatory too. I'm tempted to specify a single power supply in DT though, as 
I'd be quite surprised to see a system with individually controllable supplies 
for the different power rails (they all use the same voltage), but surprises 
happen. We've had similar cases in other bindings before (I'm afraid I can't 
recall which) where Rob was fine having a single supply. Maybe we could apply 
the same logic with a single vcc supply, and add the other supplies later as 
optional properties if we ever need to specify them separately ?

> > > +- pwnd-gpios: Power down GPIO signal. Active low.
> > 
> > As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
> > different docs?
> 
> I didn't notice the typo in first review, and I thought you were referring
> to the initial '/' which I found weird to be part of the gpio name... Then
> I now realized I badly typed in "pwnd" in place of "pwdn", which is not
> even correct because it has to be "pdwn"... Sorry about this mess, I will
> fix in v4
> 
> > Moreover there are already bindings for THC63LVDM83D with the same
> > dichotomy [2].
> 
> Seems like this is 'wrong' as well.. The chip manual says the pin is
> named "pdwn" here too..
> 
> > Out of curiosity I have googled for "pwnd pin" and it looks like some
> > vendors use this form.
> > For me both forms are quite misleading: power down signal, active low,
> > why they couldn't call it just 'enable, active high'.
> 
> It's not much the actual physical active level that bugs me, but the fact
> that the GPIO name defines if it has to be set to "active" or
> "inactive" logical state in enable/disable routines that I don't
> like..
> 
> > [1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
> > [2]: https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/
> > devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt> 
> > > +- oe-gpios: Output enable GPIO signal. 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>;
> > > +		lvcc-supply = <&reg_lvds_lvcc>;
> > > +
> > > +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > 
> > And here another variation :), should be pdwn-gpios.
> 
> Next time it will be "pndw".. Is there a prize if I do insert all
> permutations of the same name in a single bindings document? :)
> 
> Will fix this shortly.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Jacopo,

On Wednesday, 14 March 2018 11:06:51 EET jacopo mondi wrote:
> Hi Andrzej,
>     sorry for the mess :(
> 
> On Wed, Mar 14, 2018 at 09:15:42AM +0100, Andrzej Hajda wrote:
> > On 13.03.2018 15:30, Jacopo Mondi wrote:
> > > Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > ---
> > > 
> > >  .../bindings/display/bridge/thine,thc63lvd1024.txt | 63 +++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.tx
> > >  t
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt
> > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt new file mode 100644
> > > index 0000000..5b5afcd
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.t
> > > xt @@ -0,0 +1,63 @@
> > > +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 two input LVDS stream and up to
> > > two digital CMOS/TTL outputs. +
> > > +Required properties:
> > > +- compatible: Shall be "thine,thc63lvd1024",
> > > +
> > > +Optional properties:
> > > +- vcc-supply: Power supply for TTL output and digital circuitry
> > > +- cvcc-supply: Power supply for TTL CLOCKOUT signal
> > > +- lvcc-supply: Power supply for LVDS inputs
> > > +- pvcc-supply: Power supply for PLL circuitry
> > 
> > I wonder if it wouldn't be better to make them required (at least VCC) -
> > it is closer to reality.
> 
> In cases like our Eagle board, where VCC is directly connected to the
> powering rail and not through a controllable regulator, I feel like
> making this mandatory requires more effort (not much, I agree, just a
> "fixed-regulator" more) with no additional benefits.
> 
> But I understand your point, and I'm open to whatever fits better with
> the already existing DRM bridges bindings

I still haven't made up my mind on this topic. I like making the VCC power 
supply mandatory as it is mandatory, but you're right that it creates 
additional complexity in DT without much added benefit (although it might 
simplify the driver as bit). In that case I'd make the other supplies 
mandatory too. I'm tempted to specify a single power supply in DT though, as 
I'd be quite surprised to see a system with individually controllable supplies 
for the different power rails (they all use the same voltage), but surprises 
happen. We've had similar cases in other bindings before (I'm afraid I can't 
recall which) where Rob was fine having a single supply. Maybe we could apply 
the same logic with a single vcc supply, and add the other supplies later as 
optional properties if we ever need to specify them separately ?

> > > +- pwnd-gpios: Power down GPIO signal. Active low.
> > 
> > As I said before, specs[1] says about "/PDWN" pin. Is it your typo, or
> > different docs?
> 
> I didn't notice the typo in first review, and I thought you were referring
> to the initial '/' which I found weird to be part of the gpio name... Then
> I now realized I badly typed in "pwnd" in place of "pwdn", which is not
> even correct because it has to be "pdwn"... Sorry about this mess, I will
> fix in v4
> 
> > Moreover there are already bindings for THC63LVDM83D with the same
> > dichotomy [2].
> 
> Seems like this is 'wrong' as well.. The chip manual says the pin is
> named "pdwn" here too..
> 
> > Out of curiosity I have googled for "pwnd pin" and it looks like some
> > vendors use this form.
> > For me both forms are quite misleading: power down signal, active low,
> > why they couldn't call it just 'enable, active high'.
> 
> It's not much the actual physical active level that bugs me, but the fact
> that the GPIO name defines if it has to be set to "active" or
> "inactive" logical state in enable/disable routines that I don't
> like..
> 
> > [1]: http://www.thine.co.jp/files/topics/179_ext_12_0.pdf
> > [2]: https://elixir.bootlin.com/linux/v4.16-rc5/source/Documentation/
> > devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt> 
> > > +- oe-gpios: Output enable GPIO signal. 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>;
> > > +		lvcc-supply = <&reg_lvds_lvcc>;
> > > +
> > > +		pwdn-gpio = <&gpio4 15 GPIO_ACTIVE_LOW>;
> > 
> > And here another variation :), should be pdwn-gpios.
> 
> Next time it will be "pndw".. Is there a prize if I do insert all
> permutations of the same name in a single bindings document? :)
> 
> Will fix this shortly.

-- 
Regards,

Laurent Pinchart

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