Maxime Ripard <[email protected]> writes: > On Wed, May 20, 2026 at 09:40:17AM +0200, Javier Martinez Canillas wrote: >> Jani Nikula <[email protected]> writes: >> >> Hello Jani, >> >> > On Tue, 19 May 2026, Javier Martinez Canillas <[email protected]> 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
