Messages in this thread | | | Subject | Re: [PATCH v4 01/11] drm/bridge: dw-hdmi: set mtmdsclock for deep color | From | Neil Armstrong <> | Date | Mon, 2 Mar 2020 16:54:17 +0100 |
| |
Hi,
On 02/03/2020 10:05, Laurent Pinchart wrote: > Hi Neil and Jonas, > > Thank you for the patch. > > On Thu, Feb 06, 2020 at 08:18:24PM +0100, Neil Armstrong wrote: >> From: Jonas Karlman <jonas@kwiboo.se> >> >> Configure the correct mtmdsclock for deep colors to prepare support >> for 10, 12 & 16bit output. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index 67fca439bbfb..9e0927d22db6 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -1818,9 +1818,26 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi, >> >> dev_dbg(hdmi->dev, "final pixclk = %d\n", vmode->mpixelclock); > > Nitpicking a bit, I would change > > - vmode->mtmdsclock = vmode->mpixelclock = mode->clock * 1000; > + vmode->mpixelclock = mode->clock * 1000; > > above, and here add > > vmode->mtmdsclock = vmode->mpixelclock; > > to keep all mtmdsclock calculation in a single place. > >> + if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format)) { >> + switch (hdmi_bus_fmt_color_depth( >> + hdmi->hdmi_data.enc_out_bus_format)) { >> + case 16: >> + vmode->mtmdsclock = (u64)vmode->mpixelclock * 2; > > Both mpixelclock and mtmdsclock are unsigned int. Is the cast to u64 > needed ? > > On a separate but related note, what does the 'm' in tmdsclock stand for > ? It seems to originate from the 'm' prefix for mpixelclock, which has > been there from the start. Unless there's a good reason for the prefix, > renaming mtmdsclock to tmds_clock (and handling the other fields in the > hdmi_vmode structure similarly) would increase clarity I think. > >> + break; >> + case 12: >> + vmode->mtmdsclock = (u64)vmode->mpixelclock * 3 / 2; >> + break; >> + case 10: >> + vmode->mtmdsclock = (u64)vmode->mpixelclock * 5 / 4; >> + break; >> + } >> + } >> + >> if (hdmi_bus_fmt_is_yuv420(hdmi->hdmi_data.enc_out_bus_format)) >> vmode->mtmdsclock /= 2; >> >> + dev_dbg(hdmi->dev, "final tmdsclk = %d\n", vmode->mtmdsclock); > > s/tmdsclk/tmdsclock/ to match the field name ? > >> + >> /* Set up HDMI_FC_INVIDCONF */ >> inv_val = (hdmi->hdmi_data.hdcp_enable || >> (dw_hdmi_support_scdc(hdmi) && >
I fixed the calculus and the cast, but I'll rename the mtmdsclock in a following patch.
is it ok for you ?
Neil
| |