[PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]

On 02/06/17 15:04, Boris Brezillon wrote:

> +enum cdns_dsi_output_type {
> +	CDNS_DSI_PANEL = 0,
> +	CDNS_DSI_BRIDGE = 1,
> +};

Just my opinion, but I think you should only define values for enums
when those values actually mean something and are needed. In this case,
it doesn't matter which values those enums have.

> +static int cdns_dsi_init_link(struct cdns_dsi *dsi)
> +{
> +	u32 val;
> +	int i;
> +
> +	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
> +	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
> +	       dsi->regs + MCTL_DPHY_TIMEOUT1);
> +	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
> +
> +	val = WAIT_BURST_TIME(0xf);
> +	for (i = 1; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_EN(i);
> +
> +	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> +		val |= CLK_CONTINUOUS;
> +
> +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> +
> +	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
> +	       dsi->regs + MCTL_ULPOUT_TIME);
> +
> +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> +
> +	val = CLK_LANE_EN | PLL_START;
> +	for (i = 0; i < dsi->output->dev->lanes; i++)
> +		val |= DATA_LANE_START(i);
> +
> +	writel(val, dsi->regs + MCTL_MAIN_EN);
> +
> +	ndelay(100);
> +
> +	return 0;
> +}

There are quite a bit of magic values here (elsewhere too). Looking at
the names of the macros, many of them probably represent spans of time,
in clock ticks? Would it make more sense to have the times defined in,
say, nanoseconds, and a function to convert the ns to clock ticks?

And a hardcoded CLK_DIV, does that work? Is the clock rate fixed?

(I don't have the datasheet yet =)

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170606/b1424b4a/attachment.sig>