Re: [PATCH 1/8] drm/display: hdmi: Add common TMDS character rate constants

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]

On Wed, May 20, 2026 at 09:40:17AM +0200, Javier Martinez Canillas wrote:
> Jani Nikula <jani.nikula@intel.com> writes:
> 
> Hello Jani,
> 
> > On Tue, 19 May 2026, Javier Martinez Canillas <javierm@redhat.com> wrote:
> 
> [...]
> 
> >>  
> >> +/* HDMI spec TMDS character rate limits (in Hz) */
> >> +#define DRM_HDMI_TMDS_CHAR_RATE_MIN	25000000
> >> +#define DRM_HDMI_TMDS_CHAR_RATE_MAX_1_0	165000000
> >> +#define DRM_HDMI_TMDS_CHAR_RATE_MAX_1_3	340000000
> >> +#define DRM_HDMI_TMDS_CHAR_RATE_MAX_2_0	600000000
> >
> > Usually everything in DRM is in kHz, and Hz is the exception.
> >
> 
> That is correct but in this case these constants are to be used with the
> HDMI helpers. Both struct drm_connector_hdmi_state.tmds_char_rate and
> the struct drm_bridge_funcs.hdmi_tmds_char_rate_valid() callback expect
> the TMDS char rate to be defined in Hz.
> 
> If we define these in kHz, it means that drivers will have to * 1000 at
> every call site.

As long as the unit is obvious from the name, I think we'll be fine, and
we can even have both Hz and kHz defines if we want to.

> > I'm also not sure the 1_0, 1_3, and 2_0 really help anyone reading the
> > code. I won't remember what they mean in Hz or kHz, and I'll have to
> > look them up every single time.
> >
> 
> I discussed this with Maxime before posting the patches since I wondered
> the same. He suggested that the max TMDS character rate was linked to the
> HDMI spec versions and that it would be more readable to name it using
> the spec version rather than the resolution.

As usual, it's both the spec and hardware capabilities. But some like
the HDMI 1.3/1.4 max is used to know where to setup the scrambler for
example, and is used everywhere.

I still think it has value, but we don't have to force anyone to use
them either.

I'd prefer to use the DRM_HDMI_$SPEC_TMDS_CHAR_RATE_MAX_HZ though

> The other naming I suggested was:
> 
> /* HDMI spec TMDS character rate limits (in Hz) */
> #define DRM_HDMI_TMDS_CHAR_RATE_MIN		 25000000
> #define DRM_HDMI_TMDS_CHAR_RATE_74_25MHZ	 74250000
> #define DRM_HDMI_TMDS_CHAR_RATE_148_5MHZ	148500000
> #define DRM_HDMI_TMDS_CHAR_RATE_297MHZ		297000000
> #define DRM_HDMI_TMDS_CHAR_RATE_MAX_1_4		340000000

Honestly, I'm not sure if it's worth defining if we'll have the
frequency in the define name.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]