[PATCH v3 0/5] fbtft: make it work with DMA enabled SPI

[PATCH v3 0/5] fbtft: make it work with DMA enabled SPI

[Andy Shevchenko]

This series enables 64x48 OLED display and fixes the driver to work with DMA
enabled SPI properly.

Has been tested on Intel Edison board with Adafruit 2'8" and SSD1306 64x48
(Sparkfun for Intel Edison) OLED displays at their maximum speed (25MHz and
10MHz).

Since v2:
- fix kbuild bot warning
- remove duplication of might_sleep() (Noralf)
- re-do DMA appoach based on Noralf's suggestion
- append Noralf's tags

Andy Shevchenko (5):
  staging: fbtft: convert fbtft_reset() to be non-atomic
  staging: fbtft: remove custom DMA mapped buffer
  staging: fbtft: propagate error code from kstrto*()
  staging: fbtft: fb_ssd1306: Support smaller screen sizes
  staging: fbtft: fb_ssd1306: Refactor write_vmem()

 drivers/staging/fbtft/fb_ra8875.c   |  4 ----
 drivers/staging/fbtft/fb_ssd1306.c  | 37 ++++++++++++++++++++++++++++---------
 drivers/staging/fbtft/fbtft-core.c  | 30 ++++++------------------------
 drivers/staging/fbtft/fbtft-io.c    |  4 ----
 drivers/staging/fbtft/fbtft-sysfs.c |  7 +------
 drivers/staging/fbtft/fbtft.h       |  1 -
 6 files changed, 35 insertions(+), 48 deletions(-)

-- 
2.11.0

[PATCH v3 1/5] staging: fbtft: convert fbtft_reset() to be non-atomic

[Andy Shevchenko]

First of all, fbtft in current state doesn't allow to override GPIOs to be
optional, like "reset" one. It might be a bug somewhere, but rather out of
scope of this fix.

Second, not all GPIOs available on the board would be SoC based, some of them
might sit on I2C GPIO expanders, for example, on Intel Edison/Arduino, and thus
any communication with them might sleep.

Besides that using udelay() and mdelay() is kinda resource wasteful.

Summarize all of the above, convert fbtft_reset() function to non-atomic
variant by using gpio_set_value_cansleep(), usleep_range(), and msleep().

Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fbtft-core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index bbe89c9c4fb9..300a1e4505b9 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -337,10 +337,10 @@ static void fbtft_reset(struct fbtft_par *par)
 	if (par->gpio.reset == -1)
 		return;
 	fbtft_par_dbg(DEBUG_RESET, par, "%s()\n", __func__);
-	gpio_set_value(par->gpio.reset, 0);
-	udelay(20);
-	gpio_set_value(par->gpio.reset, 1);
-	mdelay(120);
+	gpio_set_value_cansleep(par->gpio.reset, 0);
+	usleep_range(20, 40);
+	gpio_set_value_cansleep(par->gpio.reset, 1);
+	msleep(120);
 }
 
 static void fbtft_update_display(struct fbtft_par *par, unsigned int start_line,
-- 
2.11.0

[PATCH v3 2/5] staging: fbtft: remove custom DMA mapped buffer

[Andy Shevchenko]

There is no need to duplicate what SPI core already does, i.e. mapping buffers
for DMA capable transfers.

Remove all related pices of code.

Note, that code, besides its redundancy, was buggy: DMA address potentially can
be 0, SPI slave device has nothing to do with DMA capable device properties and
DMA mask in particular.

Suggested-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fb_ra8875.c  |  4 ----
 drivers/staging/fbtft/fbtft-core.c | 22 ++--------------------
 drivers/staging/fbtft/fbtft-io.c   |  4 ----
 drivers/staging/fbtft/fbtft.h      |  1 -
 4 files changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/fbtft/fb_ra8875.c b/drivers/staging/fbtft/fb_ra8875.c
index 308a244972aa..a6bc1503a50e 100644
--- a/drivers/staging/fbtft/fb_ra8875.c
+++ b/drivers/staging/fbtft/fb_ra8875.c
@@ -42,10 +42,6 @@ static int write_spi(struct fbtft_par *par, void *buf, size_t len)
 	}
 
 	spi_message_init(&m);
-	if (par->txbuf.dma && buf == par->txbuf.buf) {
-		t.tx_dma = par->txbuf.dma;
-		m.is_dma_mapped = 1;
-	}
 	spi_message_add_tail(&t, &m);
 	return spi_sync(par->spi, &m);
 }
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 300a1e4505b9..4e13090c7fbd 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -32,7 +32,6 @@
 #include <linux/backlight.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
-#include <linux/dma-mapping.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <video/mipi_display.h>
@@ -44,12 +43,6 @@ static unsigned long debug;
 module_param(debug, ulong, 0);
 MODULE_PARM_DESC(debug, "override device debug level");
 
-#ifdef CONFIG_HAS_DMA
-static bool dma = true;
-module_param(dma, bool, 0);
-MODULE_PARM_DESC(dma, "Use DMA buffer");
-#endif
-
 void fbtft_dbg_hex(const struct device *dev, int groupsize,
 			void *buf, size_t len, const char *fmt, ...)
 {
@@ -836,17 +829,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 #endif
 
 	if (txbuflen > 0) {
-#ifdef CONFIG_HAS_DMA
-		if (dma) {
-			dev->coherent_dma_mask = ~0;
-			txbuf = dmam_alloc_coherent(dev, txbuflen,
-						    &par->txbuf.dma, GFP_DMA);
-		} else
-#endif
-		{
-			txbuf = devm_kzalloc(par->info->device,
-					     txbuflen, GFP_KERNEL);
-		}
+		txbuf = devm_kzalloc(par->info->device, txbuflen, GFP_KERNEL);
 		if (!txbuf)
 			goto alloc_fail;
 		par->txbuf.buf = txbuf;
@@ -975,8 +958,7 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
 	fbtft_sysfs_init(par);
 
 	if (par->txbuf.buf)
-		sprintf(text1, ", %zu KiB %sbuffer memory",
-			par->txbuf.len >> 10, par->txbuf.dma ? "DMA " : "");
+		sprintf(text1, ", %zu KiB buffer memory", par->txbuf.len >> 10);
 	if (spi)
 		sprintf(text2, ", spi%d.%d at %d MHz", spi->master->bus_num,
 			spi->chip_select, spi->max_speed_hz / 1000000);
diff --git a/drivers/staging/fbtft/fbtft-io.c b/drivers/staging/fbtft/fbtft-io.c
index 4dcea2e0b3ae..d86840548b74 100644
--- a/drivers/staging/fbtft/fbtft-io.c
+++ b/drivers/staging/fbtft/fbtft-io.c
@@ -22,10 +22,6 @@ int fbtft_write_spi(struct fbtft_par *par, void *buf, size_t len)
 	}
 
 	spi_message_init(&m);
-	if (par->txbuf.dma && buf == par->txbuf.buf) {
-		t.tx_dma = par->txbuf.dma;
-		m.is_dma_mapped = 1;
-	}
 	spi_message_add_tail(&t, &m);
 	return spi_sync(par->spi, &m);
 }
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index aacdde92cc2e..b09804773ff2 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -209,7 +209,6 @@ struct fbtft_par {
 	u32 pseudo_palette[16];
 	struct {
 		void *buf;
-		dma_addr_t dma;
 		size_t len;
 	} txbuf;
 	u8 *buf;
-- 
2.11.0

[PATCH v3 3/5] staging: fbtft: propagate error code from kstrto*()

[Andy Shevchenko]

kstrto*() functions return proper error code.

Do propogate it to the user.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fbtft-sysfs.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
index 8d8bd12b90a1..5922f1b6d8d6 100644
--- a/drivers/staging/fbtft/fbtft-sysfs.c
+++ b/drivers/staging/fbtft/fbtft-sysfs.c
@@ -4,7 +4,6 @@
 static int get_next_ulong(char **str_p, unsigned long *val, char *sep, int base)
 {
 	char *p_val;
-	int ret;
 
 	if (!str_p || !(*str_p))
 		return -EINVAL;
@@ -14,11 +13,7 @@ static int get_next_ulong(char **str_p, unsigned long *val, char *sep, int base)
 	if (!p_val)
 		return -EINVAL;
 
-	ret = kstrtoul(p_val, base, val);
-	if (ret)
-		return -EINVAL;
-
-	return 0;
+	return kstrtoul(p_val, base, val);
 }
 
 int fbtft_gamma_parse_str(struct fbtft_par *par, unsigned long *curves,
-- 
2.11.0

[PATCH v3 4/5] staging: fbtft: fb_ssd1306: Support smaller screen sizes

[Andy Shevchenko]

There is 64x48 display exists. In order to support that set multiplexer
to 48 pixels and window address to proper position in the graphic display
data RAM.

Acked-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fb_ssd1306.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/staging/fbtft/fb_ssd1306.c b/drivers/staging/fbtft/fb_ssd1306.c
index 80fc57029fee..bede2d5a5571 100644
--- a/drivers/staging/fbtft/fb_ssd1306.c
+++ b/drivers/staging/fbtft/fb_ssd1306.c
@@ -62,6 +62,8 @@ static int init_display(struct fbtft_par *par)
 	write_reg(par, 0xA8);
 	if (par->info->var.yres == 64)
 		write_reg(par, 0x3F);
+	else if (par->info->var.yres == 48)
+		write_reg(par, 0x2F);
 	else
 		write_reg(par, 0x1F);
 
@@ -95,6 +97,9 @@ static int init_display(struct fbtft_par *par)
 	if (par->info->var.yres == 64)
 		/* A[4]=1b, Alternative COM pin configuration */
 		write_reg(par, 0x12);
+	else if (par->info->var.yres == 48)
+		/* A[4]=1b, Alternative COM pin configuration */
+		write_reg(par, 0x12);
 	else
 		/* A[4]=0b, Sequential COM pin configuration */
 		write_reg(par, 0x02);
@@ -124,6 +129,19 @@ static int init_display(struct fbtft_par *par)
 	return 0;
 }
 
+static void set_addr_win_64x48(struct fbtft_par *par)
+{
+	/* Set Column Address */
+	write_reg(par, 0x21);
+	write_reg(par, 0x20);
+	write_reg(par, 0x5F);
+
+	/* Set Page Address */
+	write_reg(par, 0x22);
+	write_reg(par, 0x0);
+	write_reg(par, 0x5);
+}
+
 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 */
@@ -132,6 +150,9 @@ static void set_addr_win(struct fbtft_par *par, int xs, int ys, int xe, int ye)
 	write_reg(par, 0x10 | 0x0);
 	/* Set Display Start Line */
 	write_reg(par, 0x40 | 0x0);
+
+	if (par->info->var.xres == 64 && par->info->var.yres == 48)
+		set_addr_win_64x48(par);
 }
 
 static int blank(struct fbtft_par *par, bool on)
