On 17/10/13 17:14, Marek Belisko wrote:

Is there some particular reason to have tx_buf in the driver data? It
looks to me that it would fit better as a local variable for the funcs
that need it.

+};
+
+static struct omap_video_timings td028ttec1_panel_timings = {
+	.x_res		= 480,
+	.y_res		= 640,
+	.pixel_clock	= 22153,
+	.hfp		= 24,
+	.hsw		= 8,
+	.hbp		= 8,
+	.vfp		= 4,
+	.vsw		= 2,
+	.vbp		= 2,
+
+	.vsync_level	= OMAPDSS_SIG_ACTIVE_LOW,
+	.hsync_level	= OMAPDSS_SIG_ACTIVE_LOW,
+
+	.data_pclk_edge	= OMAPDSS_DRIVE_SIG_FALLING_EDGE,
+	.de_level	= OMAPDSS_SIG_ACTIVE_HIGH,
+	.sync_pclk_edge	= OMAPDSS_DRIVE_SIG_OPPOSITE_EDGES,
+};
+
+#define JBT_COMMAND	0x000
+#define JBT_DATA	0x100
+
+int jbt_reg_write_nodata(struct panel_drv_data *ddata, u8 reg)
+{
+	int rc;
+
+	ddata->tx_buf[0] = JBT_COMMAND | reg;
+	rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+			1*sizeof(u16));
+	if (rc != 0)
+		dev_err(&ddata->spi_dev->dev,
+			"jbt_reg_write_nodata spi_write ret %d\n", rc);
+
+	return rc;
+}
+
+int jbt_reg_write(struct panel_drv_data *ddata, u8 reg, u8 data)

Perhaps name this jbt_reg_write8 to be in line with the jbt_reg_write16.
Or is it obvious that the function without a suffix works on bytes?

Just a side note, on DSI side I have used suffixes _0, _1, _2 to
represent transmission without data, with one byte and with 2 bytes.

+{
+	int rc;
+
+	ddata->tx_buf[0] = JBT_COMMAND | reg;
+	ddata->tx_buf[1] = JBT_DATA | data;
+	rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+			2*sizeof(u16));
+	if (rc != 0)
+		dev_err(&ddata->spi_dev->dev,
+			"jbt_reg_write spi_write ret %d\n", rc);
+
+	return rc;
+}
+
+int jbt_reg_write16(struct panel_drv_data *ddata, u8 reg, u16 data)
+{
+	int rc;
+
+	ddata->tx_buf[0] = JBT_COMMAND | reg;
+	ddata->tx_buf[1] = JBT_DATA | (data >> 8);
+	ddata->tx_buf[2] = JBT_DATA | (data & 0xff);
+
+	rc = spi_write(ddata->spi_dev, (u8 *)ddata->tx_buf,
+			3*sizeof(u16));
+
+	if (rc != 0)
+		dev_err(&ddata->spi_dev->dev,
+			"jbt_reg_write16 spi_write ret %d\n", rc);
+
+	return rc;
+}
+
+enum jbt_register {
+	JBT_REG_SLEEP_IN		= 0x10,
+	JBT_REG_SLEEP_OUT		= 0x11,
+
+	JBT_REG_DISPLAY_OFF		= 0x28,
+	JBT_REG_DISPLAY_ON		= 0x29,
+
+	JBT_REG_RGB_FORMAT		= 0x3a,
+	JBT_REG_QUAD_RATE		= 0x3b,
+
+	JBT_REG_POWER_ON_OFF		= 0xb0,
+	JBT_REG_BOOSTER_OP		= 0xb1,
+	JBT_REG_BOOSTER_MODE		= 0xb2,
+	JBT_REG_BOOSTER_FREQ		= 0xb3,
+	JBT_REG_OPAMP_SYSCLK		= 0xb4,
+	JBT_REG_VSC_VOLTAGE		= 0xb5,
+	JBT_REG_VCOM_VOLTAGE		= 0xb6,
+	JBT_REG_EXT_DISPL		= 0xb7,
+	JBT_REG_OUTPUT_CONTROL		= 0xb8,
+	JBT_REG_DCCLK_DCEV		= 0xb9,
+	JBT_REG_DISPLAY_MODE1		= 0xba,
+	JBT_REG_DISPLAY_MODE2		= 0xbb,
+	JBT_REG_DISPLAY_MODE		= 0xbc,
+	JBT_REG_ASW_SLEW		= 0xbd,
+	JBT_REG_DUMMY_DISPLAY		= 0xbe,
+	JBT_REG_DRIVE_SYSTEM		= 0xbf,
+
+	JBT_REG_SLEEP_OUT_FR_A		= 0xc0,
+	JBT_REG_SLEEP_OUT_FR_B		= 0xc1,
+	JBT_REG_SLEEP_OUT_FR_C		= 0xc2,
+	JBT_REG_SLEEP_IN_LCCNT_D	= 0xc3,
+	JBT_REG_SLEEP_IN_LCCNT_E	= 0xc4,
+	JBT_REG_SLEEP_IN_LCCNT_F	= 0xc5,
+	JBT_REG_SLEEP_IN_LCCNT_G	= 0xc6,
+
+	JBT_REG_GAMMA1_FINE_1		= 0xc7,
+	JBT_REG_GAMMA1_FINE_2		= 0xc8,
+	JBT_REG_GAMMA1_INCLINATION	= 0xc9,
+	JBT_REG_GAMMA1_BLUE_OFFSET	= 0xca,
+
+	JBT_REG_BLANK_CONTROL		= 0xcf,
+	JBT_REG_BLANK_TH_TV		= 0xd0,
+	JBT_REG_CKV_ON_OFF		= 0xd1,
+	JBT_REG_CKV_1_2			= 0xd2,
+	JBT_REG_OEV_TIMING		= 0xd3,
+	JBT_REG_ASW_TIMING_1		= 0xd4,
+	JBT_REG_ASW_TIMING_2		= 0xd5,
+
+	JBT_REG_HCLOCK_VGA		= 0xec,
+	JBT_REG_HCLOCK_QVGA		= 0xed,
+};
+
+#define to_panel_data(p) container_of(p, struct panel_drv_data, dssdev)
+
+static int td028ttec1_panel_connect(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	if (omapdss_device_is_connected(dssdev))
+		return 0;
+
+	r = in->ops.dpi->connect(in, dssdev);
+	if (r)
+		return r;
+
+	return 0;
+}
+
+static void td028ttec1_panel_disconnect(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (!omapdss_device_is_connected(dssdev))
+		return;
+
+	in->ops.dpi->disconnect(in, dssdev);
+}
+
+static int td028ttec1_panel_enable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+	int r;
+
+	if (!omapdss_device_is_connected(dssdev))
+		return -ENODEV;
+
+	if (omapdss_device_is_enabled(dssdev))
+		return 0;
+
+	in->ops.dpi->set_data_lines(in, ddata->data_lines);
+	in->ops.dpi->set_timings(in, &ddata->videomode);
+
+	r = in->ops.dpi->enable(in);
+	if (r)
+		return r;
+
+	dev_dbg(dssdev->dev, "td028ttec1_panel_enable() - state %d\n",
+		dssdev->state);
+
+	/*
+	 * according to data sheet: wait 50ms (Tpos of LCM). However, 50ms
+	 * seems unreliable with later LCM batches, increasing to 90ms
+	 */
+	msleep(90);

Wait for what?

+
+	/* three times command zero */
+	r |= jbt_reg_write_nodata(ddata, 0x00);
+	msleep(1000);
+	r |= jbt_reg_write_nodata(ddata, 0x00);
+	msleep(1000);
+	r |= jbt_reg_write_nodata(ddata, 0x00);
+	msleep(1000);

Three _seconds_? That's a huge delay when enabling a panel. Is this
really required?

+
+	if (r) {
+		dev_warn(dssdev->dev, "transfer error\n");
+		goto transfer_err;
+	}
+
+	/* deep standby out */
+	r |= jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x17);
+
+	/* RGB I/F on, RAM write off, QVGA through, SIGCON enable */
+	r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE, 0x80);
+
+	/* Quad mode off */
+	r |= jbt_reg_write(ddata, JBT_REG_QUAD_RATE, 0x00);
+
+	/* AVDD on, XVDD on */
+	r |= jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x16);
+
+	/* Output control */
+	r |= jbt_reg_write16(ddata, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+	/* Sleep mode off */
+	r |= jbt_reg_write_nodata(ddata, JBT_REG_SLEEP_OUT);
+
+	/* at this point we have like 50% grey */
+
+	/* initialize register set */
+	r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE1, 0x01);
+	r |= jbt_reg_write(ddata, JBT_REG_DISPLAY_MODE2, 0x00);
+	r |= jbt_reg_write(ddata, JBT_REG_RGB_FORMAT, 0x60);
+	r |= jbt_reg_write(ddata, JBT_REG_DRIVE_SYSTEM, 0x10);
+	r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_OP, 0x56);
+	r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_MODE, 0x33);
+	r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_FREQ, 0x11);
+	r |= jbt_reg_write(ddata, JBT_REG_BOOSTER_FREQ, 0x11);
+	r |= jbt_reg_write(ddata, JBT_REG_OPAMP_SYSCLK, 0x02);
+	r |= jbt_reg_write(ddata, JBT_REG_VSC_VOLTAGE, 0x2b);
+	r |= jbt_reg_write(ddata, JBT_REG_VCOM_VOLTAGE, 0x40);
+	r |= jbt_reg_write(ddata, JBT_REG_EXT_DISPL, 0x03);
+	r |= jbt_reg_write(ddata, JBT_REG_DCCLK_DCEV, 0x04);
+	/*
+	 * default of 0x02 in JBT_REG_ASW_SLEW responsible for 72Hz requirement
+	 * to avoid red / blue flicker
+	 */
+	r |= jbt_reg_write(ddata, JBT_REG_ASW_SLEW, 0x04);
+	r |= jbt_reg_write(ddata, JBT_REG_DUMMY_DISPLAY, 0x00);
+
+	r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_A, 0x11);
+	r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_B, 0x11);
+	r |= jbt_reg_write(ddata, JBT_REG_SLEEP_OUT_FR_C, 0x11);
+	r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040);
+	r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0);
+	r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020);
+	r |= jbt_reg_write16(ddata, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0);
+
+	r |= jbt_reg_write16(ddata, JBT_REG_GAMMA1_FINE_1, 0x5533);
+	r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_FINE_2, 0x00);
+	r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_INCLINATION, 0x00);
+	r |= jbt_reg_write(ddata, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00);
+
+	r |= jbt_reg_write16(ddata, JBT_REG_HCLOCK_VGA, 0x1f0);
+	r |= jbt_reg_write(ddata, JBT_REG_BLANK_CONTROL, 0x02);
+	r |= jbt_reg_write16(ddata, JBT_REG_BLANK_TH_TV, 0x0804);
+
+	r |= jbt_reg_write(ddata, JBT_REG_CKV_ON_OFF, 0x01);
+	r |= jbt_reg_write16(ddata, JBT_REG_CKV_1_2, 0x0000);
+
+	r |= jbt_reg_write16(ddata, JBT_REG_OEV_TIMING, 0x0d0e);
+	r |= jbt_reg_write16(ddata, JBT_REG_ASW_TIMING_1, 0x11a4);
+	r |= jbt_reg_write(ddata, JBT_REG_ASW_TIMING_2, 0x0e);
+
+	r |= jbt_reg_write_nodata(ddata, JBT_REG_DISPLAY_ON);
+
+	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
+
+transfer_err:
+
+	return r ? -EIO : 0;
+}
+
+static void td028ttec1_panel_disable(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (!omapdss_device_is_enabled(dssdev))
+		return;
+
+	dev_dbg(dssdev->dev, "td028ttec1_panel_disable()\n");
+
+	in->ops.dpi->disable(in);
+
+	jbt_reg_write_nodata(ddata, JBT_REG_DISPLAY_OFF);
+	jbt_reg_write16(ddata, JBT_REG_OUTPUT_CONTROL, 0x8002);
+	jbt_reg_write_nodata(ddata, JBT_REG_SLEEP_IN);
+	jbt_reg_write(ddata, JBT_REG_POWER_ON_OFF, 0x00);

I don't know about this particular panel, but usually you should first
send a DISPLAY_OFF command, and only afterwards disable the DPI output.
Otherwise the panels I know will show garbage on the screen, as the
video stream has ended.

That would also be reverse of the enable sequence.

 Tomi