Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1692927
| From | Maxime Ripard <maxime.ripard@free-electrons.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 06/18] drm/sun4i: tcon: Don't rely on encoders to enable the TCON |
| Date | 2017-07-20 15:30 +0200 |
| Message-ID | <u5jGF-81X-1@gated-at.bofh.it> (permalink) |
| References | <u2N8d-6Ff-3@gated-at.bofh.it> <u2N8e-6Ff-9@gated-at.bofh.it> <u2ZM5-66v-3@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
[Multipart message — attachments visible in raw view] - view raw
Hi Chen-Yu,
On Fri, Jul 14, 2017 at 11:40:07AM +0800, Chen-Yu Tsai wrote:
> > static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index d9791292553e..dc70bc2a42a5 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -14,6 +14,7 @@
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_encoder.h>
> > #include <drm/drm_modes.h>
> > #include <drm/drm_of.h>
> >
> > @@ -32,66 +33,62 @@
> > #include "sun4i_tcon.h"
> > #include "sunxi_engine.h"
> >
> > -void sun4i_tcon_disable(struct sun4i_tcon *tcon)
> > +static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > + bool enabled)
> > {
> > - DRM_DEBUG_DRIVER("Disabling TCON\n");
> > + struct clk *clk;
> >
> > - /* Disable the TCON */
> > - regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > - SUN4I_TCON_GCTL_TCON_ENABLE, 0);
> > -}
> > -EXPORT_SYMBOL(sun4i_tcon_disable);
> > -
> > -void sun4i_tcon_enable(struct sun4i_tcon *tcon)
> > -{
> > - DRM_DEBUG_DRIVER("Enabling TCON\n");
> > -
> > - /* Enable the TCON */
> > - regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > - SUN4I_TCON_GCTL_TCON_ENABLE,
> > - SUN4I_TCON_GCTL_TCON_ENABLE);
> > -}
> > -EXPORT_SYMBOL(sun4i_tcon_enable);
> > -
> > -void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel)
> > -{
> > - DRM_DEBUG_DRIVER("Disabling TCON channel %d\n", channel);
> > -
> > - /* Disable the TCON's channel */
> > - if (channel == 0) {
> > + switch (channel) {
> > + case 0:
> > regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > - SUN4I_TCON0_CTL_TCON_ENABLE, 0);
> > - clk_disable_unprepare(tcon->dclk);
> > + SUN4I_TCON0_CTL_TCON_ENABLE,
> > + enabled ? SUN4I_TCON0_CTL_TCON_ENABLE : 0);
> > + clk = tcon->dclk;
> > + break;
> > + case 1:
> > + WARN_ON(!tcon->quirks->has_channel_1);
> > + regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > + SUN4I_TCON1_CTL_TCON_ENABLE,
> > + enabled ? SUN4I_TCON1_CTL_TCON_ENABLE : 0);
> > + clk = tcon->sclk1;
> > + break;
> > + default:
> > + DRM_DEBUG_DRIVER("Unknown channel... doing nothing\n");
> > return;
> > }
> >
> > - WARN_ON(!tcon->quirks->has_channel_1);
> > - regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > - SUN4I_TCON1_CTL_TCON_ENABLE, 0);
> > - clk_disable_unprepare(tcon->sclk1);
> > + if (enabled)
> > + clk_prepare_enable(clk);
>
> I wonder if it's better to enable the clk before the TCON?
I think I kept the current behaviour, which seemed to work fine with
that regard.
>
> > + else
> > + clk_disable_unprepare(clk);
> > }
> > -EXPORT_SYMBOL(sun4i_tcon_channel_disable);
> >
> > -void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel)
> > +void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
> > + struct drm_encoder *encoder,
> > + bool enabled)
> > {
> > - DRM_DEBUG_DRIVER("Enabling TCON channel %d\n", channel);
> > -
> > - /* Enable the TCON's channel */
> > - if (channel == 0) {
> > - regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > - SUN4I_TCON0_CTL_TCON_ENABLE,
> > - SUN4I_TCON0_CTL_TCON_ENABLE);
> > - clk_prepare_enable(tcon->dclk);
> > + int channel;
> > +
> > + switch (encoder->encoder_type) {
> > + case DRM_MODE_ENCODER_NONE:
> > + channel = 0;
> > + break;
> > + case DRM_MODE_ENCODER_TMDS:
> > + case DRM_MODE_ENCODER_TVDAC:
> > + channel = 1;
> > + break;
> > + default:
> > + DRM_DEBUG_DRIVER("Unknown encoder type, doing nothing...\n");
>
> We could simply add all the possible types, and print a big warning
> if someone does something unexpected. IMHO this is better than having
> the user enable some hidden debug flag to figure why the display isn't
> working properly.
I'm not sure about all types of encoders, but you're right, it should
be a warning.
> > return;
> > }
> >
> > - WARN_ON(!tcon->quirks->has_channel_1);
> > - regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > - SUN4I_TCON1_CTL_TCON_ENABLE,
> > - SUN4I_TCON1_CTL_TCON_ENABLE);
> > - clk_prepare_enable(tcon->sclk1);
> > + sun4i_tcon_channel_set_status(tcon, channel, enabled);
> > +
> > + regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > + SUN4I_TCON_GCTL_TCON_ENABLE,
> > + enabled ? SUN4I_TCON_GCTL_TCON_ENABLE : 0);
>
> The global enable bit should be set first.
ACK
> Also the manual says "When it’s disabled, the module will be reset to
> idle state."
> so you might get away with just disabling the global enable bit and returning
> directly after disabling the clock?
I'd rather keep an explicit disable, just in case one SoC is broken,
just like the DE is...
> > }
> > -EXPORT_SYMBOL(sun4i_tcon_channel_enable);
> > +EXPORT_SYMBOL(sun4i_tcon_set_status);
>
> The TCON and CRTC code are part of the same module.
> There is no need to export this function.
Ah, right. I'll remove it. Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Back to linux.kernel | Previous | Next | Find similar | Unroll thread
Re: [PATCH 06/18] drm/sun4i: tcon: Don't rely on encoders to enable the TCON Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-07-20 15:30 +0200
csiph-web