[PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Marek Belisko]

This patches is adding bypass and acbias functionality to omapdss venc driver.
In first patch we export updatin bypass and acbias in devconf1 register. Next patch
add handling for updating in venc driver and last patch add driver for opa362 which
is used on gta04 board and set bypass + acbias.

Marek Belisko (3):
  arm: omap2: Export devconf1 bypass and acbias.
  video: venc: Add new callback and handling for bypass and acbias
    setup.
  omapdss: Add OPA362 analog video amplifier driver.

 arch/arm/mach-omap2/Makefile                       |   2 +
 arch/arm/mach-omap2/control.h                      |   2 +
 arch/arm/mach-omap2/omap3-tvout.c                  |  36 +++
 drivers/video/omap2/displays-new/Kconfig           |   6 +
 drivers/video/omap2/displays-new/Makefile          |   1 +
 .../video/omap2/displays-new/amplifier-opa362.c    | 294 +++++++++++++++++++++
 drivers/video/omap2/dss/venc.c                     |  21 ++
 include/linux/omap-tvout.h                         |  14 +
 include/video/omap-panel-data.h                    |  17 ++
 include/video/omapdss.h                            |   2 +
 10 files changed, 395 insertions(+)
 create mode 100644 arch/arm/mach-omap2/omap3-tvout.c
 create mode 100644 drivers/video/omap2/displays-new/amplifier-opa362.c
 create mode 100644 include/linux/omap-tvout.h

-- 
1.8.1.2

[PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Marek Belisko]

devconf1 reg access is localized only in mach-omap2 and we need to export
updating of devconf1 from omapdss venc driver (bypass and acbias bits).
Add simple api call which update only necessary bits.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 arch/arm/mach-omap2/Makefile      |  2 ++
 arch/arm/mach-omap2/control.h     |  2 ++
 arch/arm/mach-omap2/omap3-tvout.c | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/omap-tvout.h        | 14 ++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 arch/arm/mach-omap2/omap3-tvout.c
 create mode 100644 include/linux/omap-tvout.h

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index afb457c..15e0f28 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -306,3 +306,5 @@ emac-$(CONFIG_TI_DAVINCI_EMAC)		:= am35xx-emac.o
 obj-y					+= $(emac-m) $(emac-y)
 
 obj-y					+= common-board-devices.o twl-common.o dss-common.o
+
+obj-$(CONFIG_ARCH_OMAP3)		+= omap3-tvout.o
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f7d7c2e..65277f1 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -279,6 +279,8 @@
 
 /* CONTROL_DEVCONF1 bits */
 #define OMAP243X_MMC1_ACTIVE_OVERWRITE	(1 << 31)
+#define OMAP2_TVOUTBYPASS		(1 << 18)
+#define OMAP2_TVACEN			(1 << 11)
 #define OMAP2_MMCSDIO2ADPCLKISEL	(1 << 6) /* MMC2 loop back clock */
 #define OMAP2_MCBSP5_CLKS_MASK		(1 << 4) /* > 242x */
 #define OMAP2_MCBSP4_CLKS_MASK		(1 << 2) /* > 242x */
diff --git a/arch/arm/mach-omap2/omap3-tvout.c b/arch/arm/mach-omap2/omap3-tvout.c
new file mode 100644
index 0000000..eaed225
--- /dev/null
+++ b/arch/arm/mach-omap2/omap3-tvout.c
@@ -0,0 +1,36 @@
+/*
+ * linux/arch/arm/mach-omap2/omap3-tvout.c
+ *
+ * Copyright (C) 2013 Golden Delicious Computers
+ * Author: Marek Belisko <marek@goldelico.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/kernel.h>
+#include <linux/omap-tvout.h>
+
+#include "soc.h"
+#include "control.h"
+
+void update_bypass_acbias(bool bypass, bool acbias)
+{
+#ifdef CONFIG_ARCH_OMAP3
+	int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
+
+	if (bypass)
+		val |= OMAP2_TVOUTBYPASS;
+	else
+		val &= ~OMAP2_TVOUTBYPASS;
+
+	if (acbias)
+		val |= OMAP2_TVACEN;
+	else
+		val &= ~OMAP2_TVACEN;
+
+	omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
+#endif
+}
+
diff --git a/include/linux/omap-tvout.h b/include/linux/omap-tvout.h
new file mode 100644
index 0000000..25f676d
--- /dev/null
+++ b/include/linux/omap-tvout.h
@@ -0,0 +1,14 @@
+/*
+ * OMAP TV-out support
+ *
+ * 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.
+ */
+#ifndef __LINUX_OMAP_TVOUT_H
+#define __LINUX_OMAP_TVOUT_H
+
+extern void update_bypass_acbias(bool bypass, bool acbias);
+
+#endif
+
-- 
1.8.1.2

[PATCH 2/3] video: venc: Add new callback and handling for bypass and acbias setup.

[Marek Belisko]

Add new callback to set bypass and acbias. During venc enble disable those
bit are updated in devconf1 register.

Signed-off-by: Marek Belisko <marek@goldelico.com>
---
 drivers/video/omap2/dss/venc.c | 21 +++++++++++++++++++++
 include/video/omapdss.h        |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 5f88ac4..17bd45b 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
+#include <linux/omap-tvout.h>
 
 #include <video/omapdss.h>
 
@@ -303,6 +304,8 @@ static struct {
 	struct omap_video_timings timings;
 	enum omap_dss_venc_type type;
 	bool invert_polarity;
+	bool bypass;
+	bool acbias;
 
 	struct omap_dss_device output;
 } venc;
@@ -455,6 +458,9 @@ static int venc_power_on(struct omap_dss_device *dssdev)
 
 	venc_write_reg(VENC_OUTPUT_CONTROL, l);
 
+	/* apply bypass and acbias */
+	update_bypass_acbias(venc.bypass, venc.acbias);
+
 	dss_mgr_set_timings(mgr, &venc.timings);
 
 	r = regulator_enable(venc.vdda_dac_reg);
@@ -485,6 +491,9 @@ static void venc_power_off(struct omap_dss_device *dssdev)
 	venc_write_reg(VENC_OUTPUT_CONTROL, 0);
 	dss_set_dac_pwrdn_bgz(0);
 
+	/* clear bypass and acbias */
+	update_bypass_acbias(false, false);
+
 	dss_mgr_disable(mgr);
 
 	regulator_disable(venc.vdda_dac_reg);
@@ -629,6 +638,17 @@ static void venc_invert_vid_out_polarity(struct omap_dss_device *dssdev,
 	mutex_unlock(&venc.venc_lock);
 }
 
+static void venc_bypass_and_acbias(struct omap_dss_device *dssdev,
+		bool bypass, bool acbias)
+{
+	mutex_lock(&venc.venc_lock);
+
+	venc.bypass = bypass;
+	venc.acbias = acbias;
+
+	mutex_unlock(&venc.venc_lock);
+}
+
 static int venc_init_regulator(void)
 {
 	struct regulator *vdda_dac;
@@ -777,6 +797,7 @@ static const struct omapdss_atv_ops venc_ops = {
 
 	.set_type = venc_set_type,
 	.invert_vid_out_polarity = venc_invert_vid_out_polarity,
+	.bypass_and_acbias = venc_bypass_and_acbias,
 
 	.set_wss = venc_set_wss,
 	.get_wss = venc_get_wss,
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 3d7c51a..54a185f 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -607,6 +607,8 @@ struct omapdss_atv_ops {
 		enum omap_dss_venc_type type);
 	void (*invert_vid_out_polarity)(struct omap_dss_device *dssdev,
 		bool invert_polarity);
+	void (*bypass_and_acbias)(struct omap_dss_device *dssdev,
+		bool bypass, bool acbias);
 
 	int (*set_wss)(struct omap_dss_device *dssdev, u32 wss);
 	u32 (*get_wss)(struct omap_dss_device *dssdev);
-- 
1.8.1.2

[PATCH 3/3] omapdss: Add OPA362 analog video amplifier driver.

[Marek Belisko]

This driver add handlign for opa362 chip which is
video amplifier with internal gain and filter. Enable/disable is
controlled via single gpio. This driver was tested on gta04 board.

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

diff --git a/drivers/video/omap2/displays-new/Kconfig b/drivers/video/omap2/displays-new/Kconfig
index 6c90885..5097640 100644
--- a/drivers/video/omap2/displays-new/Kconfig
+++ b/drivers/video/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 "Analog amplifier with output disable/high-Z"
+	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/omap2/displays-new/Makefile b/drivers/video/omap2/displays-new/Makefile
index 5aeb11b..2e8af8f 100644
--- a/drivers/video/omap2/displays-new/Makefile
+++ b/drivers/video/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/omap2/displays-new/amplifier-opa362.c b/drivers/video/omap2/displays-new/amplifier-opa362.c
new file mode 100644
index 0000000..677499c
--- /dev/null
+++ b/drivers/video/omap2/displays-new/amplifier-opa362.c
@@ -0,0 +1,294 @@
+/*
+ * OPA362 analog video amplifier with output/power control
+ *
+ * Copyright (C) 2013 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 <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;
+	bool bypass;
+	bool acbias;
+
+	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;
+
+	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;
+
+	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;
+
+	if (!omapdss_device_is_connected(dssdev))
+		return -ENODEV;
+
+	if (omapdss_device_is_enabled(dssdev))
+		return 0;
+
+	in->ops.atv->set_timings(in, &ddata->timings);
+	in->ops.atv->invert_vid_out_polarity(in, true);
+	in->ops.atv->bypass_and_acbias(in, ddata->bypass, ddata->acbias);
+
+	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;
+
+	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;
+
+	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);
+
+	*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;
+
+	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;
+	ddata->bypass = pdata->bypass;
+	ddata->acbias = pdata->acbias;
+
+	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(struct platform_device *pdev)
+{
+	struct panel_drv_data *ddata;
+	struct omap_dss_device *dssdev;
+	int r;
+
+	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 {
+		return -ENODEV;
+	}
+
+	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 struct platform_driver opa362_driver = {
+	.probe	= opa362_probe,
+	.remove	= __exit_p(opa362_remove),
+	.driver	= {
+		.name	= "amplifier-opa362",
+		.owner	= THIS_MODULE,
+	},
+};
+
+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 f7ac8d9..e1b3c13 100644
--- a/include/video/omap-panel-data.h
+++ b/include/video/omap-panel-data.h
@@ -33,6 +33,23 @@
 struct omap_dss_device;
 
 /**
+ * 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 pin (or -1 if not available - but
+ * then you don't need this driver)
+ * @bypass: enable TV bypass mode for external video driver
+ * @acbias: AC coupling to remove DC offset
+ */
+struct amplifier_opa362_platform_data {
+	const char *name;
+	const char *source;
+	int enable_gpio;
+	bool bypass;
+	bool acbias;
+};
+
+/**
  * encoder_tfp410 platform data
  * @name: name for this display entity
  * @power_down_gpio: gpio number for PD pin (or -1 if not available)
-- 
1.8.1.2

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Belisko Marek]

Hi,

ping.

On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
> This patches is adding bypass and acbias functionality to omapdss venc driver.
> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
> add handling for updating in venc driver and last patch add driver for opa362 which
> is used on gta04 board and set bypass + acbias.
Is there a chance to get this series to 3.13? Thanks.
>
> Marek Belisko (3):
>   arm: omap2: Export devconf1 bypass and acbias.
>   video: venc: Add new callback and handling for bypass and acbias
>     setup.
>   omapdss: Add OPA362 analog video amplifier driver.
>
>  arch/arm/mach-omap2/Makefile                       |   2 +
>  arch/arm/mach-omap2/control.h                      |   2 +
>  arch/arm/mach-omap2/omap3-tvout.c                  |  36 +++
>  drivers/video/omap2/displays-new/Kconfig           |   6 +
>  drivers/video/omap2/displays-new/Makefile          |   1 +
>  .../video/omap2/displays-new/amplifier-opa362.c    | 294 +++++++++++++++++++++
>  drivers/video/omap2/dss/venc.c                     |  21 ++
>  include/linux/omap-tvout.h                         |  14 +
>  include/video/omap-panel-data.h                    |  17 ++
>  include/video/omapdss.h                            |   2 +
>  10 files changed, 395 insertions(+)
>  create mode 100644 arch/arm/mach-omap2/omap3-tvout.c
>  create mode 100644 drivers/video/omap2/displays-new/amplifier-opa362.c
>  create mode 100644 include/linux/omap-tvout.h
>
> --
> 1.8.1.2
>

Marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Tomi Valkeinen]

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

Hi,

On 2013-11-05 09:24, Belisko Marek wrote:
> Hi,
> 
> ping.
> 
> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>> add handling for updating in venc driver and last patch add driver for opa362 which
>> is used on gta04 board and set bypass + acbias.
> Is there a chance to get this series to 3.13? Thanks.

Sorry, I haven't had time to do much reviewing.

The code in omap3-tvout.c should be included in the display.c file,
which already contains some things like muxing. Also, func(bool, bool)
style functions are rather confusing to read. Maybe an enum would be
better, so you'd instead have something like:

func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)

But the main issue is: while this series probably works well, I really
don't like it that the OPA driver needs to pass bypass and acbias. It
shouldn't know anything about such things. I'm just not certain how to
implement that with the current omapdss driver.

I'll try to find time to think about this more, but I don't think I can
merge this for 3.13.

 Tomi



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

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:

> Hi,
> 
> On 2013-11-05 09:24, Belisko Marek wrote:
>> Hi,
>> 
>> ping.
>> 
>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>> is used on gta04 board and set bypass + acbias.
>> Is there a chance to get this series to 3.13? Thanks.
> 
> Sorry, I haven't had time to do much reviewing.
> 
> The code in omap3-tvout.c should be included in the display.c file,
> which already contains some things like muxing.

Ok, that might be better - but we were not sure where to place code to
modify the DEVCONF1 registers. It is not directly DSS related but a special
control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c

> Also, func(bool, bool)
> style functions are rather confusing to read. Maybe an enum would be
> better, so you'd instead have something like:
> 
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)

We have no special preference on that.

> 
> But the main issue is: while this series probably works well, I really
> don't like it that the OPA driver needs to pass bypass and acbias. It
> shouldn't know anything about such things. I'm just not certain how to
> implement that with the current omapdss driver.

Well, it must know bypass and acbias because it requires the OMAP
CPU to be configured accordingly. I.e. it is the one who pushes the
button. Or we get a problem that people might misconfigure it.

I would see it similar as a driver must be able to control power. Here
it must be able to control bypass and acbias in a special way that it works.

> I'll try to find time to think about this more, but I don't think I can
> merge this for 3.13.

Please take your time.

And please also consider the approach to merge it into 3.13 as it is
and we can then fix it in the weeks while release candidates are pushed.

But that is your decision.

BR and thanks,
Nikolaus

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Tomi Valkeinen]

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

On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
> Hi Tomi,
> 
> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
> 
>> Hi,
>>
>> On 2013-11-05 09:24, Belisko Marek wrote:
>>> Hi,
>>>
>>> ping.
>>>
>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>> is used on gta04 board and set bypass + acbias.
>>> Is there a chance to get this series to 3.13? Thanks.
>>
>> Sorry, I haven't had time to do much reviewing.
>>
>> The code in omap3-tvout.c should be included in the display.c file,
>> which already contains some things like muxing.
> 
> Ok, that might be better - but we were not sure where to place code to
> modify the DEVCONF1 registers. It is not directly DSS related but a special
> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c

The display.c file is not strictly DSS stuff, but DSS related "glue" for
the SoC. For example, the muxing of the DSI pads is also done on the
CONTROL module, and it's also in display.c.

The file is getting a bit large, so I'm not against splitting it. But I
don't think there's point to add omap3-tvout.c file, which most likely
will ever contain only that one function.

>> Also, func(bool, bool)
>> style functions are rather confusing to read. Maybe an enum would be
>> better, so you'd instead have something like:
>>
>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
> 
> We have no special preference on that.

It's just about readability. Which one tells you better what the code does:

func(true, true);

or

func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);

>> But the main issue is: while this series probably works well, I really
>> don't like it that the OPA driver needs to pass bypass and acbias. It
>> shouldn't know anything about such things. I'm just not certain how to
>> implement that with the current omapdss driver.
> 
> Well, it must know bypass and acbias because it requires the OMAP
> CPU to be configured accordingly. I.e. it is the one who pushes the
> button. Or we get a problem that people might misconfigure it.

While the omapdss display drivers are currently OMAP specific, I want to
(or at least try to) keep them platform independent. Afaik, acbias and
bypass are OMAP VENC specific things, not something that every SoC with
an analog TV-out have. Thus, the OPA driver should not know about them,
and it should be something that only the DSS glue code in mach-omap2/
and the venc driver know about.

> I would see it similar as a driver must be able to control power. Here
> it must be able to control bypass and acbias in a special way that it works.

The difference is that on all possible SoCs where you could add OPA
chip, you'll need to manage the power for OPA, and it's done in a common
way. Whereas the bypass and acbias are OMAP specific things.

>> I'll try to find time to think about this more, but I don't think I can
>> merge this for 3.13.
> 
> Please take your time.
> 
> And please also consider the approach to merge it into 3.13 as it is
> and we can then fix it in the weeks while release candidates are pushed.

If it was purely omapdss code, I could possibly take it. But it contains
arch/arm code also, and I don't want to cause needless changes on code
that I do not maintain.

 Tomi



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

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Dr. H. Nikolaus Schaller]

Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:

> On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
>> Hi Tomi,
>> 
>> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>> 
>>> Hi,
>>> 
>>> On 2013-11-05 09:24, Belisko Marek wrote:
>>>> Hi,
>>>> 
>>>> ping.
>>>> 
>>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>>> is used on gta04 board and set bypass + acbias.
>>>> Is there a chance to get this series to 3.13? Thanks.
>>> 
>>> Sorry, I haven't had time to do much reviewing.
>>> 
>>> The code in omap3-tvout.c should be included in the display.c file,
>>> which already contains some things like muxing.
>> 
>> Ok, that might be better - but we were not sure where to place code to
>> modify the DEVCONF1 registers. It is not directly DSS related but a special
>> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
> 
> The display.c file is not strictly DSS stuff, but DSS related "glue" for
> the SoC. For example, the muxing of the DSI pads is also done on the
> CONTROL module, and it's also in display.c.
> 
> The file is getting a bit large, so I'm not against splitting it. But I
> don't think there's point to add omap3-tvout.c file, which most likely
> will ever contain only that one function.

Yes that is very likely true.

The problem is that there is no other official API to modify the DEFCONF1
register. Therefore we introduced this (propsal).

Our first idea was a readDEFCONF1() and writeDEFCONF1() and use the
constants (bit patterns) you suggested below.

But we thought that it is not robust enough because a VENC driver or other
one could then change bits it is not responsible for.

> 
>>> Also, func(bool, bool)
>>> style functions are rather confusing to read. Maybe an enum would be
>>> better, so you'd instead have something like:
>>> 
>>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>> 
>> We have no special preference on that.
> 
> It's just about readability. Which one tells you better what the code does:
> 
> func(true, true);
> 
> or
> 
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);

Hm. Depends on. If the func is explaining enough it is clear. Or if I have
access to some header file. If I don't, the enum values are probably more
readable.

> 
>>> But the main issue is: while this series probably works well, I really
>>> don't like it that the OPA driver needs to pass bypass and acbias. It
>>> shouldn't know anything about such things. I'm just not certain how to
>>> implement that with the current omapdss driver.
>> 
>> Well, it must know bypass and acbias because it requires the OMAP
>> CPU to be configured accordingly. I.e. it is the one who pushes the
>> button. Or we get a problem that people might misconfigure it.
> 
> While the omapdss display drivers are currently OMAP specific, I want to
> (or at least try to) keep them platform independent. Afaik, acbias and
> bypass are OMAP VENC specific things, not something that every SoC with
> an analog TV-out have. Thus, the OPA driver should not know about them,
> and it should be something that only the DSS glue code in mach-omap2/
> and the venc driver know about.

Well, there must be a method how the OPA can tell the VENC that it exists
and the VENC can tell the SoC DEFCONF1 to enable bias and bypass.

> 
>> I would see it similar as a driver must be able to control power. Here
>> it must be able to control bypass and acbias in a special way that it works.
> 
> The difference is that on all possible SoCs where you could add OPA
> chip, you'll need to manage the power for OPA, and it's done in a common
> way. Whereas the bypass and acbias are OMAP specific things.

Maybe it looks as if it is an unsolvable problem. The OPA works only if acbias
and bypass are enabled, but is not allowed to tell that it is there.

> 
>>> I'll try to find time to think about this more, but I don't think I can
>>> merge this for 3.13.
>> 
>> Please take your time.
>> 
>> And please also consider the approach to merge it into 3.13 as it is
>> and we can then fix it in the weeks while release candidates are pushed.
> 
> If it was purely omapdss code, I could possibly take it. But it contains
> arch/arm code also,

because we think that it is unavoidable (no API to control the DEFCONF1
register exists yet).

Maybe one of the omap3 core maintainers can comment as well?

> and I don't want to cause needless changes on code
> that I do not maintain.

Yes, this should be avoided of course.

BR,
Nikolaus

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Dr. H. Nikolaus Schaller]

Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:

> On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
>> Hi Tomi,
>> 
>> Am 11.11.2013 um 14:29 schrieb Tomi Valkeinen:
>> 
>>> Hi,
>>> 
>>> On 2013-11-05 09:24, Belisko Marek wrote:
>>>> Hi,
>>>> 
>>>> ping.
>>>> 
>>>> On Mon, Oct 14, 2013 at 11:02 PM, Marek Belisko <marek@goldelico.com> wrote:
>>>>> This patches is adding bypass and acbias functionality to omapdss venc driver.
>>>>> In first patch we export updatin bypass and acbias in devconf1 register. Next patch
>>>>> add handling for updating in venc driver and last patch add driver for opa362 which
>>>>> is used on gta04 board and set bypass + acbias.
>>>> Is there a chance to get this series to 3.13? Thanks.
>>> 
>>> Sorry, I haven't had time to do much reviewing.
>>> 
>>> The code in omap3-tvout.c should be included in the display.c file,
>>> which already contains some things like muxing.
>> 
>> Ok, that might be better - but we were not sure where to place code to
>> modify the DEVCONF1 registers. It is not directly DSS related but a special
>> control of the OMAP3 SoC. Therefore we think it is better located in omap3-tvout.c
> 
> The display.c file is not strictly DSS stuff, but DSS related "glue" for
> the SoC. For example, the muxing of the DSI pads is also done on the
> CONTROL module, and it's also in display.c.
> 
> The file is getting a bit large, so I'm not against splitting it. But I
> don't think there's point to add omap3-tvout.c file, which most likely
> will ever contain only that one function.
> 
>>> Also, func(bool, bool)
>>> style functions are rather confusing to read. Maybe an enum would be
>>> better, so you'd instead have something like:
>>> 
>>> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN)
>> 
>> We have no special preference on that.
> 
> It's just about readability. Which one tells you better what the code does:
> 
> func(true, true);
> 
> or
> 
> func(OMAP_VENC_TVOUTBYPASS | OMAP_VENC_TVACEN);
> 
>>> But the main issue is: while this series probably works well, I really
>>> don't like it that the OPA driver needs to pass bypass and acbias. It
>>> shouldn't know anything about such things. I'm just not certain how to
>>> implement that with the current omapdss driver.
>> 
>> Well, it must know bypass and acbias because it requires the OMAP
>> CPU to be configured accordingly. I.e. it is the one who pushes the
>> button. Or we get a problem that people might misconfigure it.
> 
> While the omapdss display drivers are currently OMAP specific, I want to
> (or at least try to) keep them platform independent. Afaik, acbias and
> bypass are OMAP VENC specific things, not something that every SoC with
> an analog TV-out have. Thus, the OPA driver should not know about them,
> and it should be something that only the DSS glue code in mach-omap2/
> and the venc driver know about.

What about calling it prepare_venc_for_external_amplifier (or similar). Then,
venc.c can hide that really has to be done and call whatever SoC API is
needed to program DEFCONF1?

> 
>> I would see it similar as a driver must be able to control power. Here
>> it must be able to control bypass and acbias in a special way that it works.
> 
> The difference is that on all possible SoCs where you could add OPA
> chip, you'll need to manage the power for OPA, and it's done in a common
> way. Whereas the bypass and acbias are OMAP specific things.
> 
>>> I'll try to find time to think about this more, but I don't think I can
>>> merge this for 3.13.
>> 
>> Please take your time.
>> 
>> And please also consider the approach to merge it into 3.13 as it is
>> and we can then fix it in the weeks while release candidates are pushed.
> 
> If it was purely omapdss code, I could possibly take it. But it contains
> arch/arm code also, and I don't want to cause needless changes on code
> that I do not maintain.
> 
> Tomi
> 
> 

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Tony Lindgren]

* Marek Belisko <marek@goldelico.com> [131014 14:11]:
> devconf1 reg access is localized only in mach-omap2 and we need to export
> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
> Add simple api call which update only necessary bits.
...

> +void update_bypass_acbias(bool bypass, bool acbias)
> +{
> +#ifdef CONFIG_ARCH_OMAP3
> +	int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> +
> +	if (bypass)
> +		val |= OMAP2_TVOUTBYPASS;
> +	else
> +		val &= ~OMAP2_TVOUTBYPASS;
> +
> +	if (acbias)
> +		val |= OMAP2_TVACEN;
> +	else
> +		val &= ~OMAP2_TVACEN;
> +
> +	omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
> +#endif

If this is truly a pinmux, you could already access this
using pinctrl-single,bits device tree driver.

But I guess that won't work yet, so it's best to set this
up as a separate driver like we've done for the USB PHY
registers.

Regards,

Tony

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Tony Lindgren]

* Dr. H. Nikolaus Schaller <hns@goldelico.com> [131111 06:30]:
> Am 11.11.2013 um 15:13 schrieb Tomi Valkeinen:
> > On 2013-11-11 15:57, Dr. H. Nikolaus Schaller wrote:
> >
> > The display.c file is not strictly DSS stuff, but DSS related "glue" for
> > the SoC. For example, the muxing of the DSI pads is also done on the
> > CONTROL module, and it's also in display.c.
> > 
> > The file is getting a bit large, so I'm not against splitting it. But I
> > don't think there's point to add omap3-tvout.c file, which most likely
> > will ever contain only that one function.
> 
> Yes that is very likely true.
> 
> The problem is that there is no other official API to modify the DEFCONF1
> register. Therefore we introduced this (propsal).
> 
> Our first idea was a readDEFCONF1() and writeDEFCONF1() and use the
> constants (bit patterns) you suggested below.

I posted something about accessing the ctrl module regs to the first
patch in the series. It's best to set it up as a separate driver for
now and then maybe handle it with pinctrl-single,bits when DSS is
device tree enabled.

Regards,

Tony

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Belisko Marek]

Hi Tony,

On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Marek Belisko <marek@goldelico.com> [131014 14:11]:
>> devconf1 reg access is localized only in mach-omap2 and we need to export
>> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
>> Add simple api call which update only necessary bits.
> ...
>
>> +void update_bypass_acbias(bool bypass, bool acbias)
>> +{
>> +#ifdef CONFIG_ARCH_OMAP3
>> +     int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> +
>> +     if (bypass)
>> +             val |= OMAP2_TVOUTBYPASS;
>> +     else
>> +             val &= ~OMAP2_TVOUTBYPASS;
>> +
>> +     if (acbias)
>> +             val |= OMAP2_TVACEN;
>> +     else
>> +             val &= ~OMAP2_TVACEN;
>> +
>> +     omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
>> +#endif
>
> If this is truly a pinmux, you could already access this
> using pinctrl-single,bits device tree driver.
>
> But I guess that won't work yet, so it's best to set this
> up as a separate driver like we've done for the USB PHY
> registers.
Can you please point me to that driver directly? I cannot see benefit
of new driver as as it will be only dummy driver
with export_symbol_gpl of upper function. Can it sit then in dss
directory or somewhere else? Thanks.
>
> Regards,
>
> Tony

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Belisko Marek]

Hi Tony,

On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Marek Belisko <marek@goldelico.com> [131014 14:11]:
>> devconf1 reg access is localized only in mach-omap2 and we need to export
>> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
>> Add simple api call which update only necessary bits.
> ...
>
>> +void update_bypass_acbias(bool bypass, bool acbias)
>> +{
>> +#ifdef CONFIG_ARCH_OMAP3
>> +     int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> +
>> +     if (bypass)
>> +             val |= OMAP2_TVOUTBYPASS;
>> +     else
>> +             val &= ~OMAP2_TVOUTBYPASS;
>> +
>> +     if (acbias)
>> +             val |= OMAP2_TVACEN;
>> +     else
>> +             val &= ~OMAP2_TVACEN;
>> +
>> +     omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
>> +#endif
>
> If this is truly a pinmux, you could already access this
> using pinctrl-single,bits device tree driver.
AFAIK it's not pinmux it's register where we update bits. Or am I
missing something?
>
> But I guess that won't work yet, so it's best to set this
> up as a separate driver like we've done for the USB PHY
> registers.
>
> Regards,
>
> Tony

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Tony Lindgren]

* Belisko Marek <marek.belisko@gmail.com> [131111 14:01]:
> Hi Tony,
> 
> On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Marek Belisko <marek@goldelico.com> [131014 14:11]:
> >> devconf1 reg access is localized only in mach-omap2 and we need to export
> >> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
> >> Add simple api call which update only necessary bits.
> > ...
> >
> >> +void update_bypass_acbias(bool bypass, bool acbias)
> >> +{
> >> +#ifdef CONFIG_ARCH_OMAP3
> >> +     int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
> >> +
> >> +     if (bypass)
> >> +             val |= OMAP2_TVOUTBYPASS;
> >> +     else
> >> +             val &= ~OMAP2_TVOUTBYPASS;
> >> +
> >> +     if (acbias)
> >> +             val |= OMAP2_TVACEN;
> >> +     else
> >> +             val &= ~OMAP2_TVACEN;
> >> +
> >> +     omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
> >> +#endif
> >
> > If this is truly a pinmux, you could already access this
> > using pinctrl-single,bits device tree driver.
> >
> > But I guess that won't work yet, so it's best to set this
> > up as a separate driver like we've done for the USB PHY
> > registers.
>
> Can you please point me to that driver directly? I cannot see benefit
> of new driver as as it will be only dummy driver
> with export_symbol_gpl of upper function. Can it sit then in dss
> directory or somewhere else? Thanks.

You can take a look at drivers/usb/phy/phy-omap-control.c.

Then there's a device tree using example for control_devconf0 in
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt.

Looks like CONTROL_DEVCONF1 contains a bunch of misc registers,
and arch/arm/mach-omap2/hsmmc.c seems to be tinkering with it too.

It would be best to set it up as omap-ctrl.c driver under drivers
somewhere with few functions exported for DSS and MMC drivers.

The reason why it should be a separate driver is that the system
control module can live a separate runtime PM life from the
drivers using the CONTROL_DEVCONF register bits.

Regards,

Tony

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Tony Lindgren]

* Belisko Marek <marek.belisko@gmail.com> [131111 14:21]:
> AFAIK it's not pinmux it's register where we update bits. Or am I
> missing something?

Most of the omap control module registers seem to affect either
external pin configuration or internal signal muxes. So a big
chunk of them can already be handled with pinctrl-single,bits
for device tree.

But some of the control module registers also have regulator
and clock like features, and those are better exposed as
regulators and clocks to the consumer drivers.

Regards,

Tony

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Tomi Valkeinen]

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

On 2013-11-11 16:30, Dr. H. Nikolaus Schaller wrote:

> Maybe it looks as if it is an unsolvable problem. The OPA works only if acbias
> and bypass are enabled, but is not allowed to tell that it is there.

That's why the board file or dts file is there, to have "glue" data to
make different pieces work together.

The perfect (?) solution would be a CDF like data. The idea would be
that in the board file, you would describe configuration options for
VENC and for OPA. In this particular case, you'd tell VENC that it needs
to enable acbias and bypass when OPA is to be connected to VENC. Then
there could be more entries for VENC, saying disable acbias and bypass
when BAR is to be connected to VENC.

However, we don't have such support (yet).

For now, I hope it's enough if we handle VENC in a "single
configuration" manner, i.e. you'll tell VENC to enable acbias and bypass
when VENC is enabled. This means you can't have board setups where you'd
change the VENC->OPA connection to some other analog tv output at
runtime. I hope that's the case (i.e. OPA is always in use on that board).

If so, then I think you can just pass the acbias and bypass
configuration values via omapdss platform data. And VENC driver will use
them to configure those. OPA doesn't know anything about this at all.

 Tomi



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

Re: [PATCH 0/3] omapdss: venc: Add support for bypass and acbias.

[Dr. H. Nikolaus Schaller]

Hi Tomi,

Am 18.11.2013 um 14:29 schrieb Tomi Valkeinen:

> On 2013-11-11 16:30, Dr. H. Nikolaus Schaller wrote:
> 
>> Maybe it looks as if it is an unsolvable problem. The OPA works only if acbias
>> and bypass are enabled, but is not allowed to tell that it is there.
> 
> That's why the board file or dts file is there, to have "glue" data to
> make different pieces work together.

Our implementation idea was that this makes the board file developer to specify knowlegde that the
drivers already could have and to reduce the risk of misconfiguration.

So we tried to model the invert property of the connector which is also backpropagated
to VENC.

But of course there are different ways of doing it and different priorities of contradicting
requirements.

> 
> The perfect (?) solution would be a CDF like data. The idea would be
> that in the board file, you would describe configuration options for
> VENC and for OPA. In this particular case, you'd tell VENC that it needs
> to enable acbias and bypass when OPA is to be connected to VENC. Then
> there could be more entries for VENC, saying disable acbias and bypass
> when BAR is to be connected to VENC.
> 
> However, we don't have such support (yet).
> 
> For now, I hope it's enough if we handle VENC in a "single
> configuration" manner, i.e. you'll tell VENC to enable acbias and bypass
> when VENC is enabled. This means you can't have board setups where you'd
> change the VENC->OPA connection to some other analog tv output at
> runtime. I hope that's the case (i.e. OPA is always in use on that board).

I also don't think that there is a need for runtime modifications, so your proposal
looks ok.

> If so, then I think you can just pass the acbias and bypass
> configuration values via omapdss platform data. And VENC driver will use
> them to configure those. OPA doesn't know anything about this at all.

We will change it that way.

Do you see a chance to get it into the merge window of 3.13?

BR,
Nikolaus

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Belisko Marek]

Hi Tony,

On Tue, Nov 12, 2013 at 12:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Belisko Marek <marek.belisko@gmail.com> [131111 14:01]:
>> Hi Tony,
>>
>> On Mon, Nov 11, 2013 at 5:49 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Marek Belisko <marek@goldelico.com> [131014 14:11]:
>> >> devconf1 reg access is localized only in mach-omap2 and we need to export
>> >> updating of devconf1 from omapdss venc driver (bypass and acbias bits).
>> >> Add simple api call which update only necessary bits.
>> > ...
>> >
>> >> +void update_bypass_acbias(bool bypass, bool acbias)
>> >> +{
>> >> +#ifdef CONFIG_ARCH_OMAP3
>> >> +     int val = omap_ctrl_readl(OMAP343X_CONTROL_DEVCONF1);
>> >> +
>> >> +     if (bypass)
>> >> +             val |= OMAP2_TVOUTBYPASS;
>> >> +     else
>> >> +             val &= ~OMAP2_TVOUTBYPASS;
>> >> +
>> >> +     if (acbias)
>> >> +             val |= OMAP2_TVACEN;
>> >> +     else
>> >> +             val &= ~OMAP2_TVACEN;
>> >> +
>> >> +     omap_ctrl_writel(val, OMAP343X_CONTROL_DEVCONF1);
>> >> +#endif
>> >
>> > If this is truly a pinmux, you could already access this
>> > using pinctrl-single,bits device tree driver.
>> >
>> > But I guess that won't work yet, so it's best to set this
>> > up as a separate driver like we've done for the USB PHY
>> > registers.
>>
>> Can you please point me to that driver directly? I cannot see benefit
>> of new driver as as it will be only dummy driver
>> with export_symbol_gpl of upper function. Can it sit then in dss
>> directory or somewhere else? Thanks.
>
> You can take a look at drivers/usb/phy/phy-omap-control.c.
>
> Then there's a device tree using example for control_devconf0 in
> Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt.
>
> Looks like CONTROL_DEVCONF1 contains a bunch of misc registers,
> and arch/arm/mach-omap2/hsmmc.c seems to be tinkering with it too.
>
> It would be best to set it up as omap-ctrl.c driver under drivers
> somewhere with few functions exported for DSS and MMC drivers.
I create small dummy driver based on phy-omap-control which can be
used but before sending patch (with more updates) I would like to get
some feedback if my direction is correct.
diff --git a/drivers/misc/omap-ctrl.c b/drivers/misc/omap-ctrl.c
new file mode 100644
index 0000000..ca2e576
--- /dev/null
+++ b/drivers/misc/omap-ctrl.c
@@ -0,0 +1,84 @@
+/*
+ * omap-ctrl.c - The part of omap control module.
+ *
+ * Copyright (C) 2013 Goldelico GmbH
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Author: Marek Belisko <marek@goldelico.com>
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+static u32 __iomem *devconf1;
+
+void omap_ctrl_tvoutbypass(int on)
+{
+    int val = readl(devconf1);
+
+    if (on)
+       val |= BIT(18);
+    else
+       val &= ~BIT(18);
+
+    writel(val, devconf1);
+}
+EXPORT_SYMBOL_GPL(omap_ctrl_tvoutbypass);
+
+
+void omap_ctrl_acen(int on)
+{
+    int val = readl(devconf1);
+
+    if (on)
+       val |= BIT(11);
+    else
+       val &= ~BIT(11);
+
+    writel(val, devconf1);
+}
+EXPORT_SYMBOL_GPL(omap_ctrl_acen);
+
+
+static int omap_ctrl_probe(struct platform_device *pdev)
+{
+    struct resource *res;
+
+    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+       "devconf1");
+    devconf1 = devm_ioremap_resource(
+       &pdev->dev, res);
+    if (IS_ERR(devconf1))
+       return PTR_ERR(devconf1);
+
+    return 0;
+}
+
+struct platform_driver omap_ctrl_driver = {
+    .probe = omap_ctrl_probe,
+    .driver = {
+    .name = "omap-ctrl",
+    .owner = THIS_MODULE,
+    },
+};
+
+static int __init omap_ctrl_init(void)
+{
+    return platform_driver_register(&omap_ctrl_driver);
+}
+subsys_initcall(omap_ctrl_init);
+
+static void __exit omap_ctrl_exit(void)
+{
+    platform_driver_unregister(&omap_ctrl_driver);
+}

I use that driver with platform device in board file with proper
resources and it works fine. It could be easily added also DT
bindings.
Also you mention somewhere in drivers (but I think misc isn't right
place for that). Any opinion on that? Thanks for feedback.

>
> The reason why it should be a separate driver is that the system
> control module can live a separate runtime PM life from the
> drivers using the CONTROL_DEVCONF register bits.
>
> Regards,
>
> Tony

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Tony Lindgren]

* Belisko Marek <marek.belisko@gmail.com> [131210 14:13]:
> On Tue, Nov 12, 2013 at 12:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> >
> > It would be best to set it up as omap-ctrl.c driver under drivers
> > somewhere with few functions exported for DSS and MMC drivers.
>
> I create small dummy driver based on phy-omap-control which can be
> used but before sending patch (with more updates) I would like to get
> some feedback if my direction is correct.

Cool thanks. Yeah what you have can easily be combined with the patches
Balaji has posted to make HSMMC use drivers/mfd/syscon.c via regmap
for the SCM register access. Maybe take a look at the work in progress
patches in thread:

[PATCH v4 0/7] mmc: omap_hsmmc: pbias dt and cleanup

And also see my comments regarding using the SCM GENERAL register area
as base for the syscon.c driver. That should work for your driver too,
right? And then you can access the SYSCON1 register that way from your
consumer driver ;)

Regards,

Tony

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Belisko Marek]

Hi Tony,

On Tue, Dec 10, 2013 at 11:46 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Belisko Marek <marek.belisko@gmail.com> [131210 14:13]:
>> On Tue, Nov 12, 2013 at 12:31 AM, Tony Lindgren <tony@atomide.com> wrote:
>> >
>> > It would be best to set it up as omap-ctrl.c driver under drivers
>> > somewhere with few functions exported for DSS and MMC drivers.
>>
>> I create small dummy driver based on phy-omap-control which can be
>> used but before sending patch (with more updates) I would like to get
>> some feedback if my direction is correct.
>
> Cool thanks. Yeah what you have can easily be combined with the patches
> Balaji has posted to make HSMMC use drivers/mfd/syscon.c via regmap
> for the SCM register access. Maybe take a look at the work in progress
> patches in thread:
>
> [PATCH v4 0/7] mmc: omap_hsmmc: pbias dt and cleanup
>
> And also see my comments regarding using the SCM GENERAL register area
> as base for the syscon.c driver. That should work for your driver too,
> right? And then you can access the SYSCON1 register that way from your
> consumer driver ;)
If I understand correclty I can use syscon driver (it will have in
range also devconf1 register) ad get rid of my custom driver
and then get regmap from syscon and update bits that I need for venc, right?
>
> Regards,
>
> Tony

BR,

marek

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com

Re: [PATCH 1/3] arm: omap2: Export devconf1 bypass and acbias.

[Tony Lindgren]

On Thu, Dec 12, 2013 at 09:31:02AM +0100, Belisko Marek wrote:
> Hi Tony,
> 
> On Tue, Dec 10, 2013 at 11:46 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Belisko Marek <marek.belisko@gmail.com> [131210 14:13]:
> >> On Tue, Nov 12, 2013 at 12:31 AM, Tony Lindgren <tony@atomide.com> wrote:
> >> >
> >> > It would be best to set it up as omap-ctrl.c driver under drivers
> >> > somewhere with few functions exported for DSS and MMC drivers.
> >>
> >> I create small dummy driver based on phy-omap-control which can be
> >> used but before sending patch (with more updates) I would like to get
> >> some feedback if my direction is correct.
> >
> > Cool thanks. Yeah what you have can easily be combined with the patches
> > Balaji has posted to make HSMMC use drivers/mfd/syscon.c via regmap
> > for the SCM register access. Maybe take a look at the work in progress
> > patches in thread:
> >
> > [PATCH v4 0/7] mmc: omap_hsmmc: pbias dt and cleanup
> >
> > And also see my comments regarding using the SCM GENERAL register area
> > as base for the syscon.c driver. That should work for your driver too,
> > right? And then you can access the SYSCON1 register that way from your
> > consumer driver ;)
>
> If I understand correclty I can use syscon driver (it will have in
> range also devconf1 register) ad get rid of my custom driver
> and then get regmap from syscon and update bits that I need for venc, right?

Yeah something like that. Or since the sysconf1 register is shared, it might
be best to still provide specific functions to access it if we cannot map
just specific bits of it separately for drivers using regmap.

But anyways this should for most part remove the need for a custom driver(s)
unless the register is a regulator or a clock.

Regards,

Tony