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

[Thierry Reding]

On Wed, Oct 22, 2014 at 11:45:23AM +0530, sonika.jin...@intel.com wrote:
> From: Sonika Jindal <sonika.jin...@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.jin...@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

pgpbt8DA1cFnP.pgp
Description: PGP signature

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