[Intel-gfx] [PATCH 06/17] drm/i915: Add support functions in debugfs for handling Displayport compliance configuration

[Paulo Zanoni]

2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni