Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1549125 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-02 12:40 +0100
    [PATCH v2 4/6] staging: fbtft: propagate error code from kstrto*() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-02 12:40 +0100
    Re: [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-02 12:40 +0100
      Re: [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-03 17:00 +0100
        Re: [PATCH v2 0/6] fbtft: make it work with DMA enabled SPI Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-03 17:00 +0100
    [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-02 12:40 +0100
      Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask Noralf Trønnes <noralf@tronnes.org> - 2017-01-02 19:40 +0100
        Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-03 12:00 +0100
          Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-03 15:10 +0100
            Re: [PATCH v2 2/6] staging: fbtft: do not override DMA coherent mask Noralf Trønnes <noralf@tronnes.org> - 2017-01-03 18:20 +0100
    [PATCH v2 6/6] staging: fbtft: fb_ssd1306: Refactor write_vmem() Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-02 12:40 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1549129

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

[toc] | [prev] | [next] | [standalone]

#1549905

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

[toc] | [prev] | [next] | [standalone]

#1549906

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1549131 — [PATCH v2 6/6] staging: fbtft: fb_ssd1306: Refactor write_vmem()

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web