On Thu, Dec 7, 2017 at 8:25 PM, Maxime Ripard
<maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> 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.
OK. I assume you've tried it without setting it and it failed?
I just assume that these refer to the TCON LCD output, whereas
LVDS looks like a separate module and function, and shouldn't
need it.
ChenYu
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
