Re: [PATCH] added fbtft ssd1305 controller support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 01/26/2016 05:00 PM, Dan Carpenter wrote:
The subject should be:

[PATCH] Staging: fbtft: added ssd1305 controller support

On Tue, Jan 26, 2016 at 04:07:24PM +0300, Alexey Mednyy wrote:
Signed-off-by: Alexey Mednyy <swexru@xxxxxxxxx>
---
  drivers/staging/fbtft/Kconfig      |   6 +
  drivers/staging/fbtft/Makefile     |   1 +
  drivers/staging/fbtft/fb_ssd1305.c | 271 +++++++++++++++++++++++++++++++++++++
  3 files changed, 278 insertions(+)
  create mode 100644 drivers/staging/fbtft/fb_ssd1305.c

diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 883ff5b..0bfc776 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -117,6 +117,12 @@ config FB_TFT_SSD1289
  	help
  	  Framebuffer support for SSD1289
+config FB_TFT_SSD1305
+        tristate "FB driver for the SSD1305 OLED Controller"
+        depends on FB_TFT
+        help
+          Framebuffer support for SSD1305
Maybe say who the vendor is or where this is used or something.
Checkpatch is supposed to complain about this kind of stuff.  :(
I just followed other descriptions above and below mine, every other fbtft drivers have this short description. Do I really need longer description?

+
  config FB_TFT_SSD1306
  	tristate "FB driver for the SSD1306 OLED Controller"
  	depends on FB_TFT
diff --git a/drivers/staging/fbtft/Makefile b/drivers/staging/fbtft/Makefile
index 4f9071d..1ddc764 100644
--- a/drivers/staging/fbtft/Makefile
+++ b/drivers/staging/fbtft/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_FB_TFT_RA8875)      += fb_ra8875.o
  obj-$(CONFIG_FB_TFT_S6D02A1)     += fb_s6d02a1.o
  obj-$(CONFIG_FB_TFT_S6D1121)     += fb_s6d1121.o
  obj-$(CONFIG_FB_TFT_SSD1289)     += fb_ssd1289.o
+obj-$(CONFIG_FB_TFT_SSD1305)     += fb_ssd1305.o
  obj-$(CONFIG_FB_TFT_SSD1306)     += fb_ssd1306.o
  obj-$(CONFIG_FB_TFT_SSD1331)     += fb_ssd1331.o
  obj-$(CONFIG_FB_TFT_SSD1351)     += fb_ssd1351.o
