Re: [PATCH v3 08/15] drm/sun4i: Add LVDS support

From: Maxime Ripard
Date: Thu Dec 07 2017 - 07:25:29 EST

Hi,

On Thu, Dec 07, 2017 at 02:05:47PM +0800, Chen-Yu Tsai wrote:
> > +static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> > + const struct drm_encoder *encoder,
> > + bool enabled)
> > +{
> > + if (enabled) {
> > + u8 val;
> > +
> > + regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
> > + SUN4I_TCON0_LVDS_IF_EN,
> > + SUN4I_TCON0_LVDS_IF_EN);
> > +
> > + regmap_write(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> > + SUN4I_TCON0_LVDS_ANA0_C(2) |
> > + SUN4I_TCON0_LVDS_ANA0_V(3) |
> > + SUN4I_TCON0_LVDS_ANA0_PD(2) |
> > + SUN4I_TCON0_LVDS_ANA0_EN_LDO);
> > + udelay(2);
> > +
> > + regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> > + SUN4I_TCON0_LVDS_ANA0_EN_MB,
> > + SUN4I_TCON0_LVDS_ANA0_EN_MB);
> > + udelay(2);
> > +
> > + regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> > + SUN4I_TCON0_LVDS_ANA0_EN_DRVC,
> > + SUN4I_TCON0_LVDS_ANA0_EN_DRVC);
> > +
> > + if (sun4i_tcon_get_pixel_depth(encoder) == 18)
> > + val = 7;
> > + else
> > + val = 0xf;
> > +
> > + regmap_write_bits(tcon->regs, SUN4I_TCON0_LVDS_ANA0_REG,
> > + SUN4I_TCON0_LVDS_ANA0_EN_DRVD(0xf),
> > + SUN4I_TCON0_LVDS_ANA0_EN_DRVD(val));
>
> I suggest changing the prefix of the macros of the analog bits to
> SUN6I_TCON0_*. The register definitions and sequence do not apply
> to the A10/A20. Furthermore you should add a comment saying this
> doesn't apply to the A10/A20. In the future we might want to move
> this part into a separate function, referenced by a function pointer
> from the quirks structure.

I'll change the bit field names and add a comment like you suggested.

> > + } else {
> > + regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
> > + SUN4I_TCON0_LVDS_IF_EN, 0);
> > + }
> > +}
> > +
> > void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
> > const struct drm_encoder *encoder,
> > bool enabled)
> > {
> > + bool is_lvds = false;
> > int channel;
> >
> > switch (encoder->encoder_type) {
> > + case DRM_MODE_ENCODER_LVDS:
> > + is_lvds = true;
> > + /* Fallthrough */
> > case DRM_MODE_ENCODER_NONE:
> > channel = 0;
> > break;
> > @@ -84,10 +171,16 @@ void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
> > return;
> > }
> >
> > + if (is_lvds && !enabled)
> > + sun4i_tcon_lvds_set_status(tcon, encoder, false);
> > +
> > regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > SUN4I_TCON_GCTL_TCON_ENABLE,
> > enabled ? SUN4I_TCON_GCTL_TCON_ENABLE : 0);
> >
> > + if (is_lvds && enabled)
> > + sun4i_tcon_lvds_set_status(tcon, encoder, true);
> > +
> > sun4i_tcon_channel_set_status(tcon, channel, enabled);
> > }
> >
> > @@ -170,6 +263,78 @@ static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon,
> > SUN4I_TCON0_BASIC0_Y(mode->crtc_vdisplay));
> > }
> >
> > +static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon,
> > + const struct drm_encoder *encoder,
> > + const struct drm_display_mode *mode)
> > +{
> > + unsigned int bp;
> > + u8 clk_delay;
> > + u32 reg, val = 0;
> > +
> > + tcon->dclk_min_div = 7;
> > + tcon->dclk_max_div = 7;
> > + sun4i_tcon0_mode_set_common(tcon, mode);
> > +
> > + /* Adjust clock delay */
> > + clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
> > + regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > + SUN4I_TCON0_CTL_CLK_DELAY_MASK,
> > + SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
> > +
> > + /*
> > + * This is called a backporch in the register documentation,
> > + * but it really is the back porch + hsync
> > + */
> > + bp = mode->crtc_htotal - mode->crtc_hsync_start;
> > + DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
> > + mode->crtc_htotal, bp);
> > +
> > + /* Set horizontal display timings */
> > + regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
> > + SUN4I_TCON0_BASIC1_H_TOTAL(mode->htotal) |
> > + SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
> > +
> > + /*
> > + * This is called a backporch in the register documentation,
> > + * but it really is the back porch + hsync
> > + */
> > + bp = mode->crtc_vtotal - mode->crtc_vsync_start;
> > + DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
> > + mode->crtc_vtotal, bp);
> > +
> > + /* Set vertical display timings */
> > + regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
> > + SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
> > + SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
>
> Can we move the above to a common function?

Until we have DSI support figured out I'd rather not do too much of
consolidation. We know already a few things are going to change there
(like the clk_delay), but it's not clear yet how much.

> > + /* Map output pins to channel 0 */
> > + regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > + SUN4I_TCON_GCTL_IOMAP_MASK,
> > + SUN4I_TCON_GCTL_IOMAP_TCON0);
> > +
> > + /* Enable the output on the pins */
> > + regmap_write(tcon->regs, SUN4I_TCON0_IO_TRI_REG, 0xe0000000);
>
> Is this still needed? You are no longer using the TCON LCD pins
> with LVDS.

We do. It's a separate function of the pins, but it's the same pins.

> > static const struct sun4i_tcon_quirks sun6i_a31s_quirks = {
> > .has_channel_1 = true,
> > + .has_lvds_pll = true,
>
> The A31s does not have MIPI.

I'll change that.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature