* [RFC 1/7] staging: fbtft: add lcd register abstraction
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 12:25 ` Dan Carpenter
2015-03-02 10:54 ` [RFC 2/7] staging: fbtft: lcdreg: add i2c support Noralf Trønnes
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add LCD register abstraction for MIPI DCS like controllers. This
hides LCD controller interface implementation details.
When built with debugfs, the register is available to userspace.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/Kconfig | 2 +
drivers/staging/fbtft/Makefile | 6 +-
drivers/staging/fbtft/lcdreg/Kconfig | 4 +
drivers/staging/fbtft/lcdreg/Makefile | 3 +
drivers/staging/fbtft/lcdreg/internal.h | 9 +
drivers/staging/fbtft/lcdreg/lcdreg-core.c | 187 ++++++++++++++++++
drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c | 272 ++++++++++++++++++++++++++
drivers/staging/fbtft/lcdreg/lcdreg.h | 151 ++++++++++++++
8 files changed, 632 insertions(+), 2 deletions(-)
create mode 100644 drivers/staging/fbtft/lcdreg/Kconfig
create mode 100644 drivers/staging/fbtft/lcdreg/Makefile
create mode 100644 drivers/staging/fbtft/lcdreg/internal.h
create mode 100644 drivers/staging/fbtft/lcdreg/lcdreg-core.c
create mode 100644 drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
create mode 100644 drivers/staging/fbtft/lcdreg/lcdreg.h
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 995a910..2d1490f 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -167,3 +167,5 @@ config FB_FLEX
config FB_TFT_FBTFT_DEVICE
tristate "Module to for adding FBTFT devices"
depends on FB_TFT
+
+source "drivers/staging/fbtft/lcdreg/Kconfig"
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index e773f0f..2769421 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -1,8 +1,10 @@
-# Core module
+# Core modules
obj-$(CONFIG_FB_TFT) += fbtft.o
fbtft-y += fbtft-core.o fbtft-sysfs.o fbtft-bus.o fbtft-io.o
-# drivers
+obj-$(CONFIG_LCDREG) += lcdreg/
+
+# Controller drivers
obj-$(CONFIG_FB_TFT_AGM1264K_FL) += fb_agm1264k-fl.o
obj-$(CONFIG_FB_TFT_BD663474) += fb_bd663474.o
obj-$(CONFIG_FB_TFT_HX8340BN) += fb_hx8340bn.o
diff --git a/drivers/staging/fbtft/lcdreg/Kconfig b/drivers/staging/fbtft/lcdreg/Kconfig
new file mode 100644
index 0000000..ec0c097
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/Kconfig
@@ -0,0 +1,4 @@
+config LCDREG
+ tristate
+ depends on GPIOLIB
+ default n
diff --git a/drivers/staging/fbtft/lcdreg/Makefile b/drivers/staging/fbtft/lcdreg/Makefile
new file mode 100644
index 0000000..c9ea774
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_LCDREG) += lcdreg.o
+lcdreg-y += lcdreg-core.o
+lcdreg-$(CONFIG_DEBUG_FS) += lcdreg-debugfs.o
diff --git a/drivers/staging/fbtft/lcdreg/internal.h b/drivers/staging/fbtft/lcdreg/internal.h
new file mode 100644
index 0000000..04035b9
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/internal.h
@@ -0,0 +1,9 @@
+#include "lcdreg.h"
+
+#ifdef CONFIG_DEBUG_FS
+extern void lcdreg_debugfs_init(struct lcdreg *reg);
+extern void lcdreg_debugfs_exit(struct lcdreg *reg);
+#else
+static inline void lcdreg_debugfs_init(struct lcdreg *reg) { }
+static inline void lcdreg_debugfs_exit(struct lcdreg *reg) { }
+#endif
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-core.c b/drivers/staging/fbtft/lcdreg/lcdreg-core.c
new file mode 100644
index 0000000..4a1131c
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/lcdreg-core.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (C) 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "internal.h"
+#include "lcdreg.h"
+
+/**
+ * Write to LCD register
+ *
+ * @reg: LCD register
+ * @regnr: Register number
+ * @transfer: Transfer to write
+ */
+int lcdreg_write(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer)
+{
+ if (!transfer)
+ return -EINVAL;
+
+ if (!transfer->width)
+ transfer->width = reg->def_width;
+
+ dev_dbg(reg->dev,
+ "lcdreg_write: regnr=0x%02x, index=%u, count=%u, width=%u\n",
+ regnr, transfer->index, transfer->count, transfer->width);
+ lcdreg_dbg_transfer_buf(transfer);
+
+ return reg->write(reg, regnr, transfer);
+}
+EXPORT_SYMBOL(lcdreg_write);
+
+/**
+ * Write 32-bit wide buffer to LCD register
+ * @reg: lcdreg
+ * @regnr: Register number
+ * @buf: Buffer to write
+ * @count: Number of words to write
+ */
+int lcdreg_write_buf32(struct lcdreg *reg, unsigned regnr, const u32 *buf,
+ unsigned count)
+{
+ struct lcdreg_transfer tr = {
+ .index = 1,
+ .width = reg->def_width,
+ .count = count,
+ };
+ int i, ret;
+
+ if (!buf)
+ return -EINVAL;
+
+ tr.buf = kmalloc_array(count, sizeof(*buf), GFP_KERNEL);
+ if (!tr.buf)
+ return -ENOMEM;
+
+ if (reg->def_width <= 8)
+ for (i = 0; i < tr.count; i++)
+ ((u8 *)tr.buf)[i] = buf[i];
+ else
+ for (i = 0; i < tr.count; i++)
+ ((u16 *)tr.buf)[i] = buf[i];
+ ret = lcdreg_write(reg, regnr, &tr);
+ kfree(tr.buf);
+
+ return ret;
+}
+EXPORT_SYMBOL(lcdreg_write_buf32);
+
+/**
+ * Read from LCD register
+ *
+ * @reg: LCD register
+ * @regnr: Register number
+ * @transfer: Transfer to read into
+ */
+int lcdreg_read(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer)
+{
+ int ret;
+
+ if (!transfer)
+ return -EINVAL;
+
+ if (!transfer->width)
+ transfer->width = reg->def_width;
+
+ dev_dbg(reg->dev,
+ "lcdreg_read: regnr=0x%02x, index=%u, count=%u, width=%u\n",
+ regnr, transfer->index, transfer->count, transfer->width);
+
+ ret = reg->read(reg, regnr, transfer);
+
+ lcdreg_dbg_transfer_buf(transfer);
+
+ return ret;
+}
+EXPORT_SYMBOL(lcdreg_read);
+
+/**
+ * Read from LCD register into 32-bit wide buffer
+ * @reg: LCD register
+ * @regnr: Register number
+ * @buf: Buffer to read into
+ * @count: Number of words to read
+ */
+int lcdreg_readreg_buf32(struct lcdreg *reg, unsigned regnr, u32 *buf,
+ unsigned count)
+{
+ struct lcdreg_transfer tr = {
+ .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
+ .count = count,
+ };
+ int i, ret;
+
+ if (!buf || !count)
+ return -EINVAL;
+
+ tr.buf = kmalloc_array(count, sizeof(*buf), GFP_KERNEL);
+ if (!tr.buf)
+ return -ENOMEM;
+
+ ret = lcdreg_read(reg, regnr, &tr);
+ if (ret) {
+ kfree(tr.buf);
+ return ret;
+ }
+
+ if (reg->def_width <= 8)
+ for (i = 0; i < count; i++)
+ buf[i] = ((u8 *)tr.buf)[i];
+ else
+ for (i = 0; i < count; i++)
+ buf[i] = ((u16 *)tr.buf)[i];
+ kfree(tr.buf);
+
+ return ret;
+}
+EXPORT_SYMBOL(lcdreg_readreg_buf32);
+
+static void devm_lcdreg_release(struct device *dev, void *res)
+{
+ struct lcdreg *reg = *(struct lcdreg **)res;
+
+ lcdreg_debugfs_exit(reg);
+ mutex_destroy(®->lock);
+}
+
+/**
+ * Device managed lcdreg initialization
+ *
+ * @dev: Device backing the LCD register
+ * @reg: LCD register
+ */
+struct lcdreg *devm_lcdreg_init(struct device *dev, struct lcdreg *reg)
+{
+
+ struct lcdreg **ptr;
+
+ if (!dev || !reg)
+ return ERR_PTR(-EINVAL);
+
+ ptr = devres_alloc(devm_lcdreg_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ *ptr = reg;
+ devres_add(dev, ptr);
+ reg->dev = dev;
+ mutex_init(®->lock);
+ lcdreg_debugfs_init(reg);
+
+ return reg;
+}
+EXPORT_SYMBOL(devm_lcdreg_init);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
new file mode 100644
index 0000000..1cba4c2
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
@@ -0,0 +1,272 @@
+/*
+ * Copyright (C) 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include "lcdreg.h"
+
+static struct dentry *lcdreg_debugfs_root;
+
+static int lcdreg_userbuf_to_u32(const char __user *user_buf, size_t count,
+ u32 *dest, size_t dest_size)
+{
+ char *buf, *start;
+ unsigned long value;
+ int ret = 0;
+ int i;
+
+ buf = kmalloc(count, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ if (copy_from_user(buf, user_buf, count)) {
+ kfree(buf);
+ return -EFAULT;
+ }
+ buf[count] = 0;
+ for (i = 0; i < count; i++)
+ if (buf[i] == ' ' || buf[i] == '\n')
+ buf[i] = 0;
+ i = 0;
+ start = buf;
+ while (start < buf + count) {
+ if (*start == 0) {
+ start++;
+ continue;
+ }
+ if (i == dest_size) {
+ ret = -EFBIG;
+ break;
+ }
+ if (kstrtoul(start, 16, &value)) {
+ ret = -EINVAL;
+ break;
+ }
+ dest[i++] = value;
+ while (*start != 0)
+ start++;
+ };
+ kfree(buf);
+ if (ret)
+ return ret;
+
+ return i ? i : -EINVAL;
+}
+
+static ssize_t lcdreg_debugfs_write_file(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+ int ret;
+ u32 txbuf[128];
+
+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
+ if (ret <= 0)
+ return -EINVAL;
+
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+ lcdreg_lock(reg);
+ ret = lcdreg_write_buf32(reg, txbuf[0], txbuf + 1, ret - 1);
+ lcdreg_unlock(reg);
+
+ return ret ? ret : count;
+}
+
+static const struct file_operations lcdreg_debugfs_write_fops = {
+ .open = simple_open,
+ .write = lcdreg_debugfs_write_file,
+ .llseek = default_llseek,
+};
+
+static ssize_t lcdreg_debugfs_read_wr(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+ struct lcdreg_transfer tr = {
+ .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
+ .width = reg->debugfs_read_width,
+ .count = 1,
+ };
+ u32 txbuf[1];
+ char *buf = NULL;
+ int ret;
+
+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
+ if (ret != 1)
+ return -EINVAL;
+
+ tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
+ if (!tr.buf)
+ return -ENOMEM;
+
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+ lcdreg_lock(reg);
+ ret = lcdreg_read(reg, txbuf[0], &tr);
+ lcdreg_unlock(reg);
+ if (ret)
+ goto error_out;
+
+ if (!reg->debugfs_read_result) {
+ reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
+ if (!reg->debugfs_read_result) {
+ ret = -ENOMEM;
+ goto error_out;
+ }
+ }
+
+ buf = reg->debugfs_read_result;
+
+ switch (tr.width) {
+ case 8:
+ snprintf(buf, 16, "%02x\n", *(u8 *)tr.buf);
+ break;
+ case 16:
+ snprintf(buf, 16, "%04x\n", *(u16 *)tr.buf);
+ break;
+ case 24:
+ case 32:
+ snprintf(buf, 16, "%08x\n", *(u32 *)tr.buf);
+ break;
+ default:
+ ret = -EINVAL;
+ goto error_out;
+ }
+
+error_out:
+ kfree(tr.buf);
+ if (ret) {
+ kfree(reg->debugfs_read_result);
+ reg->debugfs_read_result = NULL;
+ return ret;
+ }
+
+ return count;
+}
+
+static ssize_t lcdreg_debugfs_read_rd(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+
+ if (*ppos < 0 || !count)
+ return -EINVAL;
+
+ if (!reg->debugfs_read_result)
+ return -ENODATA;
+
+ return simple_read_from_buffer(user_buf, count, ppos,
+ reg->debugfs_read_result,
+ strlen(reg->debugfs_read_result));
+}
+
+static const struct file_operations lcdreg_debugfs_read_fops = {
+ .open = simple_open,
+ .read = lcdreg_debugfs_read_rd,
+ .write = lcdreg_debugfs_read_wr,
+ .llseek = default_llseek,
+};
+
+static ssize_t lcdreg_debugfs_reset_wr(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct lcdreg *reg = file->private_data;
+
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+ lcdreg_reset(reg);
+
+ return count;
+}
+
+static const struct file_operations lcdreg_debugfs_reset_fops = {
+ .open = simple_open,
+ .write = lcdreg_debugfs_reset_wr,
+ .llseek = default_llseek,
+};
+
+static int lcdreg_debugfs_readwidth_set(void *data, u64 val)
+{
+ struct lcdreg *reg = data;
+
+ reg->debugfs_read_width = val;
+
+ return 0;
+}
+static int lcdreg_debugfs_readwidth_get(void *data, u64 *val)
+{
+ struct lcdreg *reg = data;
+
+ /*
+ * def_width is not set when lcdreg_debugfs_init() is run, it's
+ * set later by the controller init code. Hence the need for this
+ * late assignment.
+ */
+ if (!reg->debugfs_read_width)
+ reg->debugfs_read_width = reg->def_width;
+
+ *val = reg->debugfs_read_width;
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(lcdreg_debugfs_readwidth_fops,
+ lcdreg_debugfs_readwidth_get,
+ lcdreg_debugfs_readwidth_set, "%llu\n");
+
+void lcdreg_debugfs_init(struct lcdreg *reg)
+{
+ if (IS_ERR_OR_NULL(lcdreg_debugfs_root))
+ return;
+
+ reg->debugfs = debugfs_create_dir(dev_name(reg->dev),
+ lcdreg_debugfs_root);
+ if (!reg->debugfs) {
+ dev_warn(reg->dev, "Failed to create debugfs directory\n");
+ return;
+ }
+
+ debugfs_create_file("read_width", 0440, reg->debugfs, reg,
+ &lcdreg_debugfs_readwidth_fops);
+ debugfs_create_file("read", 0660, reg->debugfs, reg,
+ &lcdreg_debugfs_read_fops);
+ debugfs_create_file("write", 0220, reg->debugfs, reg,
+ &lcdreg_debugfs_write_fops);
+ debugfs_create_file("reset", 0220, reg->debugfs, reg,
+ &lcdreg_debugfs_reset_fops);
+}
+
+void lcdreg_debugfs_exit(struct lcdreg *reg)
+{
+ debugfs_remove_recursive(reg->debugfs);
+}
+
+static int lcdreg_debugfs_module_init(void)
+{
+ lcdreg_debugfs_root = debugfs_create_dir("lcdreg", NULL);
+ if (!lcdreg_debugfs_root)
+ pr_warn("lcdreg: Failed to create debugfs root\n");
+ return 0;
+}
+module_init(lcdreg_debugfs_module_init);
+
+static void lcdreg_debugfs_module_exit(void)
+{
+ debugfs_remove_recursive(lcdreg_debugfs_root);
+}
+module_exit(lcdreg_debugfs_module_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg.h b/drivers/staging/fbtft/lcdreg/lcdreg.h
new file mode 100644
index 0000000..0ddf40e
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/lcdreg.h
@@ -0,0 +1,151 @@
+/*
+ * Copyright (C) 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_LCDREG_H
+#define __LINUX_LCDREG_H
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+
+/**
+ * struct lcdreg_transfer - LCD register transfer
+ *
+ * @index: register index (address)
+ * Known under the following names:
+ * D/C (command=0, data=1)
+ * RS (register selection: index=0, data=1)
+ * D/I (data/index: index=0, data=1)
+ * @buf: data array to transfer
+ * @count: number of items in array
+ * @width: override default regwidth
+ */
+struct lcdreg_transfer {
+ unsigned index;
+ void *buf;
+ unsigned count;
+ unsigned width;
+};
+
+/**
+ * struct lcdreg - interface to LCD register
+ *
+ * @dev: device interface
+ * @lock: mutex for register access locking
+ * @def_width: default register width
+ * @readable: LCD register is readable
+ * @write: write to LCD register
+ * @read: read from LCD register
+ * @reset: reset LCD controller
+ * @quirks: Deviations from the MIPI DBI standard
+ */
+struct lcdreg {
+ struct device *dev;
+ struct mutex lock;
+ unsigned def_width;
+ bool readable;
+
+ int (*write)(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer);
+ int (*read)(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer);
+ void (*reset)(struct lcdreg *reg);
+
+ u64 quirks;
+/* slowdown command (index=0) */
+#define LCDREG_SLOW_INDEX0_WRITE BIT(0)
+/*
+ * The MIPI DBI spec states that D/C should be HIGH during register reading.
+ * However, not all SPI master drivers support cs_change on last transfer and
+ * there are LCD controllers that ignore D/C on read.
+ */
+#define LCDREG_INDEX0_ON_READ BIT(1)
+
+#ifdef CONFIG_DEBUG_FS
+ struct dentry *debugfs;
+ u32 debugfs_read_width;
+ char *debugfs_read_result;
+#endif
+};
+
+struct lcdreg *devm_lcdreg_init(struct device *dev,
+ struct lcdreg *reg);
+extern int lcdreg_write(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer);
+extern int lcdreg_write_buf32(struct lcdreg *reg, unsigned regnr,
+ const u32 *data, unsigned count);
+
+#define lcdreg_writereg(lcdreg, regnr, seq...) \
+({\
+ u32 d[] = { seq };\
+ lcdreg_write_buf32(lcdreg, regnr, d, ARRAY_SIZE(d));\
+})
+
+extern int lcdreg_read(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer);
+extern int lcdreg_readreg_buf32(struct lcdreg *reg, unsigned regnr, u32 *buf,
+ unsigned count);
+
+static inline void lcdreg_reset(struct lcdreg *reg)
+{
+ reg->reset(reg);
+}
+
+static inline void lcdreg_lock(struct lcdreg *reg)
+{
+ mutex_lock(®->lock);
+}
+
+static inline void lcdreg_unlock(struct lcdreg *reg)
+{
+ mutex_unlock(®->lock);
+}
+
+static inline bool lcdreg_is_readable(struct lcdreg *reg)
+{
+ return reg->readable;
+}
+
+static inline unsigned lcdreg_bytes_per_word(unsigned bits_per_word)
+{
+ if (bits_per_word <= 8)
+ return 1;
+ else if (bits_per_word <= 16)
+ return 2;
+ else /* bits_per_word <= 32 */
+ return 4;
+}
+
+#if defined(CONFIG_DYNAMIC_DEBUG)
+
+#define lcdreg_dbg_transfer_buf(transfer) \
+do { \
+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
+ \
+ print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
+ groupsize, transfer->buf, len, false); \
+} while (0)
+
+#elif defined(DEBUG)
+
+#define lcdreg_dbg_transfer_buf(transfer) \
+do { \
+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
+ \
+ print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
+ groupsize, transfer->buf, len, false); \
+} while (0)
+#else
+
+#define lcdreg_dbg_transfer_buf(transfer)
+
+#endif /* DEBUG || CONFIG_DYNAMIC_DEBUG */
+
+#endif /* __LINUX_LCDREG_H */
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC 1/7] staging: fbtft: add lcd register abstraction
2015-03-02 10:54 ` [RFC 1/7] staging: fbtft: add lcd register abstraction Noralf Trønnes
@ 2015-03-02 12:25 ` Dan Carpenter
2015-03-03 12:19 ` Noralf Trønnes
0 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2015-03-02 12:25 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: gregkh, driverdev-devel
On Mon, Mar 02, 2015 at 11:54:23AM +0100, Noralf Trønnes wrote:
> diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
> new file mode 100644
> index 0000000..1cba4c2
> --- /dev/null
> +++ b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
> @@ -0,0 +1,272 @@
> +/*
> + * Copyright (C) 2015 Noralf Trønnes
> + *
> + * 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.
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#include "lcdreg.h"
> +
> +static struct dentry *lcdreg_debugfs_root;
> +
> +static int lcdreg_userbuf_to_u32(const char __user *user_buf, size_t count,
> + u32 *dest, size_t dest_size)
> +{
> + char *buf, *start;
> + unsigned long value;
> + int ret = 0;
> + int i;
> +
> + buf = kmalloc(count, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf, user_buf, count)) {
> + kfree(buf);
> + return -EFAULT;
> + }
> + buf[count] = 0;
Off-by-one overflow.
> + for (i = 0; i < count; i++)
> + if (buf[i] == ' ' || buf[i] == '\n')
> + buf[i] = 0;
> + i = 0;
> + start = buf;
> + while (start < buf + count) {
> + if (*start == 0) {
Use '\0' instead of 0.
if (*start == '\0')
> + start++;
> + continue;
> + }
> + if (i == dest_size) {
> + ret = -EFBIG;
> + break;
> + }
> + if (kstrtoul(start, 16, &value)) {
> + ret = -EINVAL;
> + break;
> + }
Consider changing this to:
ret = kstrtou32(start, 16, value);
if (ret)
break;
> + dest[i++] = value;
> + while (*start != 0)
> + start++;
This while loop is not needed because of the if statement earlier.
> + };
> + kfree(buf);
> + if (ret)
> + return ret;
> +
> + return i ? i : -EINVAL;
> +}
> +
> +static ssize_t lcdreg_debugfs_write_file(struct file *file,
> + const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct lcdreg *reg = file->private_data;
> + int ret;
> + u32 txbuf[128];
> +
> + ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
> + if (ret <= 0)
> + return -EINVAL;
This function can't return zero so preserve the error code.
if (ret < 0)
return ret;
> +
> + add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> + lcdreg_lock(reg);
> + ret = lcdreg_write_buf32(reg, txbuf[0], txbuf + 1, ret - 1);
> + lcdreg_unlock(reg);
> +
> + return ret ? ret : count;
> +}
> +
> +static const struct file_operations lcdreg_debugfs_write_fops = {
> + .open = simple_open,
> + .write = lcdreg_debugfs_write_file,
> + .llseek = default_llseek,
> +};
> +
> +static ssize_t lcdreg_debugfs_read_wr(struct file *file,
> + const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct lcdreg *reg = file->private_data;
> + struct lcdreg_transfer tr = {
> + .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
> + .width = reg->debugfs_read_width,
> + .count = 1,
> + };
> + u32 txbuf[1];
> + char *buf = NULL;
> + int ret;
> +
> + ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
> + if (ret != 1)
> + return -EINVAL;
> +
> + tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
> + if (!tr.buf)
> + return -ENOMEM;
> +
> + add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +
> + lcdreg_lock(reg);
> + ret = lcdreg_read(reg, txbuf[0], &tr);
> + lcdreg_unlock(reg);
> + if (ret)
> + goto error_out;
> +
> + if (!reg->debugfs_read_result) {
> + reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
> + if (!reg->debugfs_read_result) {
> + ret = -ENOMEM;
> + goto error_out;
> + }
> + }
Allocating here is strange. We only free ->debugfs_read_result on
error so it's sort of buggy as well. Also is it racy if we have
multiple readers and writers at once?
> +
> + buf = reg->debugfs_read_result;
> +
> + switch (tr.width) {
> + case 8:
> + snprintf(buf, 16, "%02x\n", *(u8 *)tr.buf);
> + break;
> + case 16:
> + snprintf(buf, 16, "%04x\n", *(u16 *)tr.buf);
> + break;
> + case 24:
> + case 32:
> + snprintf(buf, 16, "%08x\n", *(u32 *)tr.buf);
> + break;
> + default:
> + ret = -EINVAL;
> + goto error_out;
> + }
> +
> +error_out:
> + kfree(tr.buf);
> + if (ret) {
> + kfree(reg->debugfs_read_result);
> + reg->debugfs_read_result = NULL;
> + return ret;
> + }
> +
> + return count;
It's often cleaner to separate error paths from the success paths. This
is almost an excption because we free (tr.buf) on both paths. But it
would like like this, if you decide to go that way:
kfree(tr.buf);
return 0;
err_free:
kfree(reg->debugfs_read_result);
reg->debugfs_read_result = NULL;
kfree(tr.buf);
return ret;
> +static inline void lcdreg_lock(struct lcdreg *reg)
> +{
> + mutex_lock(®->lock);
> +}
> +
> +static inline void lcdreg_unlock(struct lcdreg *reg)
> +{
> + mutex_unlock(®->lock);
> +}
Don't add abstraction around locking. Everyone hates that. Currently
we don't have any static analysis tools which do cross function locking
analysis correctly. (I am partly to blame for that, sorry). But also
we don't want to have to look it up to see if it a mutex or a spinlock.
> +#if defined(CONFIG_DYNAMIC_DEBUG)
> +
> +#define lcdreg_dbg_transfer_buf(transfer) \
> +do { \
> + int groupsize = lcdreg_bytes_per_word(transfer->width); \
> + size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> + \
> + print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
> + groupsize, transfer->buf, len, false); \
> +} while (0)
> +
> +#elif defined(DEBUG)
> +
> +#define lcdreg_dbg_transfer_buf(transfer) \
> +do { \
> + int groupsize = lcdreg_bytes_per_word(transfer->width); \
> + size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> + \
> + print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
> + groupsize, transfer->buf, len, false); \
> +} while (0)
I don't understand this. Why can't we use print_hex_dump_debug() for
both? In other words:
#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
...
#else
Also can we make it an inline function?
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 1/7] staging: fbtft: add lcd register abstraction
2015-03-02 12:25 ` Dan Carpenter
@ 2015-03-03 12:19 ` Noralf Trønnes
2015-03-03 12:28 ` Dan Carpenter
0 siblings, 1 reply; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-03 12:19 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, driverdev-devel
Den 02.03.2015 13:25, skrev Dan Carpenter:
> On Mon, Mar 02, 2015 at 11:54:23AM +0100, Noralf Trønnes wrote:
>> diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c b/drivers/staging/fbtft/lcdreg/lcdreg-debugfs.c
[snip]
>> +static int lcdreg_userbuf_to_u32(const char __user *user_buf, size_t count,
>> + u32 *dest, size_t dest_size)
>> +{
>> + char *buf, *start;
>> + unsigned long value;
>> + int ret = 0;
>> + int i;
>> +
>> + buf = kmalloc(count, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + if (copy_from_user(buf, user_buf, count)) {
>> + kfree(buf);
>> + return -EFAULT;
>> + }
>> + buf[count] = 0;
> Off-by-one overflow.
Yes
>> + for (i = 0; i < count; i++)
>> + if (buf[i] == ' ' || buf[i] == '\n')
>> + buf[i] = 0;
>> + i = 0;
>> + start = buf;
>> + while (start < buf + count) {
>> + if (*start == 0) {
> Use '\0' instead of 0.
>
> if (*start == '\0')
Ok
>
>> + start++;
>> + continue;
>> + }
>> + if (i == dest_size) {
>> + ret = -EFBIG;
>> + break;
>> + }
>> + if (kstrtoul(start, 16, &value)) {
>> + ret = -EINVAL;
>> + break;
>> + }
> Consider changing this to:
>
> ret = kstrtou32(start, 16, value);
> if (ret)
> break;
>
>> + dest[i++] = value;
>> + while (*start != 0)
>> + start++;
> This while loop is not needed because of the if statement earlier.
I missed that in my many iterations of this debugfs code.
>
>> + };
>> + kfree(buf);
>> + if (ret)
>> + return ret;
>> +
>> + return i ? i : -EINVAL;
>> +}
>> +
>> +static ssize_t lcdreg_debugfs_write_file(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct lcdreg *reg = file->private_data;
>> + int ret;
>> + u32 txbuf[128];
>> +
>> + ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
>> + if (ret <= 0)
>> + return -EINVAL;
> This function can't return zero so preserve the error code.
>
> if (ret < 0)
> return ret;
Ok
[snip]
>> +static ssize_t lcdreg_debugfs_read_wr(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct lcdreg *reg = file->private_data;
>> + struct lcdreg_transfer tr = {
>> + .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
>> + .width = reg->debugfs_read_width,
>> + .count = 1,
>> + };
>> + u32 txbuf[1];
>> + char *buf = NULL;
>> + int ret;
>> +
>> + ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
>> + if (ret != 1)
>> + return -EINVAL;
>> +
>> + tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
>> + if (!tr.buf)
>> + return -ENOMEM;
>> +
>> + add_taint(TAINT_USER, LOCKDEP_STILL_OK);
>> +
>> + lcdreg_lock(reg);
>> + ret = lcdreg_read(reg, txbuf[0], &tr);
>> + lcdreg_unlock(reg);
>> + if (ret)
>> + goto error_out;
>> +
>> + if (!reg->debugfs_read_result) {
>> + reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
>> + if (!reg->debugfs_read_result) {
>> + ret = -ENOMEM;
>> + goto error_out;
>> + }
>> + }
> Allocating here is strange. We only free ->debugfs_read_result on
> error so it's sort of buggy as well. Also is it racy if we have
> multiple readers and writers at once?
I spent some time on figuring out how to do this.
To read a register, first write the register number to the 'read' file.
Then get the result by reading the 'read' file.
So I have to keep the result somewhere, but you're right this leaks
memory. Maybe I can use devm_kmalloc here.
And yes this is racy. I'm not sure if this is a problem.
This is to be used during development or debugging, so
the user should have this under control.
But I'm open to other ways of doing this. This is the best I could
come up with after several iterations on this code.
My other solutions where worse :-)
>> +
>> + buf = reg->debugfs_read_result;
>> +
>> + switch (tr.width) {
>> + case 8:
>> + snprintf(buf, 16, "%02x\n", *(u8 *)tr.buf);
>> + break;
>> + case 16:
>> + snprintf(buf, 16, "%04x\n", *(u16 *)tr.buf);
>> + break;
>> + case 24:
>> + case 32:
>> + snprintf(buf, 16, "%08x\n", *(u32 *)tr.buf);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + goto error_out;
>> + }
>> +
>> +error_out:
>> + kfree(tr.buf);
>> + if (ret) {
>> + kfree(reg->debugfs_read_result);
>> + reg->debugfs_read_result = NULL;
>> + return ret;
>> + }
>> +
>> + return count;
> It's often cleaner to separate error paths from the success paths. This
> is almost an excption because we free (tr.buf) on both paths. But it
> would like like this, if you decide to go that way:
>
> kfree(tr.buf);
> return 0;
>
> err_free:
> kfree(reg->debugfs_read_result);
> reg->debugfs_read_result = NULL;
> kfree(tr.buf);
>
> return ret;
I think it's best to stick to common patterns, and in this case the "cost"
is low. So I'll do this instead.
>> +static inline void lcdreg_lock(struct lcdreg *reg)
>> +{
>> + mutex_lock(®->lock);
>> +}
>> +
>> +static inline void lcdreg_unlock(struct lcdreg *reg)
>> +{
>> + mutex_unlock(®->lock);
>> +}
> Don't add abstraction around locking. Everyone hates that. Currently
> we don't have any static analysis tools which do cross function locking
> analysis correctly. (I am partly to blame for that, sorry). But also
> we don't want to have to look it up to see if it a mutex or a spinlock.
Then I'll surely stay away from doing this :-)
>> +#if defined(CONFIG_DYNAMIC_DEBUG)
>> +
>> +#define lcdreg_dbg_transfer_buf(transfer) \
>> +do { \
>> + int groupsize = lcdreg_bytes_per_word(transfer->width); \
>> + size_t len = min_t(size_t, 32, transfer->count * groupsize); \
>> + \
>> + print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
>> + groupsize, transfer->buf, len, false); \
>> +} while (0)
>> +
>> +#elif defined(DEBUG)
>> +
>> +#define lcdreg_dbg_transfer_buf(transfer) \
>> +do { \
>> + int groupsize = lcdreg_bytes_per_word(transfer->width); \
>> + size_t len = min_t(size_t, 32, transfer->count * groupsize); \
>> + \
>> + print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
>> + groupsize, transfer->buf, len, false); \
>> +} while (0)
> I don't understand this. Why can't we use print_hex_dump_debug() for
> both? In other words:
>
> #if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> ...
> #else
The reason I do this is to be able to switch on/off debugging quickly
when developing.
I have dynamic debug enabled, but it's more cumbersome to use, so I
define DEBUG
when I need this output.
print_hex_dump_debug() doesn't look at DEBUG, it only cares about
DYNAMIC_DEBUG.
But, it's not suited for production code, so I guess I have to use a
script or something
to easily switch dynamic debug on/off instead.
> Also can we make it an inline function?
Yes, that would be much better.
Thanks for your review Dan.
Noralf.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 1/7] staging: fbtft: add lcd register abstraction
2015-03-03 12:19 ` Noralf Trønnes
@ 2015-03-03 12:28 ` Dan Carpenter
0 siblings, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2015-03-03 12:28 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: gregkh, driverdev-devel
On Tue, Mar 03, 2015 at 01:19:52PM +0100, Noralf Trønnes wrote:
> >>+static ssize_t lcdreg_debugfs_read_wr(struct file *file,
> >>+ const char __user *user_buf,
> >>+ size_t count, loff_t *ppos)
> >>+{
> >>+ struct lcdreg *reg = file->private_data;
> >>+ struct lcdreg_transfer tr = {
> >>+ .index = (reg->quirks & LCDREG_INDEX0_ON_READ) ? 0 : 1,
> >>+ .width = reg->debugfs_read_width,
> >>+ .count = 1,
> >>+ };
> >>+ u32 txbuf[1];
> >>+ char *buf = NULL;
> >>+ int ret;
> >>+
> >>+ ret = lcdreg_userbuf_to_u32(user_buf, count, txbuf, ARRAY_SIZE(txbuf));
> >>+ if (ret != 1)
> >>+ return -EINVAL;
> >>+
> >>+ tr.buf = kmalloc(lcdreg_bytes_per_word(tr.width), GFP_KERNEL);
> >>+ if (!tr.buf)
> >>+ return -ENOMEM;
> >>+
> >>+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> >>+
> >>+ lcdreg_lock(reg);
> >>+ ret = lcdreg_read(reg, txbuf[0], &tr);
> >>+ lcdreg_unlock(reg);
> >>+ if (ret)
> >>+ goto error_out;
> >>+
> >>+ if (!reg->debugfs_read_result) {
> >>+ reg->debugfs_read_result = kmalloc(16, GFP_KERNEL);
> >>+ if (!reg->debugfs_read_result) {
> >>+ ret = -ENOMEM;
> >>+ goto error_out;
> >>+ }
> >>+ }
> >Allocating here is strange. We only free ->debugfs_read_result on
> >error so it's sort of buggy as well. Also is it racy if we have
> >multiple readers and writers at once?
> I spent some time on figuring out how to do this.
> To read a register, first write the register number to the 'read' file.
> Then get the result by reading the 'read' file.
> So I have to keep the result somewhere, but you're right this leaks
> memory. Maybe I can use devm_kmalloc here.
> And yes this is racy. I'm not sure if this is a problem.
> This is to be used during development or debugging, so
> the user should have this under control.
>
> But I'm open to other ways of doing this. This is the best I could
> come up with after several iterations on this code.
> My other solutions where worse :-)
We can't do it probe()? We could add another flag to say if we had
written to it. I haven't looked at this closely so whatever you decide
is fine.
> >>+#if defined(CONFIG_DYNAMIC_DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer) \
> >>+do { \
> >>+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
> >>+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+ \
> >>+ print_hex_dump_debug(" buf=", DUMP_PREFIX_NONE, 32, \
> >>+ groupsize, transfer->buf, len, false); \
> >>+} while (0)
> >>+
> >>+#elif defined(DEBUG)
> >>+
> >>+#define lcdreg_dbg_transfer_buf(transfer) \
> >>+do { \
> >>+ int groupsize = lcdreg_bytes_per_word(transfer->width); \
> >>+ size_t len = min_t(size_t, 32, transfer->count * groupsize); \
> >>+ \
> >>+ print_hex_dump(KERN_DEBUG, " buf=", DUMP_PREFIX_NONE, 32, \
> >>+ groupsize, transfer->buf, len, false); \
> >>+} while (0)
> >I don't understand this. Why can't we use print_hex_dump_debug() for
> >both? In other words:
> >
> >#if defined(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> >...
> >#else
> The reason I do this is to be able to switch on/off debugging
> quickly when developing.
> I have dynamic debug enabled, but it's more cumbersome to use, so I
> define DEBUG
> when I need this output.
> print_hex_dump_debug() doesn't look at DEBUG, it only cares about
> DYNAMIC_DEBUG.
> But, it's not suited for production code, so I guess I have to use a
> script or something
> to easily switch dynamic debug on/off instead.
>
Fine fine. Whatever you decide is ok.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC 2/7] staging: fbtft: lcdreg: add i2c support
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
2015-03-02 10:54 ` [RFC 1/7] staging: fbtft: add lcd register abstraction Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 10:54 ` [RFC 3/7] staging: fbtft: add lcd controller abstraction Noralf Trønnes
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add I2C bus support to lcdreg.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/lcdreg/Kconfig | 6 ++
drivers/staging/fbtft/lcdreg/Makefile | 2 +
drivers/staging/fbtft/lcdreg/lcdreg-i2c.c | 129 ++++++++++++++++++++++++++++++
drivers/staging/fbtft/lcdreg/lcdreg.h | 3 +
4 files changed, 140 insertions(+)
create mode 100644 drivers/staging/fbtft/lcdreg/lcdreg-i2c.c
diff --git a/drivers/staging/fbtft/lcdreg/Kconfig b/drivers/staging/fbtft/lcdreg/Kconfig
index ec0c097..8f000b5 100644
--- a/drivers/staging/fbtft/lcdreg/Kconfig
+++ b/drivers/staging/fbtft/lcdreg/Kconfig
@@ -2,3 +2,9 @@ config LCDREG
tristate
depends on GPIOLIB
default n
+
+config LCDREG_I2C
+ tristate
+ depends on I2C
+ select LCDREG
+ default n
diff --git a/drivers/staging/fbtft/lcdreg/Makefile b/drivers/staging/fbtft/lcdreg/Makefile
index c9ea774..1ec65af 100644
--- a/drivers/staging/fbtft/lcdreg/Makefile
+++ b/drivers/staging/fbtft/lcdreg/Makefile
@@ -1,3 +1,5 @@
obj-$(CONFIG_LCDREG) += lcdreg.o
lcdreg-y += lcdreg-core.o
lcdreg-$(CONFIG_DEBUG_FS) += lcdreg-debugfs.o
+
+obj-$(CONFIG_LCDREG_I2C) += lcdreg-i2c.o
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg-i2c.c b/drivers/staging/fbtft/lcdreg/lcdreg-i2c.c
new file mode 100644
index 0000000..297926f
--- /dev/null
+++ b/drivers/staging/fbtft/lcdreg/lcdreg-i2c.c
@@ -0,0 +1,129 @@
+/*
+ * lcdreg I2C support
+ *
+ * Copyright 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "lcdreg.h"
+
+struct lcdreg_i2c {
+ struct lcdreg reg;
+ struct i2c_client *client;
+ struct gpio_desc *reset;
+};
+
+static inline struct lcdreg_i2c *to_lcdreg_i2c(struct lcdreg *reg)
+{
+ return reg ? container_of(reg, struct lcdreg_i2c, reg) : NULL;
+}
+
+static int lcdreg_i2c_send(struct i2c_client *client, unsigned index,
+ void *buf, size_t len)
+{
+ u8 *txbuf;
+ int ret;
+
+ txbuf = kmalloc(1 + len, GFP_KERNEL);
+ if (!txbuf)
+ return -ENOMEM;
+
+ txbuf[0] = index ? 0x40 : 0x80;
+ memcpy(&txbuf[1], buf, len);
+
+ ret = i2c_master_send(client, txbuf, 1 + len);
+ kfree(txbuf);
+
+ return ret < 0 ? ret : 0;
+}
+
+static int lcdreg_i2c_write(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer)
+{
+ struct lcdreg_i2c *i2c = to_lcdreg_i2c(reg);
+ u8 regnr_buf[1] = { regnr };
+ int ret;
+
+ ret = lcdreg_i2c_send(i2c->client, 0, regnr_buf, 1);
+ if (ret)
+ return ret;
+
+ if (!transfer || !transfer->count)
+ return 0;
+
+ if (WARN_ON(transfer->width != 8))
+ return -EINVAL;
+
+ if (!transfer->count)
+ return 0;
+
+ return lcdreg_i2c_send(i2c->client, transfer->index, transfer->buf,
+ transfer->count);
+}
+
+static int lcdreg_i2c_read(struct lcdreg *reg, unsigned regnr,
+ struct lcdreg_transfer *transfer)
+{
+ struct lcdreg_i2c *i2c = to_lcdreg_i2c(reg);
+ int ret;
+
+ if (WARN_ON(regnr != 0 || !transfer || transfer->width != 8))
+ return -EINVAL;
+
+ if (!reg->readable)
+ return -EACCES;
+
+ ret = i2c_master_recv(i2c->client, transfer->buf, transfer->count);
+
+ return ret < 0 ? ret : 0;
+}
+
+static void lcdreg_i2c_reset(struct lcdreg *reg)
+{
+ struct lcdreg_i2c *i2c = to_lcdreg_i2c(reg);
+
+ if (!i2c->reset)
+ return;
+
+ gpiod_set_value_cansleep(i2c->reset, 0);
+ msleep(20);
+ gpiod_set_value_cansleep(i2c->reset, 1);
+ msleep(120);
+}
+
+struct lcdreg *devm_lcdreg_i2c_init(struct i2c_client *client)
+{
+ struct lcdreg_i2c *i2c;
+
+ i2c = devm_kzalloc(&client->dev, sizeof(*i2c), GFP_KERNEL);
+ if (i2c == NULL)
+ return ERR_PTR(-ENOMEM);
+
+ i2c->reg.readable = true;
+ i2c->client = client;
+ i2c->reset = devm_gpiod_get_optional(&client->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(i2c->reset))
+ return ERR_PTR(PTR_ERR(i2c->reset));
+
+ i2c->reg.write = lcdreg_i2c_write;
+ i2c->reg.read = lcdreg_i2c_read;
+ i2c->reg.reset = lcdreg_i2c_reset;
+
+ return devm_lcdreg_init(&client->dev, &i2c->reg);
+}
+EXPORT_SYMBOL(devm_lcdreg_i2c_init);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/fbtft/lcdreg/lcdreg.h b/drivers/staging/fbtft/lcdreg/lcdreg.h
index 0ddf40e..4fa5aea 100644
--- a/drivers/staging/fbtft/lcdreg/lcdreg.h
+++ b/drivers/staging/fbtft/lcdreg/lcdreg.h
@@ -11,6 +11,7 @@
#define __LINUX_LCDREG_H
#include <linux/device.h>
+#include <linux/i2c.h>
#include <linux/mutex.h>
/**
@@ -121,6 +122,8 @@ static inline unsigned lcdreg_bytes_per_word(unsigned bits_per_word)
return 4;
}
+struct lcdreg *devm_lcdreg_i2c_init(struct i2c_client *client);
+
#if defined(CONFIG_DYNAMIC_DEBUG)
#define lcdreg_dbg_transfer_buf(transfer) \
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC 3/7] staging: fbtft: add lcd controller abstraction
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
2015-03-02 10:54 ` [RFC 1/7] staging: fbtft: add lcd register abstraction Noralf Trønnes
2015-03-02 10:54 ` [RFC 2/7] staging: fbtft: lcdreg: add i2c support Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 11:48 ` Dan Carpenter
2015-03-02 10:54 ` [RFC 4/7] staging: fbtft: lcdctrl: add ssd1306 support Noralf Trønnes
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add abstraction for MIPI DCS/DBI like LCD controllers.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/Kconfig | 1 +
drivers/staging/fbtft/Makefile | 1 +
drivers/staging/fbtft/lcdctrl/Kconfig | 3 +
drivers/staging/fbtft/lcdctrl/Makefile | 1 +
drivers/staging/fbtft/lcdctrl/lcdctrl.c | 207 ++++++++++++++++++++++++++++++++
drivers/staging/fbtft/lcdctrl/lcdctrl.h | 104 ++++++++++++++++
6 files changed, 317 insertions(+)
create mode 100644 drivers/staging/fbtft/lcdctrl/Kconfig
create mode 100644 drivers/staging/fbtft/lcdctrl/Makefile
create mode 100644 drivers/staging/fbtft/lcdctrl/lcdctrl.c
create mode 100644 drivers/staging/fbtft/lcdctrl/lcdctrl.h
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 2d1490f..0ac1b41 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -168,4 +168,5 @@ config FB_TFT_FBTFT_DEVICE
tristate "Module to for adding FBTFT devices"
depends on FB_TFT
+source "drivers/staging/fbtft/lcdctrl/Kconfig"
source "drivers/staging/fbtft/lcdreg/Kconfig"
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index 2769421..69f7c2c 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_FB_TFT) += fbtft.o
fbtft-y += fbtft-core.o fbtft-sysfs.o fbtft-bus.o fbtft-io.o
obj-$(CONFIG_LCDREG) += lcdreg/
+obj-$(CONFIG_LCDCTRL) += lcdctrl/
# Controller drivers
obj-$(CONFIG_FB_TFT_AGM1264K_FL) += fb_agm1264k-fl.o
diff --git a/drivers/staging/fbtft/lcdctrl/Kconfig b/drivers/staging/fbtft/lcdctrl/Kconfig
new file mode 100644
index 0000000..5272847
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/Kconfig
@@ -0,0 +1,3 @@
+config LCDCTRL
+ tristate
+ default n
diff --git a/drivers/staging/fbtft/lcdctrl/Makefile b/drivers/staging/fbtft/lcdctrl/Makefile
new file mode 100644
index 0000000..e6e4e8c
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_LCDCTRL) += lcdctrl.o
diff --git a/drivers/staging/fbtft/lcdctrl/lcdctrl.c b/drivers/staging/fbtft/lcdctrl/lcdctrl.c
new file mode 100644
index 0000000..028fc6b
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/lcdctrl.c
@@ -0,0 +1,207 @@
+/*
+ * Copyright (C) 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include "lcdctrl.h"
+
+void lcdctrl_of_get_format(struct lcdctrl *ctrl)
+{
+ struct device *dev = ctrl->lcdreg->dev;
+ const char *fmt_str;
+ int ret;
+
+ if (!ctrl->set_format)
+ return;
+
+ ret = of_property_read_string(dev->of_node, "format", &fmt_str);
+ if (ret)
+ return;
+
+ if (!strcmp(fmt_str, "monochrome"))
+ ctrl->format = LCDCTRL_FORMAT_MONO10;
+ else if (!strcmp(fmt_str, "rgb565"))
+ ctrl->format = LCDCTRL_FORMAT_RGB565;
+ else if (!strcmp(fmt_str, "rgb888"))
+ ctrl->format = LCDCTRL_FORMAT_RGB888;
+ else if (!strcmp(fmt_str, "xrgb8888"))
+ ctrl->format = LCDCTRL_FORMAT_XRGB8888;
+ else
+ dev_err(dev, "Invalid format: %s. Using default.\n", fmt_str);
+}
+EXPORT_SYMBOL(lcdctrl_of_get_format);
+
+void lcdctrl_of_get_rotation(struct lcdctrl *ctrl)
+{
+ struct device *dev = ctrl->lcdreg->dev;
+ u32 val;
+ int ret;
+
+ if (!ctrl->rotate)
+ return;
+
+ ret = of_property_read_u32(dev->of_node, "rotation", &val);
+ if (ret) {
+ if (ret != -EINVAL)
+ dev_err(dev, "error reading property 'rotation': %i\n",
+ ret);
+ return;
+ }
+
+ switch (val) {
+ case 0:
+ case 90:
+ case 180:
+ case 270:
+ ctrl->rotation = val;
+ break;
+ default:
+ dev_err(dev, "illegal rotation value: %u\n", val);
+ break;
+ }
+}
+EXPORT_SYMBOL(lcdctrl_of_get_rotation);
+
+int lcdctrl_enable(struct lcdctrl *ctrl, void *videomem)
+{
+ struct lcdctrl_update update = {
+ .base = videomem,
+ .ys = 0,
+ .ye = lcdctrl_yres(ctrl) - 1,
+ };
+ int ret;
+
+ if (WARN_ON_ONCE(!ctrl->update))
+ return -EINVAL;
+
+ if (ctrl->initialized)
+ return 0;
+
+ lcdreg_lock(ctrl->lcdreg);
+
+ if (ctrl->power_supply) {
+ ret = regulator_enable(ctrl->power_supply);
+ if (ret) {
+ dev_err(ctrl->lcdreg->dev,
+ "failed to enable power supply: %d\n", ret);
+ goto enable_failed;
+ }
+ }
+ if (ctrl->poweron) {
+ ret = ctrl->poweron(ctrl);
+ if (ret)
+ goto enable_failed;
+ }
+
+ if (ctrl->set_format) {
+ ret = ctrl->set_format(ctrl);
+ if (ret) {
+ dev_err(ctrl->lcdreg->dev,
+ "failed to set format '%d'\n", ctrl->format);
+ goto enable_failed;
+ }
+ }
+
+ if (ctrl->rotate) {
+ ret = _lcdctrl_rotate(ctrl, ctrl->rotation);
+ if (ret)
+ goto enable_failed;
+ }
+
+ ret = ctrl->update(ctrl, &update);
+ if (ret)
+ goto enable_failed;
+
+ if (ctrl->blank) {
+ ret = ctrl->blank(ctrl, false);
+ if (ret)
+ goto enable_failed;
+ }
+
+ ctrl->initialized = true;
+
+enable_failed:
+ lcdreg_unlock(ctrl->lcdreg);
+
+ return ret;
+}
+EXPORT_SYMBOL(lcdctrl_enable);
+
+void lcdctrl_disable(struct lcdctrl *ctrl)
+{
+ lcdreg_lock(ctrl->lcdreg);
+
+ if (ctrl->blank)
+ ctrl->blank(ctrl, true);
+ if (ctrl->poweroff)
+ ctrl->poweroff(ctrl);
+ if (ctrl->power_supply)
+ regulator_disable(ctrl->power_supply);
+ ctrl->initialized = false;
+
+ lcdreg_unlock(ctrl->lcdreg);
+}
+EXPORT_SYMBOL(lcdctrl_disable);
+
+int lcdctrl_update(struct lcdctrl *ctrl, struct lcdctrl_update *update)
+{
+ u32 yres = lcdctrl_yres(ctrl);
+ int ret;
+
+ if (!ctrl->initialized)
+ return -EINVAL;
+
+ lcdreg_lock(ctrl->lcdreg);
+
+ if (update->ys > update->ye) {
+ update->ys = 0;
+ update->ye = yres - 1;
+ }
+ if (update->ye >= yres)
+ update->ye = yres - 1;
+
+ ret = ctrl->update(ctrl, update);
+
+ lcdreg_unlock(ctrl->lcdreg);
+
+ return ret;
+}
+EXPORT_SYMBOL(lcdctrl_update);
+
+/**
+ * Caller is responsible for locking.
+ */
+int _lcdctrl_rotate(struct lcdctrl *ctrl, u32 rotation)
+{
+ int ret;
+
+ if (!ctrl->rotate)
+ return -ENOSYS;
+
+ switch (rotation) {
+ case 0:
+ case 90:
+ case 180:
+ case 270:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = ctrl->rotate(ctrl, rotation);
+ if (!ret)
+ ctrl->rotation = rotation;
+
+ return ret;
+}
+EXPORT_SYMBOL(_lcdctrl_rotate);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/fbtft/lcdctrl/lcdctrl.h b/drivers/staging/fbtft/lcdctrl/lcdctrl.h
new file mode 100644
index 0000000..e74a66d
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/lcdctrl.h
@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_LCDCTRL_H
+#define __LINUX_LCDCTRL_H
+
+#include "../lcdreg/lcdreg.h"
+
+enum lcdctrl_format {
+ LCDCTRL_FORMAT_NONE = 0,
+ LCDCTRL_FORMAT_MONO10,
+ LCDCTRL_FORMAT_RGB565,
+ LCDCTRL_FORMAT_RGB888,
+ LCDCTRL_FORMAT_XRGB8888,
+};
+
+/**
+ * Description of a display update
+ *
+ * @base: Base address of video memory
+ * @ys: Horizontal line to start the transfer from (zero based)
+ * @ye: Last line to transfer (inclusive)
+ */
+struct lcdctrl_update {
+ void *base;
+ u32 ys;
+ u32 ye;
+};
+
+/**
+ * Description of LCD controller
+ *
+ * @width: Width of display in pixels
+ * @height: Height of display in pixels
+ * @rotation: Display rotation Counter Clockwise (0,90,180,270)
+ * @format: Videomemory format
+ * @initialized: Controller is initialized
+ * @poweron: Power on operation (optional)
+ * @poweroff: Power off operation (optional)
+ * @update: Updates the controllers video memory
+ * @rotate: Rotates the display (optional)
+ * @set_format: Sets format according to @format (optional)
+ * @blank: Blanks display (optional)
+ * @lcdreg: LCD register operated upon
+ * @driver_private: Driver data (not touched by core)
+ * @power_supply: Regulator for power supply
+ */
+struct lcdctrl {
+ u32 width;
+ u32 height;
+ u32 rotation;
+ enum lcdctrl_format format;
+ bool initialized;
+
+ int (*poweron)(struct lcdctrl *ctrl);
+ void (*poweroff)(struct lcdctrl *ctrl);
+ int (*update)(struct lcdctrl *ctrl, struct lcdctrl_update *update);
+ int (*rotate)(struct lcdctrl *ctrl, u32 rotation);
+ int (*set_format)(struct lcdctrl *ctrl);
+ int (*blank)(struct lcdctrl *ctrl, bool blank);
+
+ struct lcdreg *lcdreg;
+ struct regulator *power_supply;
+ void *driver_private;
+};
+
+static inline u32 lcdctrl_xres(struct lcdctrl *ctrl)
+{
+ return (ctrl->rotation % 180) ? ctrl->height : ctrl->width;
+}
+
+static inline u32 lcdctrl_yres(struct lcdctrl *ctrl)
+{
+ return (ctrl->rotation % 180) ? ctrl->width : ctrl->height;
+}
+
+static inline int lcdctrl_blank(struct lcdctrl *ctrl, bool blank)
+{
+ int ret;
+
+ if (!ctrl->blank)
+ return -ENOSYS;
+
+ lcdreg_lock(ctrl->lcdreg);
+ ret = ctrl->blank(ctrl, blank);
+ lcdreg_unlock(ctrl->lcdreg);
+
+ return ret;
+}
+
+extern void lcdctrl_of_get_format(struct lcdctrl *ctrl);
+extern void lcdctrl_of_get_rotation(struct lcdctrl *ctrl);
+extern int lcdctrl_enable(struct lcdctrl *ctrl, void *videomem);
+extern void lcdctrl_disable(struct lcdctrl *ctrl);
+extern int lcdctrl_update(struct lcdctrl *ctrl, struct lcdctrl_update *update);
+extern int _lcdctrl_rotate(struct lcdctrl *ctrl, u32 rotate);
+
+#endif /* __LINUX_LCDCTRL_H */
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC 3/7] staging: fbtft: add lcd controller abstraction
2015-03-02 10:54 ` [RFC 3/7] staging: fbtft: add lcd controller abstraction Noralf Trønnes
@ 2015-03-02 11:48 ` Dan Carpenter
2015-03-03 11:57 ` Noralf Trønnes
0 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2015-03-02 11:48 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: gregkh, driverdev-devel
On Mon, Mar 02, 2015 at 11:54:25AM +0100, Noralf Trønnes wrote:
> +int lcdctrl_enable(struct lcdctrl *ctrl, void *videomem)
> +{
> + struct lcdctrl_update update = {
> + .base = videomem,
> + .ys = 0,
> + .ye = lcdctrl_yres(ctrl) - 1,
> + };
> + int ret;
> +
> + if (WARN_ON_ONCE(!ctrl->update))
> + return -EINVAL;
> +
> + if (ctrl->initialized)
> + return 0;
> +
> + lcdreg_lock(ctrl->lcdreg);
> +
> + if (ctrl->power_supply) {
> + ret = regulator_enable(ctrl->power_supply);
> + if (ret) {
> + dev_err(ctrl->lcdreg->dev,
> + "failed to enable power supply: %d\n", ret);
> + goto enable_failed;
This kind of label naming where the label name is based on the function
which failed is a common anti-pattern.
> + }
> + }
> + if (ctrl->poweron) {
> + ret = ctrl->poweron(ctrl);
> + if (ret)
> + goto enable_failed;
It means that the other gotos don't make any sense. It's better to
pick the label name based on the label location like err_unlock,
out_unlock.
> +/**
> + * Caller is responsible for locking.
> + */
> +int _lcdctrl_rotate(struct lcdctrl *ctrl, u32 rotation)
Why the underscore? I assume it's because of the locking. Just
documentating it is sufficient, no need for the underscore.
> +{
> + int ret;
> +
> + if (!ctrl->rotate)
> + return -ENOSYS;
> +
> + switch (rotation) {
> + case 0:
> + case 90:
> + case 180:
> + case 270:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = ctrl->rotate(ctrl, rotation);
> + if (!ret)
> + ctrl->rotation = rotation;
> +
> + return ret;
Better to check "if (ret)" consistently (error handling vs success
handling).
> +}
> +/**
> + * Description of a display update
> + *
> + * @base: Base address of video memory
> + * @ys: Horizontal line to start the transfer from (zero based)
> + * @ye: Last line to transfer (inclusive)
> + */
> +struct lcdctrl_update {
> + void *base;
> + u32 ys;
> + u32 ye;
"ys" and "ye" are easy to mix up when you're reading the code. They
are not especially self documenting. Maybe use y_start and y_end.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 3/7] staging: fbtft: add lcd controller abstraction
2015-03-02 11:48 ` Dan Carpenter
@ 2015-03-03 11:57 ` Noralf Trønnes
2015-03-03 12:04 ` Dan Carpenter
0 siblings, 1 reply; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-03 11:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: thomas.petazzoni, gregkh, driverdev-devel
Den 02.03.2015 12:48, skrev Dan Carpenter:
[snip]
>> + if (ctrl->power_supply) {
>> + ret = regulator_enable(ctrl->power_supply);
>> + if (ret) {
>> + dev_err(ctrl->lcdreg->dev,
>> + "failed to enable power supply: %d\n", ret);
>> + goto enable_failed;
> This kind of label naming where the label name is based on the function
> which failed is a common anti-pattern.
>
>> + }
>> + }
>> + if (ctrl->poweron) {
>> + ret = ctrl->poweron(ctrl);
>> + if (ret)
>> + goto enable_failed;
> It means that the other gotos don't make any sense. It's better to
> pick the label name based on the label location like err_unlock,
> out_unlock.
Yes, I see that.
>> +/**
>> + * Caller is responsible for locking.
>> + */
>> +int _lcdctrl_rotate(struct lcdctrl *ctrl, u32 rotation)
> Why the underscore? I assume it's because of the locking. Just
> documentating it is sufficient, no need for the underscore.
Ok, I thought this was a common pattern based on other functions I have
seen.
>> +{
>> + int ret;
>> +
>> + if (!ctrl->rotate)
>> + return -ENOSYS;
>> +
>> + switch (rotation) {
>> + case 0:
>> + case 90:
>> + case 180:
>> + case 270:
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + ret = ctrl->rotate(ctrl, rotation);
>> + if (!ret)
>> + ctrl->rotation = rotation;
>> +
>> + return ret;
> Better to check "if (ret)" consistently (error handling vs success
> handling).
Like this?
ret = ctrl->rotate(ctrl, rotation);
if (ret)
return ret;
ctrl->rotation = rotation;
return 0;
>> +/**
>> + * Description of a display update
>> + *
>> + * @base: Base address of video memory
>> + * @ys: Horizontal line to start the transfer from (zero based)
>> + * @ye: Last line to transfer (inclusive)
>> + */
>> +struct lcdctrl_update {
>> + void *base;
>> + u32 ys;
>> + u32 ye;
> "ys" and "ye" are easy to mix up when you're reading the code. They
> are not especially self documenting. Maybe use y_start and y_end.
>
Yes, they're kind on convoluted. Not suited for fast reading.
Thanks,
Noralf.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 3/7] staging: fbtft: add lcd controller abstraction
2015-03-03 11:57 ` Noralf Trønnes
@ 2015-03-03 12:04 ` Dan Carpenter
2015-03-03 14:08 ` Noralf Trønnes
0 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2015-03-03 12:04 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: gregkh, driverdev-devel
On Tue, Mar 03, 2015 at 12:57:29PM +0100, Noralf Trønnes wrote:
> >>+ ret = ctrl->rotate(ctrl, rotation);
> >>+ if (!ret)
> >>+ ctrl->rotation = rotation;
> >>+
> >>+ return ret;
> >Better to check "if (ret)" consistently (error handling vs success
> >handling).
> Like this?
>
> ret = ctrl->rotate(ctrl, rotation);
> if (ret)
> return ret;
>
> ctrl->rotation = rotation;
>
> return 0;
>
Yeah. This is a tiny nit. I feel a bit silly for commenting on it now.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC 3/7] staging: fbtft: add lcd controller abstraction
2015-03-03 12:04 ` Dan Carpenter
@ 2015-03-03 14:08 ` Noralf Trønnes
0 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-03 14:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, driverdev-devel
Den 03.03.2015 13:04, skrev Dan Carpenter:
> On Tue, Mar 03, 2015 at 12:57:29PM +0100, Noralf Trønnes wrote:
>>>> + ret = ctrl->rotate(ctrl, rotation);
>>>> + if (!ret)
>>>> + ctrl->rotation = rotation;
>>>> +
>>>> + return ret;
>>> Better to check "if (ret)" consistently (error handling vs success
>>> handling).
>> Like this?
>>
>> ret = ctrl->rotate(ctrl, rotation);
>> if (ret)
>> return ret;
>>
>> ctrl->rotation = rotation;
>>
>> return 0;
>>
> Yeah. This is a tiny nit. I feel a bit silly for commenting on it now.
No need to be. If this enhances readability even just a tiny bit, I
welcome it.
I've read enough kernel code now to be amazed of how readable and
understandable it is.
This becomes very evident when reading some out-of-tree drivers, and
*cough* fbtft.
I'm practically blind to the code in this patchset, as it has gone
through several iterations
over the past months. To me it looks very good, but obviously not
without room for
improvements :-)
Noralf.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC 4/7] staging: fbtft: lcdctrl: add ssd1306 support
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
` (2 preceding siblings ...)
2015-03-02 10:54 ` [RFC 3/7] staging: fbtft: add lcd controller abstraction Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 10:54 ` [RFC 5/7] staging: fbtft: don't require platform data Noralf Trønnes
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add support for Solomon SSD1306 Monochrome OLED controller.
The controller supports up to 128x64 pixles and supports interfaces:
Parallel 6800, parallel 8080, SPI (3/4), I2C.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/lcdctrl/Kconfig | 5 +
drivers/staging/fbtft/lcdctrl/Makefile | 2 +
drivers/staging/fbtft/lcdctrl/ssd1306.c | 168 ++++++++++++++++++++++++++++++++
drivers/staging/fbtft/lcdctrl/ssd1306.h | 51 ++++++++++
4 files changed, 226 insertions(+)
create mode 100644 drivers/staging/fbtft/lcdctrl/ssd1306.c
create mode 100644 drivers/staging/fbtft/lcdctrl/ssd1306.h
diff --git a/drivers/staging/fbtft/lcdctrl/Kconfig b/drivers/staging/fbtft/lcdctrl/Kconfig
index 5272847..61bfef1 100644
--- a/drivers/staging/fbtft/lcdctrl/Kconfig
+++ b/drivers/staging/fbtft/lcdctrl/Kconfig
@@ -1,3 +1,8 @@
config LCDCTRL
tristate
default n
+
+config LCDCTRL_SSD1306
+ tristate
+ select LCDCTRL
+ default n
diff --git a/drivers/staging/fbtft/lcdctrl/Makefile b/drivers/staging/fbtft/lcdctrl/Makefile
index e6e4e8c..42a61f9 100644
--- a/drivers/staging/fbtft/lcdctrl/Makefile
+++ b/drivers/staging/fbtft/lcdctrl/Makefile
@@ -1 +1,3 @@
obj-$(CONFIG_LCDCTRL) += lcdctrl.o
+
+obj-$(CONFIG_LCDCTRL_SSD1306) += ssd1306.o
diff --git a/drivers/staging/fbtft/lcdctrl/ssd1306.c b/drivers/staging/fbtft/lcdctrl/ssd1306.c
new file mode 100644
index 0000000..251bcc2
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/ssd1306.c
@@ -0,0 +1,168 @@
+/*
+ * SSD1306 LCD controller support
+ *
+ * Copyright 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+
+#include "ssd1306.h"
+
+/*
+ * TODO: contrast control is not implemented
+ * If gamma control is to be user configurable, maybe the same
+ * mechanism can be used for contrast.
+ */
+
+struct ssd1306_controller {
+ struct lcdctrl lcdctrl;
+ void *buf;
+};
+
+static inline struct ssd1306_controller *to_ssd1306(struct lcdctrl *ctrl)
+{
+ return container_of(ctrl, struct ssd1306_controller, lcdctrl);
+}
+
+static int ssd1306_update(struct lcdctrl *ctrl, struct lcdctrl_update *update)
+{
+ struct lcdreg *reg = ctrl->lcdreg;
+ u8 *buf = to_ssd1306(ctrl)->buf;
+ u32 xres = lcdctrl_xres(ctrl);
+ u32 yres = lcdctrl_yres(ctrl);
+ int x, y, i;
+ struct lcdreg_transfer tr = {
+ .index = 1,
+ .width = 8,
+ .buf = buf,
+ .count = xres * yres / 8,
+ };
+
+ /*
+ * RGB565 is implemented on this monochrome controller for 2 reasons:
+ * 1. sys_imageblit and friends doesn't honour
+ * CONFIG_FB_CFB_REV_PIXELS_IN_BYTE which results in mirrored or
+ * garbeled fonts with fbcon on systems like the Raspberry Pi.
+ * 2. Very few applications and no grahics libraries supports
+ * monochrome framebuffers.
+ */
+ if (ctrl->format == LCDCTRL_FORMAT_RGB565) {
+ u16 *vmem16 = (u16 *)update->base;
+
+ /* TODO: add better conversion as done in fb_agm1264k-fl */
+ for (x = 0; x < xres; x++) {
+ for (y = 0; y < yres / 8; y++) {
+ *buf = 0x00;
+ for (i = 0; i < 8; i++) {
+ int y1 = y * 8 + i;
+
+ if (vmem16[y1 * xres + x])
+ *buf |= 1 << i;
+ }
+ buf++;
+ }
+ }
+ } else { /* LCDCTRL_FORMAT_MONO10 */
+ u8 *vmem8 = (u8 *)update->base;
+
+ for (x = 0; x < xres; x++) {
+ for (y = 0; y < yres / 8; y++) {
+ *buf = 0x00;
+ for (i = 0; i < 8; i++) {
+ int y1 = y * 8 + i;
+ int idx = y1 * xres / 8 + x / 8;
+ int mask = (1 << (7 - (x % 8)));
+
+ if (vmem8[idx] & mask)
+ *buf |= 1 << i;
+ }
+ buf++;
+ }
+ }
+ }
+ return lcdreg_write(reg, SSD1306_DISPLAY_START_LINE, &tr);
+}
+
+static int ssd1306_blank(struct lcdctrl *ctrl, bool blank)
+{
+ return lcdreg_writereg(ctrl->lcdreg, blank ? SSD1306_DISPLAY_OFF :
+ SSD1306_DISPLAY_ON);
+}
+
+static int ssd1306_set_format(struct lcdctrl *ctrl)
+{
+ switch (ctrl->format) {
+ case LCDCTRL_FORMAT_MONO10:
+ break;
+ case LCDCTRL_FORMAT_RGB565:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static const struct lcdctrl ssd1306_lcdctrl = {
+ .update = ssd1306_update,
+ .blank = ssd1306_blank,
+ .set_format = ssd1306_set_format,
+};
+
+struct lcdctrl *devm_ssd1306_init(struct lcdreg *lcdreg,
+ struct ssd1306_config *config)
+{
+ struct ssd1306_controller *ssd1306;
+ struct lcdctrl *ctrl;
+
+ ssd1306 = devm_kzalloc(lcdreg->dev, sizeof(*ssd1306),
+ GFP_KERNEL);
+ if (!ssd1306)
+ return ERR_PTR(-ENOMEM);
+
+ ctrl = &ssd1306->lcdctrl;
+ *ctrl = ssd1306_lcdctrl;
+ ctrl->lcdreg = lcdreg;
+ ctrl->lcdreg->def_width = 8;
+ ctrl->width = config->width ? : 128;
+ ctrl->height = config->height ? : 64;
+ ctrl->format = LCDCTRL_FORMAT_RGB565;
+ ssd1306->buf = devm_kzalloc(lcdreg->dev,
+ ctrl->height * ctrl->width / 8,
+ GFP_KERNEL);
+ if (!ssd1306->buf)
+ return ERR_PTR(-ENOMEM);
+
+ return ctrl;
+}
+EXPORT_SYMBOL(devm_ssd1306_init);
+
+bool ssd1306_check_status(struct lcdreg *reg, bool display_is_on)
+{
+ u32 val;
+ int ret;
+
+ ret = lcdreg_readreg_buf32(reg, 0x00, &val, 1);
+ if (ret) {
+ dev_err(reg->dev, "failed to read status: %i\n", ret);
+ return false;
+ }
+ if (((val & BIT(6)) >> 6) == display_is_on) {
+ dev_warn(reg->dev,
+ "status check failed: 0x%02x", val);
+ return false;
+ }
+ dev_dbg(reg->dev, "%s: OK\n", __func__);
+
+ return true;
+}
+EXPORT_SYMBOL(ssd1306_check_status);
+
+MODULE_DESCRIPTION("SSD1306 LCD controller support");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/fbtft/lcdctrl/ssd1306.h b/drivers/staging/fbtft/lcdctrl/ssd1306.h
new file mode 100644
index 0000000..a1fea6e
--- /dev/null
+++ b/drivers/staging/fbtft/lcdctrl/ssd1306.h
@@ -0,0 +1,51 @@
+/*
+ * SSD1306 LCD controller support
+ *
+ * Copyright 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_SSD1306_H
+#define __LINUX_SSD1306_H
+
+#include "../lcdreg/lcdreg.h"
+#include "lcdctrl.h"
+
+#define SSD1306_ADDRESS_MODE 0x20
+#define SSD1306_COL_RANGE 0x21
+#define SSD1306_PAGE_RANGE 0x22
+#define SSD1306_DISPLAY_START_LINE 0x40
+#define SSD1306_CONTRAST 0x81
+#define SSD1306_CHARGE_PUMP 0x8d
+#define SSD1306_SEG_REMAP_OFF 0xa0
+#define SSD1306_SEG_REMAP_ON 0xa1
+#define SSD1306_RESUME_TO_RAM 0xa4
+#define SSD1306_ENTIRE_DISPLAY_ON 0xa5
+#define SSD1306_NORMAL_DISPLAY 0xa6
+#define SSD1306_INVERSE_DISPLAY 0xa7
+#define SSD1306_MULTIPLEX_RATIO 0xa8
+#define SSD1306_DISPLAY_OFF 0xae
+#define SSD1306_DISPLAY_ON 0xaf
+#define SSD1306_START_PAGE_ADDRESS 0xb0
+#define SSD1306_COM_SCAN_NORMAL 0xc0
+#define SSD1306_COM_SCAN_REMAP 0xc8
+#define SSD1306_DISPLAY_OFFSET 0xd3
+#define SSD1306_CLOCK_FREQ 0xd5
+#define SSD1306_PRECHARGE_PERIOD 0xd9
+#define SSD1306_COM_PINS_CONFIG 0xda
+#define SSD1306_VCOMH 0xdb
+
+struct ssd1306_config {
+ u32 width;
+ u32 height;
+};
+
+struct lcdctrl *devm_ssd1306_init(struct lcdreg *lcdreg,
+ struct ssd1306_config *config);
+extern bool ssd1306_check_status(struct lcdreg *reg, bool display_is_on);
+
+#endif /* __LINUX_SSD1306_H */
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC 5/7] staging: fbtft: don't require platform data
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
` (3 preceding siblings ...)
2015-03-02 10:54 ` [RFC 4/7] staging: fbtft: lcdctrl: add ssd1306 support Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 11:31 ` Dan Carpenter
2015-03-02 10:54 ` [RFC 6/7] staging: fbtft: extend core to use lcdctrl Noralf Trønnes
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add dummy platform data when it's not present.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/fbtft-core.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index ac4287f..59c17c1 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -719,10 +719,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
if (!bpp)
bpp = 16;
- if (!pdata) {
- dev_err(dev, "platform data is missing\n");
- return NULL;
- }
+ if (!pdata)
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
/* override driver values? */
if (pdata->fps)
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC 5/7] staging: fbtft: don't require platform data
2015-03-02 10:54 ` [RFC 5/7] staging: fbtft: don't require platform data Noralf Trønnes
@ 2015-03-02 11:31 ` Dan Carpenter
2015-03-03 11:15 ` Noralf Trønnes
0 siblings, 1 reply; 19+ messages in thread
From: Dan Carpenter @ 2015-03-02 11:31 UTC (permalink / raw)
To: Noralf Trønnes; +Cc: gregkh, driverdev-devel
On Mon, Mar 02, 2015 at 11:54:27AM +0100, Noralf Trønnes wrote:
> Add dummy platform data when it's not present.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
> drivers/staging/fbtft/fbtft-core.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index ac4287f..59c17c1 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -719,10 +719,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
> if (!bpp)
> bpp = 16;
>
> - if (!pdata) {
> - dev_err(dev, "platform data is missing\n");
> - return NULL;
> - }
> + if (!pdata)
> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
This is weird. pdata is zeroed out so the info is a bit useless. We
don't use it outside this function. Later in the function, then should
we do?
- par->pdata = dev->platform_data;
+ par->pdata = pdata;
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 5/7] staging: fbtft: don't require platform data
2015-03-02 11:31 ` Dan Carpenter
@ 2015-03-03 11:15 ` Noralf Trønnes
0 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-03 11:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: thomas.petazzoni, gregkh, driverdev-devel
Den 02.03.2015 12:31, skrev Dan Carpenter:
> On Mon, Mar 02, 2015 at 11:54:27AM +0100, Noralf Trønnes wrote:
>> Add dummy platform data when it's not present.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>> drivers/staging/fbtft/fbtft-core.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
>> index ac4287f..59c17c1 100644
>> --- a/drivers/staging/fbtft/fbtft-core.c
>> +++ b/drivers/staging/fbtft/fbtft-core.c
>> @@ -719,10 +719,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>> if (!bpp)
>> bpp = 16;
>>
>> - if (!pdata) {
>> - dev_err(dev, "platform data is missing\n");
>> - return NULL;
>> - }
>> + if (!pdata)
>> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> This is weird. pdata is zeroed out so the info is a bit useless. We
> don't use it outside this function. Later in the function, then should
> we do?
>
> - par->pdata = dev->platform_data;
> + par->pdata = pdata;
You're right. I missed that.
thanks,
Noralf.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [RFC 6/7] staging: fbtft: extend core to use lcdctrl
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
` (4 preceding siblings ...)
2015-03-02 10:54 ` [RFC 5/7] staging: fbtft: don't require platform data Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-02 10:54 ` [RFC 7/7] staging: fbtft: add driver for Adafruit ssd1306 based displays Noralf Trønnes
2015-03-21 18:23 ` Noralf Trønnes
7 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
Add lcdctrl support in core fbtft module.
Provide new API for drivers to use.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/Makefile | 1 +
drivers/staging/fbtft/fbtft-lcdctrl.c | 311 ++++++++++++++++++++++++++++++++++
drivers/staging/fbtft/fbtft.h | 13 ++
3 files changed, 325 insertions(+)
create mode 100644 drivers/staging/fbtft/fbtft-lcdctrl.c
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index 69f7c2c..6f002a6 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -1,6 +1,7 @@
# Core modules
obj-$(CONFIG_FB_TFT) += fbtft.o
fbtft-y += fbtft-core.o fbtft-sysfs.o fbtft-bus.o fbtft-io.o
+fbtft-$(if $(CONFIG_LCDCTRL),y,) += fbtft-lcdctrl.o
obj-$(CONFIG_LCDREG) += lcdreg/
obj-$(CONFIG_LCDCTRL) += lcdctrl/
diff --git a/drivers/staging/fbtft/fbtft-lcdctrl.c b/drivers/staging/fbtft/fbtft-lcdctrl.c
new file mode 100644
index 0000000..b4e6011
--- /dev/null
+++ b/drivers/staging/fbtft/fbtft-lcdctrl.c
@@ -0,0 +1,311 @@
+#include <linux/device.h>
+#include <linux/regulator/consumer.h>
+
+#include "fbtft.h"
+
+extern void fbtft_sysfs_init(struct fbtft_par *par);
+
+static int fbtft_lcdctrl_blank(struct fbtft_par *par, bool on)
+{
+ struct lcdctrl *ctrl = par->lcdctrl;
+
+ if (ctrl->blank)
+ return lcdctrl_blank(ctrl, on);
+ else if (par->info->bl_dev)
+ return 0; /* backlight subsystem handles blanking */
+ else
+ return -EINVAL; /* no blanking support */
+}
+
+static void fbtft_lcdctrl_update_display(struct fbtft_par *par,
+ unsigned start_line, unsigned end_line)
+{
+ struct lcdctrl *ctrl = par->lcdctrl;
+ struct lcdctrl_update update = {
+ .base = (void __force *)par->info->screen_base,
+ .ys = start_line,
+ .ye = end_line,
+ };
+ int ret;
+
+ ret = lcdctrl_update(ctrl, &update);
+ if (ret)
+ pr_err_once("%s %s: lcdctrl_update failed: %i\n",
+ dev_driver_string(ctrl->lcdreg->dev),
+ dev_name(ctrl->lcdreg->dev), ret);
+}
+
+static int fbtft_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
+{
+ u32 rotate = var->rotate;
+
+ *var = info->var;
+ var->rotate = rotate;
+
+ switch (var->rotate) {
+ case 0:
+ case 90:
+ case 180:
+ case 270:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int fbtft_fb_set_par(struct fb_info *info)
+{
+ struct fbtft_par *par = info->par;
+ struct lcdctrl *ctrl = par->lcdctrl;
+ int ret;
+
+ lcdreg_lock(ctrl->lcdreg);
+
+ ret = _lcdctrl_rotate(ctrl, info->var.rotate);
+ if (ret) {
+ info->var.rotate = ctrl->rotation;
+ goto rotate_failed;
+ }
+
+ info->var.xres = lcdctrl_xres(ctrl);
+ info->var.yres = lcdctrl_yres(ctrl);
+ info->var.xres_virtual = info->var.xres;
+ info->var.yres_virtual = info->var.yres;
+
+ switch (info->var.bits_per_pixel) {
+ case 1:
+ info->fix.line_length = info->var.xres / 8;
+ break;
+ case 8:
+ info->fix.line_length = info->var.xres;
+ break;
+ case 16:
+ info->fix.line_length = info->var.xres * 2;
+ break;
+ case 24:
+ info->fix.line_length = info->var.xres * 3;
+ break;
+ case 32:
+ info->fix.line_length = info->var.xres * 4;
+ break;
+ }
+
+rotate_failed:
+ lcdreg_unlock(ctrl->lcdreg);
+
+ return ret;
+}
+
+static struct fb_info *fbtft_lcdctrl_register(struct lcdctrl *ctrl)
+{
+ struct device *dev = ctrl->lcdreg->dev;
+ struct fb_info *info;
+ struct fbtft_par *par;
+ int ret;
+ struct fbtft_display display = {
+ .width = lcdctrl_xres(ctrl),
+ .height = lcdctrl_yres(ctrl),
+ .fps = 20,
+ .txbuflen = -2, /* don't allocate txbuf */
+ };
+
+ switch (ctrl->format) {
+ case LCDCTRL_FORMAT_MONO10:
+ display.bpp = 1;
+ break;
+ case LCDCTRL_FORMAT_RGB565:
+ display.bpp = 16;
+ break;
+ case LCDCTRL_FORMAT_RGB888:
+ display.bpp = 24;
+ break;
+ case LCDCTRL_FORMAT_XRGB8888:
+ display.bpp = 32;
+ break;
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+
+ info = fbtft_framebuffer_alloc(&display, dev);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+
+ switch (ctrl->format) {
+ case LCDCTRL_FORMAT_MONO10:
+ info->fix.visual = FB_VISUAL_MONO10;
+ info->var.red.length = 1;
+ info->var.red.offset = 0;
+ info->var.green.length = 1;
+ info->var.green.offset = 0;
+ info->var.blue.length = 1;
+ info->var.blue.offset = 0;
+ break;
+ case LCDCTRL_FORMAT_RGB565:
+ /* set by fbtft_framebuffer_alloc() */
+ break;
+ case LCDCTRL_FORMAT_RGB888:
+ info->var.red.offset = 16;
+ info->var.red.length = 8;
+ info->var.green.offset = 8;
+ info->var.green.length = 8;
+ info->var.blue.offset = 0;
+ info->var.blue.length = 8;
+ break;
+ case LCDCTRL_FORMAT_XRGB8888:
+ info->var.red.offset = 16;
+ info->var.red.length = 8;
+ info->var.green.offset = 8;
+ info->var.green.length = 8;
+ info->var.blue.offset = 0;
+ info->var.blue.length = 8;
+ break;
+ default:
+ ret = -EINVAL;
+ goto err_release;
+ }
+
+ par = info->par;
+ par->lcdctrl = ctrl;
+ par->fbtftops.blank = fbtft_lcdctrl_blank;
+ par->fbtftops.update_display = fbtft_lcdctrl_update_display;
+ if (ctrl->rotate) {
+ info->var.rotate = ctrl->rotation;
+ info->fbops->fb_check_var = fbtft_fb_check_var;
+ info->fbops->fb_set_par = fbtft_fb_set_par;
+ }
+
+ ret = lcdctrl_enable(ctrl, info->screen_base);
+ if (ret)
+ goto err_release;
+
+ ret = register_framebuffer(info);
+ if (ret)
+ goto err_release;
+
+ fbtft_sysfs_init(par);
+ dev_set_drvdata(dev, info);
+
+ dev_info(info->dev, "%s frame buffer, %ux%u, %u KiB video memory\n",
+ info->fix.id, info->var.xres, info->var.yres,
+ info->fix.smem_len >> 10);
+
+ return info;
+
+err_release:
+ fbtft_framebuffer_release(info);
+
+ return ERR_PTR(ret);
+}
+
+static void fbtft_lcdctrl_release(struct fb_info *info)
+{
+ struct fbtft_par *par = info->par;
+ struct lcdctrl *ctrl = par->lcdctrl;
+
+ fb_blank(info, FB_BLANK_POWERDOWN);
+ if (info->bl_dev) {
+ put_device(&info->bl_dev->dev);
+ info->bl_dev = NULL;
+ }
+ lcdctrl_disable(ctrl);
+ fbtft_unregister_framebuffer(info);
+ fbtft_framebuffer_release(info);
+}
+
+static void devm_lcdctrl_release(struct device *dev, void *res)
+{
+ fbtft_lcdctrl_release(*(struct fb_info **)res);
+}
+
+int devm_fbtft_lcdctrl_register(struct lcdctrl *ctrl)
+{
+ struct fb_info **ptr;
+
+ if (WARN_ON(!ctrl || !ctrl->lcdreg || !ctrl->lcdreg->dev))
+ return -EINVAL;
+
+ ptr = devres_alloc(devm_lcdctrl_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return -ENOMEM;
+
+ *ptr = fbtft_lcdctrl_register(ctrl);
+ if (IS_ERR(*ptr)) {
+ int ret = PTR_ERR(*ptr);
+
+ devres_free(ptr);
+ return ret;
+ }
+
+ devres_add(ctrl->lcdreg->dev, ptr);
+
+ return 0;
+}
+EXPORT_SYMBOL(devm_fbtft_lcdctrl_register);
+
+int devm_fbtft_lcdctrl_of_register(struct lcdctrl *ctrl)
+{
+ struct device *dev;
+ struct device_node *backlight_node;
+ struct backlight_device *backlight = NULL;
+ int ret;
+
+ if (WARN_ON(!ctrl || !ctrl->lcdreg || !ctrl->lcdreg->dev))
+ return -EINVAL;
+
+ dev = ctrl->lcdreg->dev;
+ ctrl->initialized = of_property_read_bool(dev->of_node, "initialized");
+ ctrl->power_supply = devm_regulator_get(dev, "power");
+ if (IS_ERR(ctrl->power_supply))
+ return PTR_ERR(ctrl->power_supply);
+
+ backlight_node = of_parse_phandle(dev->of_node, "backlight", 0);
+ if (backlight_node) {
+ backlight = of_find_backlight_by_node(backlight_node);
+ of_node_put(backlight_node);
+ if (!backlight)
+ return -EPROBE_DEFER;
+ }
+
+ lcdctrl_of_get_format(ctrl);
+ lcdctrl_of_get_rotation(ctrl);
+
+ ret = devm_fbtft_lcdctrl_register(ctrl);
+ if (ret)
+ return ret;
+
+ if (backlight) {
+ struct fb_info *info = dev_get_drvdata(dev);
+
+ info->bl_dev = backlight;
+ get_device(&info->bl_dev->dev);
+ if (backlight->props.brightness == 0)
+ backlight->props.brightness = backlight->props.max_brightness;
+ fb_blank(info, FB_BLANK_UNBLANK);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(devm_fbtft_lcdctrl_of_register);
+
+#ifdef CONFIG_PM_SLEEP
+static int fbtft_pm_suspend(struct device *dev)
+{
+ struct fb_info *info = dev_get_drvdata(dev);
+ struct fbtft_par *par = info->par;
+
+ lcdctrl_disable(par->lcdctrl);
+
+ return 0;
+}
+
+static int fbtft_pm_resume(struct device *dev)
+{
+ struct fb_info *info = dev_get_drvdata(dev);
+ struct fbtft_par *par = info->par;
+
+ return lcdctrl_enable(par->lcdctrl, info->screen_base);
+}
+
+SIMPLE_DEV_PM_OPS(fbtft_pm_ops, fbtft_pm_suspend, fbtft_pm_resume);
+EXPORT_SYMBOL(fbtft_pm_ops);
+#endif
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 0dbf3f9..d64aded 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -24,6 +24,8 @@
#include <linux/spi/spi.h>
#include <linux/platform_device.h>
+#include "lcdctrl/lcdctrl.h"
+
#define FBTFT_NOP 0x00
#define FBTFT_SWRESET 0x01
@@ -214,6 +216,7 @@ struct fbtft_platform_data {
* @extra: Extra info needed by driver
*/
struct fbtft_par {
+ struct lcdctrl *lcdctrl;
struct spi_device *spi;
struct platform_device *pdev;
struct fb_info *info;
@@ -298,6 +301,16 @@ extern void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...);
extern void fbtft_write_reg16_bus8(struct fbtft_par *par, int len, ...);
extern void fbtft_write_reg16_bus16(struct fbtft_par *par, int len, ...);
+/* fbtft-lcdctrl.c */
+extern int devm_fbtft_lcdctrl_register(struct lcdctrl *ctrl);
+extern int devm_fbtft_lcdctrl_of_register(struct lcdctrl *ctrl);
+
+#ifdef CONFIG_PM_SLEEP
+extern const struct dev_pm_ops fbtft_pm_ops;
+#define FBTFT_PM_OPS (&fbtft_pm_ops)
+#else
+#define FBTFT_PM_OPS NULL
+#endif
#define FBTFT_REGISTER_DRIVER(_name, _compatible, _display) \
\
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* [RFC 7/7] staging: fbtft: add driver for Adafruit ssd1306 based displays
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
` (5 preceding siblings ...)
2015-03-02 10:54 ` [RFC 6/7] staging: fbtft: extend core to use lcdctrl Noralf Trønnes
@ 2015-03-02 10:54 ` Noralf Trønnes
2015-03-21 18:23 ` Noralf Trønnes
7 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-02 10:54 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: gregkh, driverdev-devel, Noralf Trønnes
The Adafuit SSD1306 based displays comes in either 128x32 or 128x64
resolutions and uses I2C or SPI interface. This driver has support
for the I2C interface. Support for SPI can be added when lcdreg has
SPI support.
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
drivers/staging/fbtft/Kconfig | 9 ++
drivers/staging/fbtft/Makefile | 3 +
drivers/staging/fbtft/ada-ssd1306fb.c | 173 ++++++++++++++++++++++++++++++++++
3 files changed, 185 insertions(+)
create mode 100644 drivers/staging/fbtft/ada-ssd1306fb.c
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 0ac1b41..b53ed5b 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -168,5 +168,14 @@ config FB_TFT_FBTFT_DEVICE
tristate "Module to for adding FBTFT devices"
depends on FB_TFT
+config FB_ADA_SSD1306
+ tristate "Framebuffer driver for Adafruit SSD1306 displays"
+ depends on FB_TFT && I2C
+ select LCDREG_I2C
+ select LCDCTRL_SSD1306
+ help
+ Framebuffer Driver for Adafruit SSD1306 based
+ Monochrome OLED displays
+
source "drivers/staging/fbtft/lcdctrl/Kconfig"
source "drivers/staging/fbtft/lcdreg/Kconfig"
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index 6f002a6..f862810 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -36,3 +36,6 @@ obj-$(CONFIG_FB_FLEX) += flexfb.o
# Device modules
obj-$(CONFIG_FB_TFT_FBTFT_DEVICE) += fbtft_device.o
+
+# Display drivers
+obj-$(CONFIG_FB_ADA_SSD1306) += ada-ssd1306fb.o
diff --git a/drivers/staging/fbtft/ada-ssd1306fb.c b/drivers/staging/fbtft/ada-ssd1306fb.c
new file mode 100644
index 0000000..eea5978
--- /dev/null
+++ b/drivers/staging/fbtft/ada-ssd1306fb.c
@@ -0,0 +1,173 @@
+/*
+ * Framebuffer Driver for Adafruit SSD1306 based Monochrome OLED displays
+ *
+ * Copyright 2015 Noralf Trønnes
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+
+#include "fbtft.h"
+#include "lcdreg/lcdreg.h"
+#include "lcdctrl/ssd1306.h"
+
+
+/* Init sequence from github.com/adafruit/Adafruit_SSD1306 */
+static int ada_ssd1306_poweron(struct lcdctrl *ctrl)
+{
+ struct lcdreg *reg = ctrl->lcdreg;
+ int ret;
+
+ lcdreg_reset(reg);
+
+ ret = lcdreg_writereg(reg, SSD1306_DISPLAY_OFF);
+ if (ret) {
+ dev_err(reg->dev,
+ "lcdreg_writereg failed: %d\n", ret);
+ return ret;
+ }
+
+ if (lcdreg_is_readable(reg))
+ ssd1306_check_status(reg, false);
+
+ lcdreg_writereg(reg, SSD1306_CLOCK_FREQ);
+ lcdreg_writereg(reg, 0x80);
+
+ lcdreg_writereg(reg, SSD1306_MULTIPLEX_RATIO);
+ if (lcdctrl_yres(ctrl) == 64)
+ lcdreg_writereg(reg, 0x3f);
+ else
+ lcdreg_writereg(reg, 0x1f);
+
+ lcdreg_writereg(reg, SSD1306_DISPLAY_OFFSET);
+ lcdreg_writereg(reg, 0x0);
+
+ lcdreg_writereg(reg, SSD1306_DISPLAY_START_LINE);
+
+ lcdreg_writereg(reg, SSD1306_CHARGE_PUMP);
+ lcdreg_writereg(reg, 0x14);
+
+ lcdreg_writereg(reg, SSD1306_ADDRESS_MODE);
+ lcdreg_writereg(reg, 0x01); /* vertical */
+
+ lcdreg_writereg(reg, SSD1306_COL_RANGE);
+ lcdreg_writereg(reg, 0x00);
+ lcdreg_writereg(reg, lcdctrl_xres(ctrl) - 1);
+
+ lcdreg_writereg(reg, SSD1306_PAGE_RANGE);
+ lcdreg_writereg(reg, 0x00);
+ lcdreg_writereg(reg, (lcdctrl_yres(ctrl) / 8) - 1);
+
+ lcdreg_writereg(reg, SSD1306_SEG_REMAP_ON);
+
+ lcdreg_writereg(reg, SSD1306_COM_SCAN_REMAP);
+
+ lcdreg_writereg(reg, SSD1306_COM_PINS_CONFIG);
+ if (lcdctrl_yres(ctrl) == 64)
+ lcdreg_writereg(reg, 0x12);
+ else
+ lcdreg_writereg(reg, 0x02);
+
+ lcdreg_writereg(reg, SSD1306_PRECHARGE_PERIOD);
+ lcdreg_writereg(reg, 0xf1);
+
+ lcdreg_writereg(reg, SSD1306_VCOMH);
+ lcdreg_writereg(reg, 0x40);
+
+ lcdreg_writereg(reg, SSD1306_RESUME_TO_RAM);
+
+ lcdreg_writereg(reg, SSD1306_NORMAL_DISPLAY);
+
+ lcdreg_writereg(reg, SSD1306_CONTRAST);
+ if (lcdctrl_yres(ctrl) == 64)
+ lcdreg_writereg(reg, 0xcf);
+ else
+ lcdreg_writereg(reg, 0x8f);
+
+ /* The display is turned on later by core */
+
+ return 0;
+}
+
+static const struct of_device_id ada_ssd1306_ids[] = {
+ { .compatible = "ada,ssd1306-128x32", .data = (void *)32 },
+ { .compatible = "ada,ssd1306-128x64", .data = (void *)64 },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ada_ssd1306_ids);
+
+static int ada_ssd1306_probe_common(struct lcdreg *reg)
+{
+ const struct of_device_id *of_id;
+ struct device *dev = reg->dev;
+ struct lcdctrl *ctrl;
+ struct ssd1306_config cfg = {
+ .width = 128,
+ };
+
+ of_id = of_match_device(ada_ssd1306_ids, dev);
+ if (!of_id)
+ return -EINVAL;
+
+ cfg.height = (u32)of_id->data;
+ ctrl = devm_ssd1306_init(reg, &cfg);
+ if (IS_ERR(ctrl))
+ return PTR_ERR(ctrl);
+
+ ctrl->poweron = ada_ssd1306_poweron;
+
+ return devm_fbtft_lcdctrl_of_register(ctrl);
+}
+
+static int ada_ssd1306_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct lcdreg *reg;
+
+ reg = devm_lcdreg_i2c_init(client);
+ if (IS_ERR(reg))
+ return PTR_ERR(reg);
+
+ return ada_ssd1306_probe_common(reg);
+}
+
+static void ada_ssd1306_shutdown(struct i2c_client *client)
+{
+ struct fb_info *info = dev_get_drvdata(&client->dev);
+ struct fbtft_par *par = info->par;
+
+ lcdctrl_disable(par->lcdctrl);
+}
+
+static const struct i2c_device_id ssd1307fb_i2c_id[] = {
+ { "ada-ssd1306fb", 0 },
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, ssd1307fb_i2c_id);
+
+static struct i2c_driver ada_ssd1306_i2c_driver = {
+ .driver = {
+ .name = "ada-ssd1306fb",
+ .pm = FBTFT_PM_OPS,
+ .of_match_table = ada_ssd1306_ids,
+ },
+ .id_table = ssd1307fb_i2c_id,
+ .probe = ada_ssd1306_i2c_probe,
+ .shutdown = ada_ssd1306_shutdown,
+};
+module_i2c_driver(ada_ssd1306_i2c_driver);
+
+MODULE_ALIAS("i2c:ssd1306-128x32");
+MODULE_ALIAS("i2c:ssd1306-128x64");
+
+MODULE_DESCRIPTION("Framebuffer Driver for Adafruit Monochrome OLED displays");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
--
2.2.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem
2015-03-02 10:54 [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem Noralf Trønnes
@ 2015-03-21 18:23 ` Noralf Trønnes
2015-03-02 10:54 ` [RFC 2/7] staging: fbtft: lcdreg: add i2c support Noralf Trønnes
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-21 18:23 UTC (permalink / raw)
To: Thomas Petazzoni, Greg Kroah-Hartman, linux-fbdev, noralf; +Cc: driverdev-devel
Den 02.03.2015 11:54, skrev Noralf Trønnes:
> Hi,
>
> This patchset introduces a new API to minimize the coupling to the
> fbdev graphics subsystem. There have been calls to deprecate fbdev and
> new fbdev drivers are discouraged.
> Currently the FBTFT drivers are tightly coupled to fbdev through the
> fbtft module and the fbtft_par structure.
>
> What is FBTFT?
> FBTFT is a framework for writing drivers for MIPI DCS/DBI [1] like controllers.
> These controllers have the following characteristics:
> - Onchip graphics RAM that is scanned out to the display panel.
> - Registers to set operational parameters and write pixel data
> - SPI, I2C and/or parallel interface
>
> To provide better layering and provide opportunity for reuse,
> I propose the following layers:
>
> +-----------------------------+
> | fbdev |
> +-----------------------------+
> | fbtft |
> +-------------------------+---+
> | Display driver | |
> +---+---------------------+ |
> | | lcdctrl |
> | +-------------------------+
> | lcdreg |
> +-----------------------------+
> | Interface (SPI, parallel) |
> +-----------------------------+
>
> lcdreg
> ------
> This layer deals with the various LCD controller interface
> implementations (SPI, Intel 8080, I2C). It provides a simple interface
> for reading and writing to the LCD controller register.
> Especially the SPI interface has several modes and variations that
> makes it rather complex.
>
> lcdreg can be reused by drivers that drive these same controllers in
> digital RGB interface mode (MIPI DPI). Even when the parallel RGB
> interface is used for pixeldata, the controller some times has to be
> configured. This is usually done through a SPI interface.
>
> lcdctrl
> -------
> Layer providing an abstraction of the LCD controller and it's
> principal funtions:
> - Poweron initialization
> - Set display rotation
> - Set pixel format
> - Display blanking
> - Update grahics RAM
>
> This decouples the controller implementation from the graphics subsystem,
> and prepares for a possible future move to DRM or other grahics subsystems.
> The controller specific functionality is kept in library modules.
>
> fbtft
> -----
> The fbtft module gets some helper functions in fbtft-lcdctrl to map
> the lcdtrl functions into the fbtft_par structure.
> Later when all the drivers have been converted, large parts of the
> fbtft module can be removed.
>
>
> The I2C implementation of lcdreg is used in this proposal, because of
> it's simplicity. This is to keep focus on the architectural design.
> Implementations for SPI and the Intel 8080 bus interface exists.
>
> Display or Controller driver?
> Currently FBTFT has drivers for each controller it supports.
> Each driver has a default controller setup which often matches many
> displays, but no all (fbtftops.init_display()). A different init
> sequence (register values and delays) can be passed in using
> platform_data or as a DT property.
> I made a post to the DT mailinglist [2] about setting an init sequence
> from the Device Tree, but didn't get any replies. Based on an earlier
> post and comments I've read, it's looks like init sequence in DT is
> not going to happen. That's why this proposal shifts to having display
> drivers, with controller specific code in library modules.
>
> Device Tree questions
> - What about Device Tree property names that are used by all drivers
> (initialized, format, rotation)? Should they be prefixed in any way?
> - Device Tree compatible string. If a driver supports both SPI and I2C,
> can the same compatible string be used for both busses (ada-ssd1306fb)?
> I have seen examples where the bus is added as a postfix (-i2c).
> And which one is preferred: 'ada-ssd1306' or 'ada-ssd1306fb'?
>
> Why RFC?
> I did this because of some uncertainties I have with this proposal:
> 1. This changes almost the entire FBTFT stack. Would it be better to
> just move directly to DRM? DRM is the preferred way to write graphics
> drivers now, but fbdev is really a perfect match for FBTFT.
> And DRM doesn't have deferred io support AFAICT.
> 2. The change from controller centric drivers to display drivers is
> that correct?
> This will deprecate all of the current controller drivers.
>
> So any help in pinning down the path for moving FBTFT forward is appreciated.
>
>
> [1]: MIPI Alliance Display Interface Specifications:
> MIPI DCS - Display Command Set
> MIPI DBI - Display Bus Interface
> MIPI DPI - Display Pixel Interface
> http://mipi.org/specifications/display-interface
> [2]: http://article.gmane.org/gmane.linux.drivers.devicetree/109103
>
Hi,
I didn't get any comments to my questions in this cover letter,
so I'm bumping it.
The fbdev mailinglist is included this time, as I'm hoping to receive
comments about a future move of fbtft into fbdev.
I have also started writing code for drm/kms integration
of this patchset, too see what that look's like.
Regards,
Noralf Trønnes
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [RFC 0/7] staging: fbtft: minimize coupling to the fbdev subsystem
@ 2015-03-21 18:23 ` Noralf Trønnes
0 siblings, 0 replies; 19+ messages in thread
From: Noralf Trønnes @ 2015-03-21 18:23 UTC (permalink / raw)
To: Thomas Petazzoni, Greg Kroah-Hartman, linux-fbdev, noralf; +Cc: driverdev-devel
Den 02.03.2015 11:54, skrev Noralf Trønnes:
> Hi,
>
> This patchset introduces a new API to minimize the coupling to the
> fbdev graphics subsystem. There have been calls to deprecate fbdev and
> new fbdev drivers are discouraged.
> Currently the FBTFT drivers are tightly coupled to fbdev through the
> fbtft module and the fbtft_par structure.
>
> What is FBTFT?
> FBTFT is a framework for writing drivers for MIPI DCS/DBI [1] like controllers.
> These controllers have the following characteristics:
> - Onchip graphics RAM that is scanned out to the display panel.
> - Registers to set operational parameters and write pixel data
> - SPI, I2C and/or parallel interface
>
> To provide better layering and provide opportunity for reuse,
> I propose the following layers:
>
> +-----------------------------+
> | fbdev |
> +-----------------------------+
> | fbtft |
> +-------------------------+---+
> | Display driver | |
> +---+---------------------+ |
> | | lcdctrl |
> | +-------------------------+
> | lcdreg |
> +-----------------------------+
> | Interface (SPI, parallel) |
> +-----------------------------+
>
> lcdreg
> ------
> This layer deals with the various LCD controller interface
> implementations (SPI, Intel 8080, I2C). It provides a simple interface
> for reading and writing to the LCD controller register.
> Especially the SPI interface has several modes and variations that
> makes it rather complex.
>
> lcdreg can be reused by drivers that drive these same controllers in
> digital RGB interface mode (MIPI DPI). Even when the parallel RGB
> interface is used for pixeldata, the controller some times has to be
> configured. This is usually done through a SPI interface.
>
> lcdctrl
> -------
> Layer providing an abstraction of the LCD controller and it's
> principal funtions:
> - Poweron initialization
> - Set display rotation
> - Set pixel format
> - Display blanking
> - Update grahics RAM
>
> This decouples the controller implementation from the graphics subsystem,
> and prepares for a possible future move to DRM or other grahics subsystems.
> The controller specific functionality is kept in library modules.
>
> fbtft
> -----
> The fbtft module gets some helper functions in fbtft-lcdctrl to map
> the lcdtrl functions into the fbtft_par structure.
> Later when all the drivers have been converted, large parts of the
> fbtft module can be removed.
>
>
> The I2C implementation of lcdreg is used in this proposal, because of
> it's simplicity. This is to keep focus on the architectural design.
> Implementations for SPI and the Intel 8080 bus interface exists.
>
> Display or Controller driver?
> Currently FBTFT has drivers for each controller it supports.
> Each driver has a default controller setup which often matches many
> displays, but no all (fbtftops.init_display()). A different init
> sequence (register values and delays) can be passed in using
> platform_data or as a DT property.
> I made a post to the DT mailinglist [2] about setting an init sequence
> from the Device Tree, but didn't get any replies. Based on an earlier
> post and comments I've read, it's looks like init sequence in DT is
> not going to happen. That's why this proposal shifts to having display
> drivers, with controller specific code in library modules.
>
> Device Tree questions
> - What about Device Tree property names that are used by all drivers
> (initialized, format, rotation)? Should they be prefixed in any way?
> - Device Tree compatible string. If a driver supports both SPI and I2C,
> can the same compatible string be used for both busses (ada-ssd1306fb)?
> I have seen examples where the bus is added as a postfix (-i2c).
> And which one is preferred: 'ada-ssd1306' or 'ada-ssd1306fb'?
>
> Why RFC?
> I did this because of some uncertainties I have with this proposal:
> 1. This changes almost the entire FBTFT stack. Would it be better to
> just move directly to DRM? DRM is the preferred way to write graphics
> drivers now, but fbdev is really a perfect match for FBTFT.
> And DRM doesn't have deferred io support AFAICT.
> 2. The change from controller centric drivers to display drivers is
> that correct?
> This will deprecate all of the current controller drivers.
>
> So any help in pinning down the path for moving FBTFT forward is appreciated.
>
>
> [1]: MIPI Alliance Display Interface Specifications:
> MIPI DCS - Display Command Set
> MIPI DBI - Display Bus Interface
> MIPI DPI - Display Pixel Interface
> http://mipi.org/specifications/display-interface
> [2]: http://article.gmane.org/gmane.linux.drivers.devicetree/109103
>
Hi,
I didn't get any comments to my questions in this cover letter,
so I'm bumping it.
The fbdev mailinglist is included this time, as I'm hoping to receive
comments about a future move of fbtft into fbdev.
I have also started writing code for drm/kms integration
of this patchset, too see what that look's like.
Regards,
Noralf Trønnes
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 19+ messages in thread