diff --git a/drivers/staging/fbtft/fb_ssd1305.c b/drivers/staging/fbtft/fb_ssd1305.c
new file mode 100644
index 0000000..2f65fb8
--- /dev/null
+++ b/drivers/staging/fbtft/fb_ssd1305.c
@@ -0,0 +1,271 @@
+/*
+ * FB driver for the SSD1305 OLED Controller
+ *
+ * based on SSD1306 driver by Noralf Tronnes
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+
+#include "fbtft.h"
+
+#define DRVNAME		"fb_ssd1305"
+#define WIDTH		128
+#define HEIGHT		64
WIDTH and HEIGHT don't add anything.  Just say ".width = 128,".

+
+/*
+ *
+ *
+ * write_reg() caveat:
+ *
+ *    This doesn't work because D/C has to be LOW for both values:
+ *      write_reg(par, val1, val2);
+ *
+ *    Do it like this:
+ *       write_reg(par, val1);
+ *      write_reg(par, val2);
Clean up this comment a bit.

+*/
+
+/* Init sequence taken from the Adafruit SSD1306 Arduino library */
+static int init_display(struct fbtft_par *par)
+{
+	par->fbtftops.reset(par);
+
+	if (par->gamma.curves[0] == 0) {
+		mutex_lock(&par->gamma.lock);
+		if (par->info->var.yres == 64)
+			par->gamma.curves[0] = 0xCF;
+		else
+			par->gamma.curves[0] = 0x8F;
+		mutex_unlock(&par->gamma.lock);
+	}
+
+	/* Set Display OFF */
+	write_reg(par, 0xAE);
+
+	/* Set Display Clock Divide Ratio/ Oscillator Frequency */
+	write_reg(par, 0xD5);
+	write_reg(par, 0x80);
+
+	/* Set Multiplex Ratio */
+	write_reg(par, 0xA8);
+	if (par->info->var.yres == 64)
+		write_reg(par, 0x3F);
+	else
+		write_reg(par, 0x1F);
+
+	/* Set Display Offset */
+	write_reg(par, 0xD3);
+	write_reg(par, 0x0);
+
+	/* Set Display Start Line */
+	write_reg(par, 0x40 | 0x0);
+
+	/* Charge Pump Setting */
+	write_reg(par, 0x8D);
+	/* A[2] = 1b, Enable charge pump during display on */
+	write_reg(par, 0x14);
+
+	/* Set Memory Addressing Mode */
+	write_reg(par, 0x20);
+	/* Vertical addressing mode  */
+	write_reg(par, 0x01);
+
+	/*Set Segment Re-map */
+	/* column address 127 is mapped to SEG0 */
+	write_reg(par, 0xA0 | ((par->info->var.rotate == 180) ? 0x0 : 0x1));
+
+	/* Set COM Output Scan Direction */
+	/* remapped mode. Scan from COM[N-1] to COM0 */
+	write_reg(par, ((par->info->var.rotate == 180) ? 0xC8 : 0xC0));
+
+	/* Set COM Pins Hardware Configuration */
+	write_reg(par, 0xDA);
+	if (par->info->var.yres == 64)
+		/* A[4]=1b, Alternative COM pin configuration */
+		write_reg(par, 0x12);
+	else
+		/* A[4]=0b, Sequential COM pin configuration */
+		write_reg(par, 0x02);
Add curly braces to multi-line indents for readability.

+
+	/* Set Pre-charge Period */
+	write_reg(par, 0xD9);
+	write_reg(par, 0xF1);
+
+	/* Set VCOMH Deselect Level */
+	write_reg(par, 0xDB);
+	/* according to the datasheet, this value is out of bounds */
Huh?  Why are we writing out of bounds then?

+	write_reg(par, 0x40);
+
+	/* Entire Display ON */
+	/* Resume to RAM content display. Output follows RAM content */
+	write_reg(par, 0xA4);
+
+	/* Set Normal Display
+	 *   0 in RAM: OFF in display panel
+	 *  1 in RAM: ON in display panel
Align this comment.

+	 */
+	write_reg(par, 0xA6);
+
+	/* Set Display ON */
+	write_reg(par, 0xAF);
+
+	return 0;
+}
+
+/* Check if all necessary GPIOS defined */
+static int verify_gpios(struct fbtft_par *par)
+{
+	int i;
+
+	dev_dbg(par->info->device, "%s()\n", __func__);
Remove this.  Use ftrace.

+
+	if (par->gpio.wr < 0) {
+		dev_err(par->info->device,
+			"Missing info about 'wr' (aka E) gpio. Aborting.\n");
+		return -EINVAL;
+	}
+	for (i = 0; i < 8; ++i) {
+		if (par->gpio.db[i] < 0) {
+			dev_err(par->info->device,
+				"Missing info about 'db[%i]' gpio. Aborting.\n",
+				i);
+			return -EINVAL;
+		}
+	}
+	if (par->gpio.dc < 0) {
+		dev_err(par->info->device,
+			"Missing info about 'dc' gpio. Aborting.\n");
+		return -EINVAL;
+	}
+	if (par->gpio.rd < 0) {
+		dev_err(par->info->device,
+			"Missing info about 'rd' gpio. Aborting.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static unsigned long
+request_gpios_match(struct fbtft_par *par, const struct fbtft_gpio *gpio)
+{
+	dev_dbg(par->info->device, "%s('%s')\n", __func__, gpio->name);
Do we really need this?

+
+	if (strcasecmp(gpio->name, "wr") == 0) {
+		par->gpio.wr = gpio->gpio;
+		return GPIOF_OUT_INIT_LOW;
+	} else if (strcasecmp(gpio->name, "dc") == 0) {
+		par->gpio.dc = gpio->gpio;
+		return GPIOF_OUT_INIT_HIGH;
+	} else if (strcasecmp(gpio->name, "rd") == 0) {
+		par->gpio.rd = gpio->gpio;
+		return GPIOF_OUT_INIT_HIGH;
+	}
+
+	return FBTFT_GPIO_NO_MATCH;
+}
+
+static void set_addr_win(struct fbtft_par *par, int xs, int ys, int xe, int ye)
+{
+	/* Set Lower Column Start Address for Page Addressing Mode */
+	write_reg(par, 0x00 | ((par->info->var.rotate == 180) ? 0x0 : 0x4));
+	/* Set Higher Column Start Address for Page Addressing Mode */
+	write_reg(par, 0x10 | 0x0);
+	/* Set Display Start Line */
+	write_reg(par, 0x40 | 0x0);
+}
+
+static int blank(struct fbtft_par *par, bool on)
+{
+	fbtft_par_dbg(DEBUG_BLANK, par, "%s(blank=%s)\n",
+		      __func__, on ? "true" : "false");
Remove.

+
+	if (on)
+		write_reg(par, 0xAE);
+	else
+		write_reg(par, 0xAF);
+	return 0;
+}
+
+/* Gamma is used to control Contrast */
+static int set_gamma(struct fbtft_par *par, unsigned long *curves)
+{
+	/* apply mask */
Don't write /* increment i by one */ style comments, assume we know C.

+	curves[0] &= 0xFF;
+
+	/* Set Contrast Control for BANK0 */
+	write_reg(par, 0x81);
+	write_reg(par, curves[0]);
+
+	return 0;
+}
+
+static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
+{
+	u16 *vmem16 = (u16 *) par->info->screen_buffer;
Doesn't checkpatch.pl complain about the space?
No, It's quiet about that :(
+	u8 *buf = par->txbuf.buf;
+	int x, y, i;
+	int ret = 0;
Don't initialize ret to a bogus value.  GCC has a unintialized variable
detector but you are disabling this error checking tool.


regards,
dan carpenter

+
+	for (x = 0; x < par->info->var.xres; x++) {
+		for (y = 0; y < par->info->var.yres / 8; y++) {
+			*buf = 0x00;
+			for (i = 0; i < 8; i++)
+				*buf |= (vmem16[(y * 8 + i) *
+						par->info->var.xres + x] ?
+					 1 : 0) << i;
+			buf++;
+		}
+	}
+
+	/* Write data */
+	gpio_set_value(par->gpio.dc, 1);
+	ret = par->fbtftops.write(par, par->txbuf.buf,
+				  par->info->var.xres * par->info->var.yres /
+				  8);
+	if (ret < 0)
+		dev_err(par->info->device, "write failed and returned: %d\n",
+			ret);
+	return ret;
+}
+
+static struct fbtft_display display = {
+	.regwidth = 8,
+	.width = WIDTH,
+	.height = HEIGHT,
+	.gamma_num = 1,
+	.gamma_len = 1,
+	.gamma = "00",
+	.fbtftops = {
+		.write_vmem = write_vmem,
+		.init_display = init_display,
+		.verify_gpios = verify_gpios,
+		.request_gpios_match = request_gpios_match,
+		.set_addr_win = set_addr_win,
+		.blank = blank,
+		.set_gamma = set_gamma,
+	},
+};
+
+FBTFT_REGISTER_DRIVER(DRVNAME, "solomon,ssd1305", &display);
+
+MODULE_ALIAS("platform:" DRVNAME);
+MODULE_ALIAS("platform:ssd1305");
+
+MODULE_DESCRIPTION("SSD1305 OLED Driver");
+MODULE_AUTHOR("Alexey Mednyy");
+MODULE_LICENSE("GPL");
--
2.5.0

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Thank you for the review.
On the way to submit patch v2
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux