[PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Grazvydas Ignotas]

VENC type (composite/svideo) doesn't have to be fixed by board wiring,
it is possible to provide both connectors, which is what pandora does.
Having to recompile the kernel for users who have TV connector types
that's don't match default board setting is very inconvenient, especially
for users of a consumer device, so add support for switching VENC type
at runtime over a new sysfs file venc_type.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 Documentation/arm/OMAP/DSS     |    1 +
 drivers/video/omap2/dss/venc.c |   55 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/Documentation/arm/OMAP/DSS b/Documentation/arm/OMAP/DSS
index 888ae7b..18e2214 100644
--- a/Documentation/arm/OMAP/DSS
+++ b/Documentation/arm/OMAP/DSS
@@ -156,6 +156,7 @@ timings		Display timings (pixclock,xres/hfp/hbp/hsw,yres/vfp/vbp/vsw)
 		"pal" and "ntsc"
 panel_name
 tear_elim	Tearing elimination 0=off, 1=on
+venc_type	Output type (video encoder only): "composite" or "svideo"
 
 There are also some debugfs files at <debugfs>/omapdss/ which show information
 about clocks and registers.
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 9c3daf7..aa2e74a 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -485,16 +485,69 @@ unsigned long venc_get_pixel_clock(void)
 	return 13500000;
 }
 
+static ssize_t display_venc_type_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct omap_dss_device *dssdev = to_dss_device(dev);
+	const char *ret;
+
+	switch (dssdev->phy.venc.type) {
+	case OMAP_DSS_VENC_TYPE_COMPOSITE:
+		ret = "composite";
+		break;
+	case OMAP_DSS_VENC_TYPE_SVIDEO:
+		ret = "svideo";
+		break;
+	default:
+		ret = "unknown";
+		break;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", ret);
+}
+
+static ssize_t display_venc_type_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct omap_dss_device *dssdev = to_dss_device(dev);
+	enum omap_dss_venc_type new_type;
+
+	if (strncmp("composite", buf, 9) = 0)
+		new_type = OMAP_DSS_VENC_TYPE_COMPOSITE;
+	else if (strncmp("svideo", buf, 6) = 0)
+		new_type = OMAP_DSS_VENC_TYPE_SVIDEO;
+	else
+		return -EINVAL;
+
+	mutex_lock(&venc.venc_lock);
+
+	if (dssdev->phy.venc.type != new_type) {
+		dssdev->phy.venc.type = new_type;
+		if (dssdev->state = OMAP_DSS_DISPLAY_ACTIVE) {
+			venc_power_off(dssdev);
+			venc_power_on(dssdev);
+		}
+	}
+
+	mutex_unlock(&venc.venc_lock);
+
+	return size;
+}
+
+static DEVICE_ATTR(venc_type, S_IRUGO | S_IWUSR,
+		display_venc_type_show, display_venc_type_store);
+
 /* driver */
 static int venc_panel_probe(struct omap_dss_device *dssdev)
 {
 	dssdev->panel.timings = omap_dss_pal_timings;
 
-	return 0;
+	return device_create_file(&dssdev->dev, &dev_attr_venc_type);
 }
 
 static void venc_panel_remove(struct omap_dss_device *dssdev)
 {
+	device_remove_file(&dssdev->dev, &dev_attr_venc_type);
 }
 
 static int venc_panel_enable(struct omap_dss_device *dssdev)
-- 
1.7.0.4

Re: [PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Tomi Valkeinen]

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

On Thu, 2012-03-29 at 02:45 +0300, Grazvydas Ignotas wrote:
> VENC type (composite/svideo) doesn't have to be fixed by board wiring,
> it is possible to provide both connectors, which is what pandora does.
> Having to recompile the kernel for users who have TV connector types
> that's don't match default board setting is very inconvenient, especially

You don't have to recompile the kernel, you could just set the venc type
in the board file depending on a boot parameter.

> for users of a consumer device, so add support for switching VENC type
> at runtime over a new sysfs file venc_type.

