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

Todd Previte
2015-02-18T16:37:20Z

On 12/15/14 6:11 AM, Jani Nikula wrote:
> 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.

Good point. That change has been integrated into V3.
>> +};
>> +
>> +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",
Makes sense. Fixed for V3.
>> +};
>> +/* 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.
Fair enough. I added a function to wrap the seq_printf function and one
to initialize the tokens
instead of having them are straight #defines. Other patches further on
in the sequence will need to
be adjusted to accommodate these changes.

>> +
>> /* 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