linux-kernel - Re: [PATCH v2 18/29] drm/sun4i: dw-hdmi: Add quirk for setting TMDS clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181008091406.yjs2u22fmj72e4rj@flea>
Date:   Mon, 8 Oct 2018 11:14:06 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Jernej Skrabec <jernej.skrabec@...l.net>
Cc:     wens@...e.org, robh+dt@...nel.org, sboyd@...nel.org,
        airlied@...ux.ie, architt@...eaurora.org, a.hajda@...sung.com,
        Laurent.pinchart@...asonboard.com, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH v2 18/29] drm/sun4i: dw-hdmi: Add quirk for setting TMDS
 clock

Hi,

On Sun, Oct 07, 2018 at 11:38:54AM +0200, Jernej Skrabec wrote:
> It turns out that H6 HDMI BSP kernel driver doesn't change TMDS rate at
> all. At this point it is not clear whether it is just not necessary or
> it would cause some kind of issues.
> 
> Add a quirk for it.
> 
> Reviewed-by: Chen-Yu Tsai <wens@...e.org>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@...l.net>
> ---
>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 5 ++++-
>  drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> index ec122136ee9d..e9e93f174b35 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> @@ -165,7 +165,9 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
>  		goto err_disable_clk_tmds;
>  	}
>  
> -	drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs);
> +	if (hdmi->quirks->set_rate)
> +		drm_encoder_helper_add(encoder,
> +				       &sun8i_dw_hdmi_encoder_helper_funcs);

That seems a bit backward, it only works because we only have mode_set
at the moment, and the only thing it does is changing the clock
rate. As soon as we change one of these two assumptions, the code will
break.

Why not just return in mode_set if that boolean is true?

>  	drm_encoder_init(drm, encoder, &sun8i_dw_hdmi_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS, NULL);
>  
> @@ -235,6 +237,7 @@ static int sun8i_dw_hdmi_remove(struct platform_device *pdev)
>  
>  static const struct sun8i_dw_hdmi_quirks sun8i_a83t_quirks = {
>  	.mode_valid = sun8i_dw_hdmi_mode_valid_a83t,
> +	.set_rate = true,
>  };
>  
>  static const struct of_device_id sun8i_dw_hdmi_dt_ids[] = {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> index a645b8bc9f58..f9eb663865a4 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
> @@ -173,6 +173,7 @@ struct sun8i_hdmi_phy {
>  struct sun8i_dw_hdmi_quirks {
>  	enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  					   const struct drm_display_mode *mode);
> +	bool set_rate;

This triggers a check in checkpatch. You should address them (and
there's several in your series).

With both addressed,
Acked-by: Maxime Ripard <maxime.ripard@...tlin.com>

Thanks!

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)