I really dislike adding new custom sysfs entries for omapdss, and I'd
like to avoid them if at all possible. Do you need to change the venc
type during runtime, or is it enough that it can be set during boot?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Grazvydas Ignotas]

On Fri, Apr 20, 2012 at 11:38 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-03-29 at 02:45 +0300, Grazvydas Ignotas wrote:
>> VENC type (composite/svideo) doesn't have to be fixed by board wiring,
>> it is possible to provide both connectors, which is what pandora does.
>> Having to recompile the kernel for users who have TV connector types
>> that's don't match default board setting is very inconvenient, especially
>
> You don't have to recompile the kernel, you could just set the venc type
> in the board file depending on a boot parameter.
>
>> for users of a consumer device, so add support for switching VENC type
>> at runtime over a new sysfs file venc_type.
>
> I really dislike adding new custom sysfs entries for omapdss, and I'd
> like to avoid them if at all possible.

Well some panels already have custom attributes, and venc could be
considered as special panel type, so if it's allowed for panels, why
not allow it for venc?

> Do you need to change the venc
> type during runtime, or is it enough that it can be set during boot?

We need this on runtime, otherwise it causes several issues:
- reboot is required to change the setting, although there is no
technical reason to really require it. This punishes users who want to
try both settings or have both TV types (with a portable device this
may sometimes happen).
- having to provide a way for users to change this in kernel boot
arguments. Note that many pandora users don't know how to handle boot
scripts, so a bootloader menu of some sort would be needed or ability
to edit u-boot environment from Linux, both of which would be
needlessly complicated solutions.


-- 
Gražvydas

Re: [PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Tomi Valkeinen]

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

On Fri, 2012-04-20 at 13:49 +0300, Grazvydas Ignotas wrote:
> On Fri, Apr 20, 2012 at 11:38 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Thu, 2012-03-29 at 02:45 +0300, Grazvydas Ignotas wrote:
> >> VENC type (composite/svideo) doesn't have to be fixed by board wiring,
> >> it is possible to provide both connectors, which is what pandora does.
> >> Having to recompile the kernel for users who have TV connector types
> >> that's don't match default board setting is very inconvenient, especially
> >
> > You don't have to recompile the kernel, you could just set the venc type
> > in the board file depending on a boot parameter.
> >
> >> for users of a consumer device, so add support for switching VENC type
> >> at runtime over a new sysfs file venc_type.
> >
> > I really dislike adding new custom sysfs entries for omapdss, and I'd
> > like to avoid them if at all possible.
> 
> Well some panels already have custom attributes, and venc could be
> considered as special panel type, so if it's allowed for panels, why
> not allow it for venc?

It's not really about "allowing". It's just that each new sysfs file is
a new non-standard custom API to userspace which we need to support
until the end of time. Adding new sysfs files carelessly will cause a
nightmare for me in the future, so by default I'm against new sysfs
files =).

> > Do you need to change the venc
> > type during runtime, or is it enough that it can be set during boot?
> 
> We need this on runtime, otherwise it causes several issues:
> - reboot is required to change the setting, although there is no
> technical reason to really require it. This punishes users who want to
> try both settings or have both TV types (with a portable device this
> may sometimes happen).
> - having to provide a way for users to change this in kernel boot
> arguments. Note that many pandora users don't know how to handle boot
> scripts, so a bootloader menu of some sort would be needed or ability
> to edit u-boot environment from Linux, both of which would be
> needlessly complicated solutions.

Ok. Sounds like we need to have dynamic configuration. I'll review the
patch.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Tomi Valkeinen]

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

On Thu, 2012-03-29 at 02:45 +0300, Grazvydas Ignotas wrote:
> VENC type (composite/svideo) doesn't have to be fixed by board wiring,
> it is possible to provide both connectors, which is what pandora does.
> Having to recompile the kernel for users who have TV connector types
> that's don't match default board setting is very inconvenient, especially
> for users of a consumer device, so add support for switching VENC type
> at runtime over a new sysfs file venc_type.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  Documentation/arm/OMAP/DSS     |    1 +
>  drivers/video/omap2/dss/venc.c |   55 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 55 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/arm/OMAP/DSS b/Documentation/arm/OMAP/DSS
> index 888ae7b..18e2214 100644
> --- a/Documentation/arm/OMAP/DSS
> +++ b/Documentation/arm/OMAP/DSS
> @@ -156,6 +156,7 @@ timings		Display timings (pixclock,xres/hfp/hbp/hsw,yres/vfp/vbp/vsw)
>  		"pal" and "ntsc"
>  panel_name
>  tear_elim	Tearing elimination 0=off, 1=on
> +venc_type	Output type (video encoder only): "composite" or "svideo"

I think we could have a better name here. "venc" name is quite obscure
on the user level. And it's not even quite correct, venc stays the same,
it's just the output that is changed. "output_type"? "connector"?
"connector_type"?

>  There are also some debugfs files at <debugfs>/omapdss/ which show information
>  about clocks and registers.
> diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
> index 9c3daf7..aa2e74a 100644
> --- a/drivers/video/omap2/dss/venc.c
> +++ b/drivers/video/omap2/dss/venc.c
> @@ -485,16 +485,69 @@ unsigned long venc_get_pixel_clock(void)
>  	return 13500000;
>  }
>  
> +static ssize_t display_venc_type_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct omap_dss_device *dssdev = to_dss_device(dev);
> +	const char *ret;
> +
> +	switch (dssdev->phy.venc.type) {
> +	case OMAP_DSS_VENC_TYPE_COMPOSITE:
> +		ret = "composite";
> +		break;
> +	case OMAP_DSS_VENC_TYPE_SVIDEO:
> +		ret = "svideo";
> +		break;
> +	default:
> +		ret = "unknown";
> +		break;
> +	}

Would it be better to return an error here? It would be nice to have
matching input and output for the sysfs file. And generally speaking,
the default branch should never happen.

> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", ret);
> +}
> +
> +static ssize_t display_venc_type_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct omap_dss_device *dssdev = to_dss_device(dev);
> +	enum omap_dss_venc_type new_type;
> +
> +	if (strncmp("composite", buf, 9) == 0)
> +		new_type = OMAP_DSS_VENC_TYPE_COMPOSITE;
> +	else if (strncmp("svideo", buf, 6) == 0)
> +		new_type = OMAP_DSS_VENC_TYPE_SVIDEO;

Here you could use sysfs_streq().

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: VENC: allow switching venc type at runtime

[Grazvydas Ignotas]

On Mon, Apr 23, 2012 at 3:23 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-03-29 at 02:45 +0300, Grazvydas Ignotas wrote:
>> VENC type (composite/svideo) doesn't have to be fixed by board wiring,
>> it is possible to provide both connectors, which is what pandora does.
>> Having to recompile the kernel for users who have TV connector types
>> that's don't match default board setting is very inconvenient, especially
>> for users of a consumer device, so add support for switching VENC type
>> at runtime over a new sysfs file venc_type.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> ---
>>  Documentation/arm/OMAP/DSS     |    1 +
>>  drivers/video/omap2/dss/venc.c |   55 +++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 55 insertions(+), 1 deletions(-)
>>
>> diff --git a/Documentation/arm/OMAP/DSS b/Documentation/arm/OMAP/DSS
>> index 888ae7b..18e2214 100644
>> --- a/Documentation/arm/OMAP/DSS
>> +++ b/Documentation/arm/OMAP/DSS
>> @@ -156,6 +156,7 @@ timings           Display timings (pixclock,xres/hfp/hbp/hsw,yres/vfp/vbp/vsw)
>>               "pal" and "ntsc"
>>  panel_name
>>  tear_elim    Tearing elimination 0=off, 1=on
>> +venc_type    Output type (video encoder only): "composite" or "svideo"
>
> I think we could have a better name here. "venc" name is quite obscure
> on the user level. And it's not even quite correct, venc stays the same,
> it's just the output that is changed. "output_type"? "connector"?
> "connector_type"?

I'll go for output_type then, connector* associates with fixed hardware to me.
I'll take care of other comments too.


-- 
Gražvydas