| Message ID | 1418255597-4716-7-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface > for Displayport debug and compliance testing". Adds two support functions for > handling Displayport configuration parameters that are used for compliance > testing. > First of all, a little comment about not only this patch but others too: you had the big 05/10 patch in the previous series, and even though it was big, I could review it since the new functions included their callers and everything. Now, I see you have patch 4 where you add just a few definitions without users. Then in patch 5 you add a debugfs file that does nothing; here in patch 6 you add 2 new functions without callers. It's kinda hard to review these things without knowing how they are used. Also, if at some later point of the review we decide to change, for example, patch 08/17, maybe some of the stuff added by patch 05 or 06 will have to be changed. I usually try to split patches into "minimal reviewable bisectable self-contained" pieces, where each patch moves the overall code a single step and add all the required structures/definitions. This way, every added function, definition and struct field has a user. For example, the 2 functions added by this patch should probably be static - judging by the fact that you don't export them in a header file -, but if you go and declare them as static, then you'll get compiler warnings complaining that they are declared static but are not used. > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index feae100..ce091c1 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = { > .write = display_crc_ctl_write > }; > > +enum dp_config_param displayport_get_config_param_type(char *input_string) > +{ > + enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID; > + int i = 0; > + > + for (i = 0; i < DP_PARAMETER_COUNT; i++) { > + if (strncmp(input_string, dp_conf_tokens[i], > + strlen(dp_conf_tokens[i])) == 0) { I guess we never really did a high-level review of your debugfs protocol, and I can't really see everything since there's no user-space side here. From this function, I can imagine you're using a string protocol. Is there any major advantage of using a string protocol compared to using a binary one? By using binary, we would be able to avoid these string operations which add a lot of complexity and danger, both on the user and kernel side: if both shared the same .h containing the right enums, they would just be able to pass the enums around. Well, that's just a suggestion in case you think it's better, not a requirement for you to rewrite everything. > + param_type = (enum dp_config_param) i; > + break; > + } > + } > + return param_type; > +} > + > +int value_for_config_param(enum dp_config_param param, > + char *input_string, > + void *output_value) > +{ This function is a little weird since it can return pointers to different types of things. I would prefer if we kept simple and avoided the " void *" here., even if that meant a little more code. But, well, let me see in the later patches how this is used first. Besides the bikeshed, the code looks correct. > + int status = 0; > + unsigned int *out_ptr = (unsigned int *)output_value; > + char *index = input_string; > + > + switch (param) { > + case DP_CONFIG_PARAM_CONNECTOR: > + output_value = (char *)index; > + break; > + case DP_CONFIG_PARAM_LINK_RATE: > + case DP_CONFIG_PARAM_LANE_COUNT: > + case DP_CONFIG_PARAM_VOLTAGE_SWING: > + case DP_CONFIG_PARAM_PREEMPHASIS: > + status = kstrtol(index, 16, (long *)out_ptr); > + break; > + case DP_CONFIG_PARAM_HRES: > + case DP_CONFIG_PARAM_VRES: > + case DP_CONFIG_PARAM_BPP: > + status = kstrtol(index, 10, (long *)out_ptr); > + break; > + default: > + /* Unhandled case */ > + status = -EINVAL; > + *out_ptr = 0; > + break; > + } > + > + return status; > +} > + > static int displayport_config_ctl_open(struct inode *inode, > struct file *file) > { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index feae100..ce091c1 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -3694,6 +3694,54 @@ static const struct file_operations i915_display_crc_ctl_fops = { .write = display_crc_ctl_write }; +enum dp_config_param displayport_get_config_param_type(char *input_string) +{ + enum dp_config_param param_type = DP_CONFIG_PARAM_INVALID; + int i = 0; + + for (i = 0; i < DP_PARAMETER_COUNT; i++) { + if (strncmp(input_string, dp_conf_tokens[i], + strlen(dp_conf_tokens[i])) == 0) { + param_type = (enum dp_config_param) i; + break; + } + } + return param_type; +} + +int value_for_config_param(enum dp_config_param param, + char *input_string, + void *output_value) +{ + int status = 0; + unsigned int *out_ptr = (unsigned int *)output_value; + char *index = input_string; + + switch (param) { + case DP_CONFIG_PARAM_CONNECTOR: + output_value = (char *)index; + break; + case DP_CONFIG_PARAM_LINK_RATE: + case DP_CONFIG_PARAM_LANE_COUNT: + case DP_CONFIG_PARAM_VOLTAGE_SWING: + case DP_CONFIG_PARAM_PREEMPHASIS: + status = kstrtol(index, 16, (long *)out_ptr); + break; + case DP_CONFIG_PARAM_HRES: + case DP_CONFIG_PARAM_VRES: + case DP_CONFIG_PARAM_BPP: + status = kstrtol(index, 10, (long *)out_ptr); + break; + default: + /* Unhandled case */ + status = -EINVAL; + *out_ptr = 0; + break; + } + + return status; +} + static int displayport_config_ctl_open(struct inode *inode, struct file *file) {
This patch was previously part of "[PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing". Adds two support functions for handling Displayport configuration parameters that are used for compliance testing. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 48 +++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)