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

[Javier Martinez Canillas]

Maxime Ripard <mripard@xxxxxxxxxx> writes:

> On Wed, May 20, 2026 at 09:40:17AM +0200, Javier Martinez Canillas wrote:
>> Jani Nikula <jani.nikula@xxxxxxxxx> writes:
>> 
>> Hello Jani,
>> 
>> > On Tue, 19 May 2026, Javier Martinez Canillas <javierm@xxxxxxxxxx> 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

Yes, I agree.

> 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
>

Ok, I'll change it in v2.

>> 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.
>

Fair. I think is still worth it but I agree with your opinion that
having the $SPEC in the name makes more sense.

> Maxime

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat