Re: [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 11 Dec 2014, Todd Previte <tprevite@gmail.com> wrote:
> This patch was part of "[PATCH 05/10] drm/i915: Add debugfs interface for
> Displayport debug and compliance testing". That patch has been split into
> smaller patches for ease of review and integration.
>
> This patch contains the definitions/declarations for some of the constants
> and data structures added to support debugfs output for Displayport compliance
> testing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 479e0c1..65b4f5e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -33,6 +33,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/list_sort.h>
> +#include <linux/string.h>
>  #include <asm/msr-index.h>
>  #include <drm/drmP.h>
>  #include "intel_drv.h"
> @@ -51,6 +52,46 @@ static const char *yesno(int v)
>  	return v ? "yes" : "no";
>  }
>  
> +#define DP_PARAMETER_COUNT		8
> +#define MAX_DP_CONFIG_LINE_COUNT	64
> +
> +enum dp_config_param {
> +	DP_CONFIG_PARAM_INVALID = -1,
> +	DP_CONFIG_PARAM_CONNECTOR = 0,
> +	DP_CONFIG_PARAM_LINK_RATE,
> +	DP_CONFIG_PARAM_LANE_COUNT,
> +	DP_CONFIG_PARAM_VOLTAGE_SWING,
> +	DP_CONFIG_PARAM_PREEMPHASIS,
> +	DP_CONFIG_PARAM_HRES,
> +	DP_CONFIG_PARAM_VRES,
> +	DP_CONFIG_PARAM_BPP

If you start the enum with DP_CONFIG_PARAM_CONNECTOR, you can add
DP_PARAMETER_COUNT here which will automatically be correct, and then
add DP_CONFIG_PARAM_INVALID = -1 after that.

> +};
> +
> +struct dp_config {
> +	enum dp_config_param type;
> +	unsigned long value;
> +};
> +
> +const char *dp_conf_tokens[DP_PARAMETER_COUNT] = {
> +		 "Connector",
> +		 "Link Rate",
> +		 "Lane Count",
> +		 "Voltage Swing Level",
> +		 "Preemphasis Level",
> +		 "Horizontal Resolution",
> +		 "Vertical Resolution",
> +		 "Bits Per Pixel"

It's customary to initialize these with designated initializers so that
the order here is not tied to the enum. I.e.

	[DP_CONFIG_PARAM_CONNECTOR] = "Connector",

> +};
> +/* Strings for writing out dp_configs */
> +#define DP_CONF_STR_CONNECTOR	"Connector:             %s\n"
> +#define DP_CONF_STR_LINKRATE	"Link Rate:             %02x\n"
> +#define DP_CONF_STR_LANES	"Lane Count:            %u\n"
> +#define DP_CONF_STR_VSWING	"Voltage Swing Level:   %u\n"
> +#define DP_CONF_STR_PREEMP	"Preemphasis Level:     %u\n"
> +#define DP_CONF_STR_HRES	"Horizontal Resolution: %d\n"
> +#define DP_CONF_STR_VRES	"Vertical Resolution:   %d\n"
> +#define DP_CONF_STR_BPP		"Bits Per Pixel:        %u\n"

I really don't like format strings as defines, because it moves the
format string away from the parameters that need to match the format.

A better option might be to have helper wrappers for seq_printf that
take the required parameters.

BR,
Jani.



> +
>  /* As the drm_debugfs_init() routines are called before dev->dev_private is
>   * allocated we need to hook into the minor for release. */
>  static int
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx