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;
[...]
