| Message ID | 1405365047-6866-2-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>: > Add the skeleton framework for supporting automation for Displayport compliance testing. This patch > adds the necessary framework for the source device to appropriately responded to test automation > requests from a sink device. These commit messages contain some long lines. Most of the patches I see go up to 70 or at most 80 columns on each line on the commit message, but your patches seem to have a 100-column limit. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 82 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b5ec489..0d11145 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) > sink_irq_vector, 1) == 1; > } > > +/* Displayport compliance testing - Link training */ Does the line above contain the formal name of the test from a given specification, or is it just a description? If it's the exact name of the test, then please state that it is the name, and also name the specification it comes from. But if it's just a description, I don't really see the value of a comment containing "DP compliance testing - link training", when the function is already called intel_dp_autotest_link_training(): that would be just duplicate information. In your patch series I see many other examples of comments that don't add any new information. For example, on patch 7 we have: /* Reset the NACK/DEFER counters */ intel_dp->aux.i2c_nack_count = 0; intel_dp->aux.i2c_defer_count = 0; You don't really need to say you're about to reset a counter if the code is already saying that. Also, if someone ever changes the code but forgets to fix the comment, you will have an inconsistency that will lead code readers confused. But think about the good side: if the comments are useless, it's because the code is readable :). And that should be the goal: code that is clear enough to not really need comments. Please read Documentation/CodingStyle, chapter 8 inside the Kernel source tree. For this and the next patches of the series, I won't go and list every single comment I think should be removed, but keep in mind that IMHO 95% of them could be removed. > +static uint8_t > +intel_dp_autotest_link_training(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - Video pattern testing */ > +static uint8_t > +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - EDID operations */ > +static uint8_t > +intel_dp_autotest_edid(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - PHY pattern testing */ > +static uint8_t > +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - Fast AUX transactions (optional) */ > +static uint8_t > +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n"); Why do we print this here and not for the other tests? > + return test_result; > +} > + > static void > intel_dp_handle_test_request(struct intel_dp *intel_dp) > { > - /* NAK by default */ > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK); > + uint8_t response = DP_TEST_NAK; > + uint8_t rxdata = 0; I would probably have named this "requested_test" or something else. > + int ret = 0; Vairable "ret" is set but never read anywhere. If you edit drivers/gpu/drm/i915/Makefile, you can get a compiler warning: drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_handle_test_request’: drivers/gpu/drm/i915/intel_dp.c:3467:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] You will also see that there are a few more points of our driver where we make similar mistakes :) > + > + DRM_DEBUG_KMS("Displayport: Received automated test request\n"); > + /* Read DP_TEST_REQUEST register to identify the requested test */ > + ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1); > + > + /* Determine which test has been requested */ > + switch (rxdata) { > + /* ACK/NAK response based on test function response > + Unimplemented/unsupported tests will NAK */ > + case DP_TEST_LINK_TRAINING: > + DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n"); We're kinda lying here, since we're still NAKing stuff, aren't we? Thanks, Paulo > + response = intel_dp_autotest_link_training(intel_dp); > + break; > + case DP_TEST_LINK_VIDEO_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n"); > + response = intel_dp_autotest_video_pattern(intel_dp); > + break; > + case DP_TEST_LINK_EDID_READ: > + DRM_DEBUG_KMS("Displayport: Executing EDID request\n"); > + response = intel_dp_autotest_edid(intel_dp); > + break; > + case DP_TEST_LINK_PHY_TEST_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n"); > + response = intel_dp_autotest_phy_pattern(intel_dp); > + break; > + /* FAUX is optional in DP 1.2*/ > + case DP_TEST_LINK_FAUX_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing FAUX_PATTERN request\n"); > + response = intel_dp_autotest_faux_pattern(intel_dp); > + break; > + /* Unsupported test case or something went wrong */ > + default: > + /* Log error here for unhandled test request */ > + DRM_DEBUG_KMS("Displayport: Unhandled test request\n"); > + break; > + } > + /* Send the response from the test result */ > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1); > } > > /* > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>: > Add the skeleton framework for supporting automation for Displayport compliance testing. This patch > adds the necessary framework for the source device to appropriately responded to test automation > requests from a sink device. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 82 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b5ec489..0d11145 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) > sink_irq_vector, 1) == 1; > } > > +/* Displayport compliance testing - Link training */ > +static uint8_t > +intel_dp_autotest_link_training(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - Video pattern testing */ > +static uint8_t > +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - EDID operations */ > +static uint8_t > +intel_dp_autotest_edid(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - PHY pattern testing */ > +static uint8_t > +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + return test_result; > +} > + > +/* Displayport compliance testing - Fast AUX transactions (optional) */ > +static uint8_t > +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp) > +{ > + uint8_t test_result = DP_TEST_NAK; > + DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n"); > + return test_result; > +} > + > static void > intel_dp_handle_test_request(struct intel_dp *intel_dp) > { > - /* NAK by default */ > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK); > + uint8_t response = DP_TEST_NAK; > + uint8_t rxdata = 0; > + int ret = 0; > + > + DRM_DEBUG_KMS("Displayport: Received automated test request\n"); > + /* Read DP_TEST_REQUEST register to identify the requested test */ > + ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1); > + > + /* Determine which test has been requested */ > + switch (rxdata) { > + /* ACK/NAK response based on test function response > + Unimplemented/unsupported tests will NAK */ > + case DP_TEST_LINK_TRAINING: > + DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n"); > + response = intel_dp_autotest_link_training(intel_dp); > + break; > + case DP_TEST_LINK_VIDEO_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n"); > + response = intel_dp_autotest_video_pattern(intel_dp); > + break; > + case DP_TEST_LINK_EDID_READ: > + DRM_DEBUG_KMS("Displayport: Executing EDID request\n"); > + response = intel_dp_autotest_edid(intel_dp); > + break; > + case DP_TEST_LINK_PHY_TEST_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n"); > + response = intel_dp_autotest_phy_pattern(intel_dp); > + break; > + /* FAUX is optional in DP 1.2*/ > + case DP_TEST_LINK_FAUX_PATTERN: > + DRM_DEBUG_KMS("Displayport: Executing FAUX_PATTERN request\n"); > + response = intel_dp_autotest_faux_pattern(intel_dp); > + break; > + /* Unsupported test case or something went wrong */ > + default: > + /* Log error here for unhandled test request */ > + DRM_DEBUG_KMS("Displayport: Unhandled test request\n"); > + break; > + } > + /* Send the response from the test result */ > + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1); Just one more idea: we could count amount of time passed during the test execution, since the spec defines some timeouts. > } > > /* > -- > 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 b5ec489..0d11145 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) sink_irq_vector, 1) == 1; } +/* Displayport compliance testing - Link training */ +static uint8_t +intel_dp_autotest_link_training(struct intel_dp *intel_dp) +{ + uint8_t test_result = DP_TEST_NAK; + return test_result; +} + +/* Displayport compliance testing - Video pattern testing */ +static uint8_t +intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) +{ + uint8_t test_result = DP_TEST_NAK; + return test_result; +} + +/* Displayport compliance testing - EDID operations */ +static uint8_t +intel_dp_autotest_edid(struct intel_dp *intel_dp) +{ + uint8_t test_result = DP_TEST_NAK; + return test_result; +} + +/* Displayport compliance testing - PHY pattern testing */ +static uint8_t +intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp) +{ + uint8_t test_result = DP_TEST_NAK; + return test_result; +} + +/* Displayport compliance testing - Fast AUX transactions (optional) */ +static uint8_t +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp) +{ + uint8_t test_result = DP_TEST_NAK; + DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n"); + return test_result; +} + static void intel_dp_handle_test_request(struct intel_dp *intel_dp) { - /* NAK by default */ - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK); + uint8_t response = DP_TEST_NAK; + uint8_t rxdata = 0; + int ret = 0; + + DRM_DEBUG_KMS("Displayport: Received automated test request\n"); + /* Read DP_TEST_REQUEST register to identify the requested test */ + ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1); + + /* Determine which test has been requested */ + switch (rxdata) { + /* ACK/NAK response based on test function response + Unimplemented/unsupported tests will NAK */ + case DP_TEST_LINK_TRAINING: + DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n"); + response = intel_dp_autotest_link_training(intel_dp); + break; + case DP_TEST_LINK_VIDEO_PATTERN: + DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n"); + response = intel_dp_autotest_video_pattern(intel_dp); + break; + case DP_TEST_LINK_EDID_READ: + DRM_DEBUG_KMS("Displayport: Executing EDID request\n"); + response = intel_dp_autotest_edid(intel_dp); + break; + case DP_TEST_LINK_PHY_TEST_PATTERN: + DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n"); + response = intel_dp_autotest_phy_pattern(intel_dp); + break; + /* FAUX is optional in DP 1.2*/ + case DP_TEST_LINK_FAUX_PATTERN: + DRM_DEBUG_KMS("Displayport: Executing FAUX_PATTERN request\n"); + response = intel_dp_autotest_faux_pattern(intel_dp); + break; + /* Unsupported test case or something went wrong */ + default: + /* Log error here for unhandled test request */ + DRM_DEBUG_KMS("Displayport: Unhandled test request\n"); + break; + } + /* Send the response from the test result */ + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1); } /*
Add the skeleton framework for supporting automation for Displayport compliance testing. This patch adds the necessary framework for the source device to appropriately responded to test automation requests from a sink device. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)