[V3, 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver

[Marek Vasut]

On 5/17/21 3:23 PM, Mike Looijmans wrote:

Which system/soc are you testing this on ?

[...]

>> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
>> +{
>> +    struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +
>> +    /*
>> +     * Reset the chip, pull EN line low for t_reset=10ms,
>> +     * then high for t_en=1ms.
>> +     */
>> +    regcache_mark_dirty(ctx->regmap);
>> +    gpiod_set_value(ctx->enable_gpio, 0);
>> +    usleep_range(10000, 11000);
>> +    gpiod_set_value(ctx->enable_gpio, 1);
>> +    usleep_range(1000, 1100);
> Taking the chip out of reset here is not needed and may even disrupt 
> things, as the DSI hasn't set up anything yet so the clock may not be 
> running. I'd move this all into enable and get rid of the pre_enable 
> call. Similar for post_disable.

I am still waiting for someone to confirm the behavior of the DSI clock 
and data lanes in pre_enable/enable() . The datasheet says the HS clock 
have to be running and data lanes must be in LP state during init, but I 
don't think that is guaranteed currently in either pre_enable or enable.

[...]

>> +static void sn65dsi83_enable(struct drm_bridge *bridge)
>> +{
>> +    struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>> +    unsigned int pval;
>> +    u16 val;
>> +    int ret;
>> +
>> +    /* Clear reset, disable PLL */
>> +    regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
> Writing 0 to the RESET register is a no-op. Has no effect whatsoever, 
> just wasting time and code.

I would rather keep it to make sure the register is initialized.

>> +    regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
>> +
>> +    /* Reference clock derived from DSI link clock. */
>> +    regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
>> +        REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
>> +        REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
>> +    regmap_write(ctx->regmap, REG_DSI_CLK,
>> +        REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
>> +    regmap_write(ctx->regmap, REG_RC_DSI_CLK,
>> +        REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
>> +
>> +    /* Set number of DSI lanes and LVDS link config. */
>> +    regmap_write(ctx->regmap, REG_DSI_LANE,
>> +        REG_DSI_LANE_LVDS_LINK_CFG_DUAL |
>> +        REG_DSI_LANE_CHA_DSI_LANES(~(ctx->dsi_lanes - 1)) |
>> +        /* CHB is DSI85-only, set to default on DSI83/DSI84 */
>> +        REG_DSI_LANE_CHB_DSI_LANES(3));
>> +    /* No equalization. */
>> +    regmap_write(ctx->regmap, REG_DSI_EQ, 0x00);
>> +
>> +    /* RGB888 is the only format supported so far. */
>> +    val = (ctx->mode.flags & DRM_MODE_FLAG_NHSYNC ?
>> +           REG_LVDS_FMT_HS_NEG_POLARITY : 0) |
>> +          (ctx->mode.flags & DRM_MODE_FLAG_NVSYNC ?
>> +           REG_LVDS_FMT_VS_NEG_POLARITY : 0) |
>> +          REG_LVDS_FMT_CHA_24BPP_MODE;
>> +    if (ctx->lvds_dual_link)
>> +        val |= REG_LVDS_FMT_CHB_24BPP_MODE;
>> +    else
>> +        val |= REG_LVDS_FMT_LVDS_LINK_CFG;
>> +
>> +    regmap_write(ctx->regmap, REG_LVDS_FMT, val);
>> +    regmap_write(ctx->regmap, REG_LVDS_VCOM, 0x05);
>> +    regmap_write(ctx->regmap, REG_LVDS_LANE,
>> +        (ctx->lvds_dual_link_even_odd_swap ?
>> +         REG_LVDS_LANE_EVEN_ODD_SWAP : 0) |
>> +        REG_LVDS_LANE_CHA_LVDS_TERM |
>> +        REG_LVDS_LANE_CHB_LVDS_TERM);
>> +    regmap_write(ctx->regmap, REG_LVDS_CM, 0x00);
>> +
>> +    regmap_bulk_write(ctx->regmap, REG_VID_CHA_ACTIVE_LINE_LENGTH_LOW,
>> +              &ctx->mode.hdisplay, 2);
> 
> I think this ignores the endian format of the data. So this would only 
> work on little-endian systems, right?

Likely, can you double-check that ?
Some cpu_to_le16() could help here then.

>> +    regmap_bulk_write(ctx->regmap, 
>> REG_VID_CHA_VERTICAL_DISPLAY_SIZE_LOW,
>> +              &ctx->mode.vdisplay, 2);
>> +    val = 32 + 1;    /* 32 + 1 pixel clock to ensure proper operation */
>> +    regmap_bulk_write(ctx->regmap, REG_VID_CHA_SYNC_DELAY_LOW, &val, 2);
>> +    val = ctx->mode.hsync_end - ctx->mode.hsync_start;

[...]