| Message ID | 1405365047-6866-3-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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) +{ + 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]; + + DRM_DEBUG_KMS("Displayport: EDID total = %d, EDID checksum = %d\n", byte_total, edid_data[EDID_LENGTH - 1]); + + /* 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)
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. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)