Thanks for your comments Thierry. I agree to all your comments. I will write a general function to return version and repost the patch

Thanks, Sonika

On Wednesday 29 October 2014 07:12 PM, Thierry Reding wrote:

On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote:

...

From: Sonika Jindal sonika.jindal@intel.com

v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh)

v3: Moving the utility function to drm_dp_helper (Daniel)

Signed-off-by: Sonika Jindal sonika.jindal@intel.com

drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ include/drm/drm_dp_helper.h | 2 ++ 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 08e33b8..a54a760 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister);

+bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE])

I'd prefer if this didn't take a dpcd argument but rather directly accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used directly rather than rely on the driver to have read a dpcd block in the appropriate format.

...

+{

• uint8_t reg;
• if (dpcd[DP_EDP_CONFIGURATION_CAP] &

•  DP_DPCD_DISPLAY_CONTROL_CAPABLE) {
  

• if (drm_dp_dpcd_read(aux, DP_EDP_REV, &reg, 1))
  

• 	if (reg == 0x03)
  

• 		return true;
  

• }
• return false;

+} +EXPORT_SYMBOL(drm_dp_is_edp_v1_4);

Does it make sense to have a function that checks for a specific version? Why not add one that returns the revision so that it can be compared, something like:

u8 value;

drm_dp_dpcd_read(aux, DP_EDP_REV, &value, 1);

return value;

Then we can do something like:

#define DP_EDP_REV_1_1 0x00 #define DP_EDP_REV_1_2 0x01 #define DP_EDP_REV_1_3 0x02 #define DP_EDP_REV_1_4 0x03

And code can simply compare against that:

drm_dp_get_edp_revision(aux, &rev);

if (rev >= DP_EDP_REV_1_4) { ... }

The check in your variant will only match v1.4 exactly, but presumably v1.5 will be backwards compatible. Having a direct check on the revision code will allow code to continue to work with future, backwards- compatible revisions.

...

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 8edeed0..b017e1e 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -102,6 +102,8 @@

#define DP_EDP_CONFIGURATION_CAP 0x00d /* XXX 1.2? */ #define DP_TRAINING_AUX_RD_INTERVAL 0x00e /* XXX 1.2? */ +#define DP_DPCD_DISPLAY_CONTROL_CAPABLE (1 << 3)

This seems to be a field in the DP_EDP_CONFIGURATION_CAP register, so it should be sorted below that register, not DP_TRAINING_AUX_RD_INTERVAL.

...

+#define DP_EDP_REV 0x700

And this belongs further down, so it properly sorts into the list of registers.

Thierry

On Fri, Oct 31, 2014 at 05:06:39PM +0100, Daniel Vetter wrote:

On Wed, Oct 29, 2014 at 02:42:29PM +0100, Thierry Reding wrote:

...

On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jindal@intel.com wrote:

...

From: Sonika Jindal sonika.jindal@intel.com

v2: Reading DP_EDP_REV, only when DISPLAY_CONTROL_CAPABLE field is set (Satheesh)

v3: Moving the utility function to drm_dp_helper (Daniel)

Signed-off-by: Sonika Jindal sonika.jindal@intel.com

drivers/gpu/drm/drm_dp_helper.c | 15 +++++++++++++++ include/drm/drm_dp_helper.h | 2 ++ 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 08e33b8..a54a760 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -768,3 +768,18 @@ void drm_dp_aux_unregister(struct drm_dp_aux *aux) i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister);

+bool drm_dp_is_edp_v1_4(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE])

I'd prefer if this didn't take a dpcd argument but rather directly accessed the DP_EDP_CONFIGURATION_CAP register so that it can be used directly rather than rely on the driver to have read a dpcd block in the appropriate format.

The idea is that you'd grab the DPCD field anyway since it's needed all over the place. We have a pile of helpers already that take exactly this block and decode parts of it. So I think this makes sense - dp aux is fast but not entirely free, so caching seems useful.

If we want to always cache part of the DPCD wouldn't it be better to add the cache to struct drm_dp_aux instead of having to duplicate this in every driver?

Thierry