Re: [RFC] drm: Add utility function to check for edp1.4

[sonika <sonika.jindal@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx