[PATCH v2 0/6] fbtft: make it work with DMA enabled SPI

[PATCH v2 0/6] 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).

Andy Shevchenko (6):
  staging: fbtft: convert fbtft_reset() to be non-atomic
  staging: fbtft: do not override DMA coherent mask
  staging: fbtft: fallback to usual allocation when DMA fails
  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_ssd1306.c  | 37 ++++++++++++++++++++++++++++---------
 drivers/staging/fbtft/fbtft-core.c  | 15 +++++++++------
 drivers/staging/fbtft/fbtft-sysfs.c |  6 +-----
 3 files changed, 38 insertions(+), 20 deletions(-)

-- 
2.11.0

[PATCH v2 1/6] 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().

To avoid potential use in atomic context annotate it via might_sleep() macro.

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

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index bbe89c9c4fb9..e8bf0d1ec11f 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -336,11 +336,14 @@ static void fbtft_reset(struct fbtft_par *par)
 {
 	if (par->gpio.reset == -1)
 		return;
+
+	might_sleep();
+
 	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 v2 2/6] staging: fbtft: do not override DMA coherent mask

[Andy Shevchenko]

Usually it's not consumer's business to override resources passed from
provider, in particularly DMA coherent mask.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fbtft-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index e8bf0d1ec11f..226be8c09768 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 	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
-- 
2.11.0

[PATCH v2 3/6] staging: fbtft: fallback to usual allocation when DMA fails

[Andy Shevchenko]

Fall back to usual allocation method if DMA coherent allocation fails.

SPI framework will map and use DMA mapped memory when possible.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/fbtft/fbtft-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 226be8c09768..9f024986aff4 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -843,7 +843,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
 		if (dma) {
 			txbuf = dmam_alloc_coherent(dev, txbuflen,
 						    &par->txbuf.dma, GFP_DMA);
-		} else
+		}
+		if (!txbuf)
 #endif
 		{
 			txbuf = devm_kzalloc(par->info->device,
-- 
2.11.0

[PATCH v2 4/6] 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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-sysfs.c b/drivers/staging/fbtft/fbtft-sysfs.c
index 8d8bd12b90a1..7ec5330a292f 100644
--- a/drivers/staging/fbtft/fbtft-sysfs.c
+++ b/drivers/staging/fbtft/fbtft-sysfs.c
@@ -14,11 +14,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 v2 5/6] 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.

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 v2 6/6] staging: fbtft: fb_ssd1306: Refactor write_vmem()

[Andy Shevchenko]

Refactor write_vmem() for sake of readability.

While here, fix indentation in one comment.

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 v2 0/6] fbtft: make it work with DMA enabled SPI

[Andy Shevchenko]

On Mon, 2017-01-02 at 13:35 +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).

It should be v1, but here we are.

> 
> Andy Shevchenko (6):
>   staging: fbtft: convert fbtft_reset() to be non-atomic
>   staging: fbtft: do not override DMA coherent mask
>   staging: fbtft: fallback to usual allocation when DMA fails
>   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_ssd1306.c  | 37
> ++++++++++++++++++++++++++++---------
>  drivers/staging/fbtft/fbtft-core.c  | 15 +++++++++------
>  drivers/staging/fbtft/fbtft-sysfs.c |  6 +-----
>  3 files changed, 38 insertions(+), 20 deletions(-)
> 

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

Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask

[Noralf Trønnes]

Den 02.01.2017 12:35, skrev Andy Shevchenko:
> Usually it's not consumer's business to override resources passed from
> provider, in particularly DMA coherent mask.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/staging/fbtft/fbtft-core.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index e8bf0d1ec11f..226be8c09768 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display,
>   	if (txbuflen > 0) {
>   #ifdef CONFIG_HAS_DMA
>   		if (dma) {
> -			dev->coherent_dma_mask = ~0;

Can we make this conditional like in of_dma_configure():

          if (!dev->coherent_dma_mask)
                 dev->coherent_dma_mask = DMA_BIT_MASK(32);

If not, I guess the mask has to be set before adding the spi device in
fbtft_device.c to keep it from breaking.

Noralf.

>   			txbuf = dmam_alloc_coherent(dev, txbuflen,
>   						    &par->txbuf.dma, GFP_DMA);
>   		} else

Re: [PATCH v2 4/6] staging: fbtft: propagate error code from kstrto*()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Hi Andy,

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.10-rc2 next-20161224]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fbtft-make-it-work-with-DMA-enabled-SPI/20170102-201151
config: openrisc-allyesconfig (attached as .config)
compiler: or32-linux-gcc (GCC) 4.5.1-or32-1.0rc1
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   drivers/staging/fbtft/fbtft-sysfs.c: In function 'get_next_ulong':
>> drivers/staging/fbtft/fbtft-sysfs.c:7:6: warning: unused variable 'ret'

vim +/ret +7 drivers/staging/fbtft/fbtft-sysfs.c

c296d5f9 Thomas Petazzoni 2014-12-31   1  #include "fbtft.h"
f3e5df43 Peter Poklop     2015-03-18   2  #include "internal.h"
c296d5f9 Thomas Petazzoni 2014-12-31   3  
c296d5f9 Thomas Petazzoni 2014-12-31   4  static int get_next_ulong(char **str_p, unsigned long *val, char *sep, int base)
c296d5f9 Thomas Petazzoni 2014-12-31   5  {
c296d5f9 Thomas Petazzoni 2014-12-31   6  	char *p_val;
c296d5f9 Thomas Petazzoni 2014-12-31  @7  	int ret;
c296d5f9 Thomas Petazzoni 2014-12-31   8  
c296d5f9 Thomas Petazzoni 2014-12-31   9  	if (!str_p || !(*str_p))
c296d5f9 Thomas Petazzoni 2014-12-31  10  		return -EINVAL;
c296d5f9 Thomas Petazzoni 2014-12-31  11  
c296d5f9 Thomas Petazzoni 2014-12-31  12  	p_val = strsep(str_p, sep);
c296d5f9 Thomas Petazzoni 2014-12-31  13  
c296d5f9 Thomas Petazzoni 2014-12-31  14  	if (!p_val)
c296d5f9 Thomas Petazzoni 2014-12-31  15  		return -EINVAL;

:::::: The code at line 7 was first introduced by commit
:::::: c296d5f9957c03994a699d6739c27d4581a9f6c7 staging: fbtft: core support

:::::: TO: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40203 bytes --]

Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask

[Andy Shevchenko]

On Mon, 2017-01-02 at 19:14 +0100, Noralf Trønnes wrote:
> Den 02.01.2017 12:35, skrev Andy Shevchenko:
> > Usually it's not consumer's business to override resources passed
> > from
> > provider, in particularly DMA coherent mask.

> > --- a/drivers/staging/fbtft/fbtft-core.c
> > +++ b/drivers/staging/fbtft/fbtft-core.c
> > @@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct
> > fbtft_display *display,
> >   	if (txbuflen > 0) {
> >   #ifdef CONFIG_HAS_DMA
> >   		if (dma) {
> > -			dev->coherent_dma_mask = ~0;
> 
> Can we make this conditional like in of_dma_configure():
> 
>           if (!dev->coherent_dma_mask)
>                  dev->coherent_dma_mask = DMA_BIT_MASK(32);
> 
> If not, I guess the mask has to be set before adding the spi device in
> fbtft_device.c to keep it from breaking.

Good point. I will check this.

>  			txbuf = dmam_alloc_coherent(dev, txbuflen,
> >   						    &par-
> > >txbuf.dma, GFP_DMA);
> >   		} else

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

Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask

[Andy Shevchenko]

On Tue, 2017-01-03 at 12:51 +0200, Andy Shevchenko wrote:
> On Mon, 2017-01-02 at 19:14 +0100, Noralf Trønnes wrote:
> > Den 02.01.2017 12:35, skrev Andy Shevchenko:
> > > Usually it's not consumer's business to override resources passed
> > > from
> > > provider, in particularly DMA coherent mask.
> > > --- a/drivers/staging/fbtft/fbtft-core.c
> > > +++ b/drivers/staging/fbtft/fbtft-core.c
> > > @@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct
> > > fbtft_display *display,
> > >   	if (txbuflen > 0) {
> > >   #ifdef CONFIG_HAS_DMA
> > >   		if (dma) {
> > > -			dev->coherent_dma_mask = ~0;
> > 
> > Can we make this conditional like in of_dma_configure():
> > 
> >           if (!dev->coherent_dma_mask)
> >                  dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > 
> > If not, I guess the mask has to be set before adding the spi device
> > in
> > fbtft_device.c to keep it from breaking.
> 
> Good point. I will check this.

So, I was too fast.

Clearly an SPI slave device does not and *should not* know about DMA
capabilities of SPI *host* controller. It's completely out of slave's
business.

The masks and other DMA properties only makes sense for the device which
*actually does DMA*, and apparently SPI slaves do not suit that category
(there are might be real SPI slaves with private DMA engines, though
it's another story).

Thus, the patch from my point of view should be kept in the same form.

Regarding to the kbuild bot warning, would you like me to resend it
fixed? Whatever the decision, I will wait for more comments and
hopefully your tags.

P.S. I dunno how it did work before, since DMA mask of slave device
basically has no effect. Perhaps first argument to allocator should be
NULL. That's only amendment I can see to this particular patch.

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

Re: [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI

[Greg Kroah-Hartman]

On Mon, Jan 02, 2017 at 01:37:43PM +0200, Andy Shevchenko wrote:
> On Mon, 2017-01-02 at 13:35 +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).
> 
> It should be v1, but here we are.

I'll wait for v3 based on the kbuild issue found :)

Re: [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI

[Andy Shevchenko]

On Tue, 2017-01-03 at 16:27 +0100, Greg Kroah-Hartman wrote:
> On Mon, Jan 02, 2017 at 01:37:43PM +0200, Andy Shevchenko wrote:
> > On Mon, 2017-01-02 at 13:35 +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).
> > 
> > It should be v1, but here we are.
> 
> I'll wait for v3 based on the kbuild issue found :)

Yes, please wait. I will re-do DMA approach as well. Need to test it
first.

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

Re: [PATCH v2 3/6] staging: fbtft: fallback to usual allocation when DMA fails

[Andy Shevchenko]

On Mon, 2017-01-02 at 13:35 +0200, Andy Shevchenko wrote:
> Fall back to usual allocation method if DMA coherent allocation fails.
> 
> SPI framework will map and use DMA mapped memory when possible.

Locally I have re-done DMA approach and thus this patch became optional.

Should I leave or remove it? Opinions?

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/staging/fbtft/fbtft-core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fbtft/fbtft-core.c
> b/drivers/staging/fbtft/fbtft-core.c
> index 226be8c09768..9f024986aff4 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -843,7 +843,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct
> fbtft_display *display,
>  		if (dma) {
>  			txbuf = dmam_alloc_coherent(dev, txbuflen,
>  						    &par->txbuf.dma,
> GFP_DMA);
> -		} else
> +		}
> +		if (!txbuf)
>  #endif
>  		{
>  			txbuf = devm_kzalloc(par->info->device,

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

Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask

[Noralf Trønnes]

Den 03.01.2017 14:58, skrev Andy Shevchenko:
> On Tue, 2017-01-03 at 12:51 +0200, Andy Shevchenko wrote:
>> On Mon, 2017-01-02 at 19:14 +0100, Noralf Trønnes wrote:
>>> Den 02.01.2017 12:35, skrev Andy Shevchenko:
>>>> Usually it's not consumer's business to override resources passed
>>>> from
>>>> provider, in particularly DMA coherent mask.
>>>> --- a/drivers/staging/fbtft/fbtft-core.c
>>>> +++ b/drivers/staging/fbtft/fbtft-core.c
>>>> @@ -841,7 +841,6 @@ struct fb_info *fbtft_framebuffer_alloc(struct
>>>> fbtft_display *display,
>>>>    	if (txbuflen > 0) {
>>>>    #ifdef CONFIG_HAS_DMA
>>>>    		if (dma) {
>>>> -			dev->coherent_dma_mask = ~0;
>>> Can we make this conditional like in of_dma_configure():
>>>
>>>            if (!dev->coherent_dma_mask)
>>>                   dev->coherent_dma_mask = DMA_BIT_MASK(32);
>>>
>>> If not, I guess the mask has to be set before adding the spi device
>>> in
>>> fbtft_device.c to keep it from breaking.
>> Good point. I will check this.
> So, I was too fast.
>
> Clearly an SPI slave device does not and *should not* know about DMA
> capabilities of SPI *host* controller. It's completely out of slave's
> business.
>
> The masks and other DMA properties only makes sense for the device which
> *actually does DMA*, and apparently SPI slaves do not suit that category
> (there are might be real SPI slaves with private DMA engines, though
> it's another story).
>
> Thus, the patch from my point of view should be kept in the same form.
>
> Regarding to the kbuild bot warning, would you like me to resend it
> fixed? Whatever the decision, I will wait for more comments and
> hopefully your tags.
>
> P.S. I dunno how it did work before, since DMA mask of slave device
> basically has no effect. Perhaps first argument to allocator should be
> NULL. That's only amendment I can see to this particular patch.
>

I have looked at this more closely and it seems that
spi_message.is_dma_mapped is deprecated. And if the spi master driver
can do dma, then the spi core will dma map the buffer (spi_map_buf()
even does vmalloc'ed buffers), and this is what happens for most dma
capable master drivers it seems. Since kmalloc allocates buffers that
is always dma mappable, we can always use kmalloc() instead of
dmam_alloc_coherent() for the transfer buffer.

In addition cleaning up these would prevent any later confusion:

drivers/staging/fbtft/fbtft.h:

  struct fbtft_par {
          struct {
                  void *buf;
-                dma_addr_t dma;
                  size_t len;
          } txbuf;


drivers/staging/fbtft/fbtft-core.c:

-#include <linux/dma-mapping.h>

-#ifdef CONFIG_HAS_DMA
-static bool dma = true;
-module_param(dma, bool, 0);
-MODULE_PARM_DESC(dma, "Use DMA buffer");
-#endif

         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);


drivers/staging/fbtft/fbtft-io.c:

  int fbtft_write_spi(struct fbtft_par *par, void *buf, size_t len)
-       if (par->txbuf.dma && buf == par->txbuf.buf) {
-               t.tx_dma = par->txbuf.dma;
-               m.is_dma_mapped = 1;
-       }

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

[Noralf Trønnes]

Den 02.01.2017 12:35, skrev 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().
>
> To avoid potential use in atomic context annotate it via might_sleep() macro.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/staging/fbtft/fbtft-core.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> index bbe89c9c4fb9..e8bf0d1ec11f 100644
> --- a/drivers/staging/fbtft/fbtft-core.c
> +++ b/drivers/staging/fbtft/fbtft-core.c
> @@ -336,11 +336,14 @@ static void fbtft_reset(struct fbtft_par *par)
>   {
>   	if (par->gpio.reset == -1)
>   		return;
> +
> +	might_sleep();

gpio_set_value_cansleep() already does might_sleep().
So with that removed:
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>

> +
>   	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,

Re: [PATCH v2 3/6] staging: fbtft: fallback to usual allocation when DMA fails

[Noralf Trønnes]

Den 03.01.2017 17:12, skrev Andy Shevchenko:
> On Mon, 2017-01-02 at 13:35 +0200, Andy Shevchenko wrote:
>> Fall back to usual allocation method if DMA coherent allocation fails.
>>
>> SPI framework will map and use DMA mapped memory when possible.
> Locally I have re-done DMA approach and thus this patch became optional.
>
> Should I leave or remove it? Opinions?

If we use kmalloc always, then this goes away.

>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/staging/fbtft/fbtft-core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/fbtft/fbtft-core.c
>> b/drivers/staging/fbtft/fbtft-core.c
>> index 226be8c09768..9f024986aff4 100644
>> --- a/drivers/staging/fbtft/fbtft-core.c
>> +++ b/drivers/staging/fbtft/fbtft-core.c
>> @@ -843,7 +843,8 @@ struct fb_info *fbtft_framebuffer_alloc(struct
>> fbtft_display *display,
>>   		if (dma) {
>>   			txbuf = dmam_alloc_coherent(dev, txbuflen,
>>   						    &par->txbuf.dma,
>> GFP_DMA);
>> -		} else
>> +		}
>> +		if (!txbuf)
>>   #endif
>>   		{
>>   			txbuf = devm_kzalloc(par->info->device,

Re: [PATCH v2 5/6] staging: fbtft: fb_ssd1306: Support smaller screen sizes

[Noralf Trønnes]

Den 02.01.2017 12:35, skrev 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.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Patches 5 and 6:
Acked-by: Noralf Trønnes <noralf@tronnes.org>


>   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)