[V3, 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver
Marek Vasut
marex at denx.de
Tue May 25 10:53:23 UTC 2021
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;
[...]
More information about the dri-devel
mailing list