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

[Kieran Bingham]

Hi Laurent,

On 23/11/2018 14:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> 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 at 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?

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

I don't think the coded table extraction helps here. Especially as it
necessitates indexing against ARRAY_SIZE - 2.

--
Regards

Kieran




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