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

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

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

On Fri, 2012-01-06 at 17:58 +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

"User" typoed.

> 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   |   39 +++++++++++++++++++++++++++++++-
>  drivers/video/omap2/dss/ti_hdmi.h      |    2 +
>  5 files changed, 71 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 3cf99a9..0b1b6f2 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -518,6 +518,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(enum hdmi_range 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 afcb593..544b172 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -476,6 +476,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
>  	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>  	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
>  #endif
> +	.configure_range	=	ti_hdmi_4xxx_configure_range,
>  
>  };
>  
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 92a6679..65397f7 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -384,6 +384,34 @@ static void hdmi_power_off(struct omap_dss_device *dssdev)
>  	hdmi_runtime_put();
>  }
>  
> +int omapdss_hdmi_set_range(enum hdmi_range range)
> +{
> +	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 == HDMI_LIMITED_RANGE) &&
> +		((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;
> +}

The error handling here doesn't look valid. The VGA case doesn't restore
the old_range, and generally speaking, storing the old value and
restoring it in the error case is not very nice.

You should first do the argument checks, and then call the
configure_range(), and if both succeed, only then set the
hdmi.ip_data.range. For this you need to pass the range parameter to
configure_range().

> +int omapdss_hdmi_get_range(void)

hdmi_range is an enum, not an int.

> +{
> +	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..3166602 100644
> --- a/drivers/video/omap2/dss/hdmi_panel.c
> +++ b/drivers/video/omap2/dss/hdmi_panel.c
> @@ -33,6 +33,37 @@ 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();

hdmi_range is an enum, not an int.

> +	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 ;
> +
> +	if (strncmp("limited", buf, 7) == 0)
> +		range = 0;
> +	else if (strncmp("full", buf, 4) == 0)
> +		range = 1;
> +	else
> +		return -EINVAL;
> +
> +	r = omapdss_hdmi_set_range(range);

hdmi_range is an enum, not an unsigned long.

 Tomi


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