* [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver @ 2007-12-18 11:08 Harald Welte [not found] ` <20071218110800.GN29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Harald Welte @ 2007-12-18 11:08 UTC (permalink / raw) To: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20071218110800.GN29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <20071218110800.GN29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> @ 2007-12-18 23:59 ` David Brownell [not found] ` <20071218235954.5351225EA51-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> 2007-12-19 21:16 ` David Brownell 1 sibling, 1 reply; 10+ messages in thread From: David Brownell @ 2007-12-18 23:59 UTC (permalink / raw) To: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW, laforge-4Bgg8jF3iZdg9hUCZPvPmw Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20071218235954.5351225EA51-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <20071218235954.5351225EA51-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> @ 2007-12-19 9:57 ` Harald Welte [not found] ` <20071219095721.GU29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Harald Welte @ 2007-12-19 9:57 UTC (permalink / raw) To: David Brownell Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20071219095721.GU29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <20071219095721.GU29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> @ 2007-12-19 20:04 ` David Brownell 0 siblings, 0 replies; 10+ messages in thread From: David Brownell @ 2007-12-19 20:04 UTC (permalink / raw) To: Harald Welte Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <20071218110800.GN29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 2007-12-18 23:59 ` David Brownell @ 2007-12-19 21:16 ` David Brownell [not found] ` <200712191316.45435.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 1 sibling, 1 reply; 10+ messages in thread From: David Brownell @ 2007-12-19 21:16 UTC (permalink / raw) To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f Cc: Harald Welte, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <200712191316.45435.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <200712191316.45435.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2007-12-20 15:06 ` Harald Welte [not found] ` <20071220150659.GP29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Harald Welte @ 2007-12-20 15:06 UTC (permalink / raw) To: David Brownell Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20071220150659.GP29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <20071220150659.GP29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> @ 2007-12-20 22:31 ` David Brownell [not found] ` <200712201431.58932.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: David Brownell @ 2007-12-20 22:31 UTC (permalink / raw) To: Harald Welte Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW > > 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <200712201431.58932.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]
* Re: [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver [not found] ` <200712201431.58932.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> @ 2007-12-21 0:01 ` Richard Purdie [not found] ` <1198195271.4651.52.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Richard Purdie @ 2007-12-21 0:01 UTC (permalink / raw) To: David Brownell Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Harald Welte, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1198195271.4651.52.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>]
* Re: display/lcm/backlight/framebuffer interaction (was [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver) [not found] ` <1198195271.4651.52.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> @ 2007-12-21 9:39 ` Harald Welte [not found] ` <20071221093934.GO6625-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 0 siblings, 1 reply; 10+ messages in thread From: Harald Welte @ 2007-12-21 9:39 UTC (permalink / raw) To: Richard Purdie Cc: David Brownell, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <20071221093934.GO6625-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org>]
* Re: display/lcm/backlight/framebuffer interaction (was [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver) [not found] ` <20071221093934.GO6625-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> @ 2007-12-21 10:50 ` Richard Purdie 0 siblings, 0 replies; 10+ messages in thread From: Richard Purdie @ 2007-12-21 10:50 UTC (permalink / raw) To: Harald Welte Cc: David Brownell, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW 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/ ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2007-12-21 10:50 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-12-18 11:08 [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver Harald Welte [not found] ` <20071218110800.GN29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 2007-12-18 23:59 ` David Brownell [not found] ` <20071218235954.5351225EA51-ZcXrCSuhvln6VZ3dlLfH/g4gEjPzgfUyLrfjE7I9kuVHxeISYlDBzl6hYfS7NtTn@public.gmane.org> 2007-12-19 9:57 ` Harald Welte [not found] ` <20071219095721.GU29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 2007-12-19 20:04 ` David Brownell 2007-12-19 21:16 ` David Brownell [not found] ` <200712191316.45435.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2007-12-20 15:06 ` Harald Welte [not found] ` <20071220150659.GP29882-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 2007-12-20 22:31 ` David Brownell [not found] ` <200712201431.58932.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> 2007-12-21 0:01 ` Richard Purdie [not found] ` <1198195271.4651.52.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> 2007-12-21 9:39 ` display/lcm/backlight/framebuffer interaction (was [PATCH 7/12] neo1973: JBT6K74 LCM control interface driver) Harald Welte [not found] ` <20071221093934.GO6625-jI4mzJ+yNOMOwssVsN95jSCwEArCW2h5@public.gmane.org> 2007-12-21 10:50 ` Richard Purdie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).