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

Jani Nikula
2014-12-15T13:11:40Z

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