Re: [PATCH] drm: rcar-du: dw-hdmi: Reject modes with a too high clock frequency

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

On Friday, 23 November 2018 17:30:43 EET Kieran Bingham wrote:
> On 23/11/2018 14:47, Laurent Pinchart wrote:
> > On Friday, 23 November 2018 16:43:28 EET Kieran Bingham wrote:
> >> On 23/11/2018 14:34, Laurent Pinchart wrote:
> >>> Implement a .mode_valid() handler in the R-Car glue layer to reject
> >>> modes with an unsupported clock frequency.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>> 
> >>> diff --git a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c index
> >>> 75490a3e0a2a..8a603235f22d
> >>> 100644
> >>> --- a/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> +++ b/drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
> >>> @@ -35,6 +35,16 @@ static const struct rcar_hdmi_phy_params
> >>> rcar_hdmi_phy_params[] = {
> >>>  	{ ~0UL,      0x0000, 0x0000, 0x0000 },
> >>>  };
> >>> 
> >>> +static enum drm_mode_status
> >>> +rcar_hdmi_mode_valid(struct drm_connector *connector,
> >>> +		     const struct drm_display_mode *mode)
> >>> +{
> >>> +	if (mode->clock > 297000)
> >> 
> >> Is 29700 constant? Can it be determined from any other location or is it
> >> just a magically known platform value?
> > 
> > It's the last entry of the rcar_hdmi_phy_params table above. I considered
> > writing it
> > 
> > 	if (mode->clock >
> > 
> > rcar_hdmi_phy_params[ARRAY_SIZE(rcar_hdmi_phy_params)-2].mpixelclock)
> > 
> > but found it a but hard to parse. Do you think it would be better ?
> 
> Well - for readability - no,
> 
> But for accuracy - yes:
> 
> 	297000000 != 297000
> 
> Unless the /1000 is intentional?

I would have had to write / 1000 indeed :-) mode->clock is expressed in kHz.

> How about keep the (corrected?) constant value, but add a comment
> referencing it's extraction.

Good idea, I'll do so.

> I don't think the coded table extraction helps here. Especially as it
> necessitates indexing against ARRAY_SIZE - 2.
> 
> >>> +		return MODE_CLOCK_HIGH;
> >>> +
> >>> +	return MODE_OK;
> >>> +}
> >>> +
> >>>  static int rcar_hdmi_phy_configure(struct dw_hdmi *hdmi,
> >>>  				   const struct dw_hdmi_plat_data *pdata,
> >>>  				   unsigned long mpixelclock)
> >>> @@ -59,6 +69,7 @@ static int rcar_hdmi_phy_configure(struct dw_hdmi
> >>> *hdmi,
> >>>  }
> >>>  
> >>>  static const struct dw_hdmi_plat_data rcar_dw_hdmi_plat_data = {
> >>> +	.mode_valid = rcar_hdmi_mode_valid,
> >>>  	.configure_phy	= rcar_hdmi_phy_configure,
> >>>  };

-- 
Regards,

Laurent Pinchart