Re: [PATCH v2 3/3] OMAPDSS: HDMI: Sysfs support to configure quantization

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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

On Wed, 2012-01-04 at 17:23 +0530, mythripk@ti.com wrote:
> From: Mythri P K <mythripk@ti.com>
> 
> Add sysfs support for the uset space to configure limited range or full range
> quantization for HDMI.
> 
> Signed-off-by: Mythri P K <mythripk@ti.com>
> ---
>  drivers/video/omap2/dss/dss.h          |    2 +
>  drivers/video/omap2/dss/dss_features.c |    1 +
>  drivers/video/omap2/dss/hdmi.c         |   28 +++++++++++++++++++++++++
>  drivers/video/omap2/dss/hdmi_panel.c   |   35 +++++++++++++++++++++++++++++++-
>  drivers/video/omap2/dss/ti_hdmi.h      |    2 +
>  5 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 6308fc5..cf1f0f9 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -498,6 +498,8 @@ int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev,
>  					struct omap_video_timings *timings);
>  int omapdss_hdmi_read_edid(u8 *buf, int len);
>  bool omapdss_hdmi_detect(void);
> +int omapdss_hdmi_get_range(void);
> +int omapdss_hdmi_set_range(int range);
>  int hdmi_panel_init(void);
>  void hdmi_panel_exit(void);
>  
> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
> index b402699..c7e71b9 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -465,6 +465,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
>  	.dump_core		=	ti_hdmi_4xxx_core_dump,
>  	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
>  	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
> +	.configure_range	=	ti_hdmi_4xxx_configure_range,
>  
>  };
>  
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 4bb7678..ae7918e 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -378,6 +378,34 @@ static void hdmi_power_off(struct omap_dss_device *dssdev)
>  	hdmi_runtime_put();
>  }
>  
> +int omapdss_hdmi_set_range(int range)

Range is an enum, not an int.

> +{
> +	int r = 0;
> +	enum hdmi_range old_range;
> +
> +	old_range = hdmi.ip_data.range;
> +	hdmi.ip_data.range = range;
> +
> +	/* HDMI 1.3 section 6.6 VGA (640x480) format requires Full Range */
> +	if ((range == 0) &&

Range is an enum, not an int.

> +		((hdmi.ip_data.cfg.cm.code == 4 &&
> +		hdmi.ip_data.cfg.cm.mode == HDMI_DVI) ||
> +		(hdmi.ip_data.cfg.cm.code == 1 &&
> +		hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)))
> +			return -EINVAL;
> +
> +	r = hdmi.ip_data.ops->configure_range(&hdmi.ip_data);
> +	if (r)
> +		hdmi.ip_data.range = old_range;
> +
> +	return r;
> +}
> +
> +int omapdss_hdmi_get_range(void)

Range is an enum, not an int... I won't comment on any more of these
cases, please check all uses of range.

> +{
> +	return hdmi.ip_data.range;
> +}
> +
>  int omapdss_hdmi_display_check_timing(struct omap_dss_device *dssdev,
>  					struct omap_video_timings *timings)
>  {
> diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
> index 533d5dc..c0aa922 100644
> --- a/drivers/video/omap2/dss/hdmi_panel.c
> +++ b/drivers/video/omap2/dss/hdmi_panel.c
> @@ -33,6 +33,33 @@ static struct {
>  	struct mutex hdmi_lock;
>  } hdmi;
>  
> +static ssize_t hdmi_range_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	int r;
> +
> +	r = omapdss_hdmi_get_range();
> +	return snprintf(buf, PAGE_SIZE, "%d\n", r);
> +}
> +
> +static ssize_t hdmi_range_store(struct device *dev,
> +	struct device_attribute *attr,
> +	const char *buf, size_t size)
> +{
> +	unsigned long range;
> +	int r = kstrtoul(buf, 0, &range);
> +
> +	if (r || range > 1)
> +		return -EINVAL;
> +
> +	r = omapdss_hdmi_set_range(range);
> +	if (r)
> +		return r;
> +
> +	return size;
> +}

I don't like to add a new custom userspace API, but I guess we don't
have much choice.

However, I don't think using 0 and 1 in the API is very good. The
choices with range should probably be "full" and "limited".

Btw, I tried to apply this patch set on top of dss master with and
without the "improve the timings..." patch set, and failed both. What
are these patches based on?

 Tomi


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