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

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 6914 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]