[PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[Harald Welte]

This driver adds support for LCM initialization and power management of
the JBT6K74 LCM as found on the FIC GTA01 hardware

Signed-off-by: Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>

---

Index: linux-2.6/drivers/spi/Kconfig
===================================================================
--- linux-2.6.orig/drivers/spi/Kconfig
+++ linux-2.6/drivers/spi/Kconfig
@@ -237,5 +237,11 @@
 
 # (slave support would go here)
 
+config SPI_SLAVE_JBT6K74
+	tristate "tpo JP6K74 LCM ASIC control interface"
+	depends on SPI_MASTER && SYSFS
+	help
+	  Driver for the tpo JP6K74-AS LCM SPI control interface.
+
 endmenu # "SPI support"
 
Index: linux-2.6/drivers/spi/Makefile
===================================================================
--- linux-2.6.orig/drivers/spi/Makefile
+++ linux-2.6/drivers/spi/Makefile
@@ -39,4 +39,5 @@
 # 	... add above this line ...
 
 # SPI slave drivers (protocol for that link)
+obj-$(CONFIG_SPI_SLAVE_JBT6K74)		+= jbt6k74.o
 # 	... add above this line ...
Index: linux-2.6/drivers/spi/jbt6k74.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/spi/jbt6k74.c
@@ -0,0 +1,628 @@
+/* Linux kernel driver for the tpo JBT6K74-AS LCM ASIC
+ *
+ * Copyright (C) 2006-2007 by OpenMoko, Inc.
+ * Author: Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>,
+ * 	   Stefan Schmidt <stefan-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#include <linux/spi/spi.h>
+
+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,
+
+	/* VGA */
+	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,
+
+	/* QVGA */
+	JBT_REG_BLANK_CONTROL_QVGA	= 0xd6,
+	JBT_REG_BLANK_TH_TV_QVGA	= 0xd7,
+	JBT_REG_CKV_ON_OFF_QVGA		= 0xd8,
+	JBT_REG_CKV_1_2_QVGA		= 0xd9,
+	JBT_REG_OEV_TIMING_QVGA		= 0xde,
+	JBT_REG_ASW_TIMING_1_QVGA	= 0xdf,
+	JBT_REG_ASW_TIMING_2_QVGA	= 0xe0,
+
+
+	JBT_REG_HCLOCK_VGA		= 0xec,
+	JBT_REG_HCLOCK_QVGA		= 0xed,
+
+};
+
+enum jbt_state {
+	JBT_STATE_DEEP_STANDBY,
+	JBT_STATE_SLEEP,
+	JBT_STATE_NORMAL,
+	JBT_STATE_QVGA_NORMAL,
+};
+
+static const char *jbt_state_names[] = {
+	[JBT_STATE_DEEP_STANDBY]	= "deep-standby",
+	[JBT_STATE_SLEEP]		= "sleep",
+	[JBT_STATE_NORMAL]		= "normal",
+	[JBT_STATE_QVGA_NORMAL]		= "qvga-normal",
+};
+
+struct jbt_info {
+	enum jbt_state state, last_state;
+	u_int16_t tx_buf[8];
+	struct spi_device *spi_dev;
+	u_int16_t reg_cache[0xEE];
+};
+
+#define JBT_COMMAND	0x000
+#define JBT_DATA	0x100
+
+static int jbt_reg_write_nodata(struct jbt_info *jbt, u_int8_t reg)
+{
+	int rc;
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+
+	rc = spi_write(jbt->spi_dev, (u_int8_t *)jbt->tx_buf,
+		       1*sizeof(u_int16_t));
+	if (rc == 0)
+		jbt->reg_cache[reg] = 0;
+
+	return rc;
+}
+
+
+static int jbt_reg_write(struct jbt_info *jbt, u_int8_t reg, u_int8_t data)
+{
+	int rc;
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+	jbt->tx_buf[1] = JBT_DATA | data;
+
+	rc = spi_write(jbt->spi_dev, (u_int8_t *)jbt->tx_buf,
+		       2*sizeof(u_int16_t));
+	if (rc == 0)
+		jbt->reg_cache[reg] = data;
+
+	return rc;
+}
+
+static int jbt_reg_write16(struct jbt_info *jbt, u_int8_t reg, u_int16_t data)
+{
+	int rc;
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+	jbt->tx_buf[1] = JBT_DATA | (data >> 8);
+	jbt->tx_buf[2] = JBT_DATA | (data & 0xff);
+
+	rc = spi_write(jbt->spi_dev, (u_int8_t *)jbt->tx_buf,
+		       3*sizeof(u_int16_t));
+	if (rc == 0)
+		jbt->reg_cache[reg] = data;
+
+	return rc;
+}
+
+static int jbt_init_regs(struct jbt_info *jbt, int qvga)
+{
+	int rc;
+
+	dev_dbg(&jbt->spi_dev->dev, "entering %cVGA mode\n", qvga ? 'Q' : ' ');
+
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE1, 0x01);
+	rc |= jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE2, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_RGB_FORMAT, 0x60);
+	rc |= jbt_reg_write(jbt, JBT_REG_DRIVE_SYSTEM, 0x10);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_OP, 0x56);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_MODE, 0x33);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_FREQ, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_OPAMP_SYSCLK, 0x02);
+	rc |= jbt_reg_write(jbt, JBT_REG_VSC_VOLTAGE, 0x2b);
+	rc |= jbt_reg_write(jbt, JBT_REG_VCOM_VOLTAGE, 0x40);
+	rc |= jbt_reg_write(jbt, JBT_REG_EXT_DISPL, 0x03);
+	rc |= jbt_reg_write(jbt, JBT_REG_DCCLK_DCEV, 0x04);
+	rc |= jbt_reg_write(jbt, JBT_REG_ASW_SLEW, 0x02);
+	rc |= jbt_reg_write(jbt, JBT_REG_DUMMY_DISPLAY, 0x00);
+
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_A, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_B, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_C, 0x11);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0);
+
+	rc |= jbt_reg_write16(jbt, JBT_REG_GAMMA1_FINE_1, 0x5533);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_FINE_2, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_INCLINATION, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00);
+
+	if (!qvga) {
+		rc |= jbt_reg_write16(jbt, JBT_REG_HCLOCK_VGA, 0x1f0);
+		rc |= jbt_reg_write(jbt, JBT_REG_BLANK_CONTROL, 0x02);
+		rc |= jbt_reg_write16(jbt, JBT_REG_BLANK_TH_TV, 0x0804);
+
+		rc |= jbt_reg_write(jbt, JBT_REG_CKV_ON_OFF, 0x01);
+		rc |= jbt_reg_write16(jbt, JBT_REG_CKV_1_2, 0x0000);
+
+		rc |= jbt_reg_write16(jbt, JBT_REG_OEV_TIMING, 0x0d0e);
+		rc |= jbt_reg_write16(jbt, JBT_REG_ASW_TIMING_1, 0x11a4);
+		rc |= jbt_reg_write(jbt, JBT_REG_ASW_TIMING_2, 0x0e);
+	} else {
+		rc |= jbt_reg_write16(jbt, JBT_REG_HCLOCK_QVGA, 0x00ff);
+		rc |= jbt_reg_write(jbt, JBT_REG_BLANK_CONTROL_QVGA, 0x02);
+		rc |= jbt_reg_write16(jbt, JBT_REG_BLANK_TH_TV_QVGA, 0x0804);
+
+		rc |= jbt_reg_write(jbt, JBT_REG_CKV_ON_OFF_QVGA, 0x01);
+		rc |= jbt_reg_write16(jbt, JBT_REG_CKV_1_2_QVGA, 0x0008);
+
+		rc |= jbt_reg_write16(jbt, JBT_REG_OEV_TIMING_QVGA, 0x050a);
+		rc |= jbt_reg_write16(jbt, JBT_REG_ASW_TIMING_1_QVGA, 0x0a19);
+		rc |= jbt_reg_write(jbt, JBT_REG_ASW_TIMING_2_QVGA, 0x0a);
+	}
+
+	return rc;
+}
+
+static int standby_to_sleep(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* three times command zero */
+	rc = jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+	rc = jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+	rc = jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+
+	/* deep standby out */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x17);
+
+	return rc;
+}
+
+static int sleep_to_normal(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* RGB I/F on, RAM wirte off, QVGA through, SIGCON enable */
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE, 0x80);
+
+	/* Quad mode off */
+	rc |= jbt_reg_write(jbt, JBT_REG_QUAD_RATE, 0x00);
+
+	/* AVDD on, XVDD on */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x16);
+
+	/* Output control */
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+	/* Sleep mode off */
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_OUT);
+
+	/* initialize register set */
+	rc |= jbt_init_regs(jbt, 0);
+	return rc;
+}
+
+static int sleep_to_qvga_normal(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* RGB I/F on, RAM wirte off, QVGA through, SIGCON enable */
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE, 0x81);
+
+	/* Quad mode on */
+	rc |= jbt_reg_write(jbt, JBT_REG_QUAD_RATE, 0x22);
+
+	/* AVDD on, XVDD on */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x16);
+
+	/* Output control */
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+	/* Sleep mode off */
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_OUT);
+
+	/* initialize register set for qvga*/
+	rc |= jbt_init_regs(jbt, 1);
+	return rc;
+}
+
+static int normal_to_sleep(struct jbt_info *jbt)
+{
+	int rc;
+
+	rc = jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_OFF);
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0x8002);
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_IN);
+
+	return rc;
+}
+
+static int sleep_to_standby(struct jbt_info *jbt)
+{
+	return jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x00);
+}
+
+/* frontend function */
+int jbt6k74_enter_state(struct jbt_info *jbt, enum jbt_state new_state)
+{
+	int rc = -EINVAL;
+
+	dev_dbg(&jbt->spi_dev->dev, "entering (old_state=%u, "
+		"new_state=%u)\n", jbt->state, new_state);
+
+	switch (jbt->state) {
+	case JBT_STATE_DEEP_STANDBY:
+		switch (new_state) {
+		case JBT_STATE_DEEP_STANDBY:
+			rc = 0;
+			break;
+		case JBT_STATE_SLEEP:
+			rc = standby_to_sleep(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			/* first transition into sleep */
+			rc = standby_to_sleep(jbt);
+			/* then transition into normal */
+			rc |= sleep_to_normal(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			/* first transition into sleep */
+			rc = standby_to_sleep(jbt);
+			/* then transition into normal */
+			rc |= sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_SLEEP:
+		switch (new_state) {
+		case JBT_STATE_SLEEP:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			rc = sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			rc = sleep_to_normal(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			rc = sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_NORMAL:
+		switch (new_state) {
+		case JBT_STATE_NORMAL:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* then transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_SLEEP:
+			rc = normal_to_sleep(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* second transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			/* third transition into sleep */
+			rc |= standby_to_sleep(jbt);
+			/* fourth transition into normal */
+			rc |= sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_QVGA_NORMAL:
+		switch (new_state) {
+		case JBT_STATE_QVGA_NORMAL:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* then transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_SLEEP:
+			rc = normal_to_sleep(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* second transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			/* third transition into sleep */
+			rc |= standby_to_sleep(jbt);
+			/* fourth transition into normal */
+			rc |= sleep_to_normal(jbt);
+			break;
+		}
+		break;
+	}
+	if (rc == 0)
+		jbt->state = new_state;
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(jbt6k74_enter_state);
+
+int jbt6k74_display_onoff(struct jbt_info *jbt, int on)
+{
+	if (on)
+		return jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_ON);
+	else
+		return jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_OFF);
+}
+EXPORT_SYMBOL_GPL(jbt6k74_display_onoff);
+
+static ssize_t state_read(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+
+	if (jbt->state >= ARRAY_SIZE(jbt_state_names))
+		return -EIO;
+
+	return sprintf(buf, "%s\n", jbt_state_names[jbt->state]);
+}
+
+static ssize_t state_write(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(jbt_state_names); i++) {
+		if (!strncmp(buf, jbt_state_names[i],
+			     strlen(jbt_state_names[i]))) {
+			jbt6k74_enter_state(jbt, i);
+			switch (i) {
+			case JBT_STATE_NORMAL:
+			case JBT_STATE_QVGA_NORMAL:
+				/* Enable display again after deep-standby */
+				jbt6k74_display_onoff(jbt, 1);
+				break;
+			default:
+				break;
+			}
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static DEVICE_ATTR(state, 0644, state_read, state_write);
+
+static int reg_by_string(const char *name)
+{
+	if (!strcmp(name, "gamma_fine1"))
+		return JBT_REG_GAMMA1_FINE_1;
+	else if (!strcmp(name, "gamma_fine2"))
+		return JBT_REG_GAMMA1_FINE_2;
+	else if (!strcmp(name, "gamma_inclination"))
+		return JBT_REG_GAMMA1_INCLINATION;
+	else
+		return JBT_REG_GAMMA1_BLUE_OFFSET;
+}
+
+static ssize_t gamma_read(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	return strlcpy(buf, "N/A\n", PAGE_SIZE);
+}
+
+static ssize_t gamma_write(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+	int reg = reg_by_string(attr->attr.name);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+
+	jbt_reg_write(jbt, reg, val & 0xff);
+
+	return count;
+}
+
+static DEVICE_ATTR(gamma_fine1, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_fine2, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_inclination, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_blue_offset, 0644, gamma_read, gamma_write);
+
+static struct attribute *jbt_sysfs_entries[] = {
+	&dev_attr_state.attr,
+	&dev_attr_gamma_fine1.attr,
+	&dev_attr_gamma_fine2.attr,
+	&dev_attr_gamma_inclination.attr,
+	&dev_attr_gamma_blue_offset.attr,
+	NULL,
+};
+
+static struct attribute_group jbt_attr_group = {
+	.name	= NULL,
+	.attrs	= jbt_sysfs_entries,
+};
+
+/* linux device model infrastructure */
+
+static int __devinit jbt_probe(struct spi_device *spi)
+{
+	int rc;
+	struct jbt_info *jbt;
+
+	jbt = kzalloc(sizeof(*jbt), GFP_KERNEL);
+	if (!jbt)
+		return -ENOMEM;
+
+	jbt->spi_dev = spi;
+	jbt->state = JBT_STATE_DEEP_STANDBY;
+
+	/* since we don't have MISO connected, we can't do detection */
+
+	dev_set_drvdata(&spi->dev, jbt);
+
+	spi->mode = SPI_CPOL | SPI_CPHA;
+	spi->bits_per_word = 9;
+
+	rc = spi_setup(spi);
+	if (rc < 0) {
+		dev_err(&spi->dev,
+			"error during spi_setup of jbt6k74 driver\n");
+		dev_set_drvdata(&spi->dev, NULL);
+		kfree(jbt);
+		return rc;
+	}
+
+	rc = jbt6k74_enter_state(jbt, JBT_STATE_NORMAL);
+	if (rc < 0)
+		dev_warn(&spi->dev, "cannot enter NORMAL state\n");
+
+	jbt6k74_display_onoff(jbt, 1);
+
+	rc = sysfs_create_group(&spi->dev.kobj, &jbt_attr_group);
+	if (rc) {
+		dev_set_drvdata(&spi->dev, NULL);
+		kfree(jbt);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int __devexit jbt_remove(struct spi_device *spi)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	sysfs_remove_group(&spi->dev.kobj, &jbt_attr_group);
+	kfree(jbt);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int jbt_suspend(struct spi_device *spi, pm_message_t state)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	switch (state.event) {
+	case PM_EVENT_SUSPEND:
+	case 3:
+		/* Save mode for resume */
+		jbt->last_state = jbt->state;
+		jbt6k74_enter_state(jbt, JBT_STATE_DEEP_STANDBY);
+		return 0;
+	default:
+		return -1;
+	}
+}
+
+static int jbt_resume(struct spi_device *spi)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	jbt6k74_enter_state(jbt, jbt->last_state);
+	jbt6k74_display_onoff(jbt, 1);
+
+	return 0;
+}
+#else
+#define jbt_suspend	NULL
+#define jbt_resume	NULL
+#endif
+
+static struct spi_driver jbt6k74_driver = {
+	.driver = {
+		.name	= "jbt6k74",
+		.owner	= THIS_MODULE,
+	},
+
+	.probe	 = jbt_probe,
+	.remove	 = __devexit_p(jbt_remove),
+	.suspend = jbt_suspend,
+	.resume	 = jbt_resume,
+};
+
+static int __init jbt_init(void)
+{
+	return spi_register_driver(&jbt6k74_driver);
+}
+
+static void __exit jbt_exit(void)
+{
+	spi_unregister_driver(&jbt6k74_driver);
+}
+
+MODULE_DESCRIPTION("SPI driver for tpo JBT6K74-AS LCM control interface");
+MODULE_AUTHOR("Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>");
+MODULE_LICENSE("GPL");
+
+module_init(jbt_init);
+module_exit(jbt_exit);
Index: linux-2.6/arch/arm/mach-s3c2410/Kconfig
===================================================================
--- linux-2.6.orig/arch/arm/mach-s3c2410/Kconfig
+++ linux-2.6/arch/arm/mach-s3c2410/Kconfig
@@ -107,6 +107,7 @@
 config MACH_QT2410
 	bool "QT2410"
 	select CPU_S3C2410
+	select SPI_SLAVE_JBT6K74
 	help
 	   Say Y here if you are using the Armzone QT2410
 
-- 
- Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[David Brownell]

Just one minor comment to start with:

> --- linux-2.6.orig/drivers/spi/Kconfig
> +++ linux-2.6/drivers/spi/Kconfig
> @@ -237,5 +237,11 @@
>  
>  # (slave support would go here)
>  
> +config SPI_SLAVE_JBT6K74
> +	tristate "tpo JP6K74 LCM ASIC control interface"
> +	depends on SPI_MASTER && SYSFS
> +	help
> +	  Driver for the tpo JP6K74-AS LCM SPI control interface.
> +
>  endmenu # "SPI support"
>  

It's not a slave-side driver though ... it's a master-side driver,
depending on SPI_MASTER, and making bus master calls like spi_write().

I think the Kconfig could stand to be more informative too.  "LCM" is
as cryptic as "tpo", there's no clue about what is controlled (SPI?),
and the symbols are internally inconsistent:  "JBT" != "JP", etc

So after I get you any technical comments, I'll want a new patch
with Kconfig and Makefile updates going in the proper place.  (I'm
not sure I'll have many technical comments.)

- Dave


-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[Harald Welte]

Hi David,

thanks for your feedback.

On Tue, Dec 18, 2007 at 03:59:54PM -0800, David Brownell wrote:

> > +config SPI_SLAVE_JBT6K74
> > +	tristate "tpo JP6K74 LCM ASIC control interface"
> > +	depends on SPI_MASTER && SYSFS
> > +	help
> > +	  Driver for the tpo JP6K74-AS LCM SPI control interface.
> 
> It's not a slave-side driver though ... it's a master-side driver,
> depending on SPI_MASTER, and making bus master calls like spi_write().

I'm sorry, I must have misread/misinterpreted the terminology then. 

I thought master is the master controller driver, and 'slave driver' is
the driver for an SPI slave attached to that master.

However, you seem to intend the names as 'slave driver' == driver for an
SPI unit running in slave mode on the Linux-controlled CPU/SoC itself.
Correct?

> I think the Kconfig could stand to be more informative too.  "LCM" is
> as cryptic as "tpo", 

LCM == LCD Module.  I will expand that abbreviation in my patchset.

TPO == Name of the manufacturer. I think one of the biggest display
manufacturers in the hardware world (http://www.tpo.biz/) producing
about 17 Million LCD displays per month.  I can't rename or explain the
manufacturer.  What is your suggestion for this?

> there's no clue about what is controlled (SPI?),

I thought "LCM SPI control interface" is quite explicit.  Many modern
LCM's (especially the small-size models for embedded devices) have a SPI
control interface in addition to the actual RGB data interface.  The
control interface is used for power management, gamma calibration as
well as video timing configuration.

> and the symbols are internally inconsistent:  "JBT" != "JP", etc

That's my mistake, JBT is correct.

> So after I get you any technical comments, I'll want a new patch
> with Kconfig and Makefile updates going in the proper place.  (I'm
> not sure I'll have many technical comments.)

Ok, I'll wait for further comments before re-submitting.
-- 
- Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[David Brownell]

On Wednesday 19 December 2007, Harald Welte wrote:
> > > +config SPI_SLAVE_JBT6K74
> > > +	tristate "tpo JP6K74 LCM ASIC control interface"
> > > +	depends on SPI_MASTER && SYSFS
> > > +	help
> > > +	  Driver for the tpo JP6K74-AS LCM SPI control interface.
> > 
> > It's not a slave-side driver though ... it's a master-side driver,
> > depending on SPI_MASTER, and making bus master calls like spi_write().
> 
> I'm sorry, I must have misread/misinterpreted the terminology then. 

See Documentation/spi/spi-summary ...


> I thought master is the master controller driver, and 'slave driver' is
> the driver for an SPI slave attached to that master.

There are potentially four drivers involved.  A "controller" driver
touches the hardware directly, and there can be both master and
slave versions (based on which side of the link is involved).  The
layer on top of each (master or slave sides) is a different flavor
of driver, insulated from the hardware ... I call those "protocol"
drivers, for lack of better terms.  That way terminology can at least
be consistent:  {Master,Slave} x {Controller,Protocol} Driver gives
four driver types.  The JBT6K74 driver is a "master protocol driver".

There are protocol stacks that adopt confusing terminology, like
having the "slave" driver not actually run inside the slave.  (So
what would you call firmware drivers, inside the slaves?)  This
doesn't happen to be one of those stacks.  ;)

 
> However, you seem to intend the names as 'slave driver' == driver for an
> SPI unit running in slave mode on the Linux-controlled CPU/SoC itself.
> Correct?

Yes; though there are two types of slave drivers:  controller (touching
hardware) and protocol (just sending/receiving messages).  If the slave
code is on a resource-scarce system it might use an integrated driver to
cut corners, at the cost of reusability.

 
> > I think the Kconfig could stand to be more informative too.  "LCM" is
> > as cryptic as "tpo", 
> 
> LCM == LCD Module.  I will expand that abbreviation in my patchset.
> 
> TPO == Name of the manufacturer. I think one of the biggest display
> manufacturers in the hardware world (http://www.tpo.biz/) producing
> about 17 Million LCD displays per month.  I can't rename or explain the
> manufacturer.  What is your suggestion for this?

"Driver for the JBT6K74 LCD Control Module, manufactured by TPO..."

 
> > there's no clue about what is controlled (SPI?),
> 
> I thought "LCM SPI control interface" is quite explicit. 

If you already know what's going on, sure.  People reading Kconfig
helptext are less likely than usual to know that.


> Many modern 
> LCM's (especially the small-size models for embedded devices) have a SPI
> control interface in addition to the actual RGB data interface.  The
> control interface is used for power management, gamma calibration as
> well as video timing configuration.

Absolutely.  Same thing happens with audio codecs:  one interface
streams data (I2S or whatever), and there's a separate control
interface using an entirely different bus(*).  One sentence saying
that's what is going on here would clarify things a lot.

- Dave

(*) And to confuse things even more ... some platforms use the same
    type serial interface controller, configured very differently, to
    talk to the two different interfaces.  One SSP implements I2S,
    another SSP implements SPI.

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[David Brownell]

Since this is a LCD controller, it should be living in the
drivers/video directory somewhere.   Maybe in "backlight"
and depending on LCD_CLASS_DEVICE like the LTV350QV code.
(Which is not dissimilar to this LCM driver, although that
Samsung panel is much simpler.)


On Tuesday 18 December 2007, Harald Welte wrote:
> 
> +struct jbt_info {
> +	enum jbt_state state, last_state;
> +	u_int16_t tx_buf[8];
> +	struct spi_device *spi_dev;
> +	u_int16_t reg_cache[0xEE];

You don't have a lock protecting one thread from clobbering
tx_buf while another thread is using it.  And that's very
possible, even just through the sysfs attributes.


> +};
> +
> +#define JBT_COMMAND	0x000
> +#define JBT_DATA	0x100
> +
> +static int jbt_reg_write_nodata(struct jbt_info *jbt, u_int8_t reg)
> +{
> +	int rc;
> +
> +	jbt->tx_buf[0] = JBT_COMMAND | reg;
> +
> +	rc = spi_write(jbt->spi_dev, (u_int8_t *)jbt->tx_buf,
> +		       1*sizeof(u_int16_t));

I prefer the "u16" style not the "uint16_t" style, but that's
neither here nor there ... except that spi_write() is specified
to take a "const u8 *" not a "u_int8_t *".  

This *looks* like a byteswap bug:  you're sending the data
in native endianness, but the slave is expecting it in one
particular byte order.  The saving grace is that you're
actually using nine-bit I/O words here, and the interface is
specified to ensure the native endianness is converted as
needed on the way out.

In short:  please add a comment there, saying that these
write operations all work with nine-bit native-endian data
that's transformed (to big-endian) on the way out the wire.

(The first time I looked at an LCD control module using SPI,
only the first/command data used nine-bit words, and the rest
of the data -- for pixel data, not control data -- used
eight bit ones.  This idiom was surprising to me in two
different ways!)


> +       if (rc == 0)
> +               jbt->reg_cache[reg] = 0;
> +
> +       return rc;
> +}

OK, so the return code for these write methods is either
zero for success, or a negative errno value.


> +static int jbt_init_regs(struct jbt_info *jbt, int qvga)
> +{
> +	int rc;
> +
> +	dev_dbg(&jbt->spi_dev->dev, "entering %cVGA mode\n", qvga ? 'Q' : ' ');
> +
> +	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE1, 0x01);
> +	rc |= jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE2, 0x00);
> +	rc |= jbt_reg_write(jbt, JBT_REG_RGB_FORMAT, 0x60);

I understand why that convention is used -- too many errors to
check for individually, for starters -- but it's not used right
in this file.


> +	...
> +
> +	return rc;

... why not "return rc ? -EIO : 0" or something, so that
the standard "int means negative errno else zero for success"
convention is followed?  Here and elsewhere.  (Some routines
return the real errno on some paths, and an invalid one on
other paths...)

Masking together a whole bunch of fault codes doesn't generate
a useful errno value!


> +static ssize_t state_write(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct jbt_info *jbt = dev_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(jbt_state_names); i++) {
> +		if (!strncmp(buf, jbt_state_names[i],
> +			     strlen(jbt_state_names[i]))) {
> +			jbt6k74_enter_state(jbt, i);

You're ignoring the return code here.  I'd save it, and just
return it (one that's fixed to return a real errno on all paths!)
instead of continuing after errors.


> +static ssize_t gamma_read(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	return strlcpy(buf, "N/A\n", PAGE_SIZE);

Ugly ... why aren't you returning the jbt->reg_cache values
that you saved?  Or just make these attributes write-only.


> +static int __devinit jbt_probe(struct spi_device *spi)
> +{
> +	int rc;
> +	struct jbt_info *jbt;
> +
> +	jbt = kzalloc(sizeof(*jbt), GFP_KERNEL);
> +	if (!jbt)
> +		return -ENOMEM;
> +
> +	jbt->spi_dev = spi;
> +	jbt->state = JBT_STATE_DEEP_STANDBY;
> +
> +	/* since we don't have MISO connected, we can't do detection */

If the chip doesn't have a data output line, say so.

Otherwise this is more of a board-specific issue, and
the comment should be more like "if we knew that this
board connects MISO, we could do detection".  When
someone adds a board with MISO connected, they could
provide and use a platform_data hook to get that kind
of board-specific config data into the driver.


> +
> +	dev_set_drvdata(&spi->dev, jbt);
> +
> +	spi->mode = SPI_CPOL | SPI_CPHA;
> +	spi->bits_per_word = 9;
> +
> +	rc = spi_setup(spi);
> +	if (rc < 0) {
> +		dev_err(&spi->dev,
> +			"error during spi_setup of jbt6k74 driver\n");
> +		dev_set_drvdata(&spi->dev, NULL);
> +		kfree(jbt);

Me, I'd rather do the spi_setup() before the kmalloc, so that
cleanup here is easier.  But this is OK too.


> +		return rc;
> +	}
> +
> +	rc = jbt6k74_enter_state(jbt, JBT_STATE_NORMAL);
> +	if (rc < 0)
> +		dev_warn(&spi->dev, "cannot enter NORMAL state\n");
> +
> +	jbt6k74_display_onoff(jbt, 1);
> +
> +	rc = sysfs_create_group(&spi->dev.kobj, &jbt_attr_group);
> +	if (rc) {
> +		dev_set_drvdata(&spi->dev, NULL);

But leave the display on, and in "normal" state??

The normal policy is that failed probes shouldn't
cause significant state changes.  If you intend
to adopt a different policy, please add a comment
saying why.


> +		kfree(jbt);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit jbt_remove(struct spi_device *spi)
> +{
> +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> +
> +	sysfs_remove_group(&spi->dev.kobj, &jbt_attr_group);

As above:  this probably shouldn't leave a user-visible
state change.  (Display on, and in "normal" state.)


> +	kfree(jbt);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int jbt_suspend(struct spi_device *spi, pm_message_t state)
> +{
> +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> +
> +	switch (state.event) {
> +	case PM_EVENT_SUSPEND:
> +	case 3:

Gaak, no "case 3".  Bad!  Evil!

> +		/* Save mode for resume */
> +		jbt->last_state = jbt->state;
> +		jbt6k74_enter_state(jbt, JBT_STATE_DEEP_STANDBY);
> +		return 0;
> +	default:
> +		return -1;

Why return "-EPERM" rather than "0"?  Are you trying to
prevent use of this driver on systems that support hibernation?


> +	}
> +}
> +
> +static int jbt_resume(struct spi_device *spi)
> +{
> +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> +
> +	jbt6k74_enter_state(jbt, jbt->last_state);
> +	jbt6k74_display_onoff(jbt, 1);

I suspect that if the last state isn't DEEP_STANDBY this
should do nothing.  Remember that during hibernation, this
can be called multiple times.

> +
> +	return 0;
> +}
> +#else
> +#define jbt_suspend	NULL
> +#define jbt_resume	NULL
> +#endif
> +
> +static struct spi_driver jbt6k74_driver = {
> +	.driver = {
> +		.name	= "jbt6k74",
> +		.owner	= THIS_MODULE,
> +	},
> +
> +	.probe	 = jbt_probe,
> +	.remove	 = __devexit_p(jbt_remove),
> +	.suspend = jbt_suspend,
> +	.resume	 = jbt_resume,
> +};
> +
> +static int __init jbt_init(void)
> +{
> +	return spi_register_driver(&jbt6k74_driver);
> +}
> +
> +static void __exit jbt_exit(void)
> +{
> +	spi_unregister_driver(&jbt6k74_driver);
> +}
> +
> +MODULE_DESCRIPTION("SPI driver for tpo JBT6K74-AS LCM control interface");
> +MODULE_AUTHOR("Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>");
> +MODULE_LICENSE("GPL");
> +
> +module_init(jbt_init);
> +module_exit(jbt_exit);

As with EXPORT_SYMBOL() I'd like to see those snugged up against the
function being used as a module init/exit function...

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[Harald Welte]

Hi David,

thanks again for your review.

On Wed, Dec 19, 2007 at 01:16:44PM -0800, David Brownell wrote:
> Since this is a LCD controller, it should be living in the
> drivers/video directory somewhere.   

Ok, no problem.

> Maybe in "backlight" and depending on LCD_CLASS_DEVICE like the
> LTV350QV code.  (Which is not dissimilar to this LCM driver, although
> that Samsung panel is much simpler.)

Well, the jbt6k74 driver does many things. But backlight switching is
not part of it :) the backlight is completely independent and machine
dependent.

> On Tuesday 18 December 2007, Harald Welte wrote:
> > 
> > +struct jbt_info {
> > +	enum jbt_state state, last_state;
> > +	u_int16_t tx_buf[8];
> > +	struct spi_device *spi_dev;
> > +	u_int16_t reg_cache[0xEE];
> 
> You don't have a lock protecting one thread from clobbering
> tx_buf while another thread is using it.  And that's very
> possible, even just through the sysfs attributes.

ok, introduced a mutex for protection.

> > +#define JBT_COMMAND	0x000
> > +#define JBT_DATA	0x100
> > +
> > +static int jbt_reg_write_nodata(struct jbt_info *jbt, u_int8_t reg)
> > +{
> > +	int rc;
> > +
> > +	jbt->tx_buf[0] = JBT_COMMAND | reg;
> > +
> > +	rc = spi_write(jbt->spi_dev, (u_int8_t *)jbt->tx_buf,
> > +		       1*sizeof(u_int16_t));
> 
> I prefer the "u16" style not the "uint16_t" style, but that's
> neither here nor there ... except that spi_write() is specified
> to take a "const u8 *" not a "u_int8_t *".  

ok, will modify accordingly.

> This *looks* like a byteswap bug:  you're sending the data
> in native endianness, but the slave is expecting it in one
> particular byte order.  The saving grace is that you're
> actually using nine-bit I/O words here, and the interface is
> specified to ensure the native endianness is converted as
> needed on the way out.
> 
> In short:  please add a comment there, saying that these
> write operations all work with nine-bit native-endian data
> that's transformed (to big-endian) on the way out the wire.

ok, will do so.

> > +       if (rc == 0)
> > +               jbt->reg_cache[reg] = 0;
> > +
> > +       return rc;
> > +}
> 
> OK, so the return code for these write methods is either
> zero for success, or a negative errno value.
> 
> 
> > +static int jbt_init_regs(struct jbt_info *jbt, int qvga)
> > +{
> > +	int rc;
> > +
> > +	dev_dbg(&jbt->spi_dev->dev, "entering %cVGA mode\n", qvga ? 'Q' : ' ');
> > +
> > +	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE1, 0x01);
> > +	rc |= jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE2, 0x00);
> > +	rc |= jbt_reg_write(jbt, JBT_REG_RGB_FORMAT, 0x60);
> 
> I understand why that convention is used -- too many errors to
> check for individually, for starters -- but it's not used right
> in this file.
> 
> 
> > +	...
> > +
> > +	return rc;
> 
> ... why not "return rc ? -EIO : 0" or something, so that
> the standard "int means negative errno else zero for success"
> convention is followed?  Here and elsewhere.  (Some routines
> return the real errno on some paths, and an invalid one on
> other paths...)
> 
> Masking together a whole bunch of fault codes doesn't generate
> a useful errno value!

I agree. changes are incorporated now.

> > +static ssize_t state_write(struct device *dev, struct device_attribute *attr,
> > +			   const char *buf, size_t count)
> > +{
> > +	struct jbt_info *jbt = dev_get_drvdata(dev);
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(jbt_state_names); i++) {
> > +		if (!strncmp(buf, jbt_state_names[i],
> > +			     strlen(jbt_state_names[i]))) {
> > +			jbt6k74_enter_state(jbt, i);
> 
> You're ignoring the return code here.  I'd save it, and just
> return it (one that's fixed to return a real errno on all paths!)
> instead of continuing after errors.

ok.

> > +static ssize_t gamma_read(struct device *dev, struct device_attribute *attr,
> > +			  char *buf)
> > +{
> > +	return strlcpy(buf, "N/A\n", PAGE_SIZE);
> 
> Ugly ... why aren't you returning the jbt->reg_cache values
> that you saved?  Or just make these attributes write-only.

indeed, that's a good question.  I guess the reg_cache was only added
after this code.

> > +	/* since we don't have MISO connected, we can't do detection */
> 
> If the chip doesn't have a data output line, say so.

Yes, there is no data output line.  So it's not platform specific

> > +	dev_set_drvdata(&spi->dev, jbt);
> > +
> > +	spi->mode = SPI_CPOL | SPI_CPHA;
> > +	spi->bits_per_word = 9;
> > +
> > +	rc = spi_setup(spi);
> > +	if (rc < 0) {
> > +		dev_err(&spi->dev,
> > +			"error during spi_setup of jbt6k74 driver\n");
> > +		dev_set_drvdata(&spi->dev, NULL);
> > +		kfree(jbt);
> 
> Me, I'd rather do the spi_setup() before the kmalloc, so that
> cleanup here is easier.  But this is OK too.
> 
> 
> > +		return rc;
> > +	}
> > +
> > +	rc = jbt6k74_enter_state(jbt, JBT_STATE_NORMAL);
> > +	if (rc < 0)
> > +		dev_warn(&spi->dev, "cannot enter NORMAL state\n");
> > +
> > +	jbt6k74_display_onoff(jbt, 1);
> > +
> > +	rc = sysfs_create_group(&spi->dev.kobj, &jbt_attr_group);
> > +	if (rc) {
> > +		dev_set_drvdata(&spi->dev, NULL);
> 
> But leave the display on, and in "normal" state??
> 
> The normal policy is that failed probes shouldn't
> cause significant state changes.  If you intend
> to adopt a different policy, please add a comment
> saying why.

ok.

> > +	sysfs_remove_group(&spi->dev.kobj, &jbt_attr_group);
> 
> As above:  this probably shouldn't leave a user-visible
> state change.  (Display on, and in "normal" state.)

well, this is arguable.  For something as fundamental as a driver
associated with the single user-visible output device (in normal
operation), I think it is better to keep the device in a
functional, powered-up state.  Yes, why would somebody unload the
module.  But then, it's module usage count likely is zero.  so let's
rmmod -> display off according to your philosophy.

I've added a comment indicating this rationale.

> > +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> > +
> > +	switch (state.event) {
> > +	case PM_EVENT_SUSPEND:
> > +	case 3:
> 
> Gaak, no "case 3".  Bad!  Evil!

sorry. I don't remember from which example I've stolen that magic
number... removed it now.

> > +	default:
> > +		return -1;
> 
> Why return "-EPERM" rather than "0"?  Are you trying to
> prevent use of this driver on systems that support hibernation?
> 

nope. I just didn't know what other cases there were, and since none of
them are implemented, I thought it's better to refuse them than to claim
we support something that was never tested.

I'm now following the suggestion of pm.h, i.e. treat all cases like
SUSPEND.  Do you agree this sounds like the right thing to do?

> > +static int jbt_resume(struct spi_device *spi)
> > +{
> > +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> > +
> > +	jbt6k74_enter_state(jbt, jbt->last_state);
> > +	jbt6k74_display_onoff(jbt, 1);
> 
> I suspect that if the last state isn't DEEP_STANDBY this
> should do nothing.  

Erm, you mean if the last state _is_ DEEP_STANDBY or SLEEP, it should do
nothing?  Thansk anyway for pointing it out, there definitely was a bug.

> Remember that during hibernation, this can be called multiple times.

And how would this be a problem? Sorry, I don't get it :/

> > +module_init(jbt_init);
> > +module_exit(jbt_exit);
> 
> As with EXPORT_SYMBOL() I'd like to see those snugged up against the
> function being used as a module init/exit function...

will do, thought all kernel code that I've been involed so far (mostly
networking related) used the style that I had in the first patch.

Here is the updated version for your review:

*************************

This driver adds support for the SPI-based control interface of the LCM (LCD
Panel) found on the FIC GTA01 hardware.  

The specific panel in this hardware is a TPO TD028TTEC1, but the driver should
be able to drive any other diplay based on the JBT6K74-AS controller ASIC.

Signed-off-by: Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>

---

Index: linux-2.6/drivers/video/display/Kconfig
===================================================================
--- linux-2.6.orig/drivers/video/display/Kconfig
+++ linux-2.6/drivers/video/display/Kconfig
@@ -21,4 +21,15 @@
 comment "Display hardware drivers"
 	depends on DISPLAY_SUPPORT
 
+config DISPLAY_JBT6K74
+	tristate "TPO JBT6K74-AS TFT display ASIC control interface"
+	depends on SPI_MASTER && SYSFS
+	help
+	  SPI driver for the control interface of TFT panels containing
+	  the TPO JBT6K74-AS controller ASIC, such as the TPO TD028TTEC1
+	  TFT diplay module used in the FIC/OpenMoko Neo1973 GSM phones.
+
+	  The control interface is required for display operation, as it
+	  controls power management, display timing and gamma calibration.
+
 endmenu
Index: linux-2.6/drivers/video/display/Makefile
===================================================================
--- linux-2.6.orig/drivers/video/display/Makefile
+++ linux-2.6/drivers/video/display/Makefile
@@ -3,4 +3,5 @@
 display-objs				:= display-sysfs.o
 
 obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
+obj-$(CONFIG_DISPLAY_JBT6K74)		+= jbt6k74.o
 
Index: linux-2.6/drivers/video/display/jbt6k74.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/video/display/jbt6k74.c
@@ -0,0 +1,673 @@
+/* Linux kernel driver for the tpo JBT6K74-AS LCM ASIC
+ *
+ * Copyright (C) 2006-2007 by OpenMoko, Inc.
+ * Author: Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>,
+ * 	   Stefan Schmidt <stefan-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>
+ * All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#include <linux/spi/spi.h>
+
+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,
+
+	/* VGA */
+	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,
+
+	/* QVGA */
+	JBT_REG_BLANK_CONTROL_QVGA	= 0xd6,
+	JBT_REG_BLANK_TH_TV_QVGA	= 0xd7,
+	JBT_REG_CKV_ON_OFF_QVGA		= 0xd8,
+	JBT_REG_CKV_1_2_QVGA		= 0xd9,
+	JBT_REG_OEV_TIMING_QVGA		= 0xde,
+	JBT_REG_ASW_TIMING_1_QVGA	= 0xdf,
+	JBT_REG_ASW_TIMING_2_QVGA	= 0xe0,
+
+
+	JBT_REG_HCLOCK_VGA		= 0xec,
+	JBT_REG_HCLOCK_QVGA		= 0xed,
+
+};
+
+enum jbt_state {
+	JBT_STATE_DEEP_STANDBY,
+	JBT_STATE_SLEEP,
+	JBT_STATE_NORMAL,
+	JBT_STATE_QVGA_NORMAL,
+};
+
+static const char *jbt_state_names[] = {
+	[JBT_STATE_DEEP_STANDBY]	= "deep-standby",
+	[JBT_STATE_SLEEP]		= "sleep",
+	[JBT_STATE_NORMAL]		= "normal",
+	[JBT_STATE_QVGA_NORMAL]		= "qvga-normal",
+};
+
+struct jbt_info {
+	enum jbt_state state, last_state;
+	struct spi_device *spi_dev;
+	struct mutex lock;		/* protects tx_buf and reg_cache */
+	u16 tx_buf[8];
+	u16 reg_cache[0xEE];
+};
+
+#define JBT_COMMAND	0x000
+#define JBT_DATA	0x100
+
+static int jbt_reg_write_nodata(struct jbt_info *jbt, u8 reg)
+{
+	int rc;
+
+	mutex_lock(&jbt->lock);
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+	rc = spi_write(jbt->spi_dev, (u8 *)jbt->tx_buf,
+		       1*sizeof(u16));
+	if (rc == 0)
+		jbt->reg_cache[reg] = 0;
+
+	mutex_unlock(&jbt->lock);
+
+	return rc;
+}
+
+
+static int jbt_reg_write(struct jbt_info *jbt, u8 reg, u8 data)
+{
+	int rc;
+
+	mutex_lock(&jbt->lock);
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+	jbt->tx_buf[1] = JBT_DATA | data;
+	rc = spi_write(jbt->spi_dev, (u8 *)jbt->tx_buf,
+		       2*sizeof(u16));
+	if (rc == 0)
+		jbt->reg_cache[reg] = data;
+	
+	mutex_unlock(&jbt->lock);
+
+	return rc;
+}
+
+static int jbt_reg_write16(struct jbt_info *jbt, u8 reg, u16 data)
+{
+	int rc;
+
+	mutex_lock(&jbt->lock);
+
+	jbt->tx_buf[0] = JBT_COMMAND | reg;
+	jbt->tx_buf[1] = JBT_DATA | (data >> 8);
+	jbt->tx_buf[2] = JBT_DATA | (data & 0xff);
+
+	rc = spi_write(jbt->spi_dev, (u8 *)jbt->tx_buf,
+		       3*sizeof(u16));
+	if (rc == 0)
+		jbt->reg_cache[reg] = data;
+
+	mutex_unlock(&jbt->lock);
+
+	return rc;
+}
+
+static int jbt_init_regs(struct jbt_info *jbt, int qvga)
+{
+	int rc;
+
+	dev_dbg(&jbt->spi_dev->dev, "entering %cVGA mode\n", qvga ? 'Q' : ' ');
+
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE1, 0x01);
+	rc |= jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE2, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_RGB_FORMAT, 0x60);
+	rc |= jbt_reg_write(jbt, JBT_REG_DRIVE_SYSTEM, 0x10);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_OP, 0x56);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_MODE, 0x33);
+	rc |= jbt_reg_write(jbt, JBT_REG_BOOSTER_FREQ, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_OPAMP_SYSCLK, 0x02);
+	rc |= jbt_reg_write(jbt, JBT_REG_VSC_VOLTAGE, 0x2b);
+	rc |= jbt_reg_write(jbt, JBT_REG_VCOM_VOLTAGE, 0x40);
+	rc |= jbt_reg_write(jbt, JBT_REG_EXT_DISPL, 0x03);
+	rc |= jbt_reg_write(jbt, JBT_REG_DCCLK_DCEV, 0x04);
+	rc |= jbt_reg_write(jbt, JBT_REG_ASW_SLEW, 0x02);
+	rc |= jbt_reg_write(jbt, JBT_REG_DUMMY_DISPLAY, 0x00);
+
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_A, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_B, 0x11);
+	rc |= jbt_reg_write(jbt, JBT_REG_SLEEP_OUT_FR_C, 0x11);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020);
+	rc |= jbt_reg_write16(jbt, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0);
+
+	rc |= jbt_reg_write16(jbt, JBT_REG_GAMMA1_FINE_1, 0x5533);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_FINE_2, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_INCLINATION, 0x00);
+	rc |= jbt_reg_write(jbt, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00);
+
+	if (!qvga) {
+		rc |= jbt_reg_write16(jbt, JBT_REG_HCLOCK_VGA, 0x1f0);
+		rc |= jbt_reg_write(jbt, JBT_REG_BLANK_CONTROL, 0x02);
+		rc |= jbt_reg_write16(jbt, JBT_REG_BLANK_TH_TV, 0x0804);
+
+		rc |= jbt_reg_write(jbt, JBT_REG_CKV_ON_OFF, 0x01);
+		rc |= jbt_reg_write16(jbt, JBT_REG_CKV_1_2, 0x0000);
+
+		rc |= jbt_reg_write16(jbt, JBT_REG_OEV_TIMING, 0x0d0e);
+		rc |= jbt_reg_write16(jbt, JBT_REG_ASW_TIMING_1, 0x11a4);
+		rc |= jbt_reg_write(jbt, JBT_REG_ASW_TIMING_2, 0x0e);
+	} else {
+		rc |= jbt_reg_write16(jbt, JBT_REG_HCLOCK_QVGA, 0x00ff);
+		rc |= jbt_reg_write(jbt, JBT_REG_BLANK_CONTROL_QVGA, 0x02);
+		rc |= jbt_reg_write16(jbt, JBT_REG_BLANK_TH_TV_QVGA, 0x0804);
+
+		rc |= jbt_reg_write(jbt, JBT_REG_CKV_ON_OFF_QVGA, 0x01);
+		rc |= jbt_reg_write16(jbt, JBT_REG_CKV_1_2_QVGA, 0x0008);
+
+		rc |= jbt_reg_write16(jbt, JBT_REG_OEV_TIMING_QVGA, 0x050a);
+		rc |= jbt_reg_write16(jbt, JBT_REG_ASW_TIMING_1_QVGA, 0x0a19);
+		rc |= jbt_reg_write(jbt, JBT_REG_ASW_TIMING_2_QVGA, 0x0a);
+	}
+
+	return rc ? -EIO : 0;
+}
+
+static int standby_to_sleep(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* three times command zero */
+	rc = jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+	rc |= jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+	rc |= jbt_reg_write_nodata(jbt, 0x00);
+	mdelay(1);
+
+	/* deep standby out */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x17);
+
+	return rc ? -EIO : 0;
+}
+
+static int sleep_to_normal(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* RGB I/F on, RAM wirte off, QVGA through, SIGCON enable */
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE, 0x80);
+
+	/* Quad mode off */
+	rc |= jbt_reg_write(jbt, JBT_REG_QUAD_RATE, 0x00);
+
+	/* AVDD on, XVDD on */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x16);
+
+	/* Output control */
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+	/* Sleep mode off */
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_OUT);
+
+	/* initialize register set */
+	rc |= jbt_init_regs(jbt, 0);
+
+	return rc ? -EIO : 0;
+}
+
+static int sleep_to_qvga_normal(struct jbt_info *jbt)
+{
+	int rc;
+
+	/* RGB I/F on, RAM wirte off, QVGA through, SIGCON enable */
+	rc = jbt_reg_write(jbt, JBT_REG_DISPLAY_MODE, 0x81);
+
+	/* Quad mode on */
+	rc |= jbt_reg_write(jbt, JBT_REG_QUAD_RATE, 0x22);
+
+	/* AVDD on, XVDD on */
+	rc |= jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x16);
+
+	/* Output control */
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0xfff9);
+
+	/* Sleep mode off */
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_OUT);
+
+	/* initialize register set for qvga*/
+	rc |= jbt_init_regs(jbt, 1);
+
+	return rc ? -EIO : 0;
+}
+
+static int normal_to_sleep(struct jbt_info *jbt)
+{
+	int rc;
+
+	rc = jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_OFF);
+	rc |= jbt_reg_write16(jbt, JBT_REG_OUTPUT_CONTROL, 0x8002);
+	rc |= jbt_reg_write_nodata(jbt, JBT_REG_SLEEP_IN);
+
+	return rc ? -EIO : 0;
+}
+
+static int sleep_to_standby(struct jbt_info *jbt)
+{
+	return jbt_reg_write(jbt, JBT_REG_POWER_ON_OFF, 0x00);
+}
+
+/* frontend function */
+int jbt6k74_enter_state(struct jbt_info *jbt, enum jbt_state new_state)
+{
+	int rc = -EINVAL;
+
+	dev_dbg(&jbt->spi_dev->dev, "entering (old_state=%u, "
+		"new_state=%u)\n", jbt->state, new_state);
+
+	switch (jbt->state) {
+	case JBT_STATE_DEEP_STANDBY:
+		switch (new_state) {
+		case JBT_STATE_DEEP_STANDBY:
+			rc = 0;
+			break;
+		case JBT_STATE_SLEEP:
+			rc = standby_to_sleep(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			/* first transition into sleep */
+			rc = standby_to_sleep(jbt);
+			/* then transition into normal */
+			rc |= sleep_to_normal(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			/* first transition into sleep */
+			rc = standby_to_sleep(jbt);
+			/* then transition into normal */
+			rc |= sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_SLEEP:
+		switch (new_state) {
+		case JBT_STATE_SLEEP:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			rc = sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			rc = sleep_to_normal(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			rc = sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_NORMAL:
+		switch (new_state) {
+		case JBT_STATE_NORMAL:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* then transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_SLEEP:
+			rc = normal_to_sleep(jbt);
+			break;
+		case JBT_STATE_QVGA_NORMAL:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* second transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			/* third transition into sleep */
+			rc |= standby_to_sleep(jbt);
+			/* fourth transition into normal */
+			rc |= sleep_to_qvga_normal(jbt);
+			break;
+		}
+		break;
+	case JBT_STATE_QVGA_NORMAL:
+		switch (new_state) {
+		case JBT_STATE_QVGA_NORMAL:
+			rc = 0;
+			break;
+		case JBT_STATE_DEEP_STANDBY:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* then transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			break;
+		case JBT_STATE_SLEEP:
+			rc = normal_to_sleep(jbt);
+			break;
+		case JBT_STATE_NORMAL:
+			/* first transition into sleep */
+			rc = normal_to_sleep(jbt);
+			/* second transition into deep standby */
+			rc |= sleep_to_standby(jbt);
+			/* third transition into sleep */
+			rc |= standby_to_sleep(jbt);
+			/* fourth transition into normal */
+			rc |= sleep_to_normal(jbt);
+			break;
+		}
+		break;
+	}
+	if (rc == 0)
+		jbt->state = new_state;
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(jbt6k74_enter_state);
+
+int jbt6k74_display_onoff(struct jbt_info *jbt, int on)
+{
+	if (on)
+		return jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_ON);
+	else
+		return jbt_reg_write_nodata(jbt, JBT_REG_DISPLAY_OFF);
+}
+EXPORT_SYMBOL_GPL(jbt6k74_display_onoff);
+
+static ssize_t state_read(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+
+	if (jbt->state >= ARRAY_SIZE(jbt_state_names))
+		return -EIO;
+
+	return sprintf(buf, "%s\n", jbt_state_names[jbt->state]);
+}
+
+static ssize_t state_write(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+	int i, rc;
+
+	for (i = 0; i < ARRAY_SIZE(jbt_state_names); i++) {
+		if (!strncmp(buf, jbt_state_names[i],
+			     strlen(jbt_state_names[i]))) {
+			rc = jbt6k74_enter_state(jbt, i);
+			if (rc)
+				return rc;
+			switch (i) {
+			case JBT_STATE_NORMAL:
+			case JBT_STATE_QVGA_NORMAL:
+				/* Enable display again after deep-standby */
+				rc = jbt6k74_display_onoff(jbt, 1);
+				if (rc)
+					return rc;
+				break;
+			default:
+				break;
+			}
+			return count;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static DEVICE_ATTR(state, 0644, state_read, state_write);
+
+static int reg_by_string(const char *name)
+{
+	if (!strcmp(name, "gamma_fine1"))
+		return JBT_REG_GAMMA1_FINE_1;
+	else if (!strcmp(name, "gamma_fine2"))
+		return JBT_REG_GAMMA1_FINE_2;
+	else if (!strcmp(name, "gamma_inclination"))
+		return JBT_REG_GAMMA1_INCLINATION;
+	else
+		return JBT_REG_GAMMA1_BLUE_OFFSET;
+}
+
+static ssize_t gamma_read(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+	int reg = reg_by_string(attr->attr.name);
+	u16 val;
+
+	mutex_lock(&jbt->lock);
+	val = jbt->reg_cache[reg];
+	mutex_unlock(&jbt->lock);
+
+	return sprintf(buf, "0x%04x\n", val);
+}
+
+static ssize_t gamma_write(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct jbt_info *jbt = dev_get_drvdata(dev);
+	int reg = reg_by_string(attr->attr.name);
+	unsigned long val = simple_strtoul(buf, NULL, 10);
+
+	jbt_reg_write(jbt, reg, val & 0xff);
+
+	return count;
+}
+
+static DEVICE_ATTR(gamma_fine1, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_fine2, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_inclination, 0644, gamma_read, gamma_write);
+static DEVICE_ATTR(gamma_blue_offset, 0644, gamma_read, gamma_write);
+
+static struct attribute *jbt_sysfs_entries[] = {
+	&dev_attr_state.attr,
+	&dev_attr_gamma_fine1.attr,
+	&dev_attr_gamma_fine2.attr,
+	&dev_attr_gamma_inclination.attr,
+	&dev_attr_gamma_blue_offset.attr,
+	NULL,
+};
+
+static struct attribute_group jbt_attr_group = {
+	.name	= NULL,
+	.attrs	= jbt_sysfs_entries,
+};
+
+/* linux device model infrastructure */
+
+static int __devinit jbt_probe(struct spi_device *spi)
+{
+	int rc;
+	struct jbt_info *jbt;
+
+	/* the controller doesn't have a MISO pin; we can't do detection */
+
+	spi->mode = SPI_CPOL | SPI_CPHA;
+	spi->bits_per_word = 9;
+
+	rc = spi_setup(spi);
+	if (rc < 0) {
+		dev_err(&spi->dev,
+			"error during spi_setup of jbt6k74 driver\n");
+		return rc;
+	}
+
+	jbt = kzalloc(sizeof(*jbt), GFP_KERNEL);
+	if (!jbt)
+		return -ENOMEM;
+
+	jbt->spi_dev = spi;
+	jbt->state = JBT_STATE_DEEP_STANDBY;
+
+	dev_set_drvdata(&spi->dev, jbt);
+
+	rc = jbt6k74_enter_state(jbt, JBT_STATE_NORMAL);
+	if (rc < 0) {
+		dev_err(&spi->dev, "cannot enter NORMAL state\n");
+		goto err_free_drvdata;
+	}
+
+	rc = jbt6k74_display_onoff(jbt, 1);
+	if (rc < 0) {
+		dev_err(&spi->dev, "cannot switch display on\n");
+		goto err_standby;
+	}
+
+	rc = sysfs_create_group(&spi->dev.kobj, &jbt_attr_group);
+	if (rc < 0) {
+		dev_err(&spi->dev, "cannot create sysfs group\n");
+		goto err_off;
+	}
+
+	return 0;
+
+err_off:
+	jbt6k74_display_onoff(jbt, 0);
+err_standby:
+	jbt6k74_enter_state(jbt, JBT_STATE_DEEP_STANDBY);
+err_free_drvdata:
+	dev_set_drvdata(&spi->dev, NULL);
+	kfree(jbt);
+
+	return rc;
+}
+
+static int __devexit jbt_remove(struct spi_device *spi)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	/* We don't want to switch off the display in case the user
+	 * accidentially onloads the module (whose use count normally is 0) */
+
+	sysfs_remove_group(&spi->dev.kobj, &jbt_attr_group);
+	dev_set_drvdata(&spi->dev, NULL);
+	kfree(jbt);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int jbt_suspend(struct spi_device *spi, pm_message_t state)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	/* Save mode for resume */
+	jbt->last_state = jbt->state;
+	jbt6k74_enter_state(jbt, JBT_STATE_DEEP_STANDBY);
+
+	return 0;
+}
+
+static int jbt_resume(struct spi_device *spi)
+{
+	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
+
+	jbt6k74_enter_state(jbt, jbt->last_state);
+
+	switch (jbt->last_state) {
+	case JBT_STATE_NORMAL:
+	case JBT_STATE_QVGA_NORMAL:
+		jbt6k74_display_onoff(jbt, 1);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+#else
+#define jbt_suspend	NULL
+#define jbt_resume	NULL
+#endif
+
+static struct spi_driver jbt6k74_driver = {
+	.driver = {
+		.name	= "jbt6k74",
+		.owner	= THIS_MODULE,
+	},
+
+	.probe	 = jbt_probe,
+	.remove	 = __devexit_p(jbt_remove),
+	.suspend = jbt_suspend,
+	.resume	 = jbt_resume,
+};
+
+static int __init jbt_init(void)
+{
+	return spi_register_driver(&jbt6k74_driver);
+}
+
+static void __exit jbt_exit(void)
+{
+	spi_unregister_driver(&jbt6k74_driver);
+}
+
+MODULE_DESCRIPTION("SPI driver for tpo JBT6K74-AS LCM control interface");
+MODULE_AUTHOR("Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>");
+MODULE_LICENSE("GPL");
+
+module_init(jbt_init);
+module_exit(jbt_exit);
Index: linux-2.6/arch/arm/mach-s3c2410/Kconfig
===================================================================
--- linux-2.6.orig/arch/arm/mach-s3c2410/Kconfig
+++ linux-2.6/arch/arm/mach-s3c2410/Kconfig
@@ -107,6 +107,7 @@
 config MACH_QT2410
 	bool "QT2410"
 	select CPU_S3C2410
+	select DISPLAY_JBT6K74
 	help
 	   Say Y here if you are using the Armzone QT2410
 
-- 
- Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[David Brownell]

> > Since this is a LCD controller, it should be living in the
> > drivers/video directory somewhere.   
> 
> Ok, no problem.
> 
> > Maybe in "backlight" and depending on LCD_CLASS_DEVICE like the
> > LTV350QV code.  (Which is not dissimilar to this LCM driver, although
> > that Samsung panel is much simpler.)
> 
> Well, the jbt6k74 driver does many things. But backlight switching is
> not part of it :) the backlight is completely independent and machine
> dependent.

The "backlight" directory is a bit misnamed, since it also holds
the LCD_CLASS_DEVICE stuff which is just "lowlevel LCD controls".
You'll observe the LTV driver controls power and gamma just like
this JBT driver does...

Of course there aren't yet many "lowlevel LCD controls" drivers,
and this driver might suggest revisiting that class.  There's one
LCD controller I know of with integral PWM contrast control, which
might help motivate relocating that class code.  How about a new
"lcm" directory?  :)


> > > +	default:
> > > +		return -1;
> > 
> > Why return "-EPERM" rather than "0"?  Are you trying to
> > prevent use of this driver on systems that support hibernation?
> > 
> 
> nope. I just didn't know what other cases there were, and since none of
> them are implemented, I thought it's better to refuse them than to claim
> we support something that was never tested.
> 
> I'm now following the suggestion of pm.h, i.e. treat all cases like
> SUSPEND.  Do you agree this sounds like the right thing to do?

Yes.  If it's not, then most Linux device drivers are
broken, and at least this one would be compatible!


> > > +static int jbt_resume(struct spi_device *spi)
> > > +{
> > > +	struct jbt_info *jbt = dev_get_drvdata(&spi->dev);
> > > +
> > > +	jbt6k74_enter_state(jbt, jbt->last_state);
> > > +	jbt6k74_display_onoff(jbt, 1);
> > 
> > I suspect that if the last state isn't DEEP_STANDBY this
> > should do nothing.  
> 
> Erm, you mean if the last state _is_ DEEP_STANDBY or SLEEP, it should do
> nothing?  Thansk anyway for pointing it out, there definitely was a bug.

I just meant that in a series of resume calls (matching the series of
suspend calls that will happen with hibernation), it's probably best
to do nothing if the device isn't in a suspended state.  And if the
system suspended while this device was in any kind of runtime suspend,
it should normally resume into that same runtime suspend state.


> > Remember that during hibernation, this can be called multiple times.
> 
> And how would this be a problem? Sorry, I don't get it :/

Waking up a power-aware system normally shouldn't force every device
into its highest power state.  I don't know how much runtime PM is
done on the systems you're working with, but I was assuming you do at
least some of it as a way to minimize battery drain.



> *************************
> 
> This driver adds support for the SPI-based control interface of the LCM (LCD
> Panel) found on the FIC GTA01 hardware.  
> 
> The specific panel in this hardware is a TPO TD028TTEC1, but the driver should
> be able to drive any other diplay based on the JBT6K74-AS controller ASIC.
> 
> Signed-off-by: Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>

Looks much better IMO.  I still think that since this exposes
"low level controls" it should depend on LCD_CLASS_DEVICE, since
that's allegedly what that infrastructure is there for.  But I
expect that feedback would be resolved by folk maintaining that
part of the driver stack.

Also:


> +/* frontend function */
> +int jbt6k74_enter_state(struct jbt_info *jbt, enum jbt_state new_state)
> +{
> +	int rc = -EINVAL;
> +
> +	...
> +			/* first transition into sleep */
> +			rc = standby_to_sleep(jbt);
> +			/* then transition into normal */
> +			rc |= sleep_to_normal(jbt);
> +			break;
> +	...
> +	return rc;
> +}

This routine still has that error handling bug on most paths.

- Dave


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver

[Richard Purdie]

On Thu, 2007-12-20 at 14:31 -0800, David Brownell wrote:
> The "backlight" directory is a bit misnamed, since it also holds
> the LCD_CLASS_DEVICE stuff which is just "lowlevel LCD controls".
> You'll observe the LTV driver controls power and gamma just like
> this JBT driver does...
> 
> Of course there aren't yet many "lowlevel LCD controls" drivers,
> and this driver might suggest revisiting that class.  There's one
> LCD controller I know of with integral PWM contrast control, which
> might help motivate relocating that class code.  How about a new
> "lcm" directory?  :)

Backlights and lcd controllers mainly reside in drivers/video/backlight
although there are a number of backlight drivers spread about the tree.
I'm effectively the maintainer of the lcd class alongside backlights
since I've tried to keep the two roughly in sync style wise. 

> Looks much better IMO.  I still think that since this exposes
> "low level controls" it should depend on LCD_CLASS_DEVICE, since
> that's allegedly what that infrastructure is there for.  But I
> expect that feedback would be resolved by folk maintaining that
> part of the driver stack.

I'm not sure the LCD class would be very helpful to this driver. As you
say, this really suggests shortcomings of the LCD class but with only
one LCD driver in mainline changing that class to work better for LCDs
is possible.

The main question is whether the functionality here is suitably generic
to turn into a class? Personally I'm not sure it is but I'm open to
opinions (and patches).

Cheers,

Richard






-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: display/lcm/backlight/framebuffer interaction (was [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver)

[Harald Welte]

Hi RP, David, thanks for your continuos comments,

On Fri, Dec 21, 2007 at 12:01:11AM +0000, Richard Purdie wrote:
> On Thu, 2007-12-20 at 14:31 -0800, David Brownell wrote:
> > The "backlight" directory is a bit misnamed, since it also holds
> > the LCD_CLASS_DEVICE stuff which is just "lowlevel LCD controls".
> > You'll observe the LTV driver controls power and gamma just like
> > this JBT driver does...
> > 
> > Of course there aren't yet many "lowlevel LCD controls" drivers,
> > and this driver might suggest revisiting that class.  There's one
> > LCD controller I know of with integral PWM contrast control, which
> > might help motivate relocating that class code.  How about a new
> > "lcm" directory?  :)
> 
> Backlights and lcd controllers mainly reside in drivers/video/backlight
> although there are a number of backlight drivers spread about the tree.

well, there alsi is drivers/video/display now, which is where I have put
my driver now.  However, I'm not using the DISPLAY_CLASS, since it seems
to deal mainly with contrast settings.

> I'm not sure the LCD class would be very helpful to this driver. As you
> say, this really suggests shortcomings of the LCD class but with only
> one LCD driver in mainline changing that class to work better for LCDs
> is possible.
> 
> The main question is whether the functionality here is suitably generic
> to turn into a class? Personally I'm not sure it is but I'm open to
> opinions (and patches).

I think the power states and configuration parameters of different TFT
panels (in embedded devices) are fairly device-specific.  Even the power
states of different panels can differ a lot.  So I don't know how much
common abstrction is possible to implement.

One thing that we still need for the Neo1973 is some kind of interaction
betwee the display driver and the framebuffer driver.  The issue is that
if we change the framebuffer resolution from VGA to QVGA, the display
driver needs to alter the timing configuration of the display ASIC.

I was thinking of something abstact like a notifier_chain for resolution
changes.  This way the display driver (and other potential users) can
get informed about resolution changes. 

And then, after all, we have the issue of backlight, display and
framebuffer all together.  If you want to blank the screen, you would
want to switch off the backlight, but also put the display into a
lower-power mode.  Right now we're not doing any of this in the kenrel,
but I believe we definitely should.  Any suggestions on that?

Cheers,
-- 
- Harald Welte <laforge-4Bgg8jF3iZdg9hUCZPvPmw@public.gmane.org>          	        http://openmoko.org/
============================================================================
Software for the world's first truly open Free Software mobile phone

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: display/lcm/backlight/framebuffer interaction (was [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver)

[Richard Purdie]

On Fri, 2007-12-21 at 10:39 +0100, Harald Welte wrote:
> On Fri, Dec 21, 2007 at 12:01:11AM +0000, Richard Purdie wrote:
> > Backlights and lcd controllers mainly reside in drivers/video/backlight
> > although there are a number of backlight drivers spread about the tree.
> 
> well, there alsi is drivers/video/display now, which is where I have put
> my driver now.  However, I'm not using the DISPLAY_CLASS, since it seems
> to deal mainly with contrast settings.
> 
> > I'm not sure the LCD class would be very helpful to this driver. As you
> > say, this really suggests shortcomings of the LCD class but with only
> > one LCD driver in mainline changing that class to work better for LCDs
> > is possible.
> > 
> > The main question is whether the functionality here is suitably generic
> > to turn into a class? Personally I'm not sure it is but I'm open to
> > opinions (and patches).
> 
> I think the power states and configuration parameters of different TFT
> panels (in embedded devices) are fairly device-specific.  Even the power
> states of different panels can differ a lot.  So I don't know how much
> common abstrction is possible to implement.

Agreed, I'm not sure how much common ground there is either.

> One thing that we still need for the Neo1973 is some kind of interaction
> betwee the display driver and the framebuffer driver.  The issue is that
> if we change the framebuffer resolution from VGA to QVGA, the display
> driver needs to alter the timing configuration of the display ASIC.

I had this problem with the Zaurus and ended up with
arch/arm/mach-pxa/corgi_lcd.c. It has two different mechanisms for
connecting into w100fb and pxafb. I would like to see a common method
for doing this and would be happy to change the corgi code to some kind
of common framework. A problem, at least with w100fb was that the
ordering of the calls to the LCD and w100 were quite critical, one wrong
move and it doesn't work...

> I was thinking of something abstact like a notifier_chain for resolution
> changes.  This way the display driver (and other potential users) can
> get informed about resolution changes. 

In the w100 case we ended up with change(), suspend() and resume()
function pointers. A call notifier could do the same job though. For
pxafb its just a case of LCD on/off...

> And then, after all, we have the issue of backlight, display and
> framebuffer all together.  If you want to blank the screen, you would
> want to switch off the backlight, but also put the display into a
> lower-power mode.  Right now we're not doing any of this in the kenrel,
> but I believe we definitely should.  Any suggestions on that?

The backlight class does have a hook into the framebuffer blank call
notifier chain (see drivers/video/backlight.c:fb_notifier_callback())
and this works very effectively.

The LCD class does also power down when the display is blanked through a
similar mechanism. One tricky problem here is working out which blank
call belongs to which display. In the end I left this problem to the
driver though the "check_fb" function pointer. On a device with a single
display there is no problem...

Adding a resolution change notifier chain does sound good, the LCD class
could then at least become useful for turning the LCD on/off and
blanking + resolution change notification.

Cheers,

Richard





-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/