[Intel-gfx] [PATCH 02/12] drm/i915: Add a function to compute the EDID checksum for Displayport compliance

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite at gmail.com>:
> This function computes the EDID checksum for a block of EDID data. This function
> is necessary for Displayport compliance testing as it does not not require the
> complete EDID checking functionality provided by the DRM layer functions.

Can you give specific reasons why you can't reuse
drm_edid_block_valid(), or extract a piece of it to a new function
that will be called by both the i915 code and drm_edid_block_valid()?


>
> Signed-off-by: Todd Previte <tprevite at gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0d11145..f61502e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3328,6 +3328,29 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>                                        sink_irq_vector, 1) == 1;
>  }
>
> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_checksum)

A little note about patch-splitting: here you are adding a function
that is not called anywhere. This is not a common thing, we usually
only add a function on the patch where it gets called for the first
time. It is much easier to review a function when it has callers,
since you can know the context of why it is really needed, and see if
it is being called correctly more easily. You can also judge if it
needs to be static or exported. Also, adding functions without callers
can produce compiler warnings, like this:

drivers/gpu/drm/i915/intel_dp.c:3421:13: warning:
‘intel_dp_compute_edid_checksum’ defined but not used
[-Wunused-function]

You do the same thing with patch 4, 5 and 6. IMHO, you should squash
all these patches on patch 7. No problem that patch 7 is going to get
big: it is still one logical change.

Another example of why I think this should be squashed: when I see
patch 7, I don't see the need for the "edid_checksum" argument of this
function. Even if you're going to use the argument later, it's better
if you only patch the function to add the argument at the point you
really need it.


> +{
> +       uint32_t byte_total = 0;
> +       uint8_t i = 0;
> +       bool edid_ok = true;
> +
> +       /* Compute byte total w/o the checksum value*/
> +       for (i = 0; i < EDID_LENGTH - 2; i++)
> +                       byte_total += edid_data[i];

Too many tabs here.


> +
> +       DRM_DEBUG_KMS("Displayport: EDID total = %d, EDID checksum =  %d\n", byte_total, edid_data[EDID_LENGTH - 1]);

We allow a line to have more than 80 chars when the message being
print is too big and you would need to cut it in the middle, but on
this case you could have added a line break after the end of the
string, leaving "bytes_total, edid_data[...]);" on the line below.
This would have kept both lines smaller than the 80-column
restriction, without breaking a greppable debug message. Also, keep in
mind that DRM_DEBUG_KMS also prints the name of the function, so you
probably don't need to print, for example, the "Displayport"
characters. The same comment applies for the DRM_DEBUG_KMS call below.

Thanks,
Paulo

> +
> +       /* Compute the checksum */
> +       *edid_checksum = 256 - (byte_total % 256);
> +
> +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> +               DRM_DEBUG_KMS("Displayport: Invalid EDID checksum %d, should be %d\n", edid_data[EDID_LENGTH - 1], *edid_checksum);
> +               edid_ok = false;
> +       }
> +
> +       return edid_ok;
> +}
> +
>  /* Displayport compliance testing - Link training */
>  static uint8_t
>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni