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]
Date:   Fri, 14 Jul 2017 11:56:18 +0800
From:   Chen-Yu Tsai <wens@...e.org>
To:     Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Chen-Yu Tsai <wens@...e.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        devicetree <devicetree@...r.kernel.org>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Subject: Re: [PATCH 07/18] drm/sun4i: tcon: Don't rely on encoders to set the
 TCON mode

On Thu, Jul 13, 2017 at 10:13 PM, Maxime Ripard
<maxime.ripard@...e-electrons.com> wrote:
> Just like we did for the TCON enable and disable, for historical reasons we
> used to rely on the encoders calling the TCON mode_set function, while the
> CRTC has a callback for that.
>
> Let's implement it in order to reduce the boilerplate code.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_crtc.c          | 11 ++++-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_ddc_clk.c  |  1 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c      |  7 +---
>  drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c |  1 +-
>  drivers/gpu/drm/sun4i/sun4i_rgb.c           | 15 +------
>  drivers/gpu/drm/sun4i/sun4i_tcon.c          | 56 ++++++++++------------
>  drivers/gpu/drm/sun4i/sun4i_tcon.h          | 10 +----
>  drivers/gpu/drm/sun4i/sun4i_tv.c            |  6 +--
>  8 files changed, 40 insertions(+), 67 deletions(-)
>

[...]

> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index dc70bc2a42a5..c4407910dfaf 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -106,29 +106,6 @@ void sun4i_tcon_enable_vblank(struct sun4i_tcon *tcon, bool enable)
>  }
>  EXPORT_SYMBOL(sun4i_tcon_enable_vblank);
>
> -void sun4i_tcon_set_mux(struct sun4i_tcon *tcon, int channel,
> -                       struct drm_encoder *encoder)
> -{
> -       u32 val;
> -
> -       if (!tcon->quirks->has_unknown_mux)
> -               return;
> -
> -       if (channel != 1)
> -               return;
> -
> -       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
> -               val = 1;
> -       else
> -               val = 0;
> -
> -       /*
> -        * FIXME: Undocumented bits
> -        */
> -       regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
> -}
> -EXPORT_SYMBOL(sun4i_tcon_set_mux);
> -
>  static int sun4i_tcon_get_clk_delay(struct drm_display_mode *mode,
>                                     int channel)
>  {
> @@ -147,8 +124,8 @@ static int sun4i_tcon_get_clk_delay(struct drm_display_mode *mode,
>         return delay;
>  }
>
> -void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
> -                         struct drm_display_mode *mode)
> +static void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
> +                                struct drm_display_mode *mode)

Nit on the side: maybe we could mark mode as constant?
Since the function doesn't change it. Same applies to the
other mode_set functions. But this could be left to another
patch.

>  {
>         unsigned int bp, hsync, vsync;
>         u8 clk_delay;
> @@ -221,10 +198,9 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon,
>         /* Enable the output on the pins */
>         regmap_write(tcon->regs, SUN4I_TCON0_IO_TRI_REG, 0);
>  }
> -EXPORT_SYMBOL(sun4i_tcon0_mode_set);
>
> -void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
> -                         struct drm_display_mode *mode)
> +static void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
> +                                struct drm_display_mode *mode)
>  {
>         unsigned int bp, hsync, vsync, vtotal;
>         u8 clk_delay;
> @@ -312,7 +288,29 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon,
>                            SUN4I_TCON_GCTL_IOMAP_MASK,
>                            SUN4I_TCON_GCTL_IOMAP_TCON1);
>  }
> -EXPORT_SYMBOL(sun4i_tcon1_mode_set);
> +
> +void sun4i_tcon_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
> +                        struct drm_display_mode *mode)

(also mark encoder as const?)

> +{
> +       switch (encoder->encoder_type) {
> +       case DRM_MODE_ENCODER_NONE:
> +               sun4i_tcon0_mode_set(tcon, mode);
> +               break;
> +       case DRM_MODE_ENCODER_TVDAC:
> +               /*
> +                * FIXME: Undocumented bits
> +                */
> +               if (tcon->quirks->has_unknown_mux)
> +                       regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
> +               /* Fallthrough */
> +       case DRM_MODE_ENCODER_TMDS:
> +               sun4i_tcon1_mode_set(tcon, mode);

IIRC you need to clear the mux bit here. So ...

> +               break;
> +       default:
> +               DRM_DEBUG_DRIVER("Unknown encoder type, doing nothing...\n");
> +       }

I think keeping the muxing in a separate function would be cleaner.
The above is already slightly messy if you add the bit clearing part.
With all the other muxing possibilities in the other SoC this is
going to get really messy.

> +}
> +EXPORT_SYMBOL(sun4i_tcon_mode_set);
>
>  static void sun4i_tcon_finish_page_flip(struct drm_device *dev,
>                                         struct sun4i_crtc *scrtc)

[...]

Thanks for working on this. Now we've decoupled the TCON/CRTC code
from all the encoders.

Regards
ChenYu

Powered by blists - more mailing lists