-- 
2.11.0

[PATCH v3 5/5] staging: fbtft: fb_ssd1306: Refactor write_vmem()

[Andy Shevchenko]

Refactor write_vmem() for sake of readability.

While here, fix indentation in one comment.

Acked-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fb_ssd1306.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/fbtft/fb_ssd1306.c b/drivers/staging/fbtft/fb_ssd1306.c
index bede2d5a5571..76f7da3c7703 100644
--- a/drivers/staging/fbtft/fb_ssd1306.c
+++ b/drivers/staging/fbtft/fb_ssd1306.c
@@ -84,7 +84,7 @@ static int init_display(struct fbtft_par *par)
 	/* Vertical addressing mode  */
 	write_reg(par, 0x01);
 
-	/*Set Segment Re-map */
+	/* Set Segment Re-map */
 	/* column address 127 is mapped to SEG0 */
 	write_reg(par, 0xA0 | 0x1);
 
@@ -183,26 +183,24 @@ static int set_gamma(struct fbtft_par *par, unsigned long *curves)
 static int write_vmem(struct fbtft_par *par, size_t offset, size_t len)
 {
 	u16 *vmem16 = (u16 *)par->info->screen_buffer;
+	u32 xres = par->info->var.xres;
+	u32 yres = par->info->var.yres;
 	u8 *buf = par->txbuf.buf;
 	int x, y, i;
 	int ret = 0;
 
-	for (x = 0; x < par->info->var.xres; x++) {
-		for (y = 0; y < par->info->var.yres/8; y++) {
+	for (x = 0; x < xres; x++) {
+		for (y = 0; y < 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 |= (vmem16[(y * 8 + i) * 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);
+	ret = par->fbtftops.write(par, par->txbuf.buf, xres * yres / 8);
 	if (ret < 0)
 		dev_err(par->info->device, "write failed and returned: %d\n",
 			ret);
-- 
2.11.0

Re: [PATCH v3 0/5] fbtft: make it work with DMA enabled SPI

[Andy Shevchenko]

On Tue, 2017-01-03 at 20:29 +0200, Andy Shevchenko wrote:
> This series enables 64x48 OLED display and fixes the driver to work
> with DMA
> enabled SPI properly.
> 
> Has been tested on Intel Edison board with Adafruit 2'8" and SSD1306
> 64x48
> (Sparkfun for Intel Edison) OLED displays at their maximum speed
> (25MHz and
> 10MHz).

Noralf, are you okay with this version of series?

> 
> Since v2:
> - fix kbuild bot warning
> - remove duplication of might_sleep() (Noralf)
> - re-do DMA appoach based on Noralf's suggestion
> - append Noralf's tags
> 
> Andy Shevchenko (5):
>   staging: fbtft: convert fbtft_reset() to be non-atomic
>   staging: fbtft: remove custom DMA mapped buffer
>   staging: fbtft: propagate error code from kstrto*()
>   staging: fbtft: fb_ssd1306: Support smaller screen sizes
>   staging: fbtft: fb_ssd1306: Refactor write_vmem()
> 
>  drivers/staging/fbtft/fb_ra8875.c   |  4 ----
>  drivers/staging/fbtft/fb_ssd1306.c  | 37
> ++++++++++++++++++++++++++++---------
>  drivers/staging/fbtft/fbtft-core.c  | 30 ++++++--------------------
> ----
>  drivers/staging/fbtft/fbtft-io.c    |  4 ----
>  drivers/staging/fbtft/fbtft-sysfs.c |  7 +------
>  drivers/staging/fbtft/fbtft.h       |  1 -
>  6 files changed, 35 insertions(+), 48 deletions(-)
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/5] fbtft: make it work with DMA enabled SPI

[Noralf Trønnes]

Den 11.01.2017 15:43, skrev Andy Shevchenko:
> On Tue, 2017-01-03 at 20:29 +0200, Andy Shevchenko wrote:
>> This series enables 64x48 OLED display and fixes the driver to work
>> with DMA
>> enabled SPI properly.
>>
>> Has been tested on Intel Edison board with Adafruit 2'8" and SSD1306
>> 64x48
>> (Sparkfun for Intel Edison) OLED displays at their maximum speed
>> (25MHz and
>> 10MHz).
> Noralf, are you okay with this version of series?

Yes, Greg applies them without my ack, unless it touches core stuff.

Series:
Acked-by: Noralf Trønnes <noralf@tronnes.org>


>> Since v2:
>> - fix kbuild bot warning
>> - remove duplication of might_sleep() (Noralf)
>> - re-do DMA appoach based on Noralf's suggestion
>> - append Noralf's tags
>>
>> Andy Shevchenko (5):
>>    staging: fbtft: convert fbtft_reset() to be non-atomic
>>    staging: fbtft: remove custom DMA mapped buffer
>>    staging: fbtft: propagate error code from kstrto*()
>>    staging: fbtft: fb_ssd1306: Support smaller screen sizes
>>    staging: fbtft: fb_ssd1306: Refactor write_vmem()
>>
>>   drivers/staging/fbtft/fb_ra8875.c   |  4 ----
>>   drivers/staging/fbtft/fb_ssd1306.c  | 37
>> ++++++++++++++++++++++++++++---------
>>   drivers/staging/fbtft/fbtft-core.c  | 30 ++++++--------------------
>> ----
>>   drivers/staging/fbtft/fbtft-io.c    |  4 ----
>>   drivers/staging/fbtft/fbtft-sysfs.c |  7 +------
>>   drivers/staging/fbtft/fbtft.h       |  1 -
>>   6 files changed, 35 insertions(+), 48 deletions(-)
>>

Re: [PATCH v3 1/5] staging: fbtft: convert fbtft_reset() to be non-atomic

[Greg Kroah-Hartman]

On Tue, Jan 03, 2017 at 08:29:45PM +0200, Andy Shevchenko wrote:
> First of all, fbtft in current state doesn't allow to override GPIOs to be
> optional, like "reset" one. It might be a bug somewhere, but rather out of
> scope of this fix.
> 
> Second, not all GPIOs available on the board would be SoC based, some of them
> might sit on I2C GPIO expanders, for example, on Intel Edison/Arduino, and thus
> any communication with them might sleep.
> 
> Besides that using udelay() and mdelay() is kinda resource wasteful.
> 
> Summarize all of the above, convert fbtft_reset() function to non-atomic
> variant by using gpio_set_value_cansleep(), usleep_range(), and msleep().
> 
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Andy, this series came through with the charmap all corrupted and git
didn't like them at all.  Can you fix up and resend?

thanks,

greg k-h

Re: [PATCH v3 1/5] staging: fbtft: convert fbtft_reset() to be non-atomic

[Andy Shevchenko]

On Thu, 2017-01-19 at 10:49 +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 03, 2017 at 08:29:45PM +0200, Andy Shevchenko wrote:
> > First of all, fbtft in current state doesn't allow to override GPIOs
> > to be
> > optional, like "reset" one. It might be a bug somewhere, but rather
> > out of
> > scope of this fix.
> > 
> > Second, not all GPIOs available on the board would be SoC based,
> > some of them
> > might sit on I2C GPIO expanders, for example, on Intel
> > Edison/Arduino, and thus
> > any communication with them might sleep.
> > 
> > Besides that using udelay() and mdelay() is kinda resource wasteful.
> > 
> > Summarize all of the above, convert fbtft_reset() function to non-
> > atomic
> > variant by using gpio_set_value_cansleep(), usleep_range(), and
> > msleep().
> > 
> > Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Andy, this series came through with the charmap all corrupted and git
> didn't like them at all.  Can you fix up and resend?

Strange. I used usual git send-email. Okay, just resent (as v4).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy