[PATCH v2 0/5] omapdss: Add video output support for gta04 board

[PATCH v2 0/5] omapdss: Add video output support for gta04 board

[Marek Belisko]

This patch series adds support for video output on gta04 board. It consist
of opa362 video amplifier driver + DT bindings, adding venc -> opa362 -> svideo-connecor
setup in DT and finally static setup for tvbypass anc acbias in devconf1 register
via pinctrl subsystem.

Changes from v1:
- fix opa362 compilation error
- fix opa362 DT documentation
- move devconf1 definition to omap3.dtsi

Marek Belisko (5):
  video: omapdss: Add opa362 driver
  Documentation: DT: Add documentation for ti,opa362 bindings
  arm: dts: omap3-gta04: Add handling for tv output
  arm: dts: omap3: Add definition for devconf1 register
  arm: dts: omap3-gta04: Add static configuration for devconf1 register

 .../devicetree/bindings/video/ti,opa362.txt        |  38 +++
 arch/arm/boot/dts/omap3-gta04.dtsi                 |  59 ++++
 arch/arm/boot/dts/omap3.dtsi                       |  13 +
 arch/arm/boot/dts/omap34xx.dtsi                    |   4 +
 arch/arm/boot/dts/omap36xx.dtsi                    |   4 +
 drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
 drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
 .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++
 include/video/omap-panel-data.h                    |  12 +
 9 files changed, 480 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt
 create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c

-- 
1.9.1

[PATCH v2 1/5] video: omapdss: Add opa362 driver

[Marek Belisko]

opa362 is amplifier for video and can be connected to the tvout pads
of the OMAP3. It has one gpio control for enable/disable of the output
(high impedance).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
 drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
 .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++
 include/video/omap-panel-data.h                    |  12 +
 4 files changed, 362 insertions(+)
 create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c

diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
index e6cfc38..211b3ec 100644
--- a/drivers/video/fbdev/omap2/displays-new/Kconfig
+++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
@@ -1,6 +1,12 @@
 menu "OMAP Display Device Drivers (new device model)"
         depends on OMAP2_DSS
 
+config DISPLAY_AMPLIFIER_OPA362
+        tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
+	help
+	  Driver to enable an external analog TV amplifier (e.g. OPA362)
+	  through a GPIO.
+
 config DISPLAY_ENCODER_TFP410
         tristate "TFP410 DPI to DVI Encoder"
 	help
diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
index 0323a8a..b311542 100644
--- a/drivers/video/fbdev/omap2/displays-new/Makefile
+++ b/drivers/video/fbdev/omap2/displays-new/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
 obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
 obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
 obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
new file mode 100644
index 0000000..8065a28
--- /dev/null
+++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
@@ -0,0 +1,343 @@
+/*
+ * OPA362 analog video amplifier with output/power control
+ *
+ * Copyright (C) 2014 Golden Delicious Computers
+ * Author: H. Nikolaus Schaller <hns@goldelico.com>
+ *
+ * based on encoder-tfp410
+ *
+ * Copyright (C) 2013 Texas Instruments
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of_gpio.h>
+
+#include <video/omapdss.h>
+#include <video/omap-panel-data.h>
+
+struct panel_drv_data {
+	struct omap_dss_device dssdev;
+	struct omap_dss_device *in;
+
+	int enable_gpio;
+
+	struct omap_video_timings timings;
+};
+
+#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
+
+static int opa362_connect(struct omap_dss_device *dssdev,
+		struct omap_dss_device *dst)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	dev_dbg(dssdev->dev, "connect\n");
+
+	if (omapdss_device_is_connected(dssdev))
+		return -EBUSY;
+
+	r = in->ops.atv->connect(in, dssdev);
+	if (r)
+		return r;
+
+	dst->src = dssdev;
+	dssdev->dst = dst;
+
+	return 0;
+}
+
+static void opa362_disconnect(struct omap_dss_device *dssdev,
+		struct omap_dss_device *dst)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "disconnect\n");
+
+	WARN_ON(!omapdss_device_is_connected(dssdev));
+	if (!omapdss_device_is_connected(dssdev))
+		return;
+
+	WARN_ON(dst != dssdev->dst);
+	if (dst != dssdev->dst)
+		return;
+
+	dst->src = NULL;
+	dssdev->dst = NULL;
+
+	in->ops.atv->disconnect(in, &ddata->dssdev);
+}
+
+static int opa362_enable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	dev_dbg(dssdev->dev, "enable\n");
+
+	if (!omapdss_device_is_connected(dssdev))
+		return -ENODEV;
+
+	if (omapdss_device_is_enabled(dssdev))
+		return 0;
+
+	in->ops.atv->set_timings(in, &ddata->timings);
+	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
+	in->ops.atv->invert_vid_out_polarity(in, true);
+
+	r = in->ops.atv->enable(in);
+	if (r)
+		return r;
+
+	if (gpio_is_valid(ddata->enable_gpio))
+		gpio_set_value_cansleep(ddata->enable_gpio, 1);
+
+	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
+
+	return 0;
+}
+
+static void opa362_disable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "disable\n");
+
+	if (!omapdss_device_is_enabled(dssdev))
+		return;
+
+	if (gpio_is_valid(ddata->enable_gpio))
+		gpio_set_value_cansleep(ddata->enable_gpio, 0);
+
+	in->ops.atv->disable(in);
+
+	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
+}
+
+static void opa362_set_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "set_timings\n");
+
+	ddata->timings = *timings;
+	dssdev->panel.timings = *timings;
+
+	in->ops.atv->set_timings(in, timings);
+}
+
+static void opa362_get_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+
+	dev_dbg(dssdev->dev, "get_timings\n");
+
+	*timings = ddata->timings;
+}
+
+static int opa362_check_timings(struct omap_dss_device *dssdev,
+		struct omap_video_timings *timings)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	dev_dbg(dssdev->dev, "check_timings\n");
+
+	return in->ops.atv->check_timings(in, timings);
+}
+
+static void opa362_set_type(struct omap_dss_device *dssdev,
+		enum omap_dss_venc_type type)
+{
+	/* we can only drive a COMPOSITE output */
+	WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
+
+}
+
+static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
+		bool invert_polarity)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	/* OPA362 inverts the polarity */
+	in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
+}
+
+static const struct omapdss_atv_ops opa362_atv_ops = {
+	.connect	= opa362_connect,
+	.disconnect	= opa362_disconnect,
+
+	.enable		= opa362_enable,
+	.disable	= opa362_disable,
+
+	.check_timings	= opa362_check_timings,
+	.set_timings	= opa362_set_timings,
+	.get_timings	= opa362_get_timings,
+
+	.set_type	= opa362_set_type,
+	.invert_vid_out_polarity	= opa362_invert_vid_out_polarity,
+};
+
+static int opa362_probe_pdata(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct amplifier_opa362_platform_data *pdata;
+	struct omap_dss_device *dssdev, *in;
+
+	pdata = dev_get_platdata(&pdev->dev);
+
+	ddata->enable_gpio = pdata->enable_gpio;
+
+	in = omap_dss_find_output(pdata->source);
+	if (in == NULL) {
+		dev_err(&pdev->dev, "Failed to find video source\n");
+		return -ENODEV;
+	}
+
+	ddata->in = in;
+
+	dssdev = &ddata->dssdev;
+	dssdev->name = pdata->name;
+
+	return 0;
+}
+
+static int opa362_probe_of(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct omap_dss_device *in;
+	int gpio;
+
+	gpio = of_get_gpio(node, 0);
+
+	if (gpio_is_valid(gpio) || gpio == -ENOENT) {
+		ddata->enable_gpio = gpio;
+	} else {
+		dev_err(&pdev->dev, "failed to parse enable gpio\n");
+		return gpio;
+	}
+
+	in = omapdss_of_find_source_for_first_ep(node);
+	if (IS_ERR(in)) {
+		dev_err(&pdev->dev, "failed to find video source\n");
+		return PTR_ERR(in);
+	}
+
+	ddata->in = in;
+
+	return 0;
+}
+
+static int opa362_probe(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata;
+	struct omap_dss_device *dssdev;
+	int r;
+
+	dev_dbg(&pdev->dev, "probe\n");
+
+	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, ddata);
+
+	if (dev_get_platdata(&pdev->dev)) {
+		r = opa362_probe_pdata(pdev);
+		if (r)
+			return r;
+	} else {
+		r = opa362_probe_of(pdev);
+		if (r)
+			return r;
+	}
+
+	if (gpio_is_valid(ddata->enable_gpio)) {
+		r = devm_gpio_request_one(&pdev->dev, ddata->enable_gpio,
+				GPIOF_OUT_INIT_LOW, "opa362 enable");
+		if (r) {
+			dev_err(&pdev->dev, "Failed to request enable GPIO %d\n",
+					ddata->enable_gpio);
+			goto err_gpio;
+		}
+	}
+
+	dssdev = &ddata->dssdev;
+	dssdev->ops.atv = &opa362_atv_ops;
+	dssdev->dev = &pdev->dev;
+	dssdev->type = OMAP_DISPLAY_TYPE_VENC;
+	dssdev->output_type = OMAP_DISPLAY_TYPE_VENC;
+	dssdev->owner = THIS_MODULE;
+
+	r = omapdss_register_output(dssdev);
+	if (r) {
+		dev_err(&pdev->dev, "Failed to register output\n");
+		goto err_reg;
+	}
+
+	return 0;
+err_reg:
+err_gpio:
+	omap_dss_put_device(ddata->in);
+	return r;
+}
+
+static int __exit opa362_remove(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
+	struct omap_dss_device *dssdev = &ddata->dssdev;
+	struct omap_dss_device *in = ddata->in;
+
+	omapdss_unregister_output(&ddata->dssdev);
+
+	WARN_ON(omapdss_device_is_enabled(dssdev));
+	if (omapdss_device_is_enabled(dssdev))
+		opa362_disable(dssdev);
+
+	WARN_ON(omapdss_device_is_connected(dssdev));
+	if (omapdss_device_is_connected(dssdev))
+		opa362_disconnect(dssdev, dssdev->dst);
+
+	omap_dss_put_device(in);
+
+	return 0;
+}
+
+static const struct of_device_id opa362_of_match[] = {
+	{ .compatible = "omapdss,ti,opa362", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, opa362_of_match);
+
+static struct platform_driver opa362_driver = {
+	.probe	= opa362_probe,
+	.remove	= __exit_p(opa362_remove),
+	.driver	= {
+		.name	= "amplifier-opa362",
+		.owner	= THIS_MODULE,
+		.of_match_table = opa362_of_match,
+	},
+};
+
+module_platform_driver(opa362_driver);
+
+MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
+MODULE_DESCRIPTION("OPA362 analog video amplifier with output/power control");
+MODULE_LICENSE("GPL");
diff --git a/include/video/omap-panel-data.h b/include/video/omap-panel-data.h
index 69279c0..5c3076c 100644
--- a/include/video/omap-panel-data.h
+++ b/include/video/omap-panel-data.h
@@ -251,4 +251,16 @@ struct panel_tpo_td028ttec1_platform_data {
 	int data_lines;
 };
 
+/**
+ * amplifier_opa362_platform_data platform data
+ * @name: name for this display entity
+ * @source: name of the display entity used as a video source
+ * @enable_gpio: gpio number for enable pi
+ */
+struct amplifier_opa362_platform_data {
+	const char *name;
+	const char *source;
+	int enable_gpio;
+};
+
 #endif /* __OMAP_PANEL_DATA_H */
-- 
1.9.1

[PATCH v2 2/5] Documentation: DT: Add documentation for ti,opa362 bindings

[Marek Belisko]

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 .../devicetree/bindings/video/ti,opa362.txt        | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/ti,opa362.txt

diff --git a/Documentation/devicetree/bindings/video/ti,opa362.txt b/Documentation/devicetree/bindings/video/ti,opa362.txt
new file mode 100644
index 0000000..4747ef9
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/ti,opa362.txt
@@ -0,0 +1,38 @@
+OPA362 analog video amplifier
+
+Required properties:
+- compatible: "ti,opa362"
+- gpio: enable/disable output gpio
+
+Required node:
+- Video port 0 for opa362 input
+- Video port 1 for opa362 output
+
+Example:
+
+tv_amp: opa362 {
+	compatible = "ti,opa362";
+	gpios = <&gpio1 23 0>;  /* GPIO to enable video out amplifier */
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			opa_in: endpoint@0 {
+				remote-endpoint = <&venc_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			opa_out: endpoint@0 {
+				remote-endpoint = <&tv_connector_in>;
+			};
+		};
+	};
+};
+
+
+
-- 
1.9.1

[PATCH v2 3/5] arm: dts: omap3-gta04: Add handling for tv output

[Marek Belisko]

Add handling for gta04 tv out chain:
venc -> opa362 -> svideo

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 48 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index fd34f91..b50dc10 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -83,6 +83,41 @@
 		compatible = "usb-nop-xceiv";
 		reset-gpios = <&gpio6 14 GPIO_ACTIVE_LOW>;
 	};
+
+	tv0: connector@1 {
+		compatible = "svideo-connector";
+		label = "tv";
+
+		port {
+			tv_connector_in: endpoint {
+				remote-endpoint = <&opa_out>;
+			};
+		};
+	};
+
+	tv_amp: opa362 {
+		compatible = "ti,opa362";
+		gpios = <&gpio1 23 0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				opa_in: endpoint@0 {
+					remote-endpoint = <&venc_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				opa_out: endpoint@0 {
+					remote-endpoint = <&tv_connector_in>;
+				};
+			};
+		};
+	};
 };
 
 &omap3_pmx_core {
@@ -396,6 +431,19 @@
 	};
 };
 
+&venc {
+	status = "okay";
+
+	vdda-supply = <&vdac>;
+
+	port {
+		venc_out: endpoint {
+			remote-endpoint = <&opa_in>;
+			ti,channels = <2>;
+		};
+	};
+};
+
 &gpmc {
 	ranges = <0 0 0x30000000 0x04>; /* CS0: NAND */
 
-- 
1.9.1

[PATCH v2 4/5] arm: dts: omap3: Add definition for devconf1 register

[Marek Belisko]

This patch expose DEVCONF1 register via pincrtl-single. Because reserved bits
are different for omap34xx and omap36xx functional-mask is defined in omap3
variant dtsi files. Bit MPUFORCEWRNP is leaved out.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3.dtsi    | 13 +++++++++++++
 arch/arm/boot/dts/omap34xx.dtsi |  4 ++++
 arch/arm/boot/dts/omap36xx.dtsi |  4 ++++
 3 files changed, 21 insertions(+)

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index d0e884d..75aaab3 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -181,6 +181,19 @@
 			pinctrl-single,function-mask = <0xff1f>;
 		};
 
+		control_devconf1: pinmux@480022d8 {
+			compatible = "pinctrl-single";
+			reg = <0x480022d8 4>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			pinctrl-single,bit-per-mux;
+			pinctrl-single,register-width = <32>;
+			/*
+			 * reserved bits differs for omap34xx and omap36xx
+			 * so function-mask is defined in appropriate dtsi files
+			 */
+		};
+
 		omap3_scm_general: tisyscon@48002270 {
 			compatible = "syscon";
 			reg = <0x48002270 0x2f0>;
diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index 3819c1e..18299b0 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -51,6 +51,10 @@
 		      "ssi_ick";
 };
 
+&control_devconf1 {
+	pinctrl-single,function-mask = <0xfc79d5>;
+};
+
 /include/ "omap34xx-omap36xx-clocks.dtsi"
 /include/ "omap36xx-omap3430es2plus-clocks.dtsi"
 /include/ "omap36xx-am35xx-omap3430es2plus-clocks.dtsi"
diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
index 541704a..736d35d 100644
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -89,6 +89,10 @@
 		      "ssi_ick";
 };
 
+&control_devconf1 {
+	pinctrl-single,function-mask = <0xfc09d5>;
+};
+
 /include/ "omap34xx-omap36xx-clocks.dtsi"
 /include/ "omap36xx-omap3430es2plus-clocks.dtsi"
 /include/ "omap36xx-am35xx-omap3430es2plus-clocks.dtsi"
-- 
1.9.1

[PATCH v2 5/5] arm: dts: omap3-gta04: Add static configuration for devconf1 register

[Marek Belisko]

gta04 board need for tvout enabled 2 bits in devconf1 register (tvbypass and acbias).
Add pinctrl single entry and enable it.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/boot/dts/omap3-gta04.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index b50dc10..0015316 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -497,3 +497,14 @@
 		};
 	};
 };
+
+&control_devconf1 {
+	pinctrl-name = "default";
+	pinctrl-0 = <&tv_acbias_pins>;
+
+	tv_acbias_pins: pinmux_tv_acbias_pins {
+		pinctrl-single,bits = <
+			0 0x40800 0x40800
+		>;
+	};
+};
-- 
1.9.1

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Tomi Valkeinen]

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

On 13/11/14 00:10, Marek Belisko wrote:
> opa362 is amplifier for video and can be connected to the tvout pads
> of the OMAP3. It has one gpio control for enable/disable of the output
> (high impedance).
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>  drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>  .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++

I think it would be better to rename this to encoder-opa362.c. It's not
encoder as such, but it falls into the same category.

>  include/video/omap-panel-data.h                    |  12 +
>  4 files changed, 362 insertions(+)
>  create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> 
> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
> index e6cfc38..211b3ec 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
> @@ -1,6 +1,12 @@
>  menu "OMAP Display Device Drivers (new device model)"
>          depends on OMAP2_DSS
>  
> +config DISPLAY_AMPLIFIER_OPA362

Here also use ENCODER instead.

> +        tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
> +	help
> +	  Driver to enable an external analog TV amplifier (e.g. OPA362)
> +	  through a GPIO.

The indentation above seems funny.

The text looks a bit odd. So is this a driver for OPA362, or is this a
generic driver for any similar devices? Most of the names and code makes
me think this is a driver for OPA362, but the text above quite clearly
gives the impression that this is a driver for any analog video amp,
with single enable gpio.

> +
>  config DISPLAY_ENCODER_TFP410
>          tristate "TFP410 DPI to DVI Encoder"
>  	help
> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
> index 0323a8a..b311542 100644
> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>  obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>  obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>  obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> new file mode 100644
> index 0000000..8065a28
> --- /dev/null
> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
> @@ -0,0 +1,343 @@
> +/*
> + * OPA362 analog video amplifier with output/power control
> + *
> + * Copyright (C) 2014 Golden Delicious Computers
> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
> + *
> + * based on encoder-tfp410
> + *
> + * Copyright (C) 2013 Texas Instruments
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of_gpio.h>
> +
> +#include <video/omapdss.h>
> +#include <video/omap-panel-data.h>
> +
> +struct panel_drv_data {
> +	struct omap_dss_device dssdev;
> +	struct omap_dss_device *in;
> +
> +	int enable_gpio;
> +
> +	struct omap_video_timings timings;
> +};
> +
> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> +
> +static int opa362_connect(struct omap_dss_device *dssdev,
> +		struct omap_dss_device *dst)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +	int r;
> +
> +	dev_dbg(dssdev->dev, "connect\n");
> +
> +	if (omapdss_device_is_connected(dssdev))
> +		return -EBUSY;
> +
> +	r = in->ops.atv->connect(in, dssdev);
> +	if (r)
> +		return r;
> +
> +	dst->src = dssdev;
> +	dssdev->dst = dst;
> +
> +	return 0;
> +}
> +
> +static void opa362_disconnect(struct omap_dss_device *dssdev,
> +		struct omap_dss_device *dst)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	dev_dbg(dssdev->dev, "disconnect\n");
> +
> +	WARN_ON(!omapdss_device_is_connected(dssdev));
> +	if (!omapdss_device_is_connected(dssdev))
> +		return;
> +
> +	WARN_ON(dst != dssdev->dst);
> +	if (dst != dssdev->dst)
> +		return;
> +
> +	dst->src = NULL;
> +	dssdev->dst = NULL;
> +
> +	in->ops.atv->disconnect(in, &ddata->dssdev);
> +}
> +
> +static int opa362_enable(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +	int r;
> +
> +	dev_dbg(dssdev->dev, "enable\n");
> +
> +	if (!omapdss_device_is_connected(dssdev))
> +		return -ENODEV;
> +
> +	if (omapdss_device_is_enabled(dssdev))
> +		return 0;
> +
> +	in->ops.atv->set_timings(in, &ddata->timings);
> +	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
> +	in->ops.atv->invert_vid_out_polarity(in, true);

Well this does seem to be broken. I don't know what the answer to the
question above is, but the code doesn't work properly.

In the opa362_invert_vid_out_polarity function below, you get the invert
boolean from the connector. This you pass to the OMAP venc. However,
above you always override that value in venc with true.

So, either the invert_vid_out_polarity value has to be always true or
false, because _OPA362_ requires it to be true or false, OR you need use
the value from the connector.

Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
latter, and the call above should be removed.

> +
> +	r = in->ops.atv->enable(in);
> +	if (r)
> +		return r;
> +
> +	if (gpio_is_valid(ddata->enable_gpio))
> +		gpio_set_value_cansleep(ddata->enable_gpio, 1);
> +
> +	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
> +
> +	return 0;
> +}
> +
> +static void opa362_disable(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	dev_dbg(dssdev->dev, "disable\n");
> +
> +	if (!omapdss_device_is_enabled(dssdev))
> +		return;
> +
> +	if (gpio_is_valid(ddata->enable_gpio))
> +		gpio_set_value_cansleep(ddata->enable_gpio, 0);
> +
> +	in->ops.atv->disable(in);
> +
> +	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
> +}
> +
> +static void opa362_set_timings(struct omap_dss_device *dssdev,
> +		struct omap_video_timings *timings)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	dev_dbg(dssdev->dev, "set_timings\n");
> +
> +	ddata->timings = *timings;
> +	dssdev->panel.timings = *timings;
> +
> +	in->ops.atv->set_timings(in, timings);
> +}
> +
> +static void opa362_get_timings(struct omap_dss_device *dssdev,
> +		struct omap_video_timings *timings)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	dev_dbg(dssdev->dev, "get_timings\n");
> +
> +	*timings = ddata->timings;
> +}
> +
> +static int opa362_check_timings(struct omap_dss_device *dssdev,
> +		struct omap_video_timings *timings)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	dev_dbg(dssdev->dev, "check_timings\n");
> +
> +	return in->ops.atv->check_timings(in, timings);
> +}
> +
> +static void opa362_set_type(struct omap_dss_device *dssdev,
> +		enum omap_dss_venc_type type)
> +{
> +	/* we can only drive a COMPOSITE output */
> +	WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
> +
> +}
> +
> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
> +		bool invert_polarity)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	/* OPA362 inverts the polarity */
> +	in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
> +}
> +
> +static const struct omapdss_atv_ops opa362_atv_ops = {
> +	.connect	= opa362_connect,
> +	.disconnect	= opa362_disconnect,
> +
> +	.enable		= opa362_enable,
> +	.disable	= opa362_disable,
> +
> +	.check_timings	= opa362_check_timings,
> +	.set_timings	= opa362_set_timings,
> +	.get_timings	= opa362_get_timings,
> +
> +	.set_type	= opa362_set_type,
> +	.invert_vid_out_polarity	= opa362_invert_vid_out_polarity,
> +};
> +
> +static int opa362_probe_pdata(struct platform_device *pdev)
> +{
> +	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> +	struct amplifier_opa362_platform_data *pdata;
> +	struct omap_dss_device *dssdev, *in;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +
> +	ddata->enable_gpio = pdata->enable_gpio;
> +
> +	in = omap_dss_find_output(pdata->source);
> +	if (in == NULL) {
> +		dev_err(&pdev->dev, "Failed to find video source\n");
> +		return -ENODEV;
> +	}
> +
> +	ddata->in = in;
> +
> +	dssdev = &ddata->dssdev;
> +	dssdev->name = pdata->name;
> +
> +	return 0;
> +}

We are going to support only DT boot at some point. Thus I think the
whole platform data code should be left out.

> +
> +static int opa362_probe_of(struct platform_device *pdev)
> +{
> +	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> +	struct device_node *node = pdev->dev.of_node;
> +	struct omap_dss_device *in;
> +	int gpio;
> +
> +	gpio = of_get_gpio(node, 0);
> +
> +	if (gpio_is_valid(gpio) || gpio == -ENOENT) {
> +		ddata->enable_gpio = gpio;
> +	} else {
> +		dev_err(&pdev->dev, "failed to parse enable gpio\n");
> +		return gpio;
> +	}

You should use the new GPIO API to get full support from the DT data.
For an example, see panel-dpi.c's enable_gpio.

 Tomi



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

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Dr. H. Nikolaus Schaller]

Hi,

Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:

> On 13/11/14 00:10, Marek Belisko wrote:
>> opa362 is amplifier for video and can be connected to the tvout pads
>> of the OMAP3. It has one gpio control for enable/disable of the output
>> (high impedance).
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>> drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>> .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++
> 
> I think it would be better to rename this to encoder-opa362.c. It's not

When developing this driver we did simply rename the encoder-tfp410 file,
but thent hough that it does not fit into the „encoder“ category, because we
would expect something digital or digital to analog „encoding“ which it does not.

> encoder as such, but it falls into the same category.

But we can change it.

> 
>> include/video/omap-panel-data.h                    |  12 +
>> 4 files changed, 362 insertions(+)
>> create mode 100644 drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> 
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Kconfig b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> index e6cfc38..211b3ec 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Kconfig
>> +++ b/drivers/video/fbdev/omap2/displays-new/Kconfig
>> @@ -1,6 +1,12 @@
>> menu "OMAP Display Device Drivers (new device model)"
>>         depends on OMAP2_DSS
>> 
>> +config DISPLAY_AMPLIFIER_OPA362
> 
> Here also use ENCODER instead.
> 
>> +        tristate "external analog amplifier with output disable/high-Z (e.g. OPA362)"
>> +	help
>> +	  Driver to enable an external analog TV amplifier (e.g. OPA362)
>> +	  through a GPIO.
> 
> The indentation above seems funny.
> 
> The text looks a bit odd. So is this a driver for OPA362, or is this a
> generic driver for any similar devices? Most of the names and code makes
> me think this is a driver for OPA362, but the text above quite clearly
> gives the impression that this is a driver for any analog video amp,
> with single enable gpio.

Hm. We can imagine that there are other devices with similar functionality
and gpio but we have not tested any. So it is indeed better to describe it as
a pure OPA362 driver.

> 
>> +
>> config DISPLAY_ENCODER_TFP410
>>         tristate "TFP410 DPI to DVI Encoder"
>> 	help
>> diff --git a/drivers/video/fbdev/omap2/displays-new/Makefile b/drivers/video/fbdev/omap2/displays-new/Makefile
>> index 0323a8a..b311542 100644
>> --- a/drivers/video/fbdev/omap2/displays-new/Makefile
>> +++ b/drivers/video/fbdev/omap2/displays-new/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_DISPLAY_AMPLIFIER_OPA362) += amplifier-opa362.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TFP410) += encoder-tfp410.o
>> obj-$(CONFIG_DISPLAY_ENCODER_TPD12S015) += encoder-tpd12s015.o
>> obj-$(CONFIG_DISPLAY_CONNECTOR_DVI) += connector-dvi.o
>> diff --git a/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> new file mode 100644
>> index 0000000..8065a28
>> --- /dev/null
>> +++ b/drivers/video/fbdev/omap2/displays-new/amplifier-opa362.c
>> @@ -0,0 +1,343 @@
>> +/*
>> + * OPA362 analog video amplifier with output/power control
>> + *
>> + * Copyright (C) 2014 Golden Delicious Computers
>> + * Author: H. Nikolaus Schaller <hns@goldelico.com>
>> + *
>> + * based on encoder-tfp410
>> + *
>> + * Copyright (C) 2013 Texas Instruments
>> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of_gpio.h>
>> +
>> +#include <video/omapdss.h>
>> +#include <video/omap-panel-data.h>
>> +
>> +struct panel_drv_data {
>> +	struct omap_dss_device dssdev;
>> +	struct omap_dss_device *in;
>> +
>> +	int enable_gpio;
>> +
>> +	struct omap_video_timings timings;
>> +};
>> +
>> +#define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>> +
>> +static int opa362_connect(struct omap_dss_device *dssdev,
>> +		struct omap_dss_device *dst)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +	int r;
>> +
>> +	dev_dbg(dssdev->dev, "connect\n");
>> +
>> +	if (omapdss_device_is_connected(dssdev))
>> +		return -EBUSY;
>> +
>> +	r = in->ops.atv->connect(in, dssdev);
>> +	if (r)
>> +		return r;
>> +
>> +	dst->src = dssdev;
>> +	dssdev->dst = dst;
>> +
>> +	return 0;
>> +}
>> +
>> +static void opa362_disconnect(struct omap_dss_device *dssdev,
>> +		struct omap_dss_device *dst)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +
>> +	dev_dbg(dssdev->dev, "disconnect\n");
>> +
>> +	WARN_ON(!omapdss_device_is_connected(dssdev));
>> +	if (!omapdss_device_is_connected(dssdev))
>> +		return;
>> +
>> +	WARN_ON(dst != dssdev->dst);
>> +	if (dst != dssdev->dst)
>> +		return;
>> +
>> +	dst->src = NULL;
>> +	dssdev->dst = NULL;
>> +
>> +	in->ops.atv->disconnect(in, &ddata->dssdev);
>> +}
>> +
>> +static int opa362_enable(struct omap_dss_device *dssdev)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +	int r;
>> +
>> +	dev_dbg(dssdev->dev, "enable\n");
>> +
>> +	if (!omapdss_device_is_connected(dssdev))
>> +		return -ENODEV;
>> +
>> +	if (omapdss_device_is_enabled(dssdev))
>> +		return 0;
>> +
>> +	in->ops.atv->set_timings(in, &ddata->timings);
>> +	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
>> +	in->ops.atv->invert_vid_out_polarity(in, true);
> 
> Well this does seem to be broken. I don't know what the answer to the
> question above is, but the code doesn't work properly.
> 
> In the opa362_invert_vid_out_polarity function below, you get the invert
> boolean from the connector. This you pass to the OMAP venc. However,
> above you always override that value in venc with true.
> 
> So, either the invert_vid_out_polarity value has to be always true or
> false, because _OPA362_ requires it to be true or false, OR you need use
> the value from the connector.
> 
> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
> latter, and the call above should be removed.

Yes, you are right - this is not systematic.

But the problem is that we can’t ask the connector here what it wants
to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
which we can then forward to overwrite the inital state of this opa362_enable().

What it should be is something like

in->ops.atv->invert_vid_out_polarity(in, ! out->ops.atv->get_expected_vid_output_polarity())

but that does not exist and I am not sure if you want to introduce it.

So I only see the option that we always use 

in->ops.atv->invert_vid_out_polarity(in, true);

and do a sanity check in opa362_invert_vid_out_polarity() to block a connector
that is requesting the wrong polarity.

> 
>> +
>> +	r = in->ops.atv->enable(in);
>> +	if (r)
>> +		return r;
>> +
>> +	if (gpio_is_valid(ddata->enable_gpio))
>> +		gpio_set_value_cansleep(ddata->enable_gpio, 1);
>> +
>> +	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
>> +
>> +	return 0;
>> +}
>> +
>> +static void opa362_disable(struct omap_dss_device *dssdev)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +
>> +	dev_dbg(dssdev->dev, "disable\n");
>> +
>> +	if (!omapdss_device_is_enabled(dssdev))
>> +		return;
>> +
>> +	if (gpio_is_valid(ddata->enable_gpio))
>> +		gpio_set_value_cansleep(ddata->enable_gpio, 0);
>> +
>> +	in->ops.atv->disable(in);
>> +
>> +	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
>> +}
>> +
>> +static void opa362_set_timings(struct omap_dss_device *dssdev,
>> +		struct omap_video_timings *timings)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +
>> +	dev_dbg(dssdev->dev, "set_timings\n");
>> +
>> +	ddata->timings = *timings;
>> +	dssdev->panel.timings = *timings;
>> +
>> +	in->ops.atv->set_timings(in, timings);
>> +}
>> +
>> +static void opa362_get_timings(struct omap_dss_device *dssdev,
>> +		struct omap_video_timings *timings)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +
>> +	dev_dbg(dssdev->dev, "get_timings\n");
>> +
>> +	*timings = ddata->timings;
>> +}
>> +
>> +static int opa362_check_timings(struct omap_dss_device *dssdev,
>> +		struct omap_video_timings *timings)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +
>> +	dev_dbg(dssdev->dev, "check_timings\n");
>> +
>> +	return in->ops.atv->check_timings(in, timings);
>> +}
>> +
>> +static void opa362_set_type(struct omap_dss_device *dssdev,
>> +		enum omap_dss_venc_type type)
>> +{
>> +	/* we can only drive a COMPOSITE output */
>> +	WARN_ON(type != OMAP_DSS_VENC_TYPE_COMPOSITE);
>> +
>> +}
>> +
>> +static void opa362_invert_vid_out_polarity(struct omap_dss_device *dssdev,
>> +		bool invert_polarity)
>> +{
>> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
>> +	struct omap_dss_device *in = ddata->in;
>> +
>> +	/* OPA362 inverts the polarity */
>> +	in->ops.atv->invert_vid_out_polarity(in, !invert_polarity);
>> +}
>> +
>> +static const struct omapdss_atv_ops opa362_atv_ops = {
>> +	.connect	= opa362_connect,
>> +	.disconnect	= opa362_disconnect,
>> +
>> +	.enable		= opa362_enable,
>> +	.disable	= opa362_disable,
>> +
>> +	.check_timings	= opa362_check_timings,
>> +	.set_timings	= opa362_set_timings,
>> +	.get_timings	= opa362_get_timings,
>> +
>> +	.set_type	= opa362_set_type,
>> +	.invert_vid_out_polarity	= opa362_invert_vid_out_polarity,
>> +};
>> +
>> +static int opa362_probe_pdata(struct platform_device *pdev)
>> +{
>> +	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> +	struct amplifier_opa362_platform_data *pdata;
>> +	struct omap_dss_device *dssdev, *in;
>> +
>> +	pdata = dev_get_platdata(&pdev->dev);
>> +
>> +	ddata->enable_gpio = pdata->enable_gpio;
>> +
>> +	in = omap_dss_find_output(pdata->source);
>> +	if (in == NULL) {
>> +		dev_err(&pdev->dev, "Failed to find video source\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ddata->in = in;
>> +
>> +	dssdev = &ddata->dssdev;
>> +	dssdev->name = pdata->name;
>> +
>> +	return 0;
>> +}
> 
> We are going to support only DT boot at some point. Thus I think the
> whole platform data code should be left out.

Is there already a decision? I think it should not be done before. And it
does not harm to still have it.

> 
>> +
>> +static int opa362_probe_of(struct platform_device *pdev)
>> +{
>> +	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct omap_dss_device *in;
>> +	int gpio;
>> +
>> +	gpio = of_get_gpio(node, 0);
>> +
>> +	if (gpio_is_valid(gpio) || gpio == -ENOENT) {
>> +		ddata->enable_gpio = gpio;
>> +	} else {
>> +		dev_err(&pdev->dev, "failed to parse enable gpio\n");
>> +		return gpio;
>> +	}
> 
> You should use the new GPIO API to get full support from the DT data.
> For an example, see panel-dpi.c's enable_gpio.

Ok.

BR,
Nikolaus

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Tomi Valkeinen]

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

On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
> Hi,
> 
> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
> 
>> On 13/11/14 00:10, Marek Belisko wrote:
>>> opa362 is amplifier for video and can be connected to the tvout pads
>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>> (high impedance).
>>>
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>>> drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>>> .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++
>>
>> I think it would be better to rename this to encoder-opa362.c. It's not
> 
> When developing this driver we did simply rename the encoder-tfp410 file,
> but thent hough that it does not fit into the „encoder“ category, because we
> would expect something digital or digital to analog „encoding“ which it does not.

That is true, but we already have other "encoders" like
encoder-tpd12s015.c, which also do not encode. "encoder" in this context
means something that takes a video input, and has a video output. In
contrast to a panel or a connector.

>>> +
>>> +	in->ops.atv->set_timings(in, &ddata->timings);
>>> +	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>> +	in->ops.atv->invert_vid_out_polarity(in, true);
>>
>> Well this does seem to be broken. I don't know what the answer to the
>> question above is, but the code doesn't work properly.
>>
>> In the opa362_invert_vid_out_polarity function below, you get the invert
>> boolean from the connector. This you pass to the OMAP venc. However,
>> above you always override that value in venc with true.
>>
>> So, either the invert_vid_out_polarity value has to be always true or
>> false, because _OPA362_ requires it to be true or false, OR you need use
>> the value from the connector.
>>
>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>> latter, and the call above should be removed.
> 
> Yes, you are right - this is not systematic.
> 
> But the problem is that we can’t ask the connector here what it wants
> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
> which we can then forward to overwrite the inital state of this opa362_enable().

You don't need to ask. The connector calls invert_vid_out_polarity
before enabling the output. You can just pass it forward inverted, as
you already do in this driver. If it doesn't, it's either a bug or you
can just rely on the value that is already programmed to venc.

>> We are going to support only DT boot at some point. Thus I think the
>> whole platform data code should be left out.
> 
> Is there already a decision? I think it should not be done before. And it
> does not harm to still have it.

It's just a matter of time. I don't accept any new boards using platform
data for display, or new display drivers using platform data, because I
don't want to spend my time converting them later. And as this is a new
driver, no existing board can be using the opa362 platform_data. So we
can support DT only.

 Tomi



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

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Dr. H. Nikolaus Schaller]

Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:

> On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen <tomi.valkeinen@ti.com>:
>> 
>>> On 13/11/14 00:10, Marek Belisko wrote:
>>>> opa362 is amplifier for video and can be connected to the tvout pads
>>>> of the OMAP3. It has one gpio control for enable/disable of the output
>>>> (high impedance).
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/video/fbdev/omap2/displays-new/Kconfig     |   6 +
>>>> drivers/video/fbdev/omap2/displays-new/Makefile    |   1 +
>>>> .../fbdev/omap2/displays-new/amplifier-opa362.c    | 343 +++++++++++++++++++++
>>> 
>>> I think it would be better to rename this to encoder-opa362.c. It's not
>> 
>> When developing this driver we did simply rename the encoder-tfp410 file,
>> but thent hough that it does not fit into the „encoder“ category, because we
>> would expect something digital or digital to analog „encoding“ which it does not.
> 
> That is true, but we already have other "encoders" like
> encoder-tpd12s015.c, which also do not encode. "encoder" in this context
> means something that takes a video input, and has a video output. In
> contrast to a panel or a connector.
> 
>>>> +
>>>> +	in->ops.atv->set_timings(in, &ddata->timings);
>>>> +	/* fixme: should we receive the invert from our consumer, i.e. the connector? */
>>>> +	in->ops.atv->invert_vid_out_polarity(in, true);
>>> 
>>> Well this does seem to be broken. I don't know what the answer to the
>>> question above is, but the code doesn't work properly.
>>> 
>>> In the opa362_invert_vid_out_polarity function below, you get the invert
>>> boolean from the connector. This you pass to the OMAP venc. However,
>>> above you always override that value in venc with true.
>>> 
>>> So, either the invert_vid_out_polarity value has to be always true or
>>> false, because _OPA362_ requires it to be true or false, OR you need use
>>> the value from the connector.
>>> 
>>> Seeing the comment in opa362_invert_vid_out_polarity, my guess is the
>>> latter, and the call above should be removed.
>> 
>> Yes, you are right - this is not systematic.
>> 
>> But the problem is that we can’t ask the connector here what it wants
>> to see. It might (or might not) call our opa362_invert_vid_out_polarity() later
>> which we can then forward to overwrite the inital state of this opa362_enable().
> 
> You don't need to ask. The connector calls invert_vid_out_polarity
> before enabling the output.

Unfortunately it doesn’t. At least not always.

It does only for pdata systems but not for DT based systems:

	if (!ddata->dev->of_node) {
		in->ops.atv->set_type(in, ddata->connector_type);
		in->ops.atv->invert_vid_out_polarity(in,
			ddata->invert_polarity);
	}

Not calling is in our case different from calling with ddata->invert_polarity == 0.

> You can just pass it forward inverted, as
> you already do in this driver. If it doesn't, it's either a bug or you
> can just rely on the value that is already programmed to venc.

Therefore it is not called with “false” which would make our invert_vid_out_polarity
invert it and send “true” towards the VENC. So VENC remains non-inverted.

We will also add a patch for the connector-analog.c

>>> We are going to support only DT boot at some point. Thus I think the
>>> whole platform data code should be left out.
>> 
>> Is there already a decision? I think it should not be done before. And it
>> does not harm to still have it.
> 
> It's just a matter of time. I don't accept any new boards using platform
> data for display, or new display drivers using platform data, because I
> don't want to spend my time converting them later. And as this is a new
> driver, no existing board can be using the opa362 platform_data. So we
> can support DT only.

Ok, that looks reasonable - as long as we can rely on that all mainline DSS
drivers are already fully converted to DT :)

BR,
Nikolaus

Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver

[Tomi Valkeinen]

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

On 19/11/14 17:10, Dr. H. Nikolaus Schaller wrote:

>> You don't need to ask. The connector calls invert_vid_out_polarity
>> before enabling the output.
> 
> Unfortunately it doesn’t. At least not always.
> 
> It does only for pdata systems but not for DT based systems:
> 
> 	if (!ddata->dev->of_node) {
> 		in->ops.atv->set_type(in, ddata->connector_type);
> 		in->ops.atv->invert_vid_out_polarity(in,
> 			ddata->invert_polarity);
> 	}
> 
> Not calling is in our case different from calling with ddata->invert_polarity == 0.

Ah, sorry, my mistake. I should've read the code more carefully =).

So, with DT only approach, those calls above are not supported. If you
make the OPA driver DT only, you can remove those functions.

You only need to set the invert-polarity property in the venc DT node.

 Tomi



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