Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
Am 13.11.2014 um 17:41 schrieb Tomi Valkeinen tomi.valkei...@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.valkei...@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 h...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
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 h...@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 000..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 h...@goldelico.com
+ *
+ * based on encoder-tfp410
+ *
+ * Copyright (C) 2013 Texas Instruments
+ * Author: Tomi Valkeinen tomi.valkei...@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))
+
Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
Hi,
Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen tomi.valkei...@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 h...@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 000..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 h...@goldelico.com
+ *
+ * based on encoder-tfp410
+ *
+ * Copyright (C) 2013 Texas Instruments
+ * Author: Tomi Valkeinen tomi.valkei...@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))
+
Re: [PATCH v2 1/5] video: omapdss: Add opa362 driver
On 13/11/14 18:25, Dr. H. Nikolaus Schaller wrote: Hi, Am 13.11.2014 um 12:51 schrieb Tomi Valkeinen tomi.valkei...@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 h...@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 signature.asc Description: OpenPGP digital signature
[PATCH v2 1/5] video: omapdss: Add opa362 driver
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 h...@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 000..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 h...@goldelico.com
+ *
+ * based on encoder-tfp410
+ *
+ * Copyright (C) 2013 Texas Instruments
+ * Author: Tomi Valkeinen tomi.valkei...@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
