Hi Martin,

Am Mittwoch, den 27.03.2013, 19:40 +0100 schrieb Martin Fuzzey:

Hi Philipp,

On 27/03/13 18:30, Philipp Zabel wrote:

quoted

+static bool imx_ldb_encoder_mode_fixup(struct drm_encoder *encoder,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode)
+{
+/*
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+
+	adjusted_mode->clock = clk_round_rate(imx_ldb_ch->ldb->clk_pll[imx_ldb_ch->chno],
+			adjusted_mode->clock * 1000) / 1000;
+*/

This should probably be removed :)

Right.

quoted

+static void imx_ldb_set_clock(struct imx_ldb *ldb, int mux, int chno,
+		unsigned long serial_clk, unsigned long di_clk)
+{
+	int ret;
+
+	dev_dbg(ldb->dev, "%s: now: %ld want: %ld\n", __func__,
+			clk_get_rate(ldb->clk_pll[chno]), serial_clk);
+	clk_set_rate(ldb->clk_pll[chno], serial_clk);
+
+	dev_dbg(ldb->dev, "%s after: %ld\n", __func__,
+			clk_get_rate(ldb->clk_pll[chno]));
+
+	dev_dbg(ldb->dev, "%s: now: %ld want: %ld\n", __func__,
+			clk_get_rate(ldb->clk[chno]),
+			(long int)di_clk);
+	clk_set_rate(ldb->clk[chno], di_clk);
+
+	dev_dbg(ldb->dev, "%s after: %ld\n", __func__,
+			clk_get_rate(ldb->clk[chno]));
+

Are all these debug statements still useful?

The clocking is a hairy, especially if the board code decided to use an
unexpected PLL as source for the LVDS serial clock. But I agree this
could be condensed a bit.

quoted

+static void imx_ldb_encoder_commit(struct drm_encoder *encoder)
+{
+	if (imx_ldb_ch == &ldb->channel[0] || dual) {
+		ldb->ldb_ctrl &= ~0x3;
+		if (mux == 0 || ldb->lvds_mux)
+			ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI0;
+		else if (mux == 1)
+			ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI1;
+	}
+	if (imx_ldb_ch == &ldb->channel[1] || dual) {
+		ldb->ldb_ctrl &= ~0xc;
+		if (mux == 1 || ldb->lvds_mux)
+			ldb->ldb_ctrl |= LDB_CH1_MODE_EN_TO_DI1;
+		else if (mux == 0)
+			ldb->ldb_ctrl |= LDB_CH1_MODE_EN_TO_DI0;
+	}

Maybe avoid the magic 0x3 and 0x0c by building from LDB_CHx_MODE_EN_TO_DIx?

Ok, I'll prune the magic constants.

quoted

+
+static void imx_ldb_encoder_disable(struct drm_encoder *encoder)
+{
+	struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
+	struct imx_ldb *ldb = imx_ldb_ch->ldb;
+
+	/*
+	 * imx_ldb_encoder_disable is called by
+	 * drm_helper_disable_unused_functions without
+	 * the encoder being enabled before.
+	 */
+	if (imx_ldb_ch == &ldb->channel[0] && (ldb->ldb_ctrl & 0x3) == 0)
+		return;
+	else if (imx_ldb_ch == &ldb->channel[1] && (ldb->ldb_ctrl & 0xc) == 0)
+		return;
+
+	if (imx_ldb_ch == &ldb->channel[0])
+		ldb->ldb_ctrl &= ~0x3;
+	else if (imx_ldb_ch == &ldb->channel[1])
+		ldb->ldb_ctrl &= ~0xc;
+

Idem magic numbers

thanks
Philipp