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

[Todd Previte <tprevite@gmail.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx