[PATCH v3 0/6] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

[PATCH v3 0/6] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

[Thomas Zimmermann]

(was: fbdev: Use regular I/O function for framebuffers)

Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
depends on the architecture, but they are all equivalent to regular
I/O functions of similar names. So use regular functions instead and
move all helpers into <asm-generic/fb.h>

The first patch a simple whitespace cleanup.

Until now, <linux/fb.h> contained an include of <asm/io.h>. As this
will go away patches 2 to 4 prepare include statements in the various
drivers. Source files that use regular I/O helpers, such as readl(),
now include <linux/io.h>. Source files that use framebuffer I/O
helpers, such as fb_readl(), also include <asm/fb.h>.

Patch 5 replaces the architecture-based if-else branching in 
<linux/fb.h> by helpers in <asm-generic/fb.h>. All helpers use Linux'
existing I/O functions.

Patch 6 harmonizes naming among fbdev and existing I/O functions.

The patchset has been built for a variety of platforms, such as x86-64,
arm, aarch64, ppc64, parisc, m64k, mips and sparc.

v3:
	* add the new helpers in <asm-generic/fb.h>
	* support reordering and native byte order (Geert, Arnd)
v2:
	* use Linux I/O helpers (Sam, Arnd)

Thomas Zimmermann (6):
  fbdev/matrox: Remove trailing whitespaces
  ipu-v3: Include <linux/io.h>
  fbdev: Include <linux/io.h> in various drivers
  fbdev: Include <linux/io.h> via <asm/fb.h>
  fbdev: Move framebuffer I/O helpers into <asm/fb.h>
  fbdev: Rename fb_mem*() helpers

 drivers/gpu/ipu-v3/ipu-prv.h                |   1 +
 drivers/video/fbdev/arcfb.c                 |   1 +
 drivers/video/fbdev/arkfb.c                 |   2 +
 drivers/video/fbdev/aty/atyfb.h             |   2 +
 drivers/video/fbdev/aty/mach64_cursor.c     |   4 +-
 drivers/video/fbdev/chipsfb.c               |   3 +-
 drivers/video/fbdev/cirrusfb.c              |   2 +
 drivers/video/fbdev/core/cfbcopyarea.c      |   2 +-
 drivers/video/fbdev/core/cfbfillrect.c      |   1 +
 drivers/video/fbdev/core/cfbimgblt.c        |   1 +
 drivers/video/fbdev/core/fbmem.c            |   4 +-
 drivers/video/fbdev/core/svgalib.c          |   3 +-
 drivers/video/fbdev/cyber2000fb.c           |   2 +
 drivers/video/fbdev/ep93xx-fb.c             |   2 +
 drivers/video/fbdev/hgafb.c                 |   3 +-
 drivers/video/fbdev/hitfb.c                 |   2 +-
 drivers/video/fbdev/kyro/fbdev.c            |   5 +-
 drivers/video/fbdev/matrox/matroxfb_accel.c |   8 +-
 drivers/video/fbdev/matrox/matroxfb_base.h  |   6 +-
 drivers/video/fbdev/pm2fb.c                 |   3 +
 drivers/video/fbdev/pm3fb.c                 |   2 +
 drivers/video/fbdev/pvr2fb.c                |   4 +-
 drivers/video/fbdev/s3fb.c                  |   2 +
 drivers/video/fbdev/sm712fb.c               |   2 +
 drivers/video/fbdev/sstfb.c                 |   4 +-
 drivers/video/fbdev/stifb.c                 |   6 +-
 drivers/video/fbdev/tdfxfb.c                |   5 +-
 drivers/video/fbdev/tridentfb.c             |   2 +
 drivers/video/fbdev/vga16fb.c               |   3 +-
 drivers/video/fbdev/vt8623fb.c              |   2 +
 drivers/video/fbdev/wmt_ge_rops.c           |   2 +
 include/asm-generic/fb.h                    | 102 ++++++++++++++++++++
 include/linux/fb.h                          |  53 ----------
 33 files changed, 167 insertions(+), 79 deletions(-)

-- 
2.40.1

[PATCH v3 1/6] fbdev/matrox: Remove trailing whitespaces

[Thomas Zimmermann]

Fix coding style. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/video/fbdev/matrox/matroxfb_accel.c | 6 +++---
 drivers/video/fbdev/matrox/matroxfb_base.h  | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c
index 9cb0685feddd..ce51227798a1 100644
--- a/drivers/video/fbdev/matrox/matroxfb_accel.c
+++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
@@ -88,7 +88,7 @@
 
 static inline void matrox_cfb4_pal(u_int32_t* pal) {
 	unsigned int i;
-	
+
 	for (i = 0; i < 16; i++) {
 		pal[i] = i * 0x11111111U;
 	}
@@ -96,7 +96,7 @@ static inline void matrox_cfb4_pal(u_int32_t* pal) {
 
 static inline void matrox_cfb8_pal(u_int32_t* pal) {
 	unsigned int i;
-	
+
 	for (i = 0; i < 16; i++) {
 		pal[i] = i * 0x01010101U;
 	}
@@ -482,7 +482,7 @@ static void matroxfb_1bpp_imageblit(struct matrox_fb_info *minfo, u_int32_t fgx,
 			/* Tell... well, why bother... */
 			while (height--) {
 				size_t i;
-				
+
 				for (i = 0; i < step; i += 4) {
 					/* Hope that there are at least three readable bytes beyond the end of bitmap */
 					fb_writel(get_unaligned((u_int32_t*)(chardata + i)),mmio.vaddr);
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h
index 958be6805f87..c93c69bbcd57 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.h
+++ b/drivers/video/fbdev/matrox/matroxfb_base.h
@@ -301,9 +301,9 @@ struct matrox_altout {
 	int		(*verifymode)(void* altout_dev, u_int32_t mode);
 	int		(*getqueryctrl)(void* altout_dev,
 					struct v4l2_queryctrl* ctrl);
-	int		(*getctrl)(void* altout_dev, 
+	int		(*getctrl)(void *altout_dev,
 				   struct v4l2_control* ctrl);
-	int		(*setctrl)(void* altout_dev, 
+	int		(*setctrl)(void *altout_dev,
 				   struct v4l2_control* ctrl);
 };
 
-- 
2.40.1

[PATCH v3 2/6] ipu-v3: Include <linux/io.h>

[Thomas Zimmermann]

The code uses readl() and writel(). Include the header file to
get the declarations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/ipu-v3/ipu-prv.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/ipu-v3/ipu-prv.h b/drivers/gpu/ipu-v3/ipu-prv.h
index 291ac1bab66d..d4621b1ea7f1 100644
--- a/drivers/gpu/ipu-v3/ipu-prv.h
+++ b/drivers/gpu/ipu-v3/ipu-prv.h
@@ -8,6 +8,7 @@
 
 struct ipu_soc;
 
+#include <linux/io.h>
 #include <linux/types.h>
 #include <linux/device.h>
 #include <linux/clk.h>
-- 
2.40.1

[PATCH v3 3/6] fbdev: Include <linux/io.h> in various drivers

[Thomas Zimmermann]

The code uses writel() and similar I/O-memory helpers. Include
the header file to get the declarations.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/video/fbdev/arcfb.c       | 1 +
 drivers/video/fbdev/aty/atyfb.h   | 2 ++
 drivers/video/fbdev/wmt_ge_rops.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/video/fbdev/arcfb.c b/drivers/video/fbdev/arcfb.c
index 45e64016db32..d631d53f42ad 100644
--- a/drivers/video/fbdev/arcfb.c
+++ b/drivers/video/fbdev/arcfb.c
@@ -41,6 +41,7 @@
 #include <linux/vmalloc.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/arcfb.h>
diff --git a/drivers/video/fbdev/aty/atyfb.h b/drivers/video/fbdev/aty/atyfb.h
index 465f55beb97f..30da3e82ed3c 100644
--- a/drivers/video/fbdev/aty/atyfb.h
+++ b/drivers/video/fbdev/aty/atyfb.h
@@ -3,8 +3,10 @@
  *  ATI Frame Buffer Device Driver Core Definitions
  */
 
+#include <linux/io.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
+
     /*
      *  Elements of the hardware specific atyfb_par structure
      */
diff --git a/drivers/video/fbdev/wmt_ge_rops.c b/drivers/video/fbdev/wmt_ge_rops.c
index 42255d27a1db..99c7b0aea615 100644
--- a/drivers/video/fbdev/wmt_ge_rops.c
+++ b/drivers/video/fbdev/wmt_ge_rops.c
@@ -9,7 +9,9 @@
 
 #include <linux/module.h>
 #include <linux/fb.h>
+#include <linux/io.h>
 #include <linux/platform_device.h>
+
 #include "core/fb_draw.h"
 #include "wmt_ge_rops.h"
 
-- 
2.40.1

[PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Thomas Zimmermann]

Fbdev's main header file, <linux/fb.h>, includes <asm/io.h> to get
declarations for I/O helper functions. From these declarations, it
later defines framebuffer I/O helpers, such as fb_{read,write}[bwlq]()
or fb_memset().

The framebuffer I/O helpers depend on the system architecture and
will therefore be moved into <asm/fb.h>. Prepare this change by first
adding an include statement for <linux/io.h> to <asm-generic/fb.h>.
Include <asm/fb.h> in all source files that use the framebuffer I/O
helpers, so that they still get the necessary I/O functions.

v3:
	* fix grammar in commit message

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/arkfb.c                 | 2 ++
 drivers/video/fbdev/aty/mach64_cursor.c     | 2 +-
 drivers/video/fbdev/chipsfb.c               | 1 +
 drivers/video/fbdev/cirrusfb.c              | 2 ++
 drivers/video/fbdev/core/cfbcopyarea.c      | 2 +-
 drivers/video/fbdev/core/cfbfillrect.c      | 1 +
 drivers/video/fbdev/core/cfbimgblt.c        | 1 +
 drivers/video/fbdev/core/svgalib.c          | 3 +--
 drivers/video/fbdev/cyber2000fb.c           | 2 ++
 drivers/video/fbdev/ep93xx-fb.c             | 2 ++
 drivers/video/fbdev/hgafb.c                 | 3 ++-
 drivers/video/fbdev/hitfb.c                 | 2 +-
 drivers/video/fbdev/kyro/fbdev.c            | 3 ++-
 drivers/video/fbdev/matrox/matroxfb_accel.c | 2 ++
 drivers/video/fbdev/matrox/matroxfb_base.h  | 2 +-
 drivers/video/fbdev/pm2fb.c                 | 3 +++
 drivers/video/fbdev/pm3fb.c                 | 2 ++
 drivers/video/fbdev/pvr2fb.c                | 2 ++
 drivers/video/fbdev/s3fb.c                  | 2 ++
 drivers/video/fbdev/sm712fb.c               | 2 ++
 drivers/video/fbdev/sstfb.c                 | 2 +-
 drivers/video/fbdev/stifb.c                 | 2 ++
 drivers/video/fbdev/tdfxfb.c                | 3 ++-
 drivers/video/fbdev/tridentfb.c             | 2 ++
 drivers/video/fbdev/vga16fb.c               | 3 ++-
 drivers/video/fbdev/vt8623fb.c              | 2 ++
 include/asm-generic/fb.h                    | 1 +
 27 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
index 60a96fdb5dd8..fd38e8a073b8 100644
--- a/drivers/video/fbdev/arkfb.c
+++ b/drivers/video/fbdev/arkfb.c
@@ -27,6 +27,8 @@
 #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
 #include <video/vga.h>
 
+#include <asm/fb.h>
+
 struct arkfb_info {
 	int mclk_freq;
 	int wc_cookie;
diff --git a/drivers/video/fbdev/aty/mach64_cursor.c b/drivers/video/fbdev/aty/mach64_cursor.c
index 4ad0331a8c57..a848aaff510c 100644
--- a/drivers/video/fbdev/aty/mach64_cursor.c
+++ b/drivers/video/fbdev/aty/mach64_cursor.c
@@ -8,7 +8,7 @@
 #include <linux/string.h>
 #include "../core/fb_draw.h"
 
-#include <asm/io.h>
+#include <asm/fb.h>
 
 #ifdef __sparc__
 #include <asm/fbio.h>
diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
index 7799d52a651f..9f9ee13ba2be 100644
--- a/drivers/video/fbdev/chipsfb.c
+++ b/drivers/video/fbdev/chipsfb.c
@@ -32,6 +32,7 @@
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
 #endif
+#include <asm/fb.h>
 
 /*
  * Since we access the display with inb/outb to fixed port numbers,
diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c
index ba45e2147c52..cc306b3733e2 100644
--- a/drivers/video/fbdev/cirrusfb.c
+++ b/drivers/video/fbdev/cirrusfb.c
@@ -57,6 +57,8 @@
 #include <video/vga.h>
 #include <video/cirrus.h>
 
+#include <asm/fb.h>
+
 /*****************************************************************
  *
  * debugging and utility macros
diff --git a/drivers/video/fbdev/core/cfbcopyarea.c b/drivers/video/fbdev/core/cfbcopyarea.c
index 6d4bfeecee35..128fdd0cdcdc 100644
--- a/drivers/video/fbdev/core/cfbcopyarea.c
+++ b/drivers/video/fbdev/core/cfbcopyarea.c
@@ -26,8 +26,8 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/fb.h>
+#include <asm/fb.h>
 #include <asm/types.h>
-#include <asm/io.h>
 #include "fb_draw.h"
 
 #if BITS_PER_LONG == 32
diff --git a/drivers/video/fbdev/core/cfbfillrect.c b/drivers/video/fbdev/core/cfbfillrect.c
index ba9f58b2a5e8..2c6aac987786 100644
--- a/drivers/video/fbdev/core/cfbfillrect.c
+++ b/drivers/video/fbdev/core/cfbfillrect.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/fb.h>
+#include <asm/fb.h>
 #include <asm/types.h>
 #include "fb_draw.h"
 
diff --git a/drivers/video/fbdev/core/cfbimgblt.c b/drivers/video/fbdev/core/cfbimgblt.c
index 9ebda4e0dc7a..d1e071148a4b 100644
--- a/drivers/video/fbdev/core/cfbimgblt.c
+++ b/drivers/video/fbdev/core/cfbimgblt.c
@@ -32,6 +32,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/fb.h>
+#include <asm/fb.h>
 #include <asm/types.h>
 #include "fb_draw.h"
 
diff --git a/drivers/video/fbdev/core/svgalib.c b/drivers/video/fbdev/core/svgalib.c
index 9e01322fabe3..5ddd498024a8 100644
--- a/drivers/video/fbdev/core/svgalib.c
+++ b/drivers/video/fbdev/core/svgalib.c
@@ -15,9 +15,8 @@
 #include <linux/string.h>
 #include <linux/fb.h>
 #include <linux/svga.h>
+#include <asm/fb.h>
 #include <asm/types.h>
-#include <asm/io.h>
-
 
 /* Write a CRT register value spread across multiple registers */
 void svga_wcrt_multi(void __iomem *regbase, const struct vga_regset *regset, u32 value)
diff --git a/drivers/video/fbdev/cyber2000fb.c b/drivers/video/fbdev/cyber2000fb.c
index 38c0a6866d76..9fbc0994b3ae 100644
--- a/drivers/video/fbdev/cyber2000fb.c
+++ b/drivers/video/fbdev/cyber2000fb.c
@@ -48,6 +48,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
+#include <asm/fb.h>
+
 #ifdef __arm__
 #include <asm/mach-types.h>
 #endif
diff --git a/drivers/video/fbdev/ep93xx-fb.c b/drivers/video/fbdev/ep93xx-fb.c
index 305f1587bd89..5dce00500f0a 100644
--- a/drivers/video/fbdev/ep93xx-fb.c
+++ b/drivers/video/fbdev/ep93xx-fb.c
@@ -23,6 +23,8 @@
 
 #include <linux/platform_data/video-ep93xx.h>
 
+#include <asm/fb.h>
+
 /* Vertical Frame Timing Registers */
 #define EP93XXFB_VLINES_TOTAL			0x0000	/* SW locked */
 #define EP93XXFB_VSYNC				0x0004	/* SW locked */
diff --git a/drivers/video/fbdev/hgafb.c b/drivers/video/fbdev/hgafb.c
index 40879d9facdf..b15271c52d05 100644
--- a/drivers/video/fbdev/hgafb.c
+++ b/drivers/video/fbdev/hgafb.c
@@ -41,7 +41,8 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
-#include <asm/io.h>
+
+#include <asm/fb.h>
 #include <asm/vga.h>
 
 #if 0
diff --git a/drivers/video/fbdev/hitfb.c b/drivers/video/fbdev/hitfb.c
index bbb0f1d953cc..a2b5c58f7b7c 100644
--- a/drivers/video/fbdev/hitfb.c
+++ b/drivers/video/fbdev/hitfb.c
@@ -23,7 +23,7 @@
 
 #include <asm/machvec.h>
 #include <linux/uaccess.h>
-#include <asm/io.h>
+#include <asm/fb.h>
 #include <asm/hd64461.h>
 #include <cpu/dac.h>
 
diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index 0596573ef140..8b6c3318bf8c 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -21,9 +21,10 @@
 #include <linux/ioctl.h>
 #include <linux/init.h>
 #include <linux/pci.h>
-#include <asm/io.h>
 #include <linux/uaccess.h>
 
+#include <asm/fb.h>
+
 #include <video/kyro.h>
 
 #include "STG4000Reg.h"
diff --git a/drivers/video/fbdev/matrox/matroxfb_accel.c b/drivers/video/fbdev/matrox/matroxfb_accel.c
index ce51227798a1..c982cfe68ab8 100644
--- a/drivers/video/fbdev/matrox/matroxfb_accel.c
+++ b/drivers/video/fbdev/matrox/matroxfb_accel.c
@@ -82,6 +82,8 @@
 #include "matroxfb_Ti3026.h"
 #include "matroxfb_misc.h"
 
+#include <asm/fb.h>
+
 #define curr_ydstorg(x)	((x)->curr.ydstorg.pixels)
 
 #define mga_ydstlen(y,l) mga_outl(M_YDSTLEN | M_EXEC, ((y) << 16) | (l))
diff --git a/drivers/video/fbdev/matrox/matroxfb_base.h b/drivers/video/fbdev/matrox/matroxfb_base.h
index c93c69bbcd57..184a6d733b93 100644
--- a/drivers/video/fbdev/matrox/matroxfb_base.h
+++ b/drivers/video/fbdev/matrox/matroxfb_base.h
@@ -43,7 +43,7 @@
 #include <linux/spinlock.h>
 #include <linux/kd.h>
 
-#include <asm/io.h>
+#include <asm/fb.h>
 #include <asm/unaligned.h>
 
 #if defined(CONFIG_PPC_PMAC)
diff --git a/drivers/video/fbdev/pm2fb.c b/drivers/video/fbdev/pm2fb.c
index 47d212944f30..b6a37aff057e 100644
--- a/drivers/video/fbdev/pm2fb.c
+++ b/drivers/video/fbdev/pm2fb.c
@@ -39,6 +39,9 @@
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+
+#include <asm/fb.h>
+
 #include <video/permedia2.h>
 #include <video/cvisionppc.h>
 
diff --git a/drivers/video/fbdev/pm3fb.c b/drivers/video/fbdev/pm3fb.c
index b46a471df9ae..95e152969d30 100644
--- a/drivers/video/fbdev/pm3fb.c
+++ b/drivers/video/fbdev/pm3fb.c
@@ -34,6 +34,8 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 
+#include <asm/fb.h>
+
 #include <video/pm3fb.h>
 
 #if !defined(CONFIG_PCI)
diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index 6888127a5eb8..1dfb75b15eea 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -74,6 +74,8 @@
 #include <cpu/sq.h>
 #endif
 
+#include <asm/fb.h>
+
 #ifndef PCI_DEVICE_ID_NEC_NEON250
 #  define PCI_DEVICE_ID_NEC_NEON250	0x0067
 #endif
diff --git a/drivers/video/fbdev/s3fb.c b/drivers/video/fbdev/s3fb.c
index 7d257489edcc..eb16beba10c5 100644
--- a/drivers/video/fbdev/s3fb.c
+++ b/drivers/video/fbdev/s3fb.c
@@ -29,6 +29,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
+#include <asm/fb.h>
+
 struct s3fb_info {
 	int chip, rev, mclk_freq;
 	int wc_cookie;
diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
index b528776c7612..ca15938ce603 100644
--- a/drivers/video/fbdev/sm712fb.c
+++ b/drivers/video/fbdev/sm712fb.c
@@ -31,6 +31,8 @@
 
 #include <linux/pm.h>
 
+#include <asm/fb.h>
+
 #include "sm712.h"
 
 /*
diff --git a/drivers/video/fbdev/sstfb.c b/drivers/video/fbdev/sstfb.c
index da296b2ab54a..1ee4bea467b4 100644
--- a/drivers/video/fbdev/sstfb.c
+++ b/drivers/video/fbdev/sstfb.c
@@ -88,10 +88,10 @@
 #include <linux/pci.h>
 #include <linux/delay.h>
 #include <linux/init.h>
-#include <asm/io.h>
 #include <linux/uaccess.h>
 #include <video/sstfb.h>
 
+#include <asm/fb.h>
 
 /* initialized by setup */
 
diff --git a/drivers/video/fbdev/stifb.c b/drivers/video/fbdev/stifb.c
index baca6974e288..a3b837a5fb81 100644
--- a/drivers/video/fbdev/stifb.c
+++ b/drivers/video/fbdev/stifb.c
@@ -69,6 +69,8 @@
 #include <asm/grfioctl.h>	/* for HP-UX compatibility */
 #include <linux/uaccess.h>
 
+#include <asm/fb.h>
+
 #include <video/sticore.h>
 
 /* REGION_BASE(fb_info, index) returns the virtual address for region <index> */
diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index d17e5e1472aa..5ed8f670f51c 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -74,7 +74,8 @@
 #include <linux/fb.h>
 #include <linux/init.h>
 #include <linux/pci.h>
-#include <asm/io.h>
+
+#include <asm/fb.h>
 
 #include <video/tdfx.h>
 
diff --git a/drivers/video/fbdev/tridentfb.c b/drivers/video/fbdev/tridentfb.c
index 6099b9768ba1..1bd12606c9e0 100644
--- a/drivers/video/fbdev/tridentfb.c
+++ b/drivers/video/fbdev/tridentfb.c
@@ -30,6 +30,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
+#include <asm/fb.h>
+
 struct tridentfb_par {
 	void __iomem *io_virt;	/* iospace virtual memory address */
 	u32 pseudo_pal[16];
diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c
index 1a8ffdb2be26..2899d4ce0f6f 100644
--- a/drivers/video/fbdev/vga16fb.c
+++ b/drivers/video/fbdev/vga16fb.c
@@ -23,7 +23,8 @@
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 
-#include <asm/io.h>
+#include <asm/fb.h>
+
 #include <video/vga.h>
 
 #define MODE_SKIP4	1
diff --git a/drivers/video/fbdev/vt8623fb.c b/drivers/video/fbdev/vt8623fb.c
index 034333ee6e45..bc345d4fee9e 100644
--- a/drivers/video/fbdev/vt8623fb.c
+++ b/drivers/video/fbdev/vt8623fb.c
@@ -27,6 +27,8 @@
 #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
 #include <video/vga.h>
 
+#include <asm/fb.h>
+
 struct vt8623fb_info {
 	char __iomem *mmio_base;
 	int wc_cookie;
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index c8af99f5a535..6922dd248c51 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -7,6 +7,7 @@
  * Only include this header file from your architecture's <asm/fb.h>.
  */
 
+#include <linux/io.h>
 #include <linux/mm_types.h>
 #include <linux/pgtable.h>
 
-- 
2.40.1

[PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Thomas Zimmermann]

Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
in the architecture's <asm/fb.h> header file or the generic one.

The common case has been the use of regular I/O functions, such as
__raw_readb() or memset_io(). A few architectures used plain system-
memory reads and writes. Sparc used helpers for its SBus.

The architectures that used special cases provide the same code in
their __raw_*() I/O helpers. So the patch replaces this code with the
__raw_*() functions and moves it to <asm-generic/fb.h> for all
architectures.

v3:
	* implement all architectures with generic helpers
	* support reordering and native byte order (Geert, Arnd)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++
 include/linux/fb.h       |  53 --------------------
 2 files changed, 101 insertions(+), 53 deletions(-)

diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index 6922dd248c51..0540eccdbeca 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info)
 }
 #endif
 
+/*
+ * I/O helpers for the framebuffer. Prefer these functions over their
+ * regular counterparts. The regular I/O functions provide in-order
+ * access and swap bytes to/from little-endian ordering. Neither is
+ * required for framebuffers. Instead, the helpers read and write
+ * raw framebuffer data. Independent operations can be reordered for
+ * improved performance.
+ */
+
+#ifndef fb_readb
+static inline u8 fb_readb(const volatile void __iomem *addr)
+{
+	return __raw_readb(addr);
+}
+#define fb_readb fb_readb
+#endif
+
+#ifndef fb_readw
+static inline u16 fb_readw(const volatile void __iomem *addr)
+{
+	return __raw_readw(addr);
+}
+#define fb_readw fb_readw
+#endif
+
+#ifndef fb_readl
+static inline u32 fb_readl(const volatile void __iomem *addr)
+{
+	return __raw_readl(addr);
+}
+#define fb_readl fb_readl
+#endif
+
+#ifndef fb_readq
+#if defined(__raw_readq)
+static inline u64 fb_readq(const volatile void __iomem *addr)
+{
+	return __raw_readq(addr);
+}
+#define fb_readq fb_readq
+#endif
+#endif
+
+#ifndef fb_writeb
+static inline void fb_writeb(u8 b, volatile void __iomem *addr)
+{
+	__raw_writeb(b, addr);
+}
+#define fb_writeb fb_writeb
+#endif
+
+#ifndef fb_writew
+static inline void fb_writew(u16 b, volatile void __iomem *addr)
+{
+	__raw_writew(b, addr);
+}
+#define fb_writew fb_writew
+#endif
+
+#ifndef fb_writel
+static inline void fb_writel(u32 b, volatile void __iomem *addr)
+{
+	__raw_writel(b, addr);
+}
+#define fb_writel fb_writel
+#endif
+
+#ifndef fb_writeq
+#if defined(__raw_writeq)
+static inline void fb_writeq(u64 b, volatile void __iomem *addr)
+{
+	__raw_writeq(b, addr);
+}
+#define fb_writeq fb_writeq
+#endif
+#endif
+
+#ifndef fb_memcpy_fromfb
+static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+{
+	memcpy_fromio(to, from, n);
+}
+#define fb_memcpy_fromfb fb_memcpy_fromfb
+#endif
+
+#ifndef fb_memcpy_tofb
+static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+{
+	memcpy_toio(to, from, n);
+}
+#define fb_memcpy_tofb fb_memcpy_tofb
+#endif
+
+#ifndef fb_memset
+static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+{
+	memset_io(addr, c, n);
+}
+#define fb_memset fb_memset
+#endif
+
 #endif /* __ASM_GENERIC_FB_H_ */
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 08cb47da71f8..7d80ee62a9d5 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -15,7 +15,6 @@
 #include <linux/list.h>
 #include <linux/backlight.h>
 #include <linux/slab.h>
-#include <asm/io.h>
 
 struct vm_area_struct;
 struct fb_info;
@@ -511,58 +510,6 @@ struct fb_info {
  */
 #define STUPID_ACCELF_TEXT_SHIT
 
-// This will go away
-#if defined(__sparc__)
-
-/* We map all of our framebuffers such that big-endian accesses
- * are what we want, so the following is sufficient.
- */
-
-// This will go away
-#define fb_readb sbus_readb
-#define fb_readw sbus_readw
-#define fb_readl sbus_readl
-#define fb_readq sbus_readq
-#define fb_writeb sbus_writeb
-#define fb_writew sbus_writew
-#define fb_writel sbus_writel
-#define fb_writeq sbus_writeq
-#define fb_memset sbus_memset_io
-#define fb_memcpy_fromfb sbus_memcpy_fromio
-#define fb_memcpy_tofb sbus_memcpy_toio
-
-#elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) ||	\
-	defined(__hppa__) || defined(__sh__) || defined(__powerpc__) ||	\
-	defined(__arm__) || defined(__aarch64__) || defined(__mips__)
-
-#define fb_readb __raw_readb
-#define fb_readw __raw_readw
-#define fb_readl __raw_readl
-#define fb_readq __raw_readq
-#define fb_writeb __raw_writeb
-#define fb_writew __raw_writew
-#define fb_writel __raw_writel
-#define fb_writeq __raw_writeq
-#define fb_memset memset_io
-#define fb_memcpy_fromfb memcpy_fromio
-#define fb_memcpy_tofb memcpy_toio
-
-#else
-
-#define fb_readb(addr) (*(volatile u8 *) (addr))
-#define fb_readw(addr) (*(volatile u16 *) (addr))
-#define fb_readl(addr) (*(volatile u32 *) (addr))
-#define fb_readq(addr) (*(volatile u64 *) (addr))
-#define fb_writeb(b,addr) (*(volatile u8 *) (addr) = (b))
-#define fb_writew(b,addr) (*(volatile u16 *) (addr) = (b))
-#define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b))
-#define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b))
-#define fb_memset memset
-#define fb_memcpy_fromfb memcpy
-#define fb_memcpy_tofb memcpy
-
-#endif
-
 #define FB_LEFT_POS(p, bpp)          (fb_be_math(p) ? (32 - (bpp)) : 0)
 #define FB_SHIFT_HIGH(p, val, bits)  (fb_be_math(p) ? (val) >> (bits) : \
 						      (val) << (bits))
-- 
2.40.1

[PATCH v3 6/6] fbdev: Rename fb_mem*() helpers

[Thomas Zimmermann]

Update the names of the fb_mem*() helpers to be consistent with their
regular counterparts. Hence, fb_memset() now becomes fb_memset_io(),
fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb()
becomes fb_memcpy_toio(). No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/aty/mach64_cursor.c |  2 +-
 drivers/video/fbdev/chipsfb.c           |  2 +-
 drivers/video/fbdev/core/fbmem.c        |  4 ++--
 drivers/video/fbdev/kyro/fbdev.c        |  2 +-
 drivers/video/fbdev/pvr2fb.c            |  2 +-
 drivers/video/fbdev/sstfb.c             |  2 +-
 drivers/video/fbdev/stifb.c             |  4 ++--
 drivers/video/fbdev/tdfxfb.c            |  2 +-
 include/asm-generic/fb.h                | 16 ++++++++--------
 9 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/video/fbdev/aty/mach64_cursor.c b/drivers/video/fbdev/aty/mach64_cursor.c
index a848aaff510c..6adbcf366a98 100644
--- a/drivers/video/fbdev/aty/mach64_cursor.c
+++ b/drivers/video/fbdev/aty/mach64_cursor.c
@@ -153,7 +153,7 @@ static int atyfb_cursor(struct fb_info *info, struct fb_cursor *cursor)
 	    u8 m, b;
 
 	    // Clear cursor image with 1010101010...
-	    fb_memset(dst, 0xaa, 1024);
+	    fb_memset_io(dst, 0xaa, 1024);
 
 	    offset = align - width*2;
 
diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
index 9f9ee13ba2be..297ca6eeac1e 100644
--- a/drivers/video/fbdev/chipsfb.c
+++ b/drivers/video/fbdev/chipsfb.c
@@ -333,7 +333,7 @@ static const struct fb_var_screeninfo chipsfb_var = {
 
 static void init_chips(struct fb_info *p, unsigned long addr)
 {
-	fb_memset(p->screen_base, 0, 0x100000);
+	fb_memset_io(p->screen_base, 0, 0x100000);
 
 	p->fix = chipsfb_fix;
 	p->fix.smem_start = addr;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3fd95a79e4c3..a696399f2160 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -804,7 +804,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	while (count) {
 		c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		dst = buffer;
-		fb_memcpy_fromfb(dst, src, c);
+		fb_memcpy_fromio(dst, src, c);
 		dst += c;
 		src += c;
 
@@ -881,7 +881,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 			break;
 		}
 
-		fb_memcpy_tofb(dst, src, c);
+		fb_memcpy_toio(dst, src, c);
 		dst += c;
 		src += c;
 		*ppos += c;
diff --git a/drivers/video/fbdev/kyro/fbdev.c b/drivers/video/fbdev/kyro/fbdev.c
index 8b6c3318bf8c..59a6b71fba44 100644
--- a/drivers/video/fbdev/kyro/fbdev.c
+++ b/drivers/video/fbdev/kyro/fbdev.c
@@ -738,7 +738,7 @@ static int kyrofb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			       info->var.bits_per_pixel);
 	size *= info->var.yres_virtual;
 
-	fb_memset(info->screen_base, 0, size);
+	fb_memset_io(info->screen_base, 0, size);
 
 	if (register_framebuffer(info) < 0)
 		goto out_unmap;
diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index 1dfb75b15eea..5f85207e91f7 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -800,7 +800,7 @@ static int __maybe_unused pvr2fb_common_init(void)
 		goto out_err;
 	}
 
-	fb_memset(fb_info->screen_base, 0, pvr2_fix.smem_len);
+	fb_memset_io(fb_info->screen_base, 0, pvr2_fix.smem_len);
 
 	pvr2_fix.ypanstep	= nopan  ? 0 : 1;
 	pvr2_fix.ywrapstep	= nowrap ? 0 : 1;
diff --git a/drivers/video/fbdev/sstfb.c b/drivers/video/fbdev/sstfb.c
index 1ee4bea467b4..327831b64b85 100644
--- a/drivers/video/fbdev/sstfb.c
+++ b/drivers/video/fbdev/sstfb.c
@@ -335,7 +335,7 @@ static int sst_calc_pll(const int freq, int *freq_out, struct pll_timing *t)
 static void sstfb_clear_screen(struct fb_info *info)
 {
 	/* clear screen */
-	fb_memset(info->screen_base, 0, info->fix.smem_len);
+	fb_memset_io(info->screen_base, 0, info->fix.smem_len);
 }
 
 
diff --git a/drivers/video/fbdev/stifb.c b/drivers/video/fbdev/stifb.c
index a3b837a5fb81..93107909155a 100644
--- a/drivers/video/fbdev/stifb.c
+++ b/drivers/video/fbdev/stifb.c
@@ -529,8 +529,8 @@ rattlerSetupPlanes(struct stifb_info *fb)
 	fb->id = saved_id;
 
 	for (y = 0; y < fb->info.var.yres; ++y)
-		fb_memset(fb->info.screen_base + y * fb->info.fix.line_length,
-			0xff, fb->info.var.xres * fb->info.var.bits_per_pixel/8);
+		fb_memset_io(fb->info.screen_base + y * fb->info.fix.line_length,
+			     0xff, fb->info.var.xres * fb->info.var.bits_per_pixel/8);
 
 	CRX24_SET_OVLY_MASK(fb);
 	SETUP_FB(fb);
diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
index 5ed8f670f51c..bc8108396c22 100644
--- a/drivers/video/fbdev/tdfxfb.c
+++ b/drivers/video/fbdev/tdfxfb.c
@@ -1117,7 +1117,7 @@ static int tdfxfb_cursor(struct fb_info *info, struct fb_cursor *cursor)
 		u8 *mask = (u8 *)cursor->mask;
 		int i;
 
-		fb_memset(cursorbase, 0, 1024);
+		fb_memset_io(cursorbase, 0, 1024);
 
 		for (i = 0; i < cursor->image.height; i++) {
 			int h = 0;
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index 0540eccdbeca..bb7ee9c70e60 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -108,28 +108,28 @@ static inline void fb_writeq(u64 b, volatile void __iomem *addr)
 #endif
 #endif
 
-#ifndef fb_memcpy_fromfb
-static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
+#ifndef fb_memcpy_fromio
+static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
 {
 	memcpy_fromio(to, from, n);
 }
-#define fb_memcpy_fromfb fb_memcpy_fromfb
+#define fb_memcpy_fromio fb_memcpy_fromio
 #endif
 
-#ifndef fb_memcpy_tofb
-static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
+#ifndef fb_memcpy_toio
+static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
 {
 	memcpy_toio(to, from, n);
 }
-#define fb_memcpy_tofb fb_memcpy_tofb
+#define fb_memcpy_toio fb_memcpy_toio
 #endif
 
 #ifndef fb_memset
-static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
+static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
 {
 	memset_io(addr, c, n);
 }
-#define fb_memset fb_memset
+#define fb_memset fb_memset_io
 #endif
 
 #endif /* __ASM_GENERIC_FB_H_ */
-- 
2.40.1

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Sam Ravnborg]

Hi Thomas,

On Tue, May 02, 2023 at 03:02:21PM +0200, Thomas Zimmermann wrote:
> Fbdev's main header file, <linux/fb.h>, includes <asm/io.h> to get
> declarations for I/O helper functions. From these declarations, it
> later defines framebuffer I/O helpers, such as fb_{read,write}[bwlq]()
> or fb_memset().
> 
> The framebuffer I/O helpers depend on the system architecture and
> will therefore be moved into <asm/fb.h>. Prepare this change by first
> adding an include statement for <linux/io.h> to <asm-generic/fb.h>.
> Include <asm/fb.h> in all source files that use the framebuffer I/O
> helpers, so that they still get the necessary I/O functions.
> 
...
> 
> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
> index 60a96fdb5dd8..fd38e8a073b8 100644
> --- a/drivers/video/fbdev/arkfb.c
> +++ b/drivers/video/fbdev/arkfb.c
> @@ -27,6 +27,8 @@
>  #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
>  #include <video/vga.h>
>  
> +#include <asm/fb.h>

When we have a header like linux/fb.h - it is my understanding that it is
preferred to include that file, and not the asm/fb.h variant.

This is assuming the linux/fb.h contains the generic stuff, and includes
asm/fb.h for the architecture specific parts.

So drivers will include linux/fb.h and then they automatically get the
architecture specific parts from asm/fb.h.

In other words, drivers are not supposed to include asm/fb.h, if
linux.fb.h exists - and linux/fb.h shall include the asm/fb.h.

If the above holds true, then it is wrong and not needed to add asm/fb.h
as seen above.


There are countless examples where the above are not followed,
but to my best understanding the above it the preferred way to do it.

	Sam

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Sam Ravnborg]

Hi Thomas,

On Tue, May 02, 2023 at 03:02:22PM +0200, Thomas Zimmermann wrote:
> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
> in the architecture's <asm/fb.h> header file or the generic one.

In reality they are now all implemented in the generic one.

> 
> The common case has been the use of regular I/O functions, such as
> __raw_readb() or memset_io(). A few architectures used plain system-
> memory reads and writes. Sparc used helpers for its SBus.
> 
> The architectures that used special cases provide the same code in
> their __raw_*() I/O helpers. So the patch replaces this code with the
> __raw_*() functions and moves it to <asm-generic/fb.h> for all
> architectures.
Which is also documented here.

> 
> v3:
> 	* implement all architectures with generic helpers
> 	* support reordering and native byte order (Geert, Arnd)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++
>  include/linux/fb.h       |  53 --------------------
>  2 files changed, 101 insertions(+), 53 deletions(-)
> 
> diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
> index 6922dd248c51..0540eccdbeca 100644
> --- a/include/asm-generic/fb.h
> +++ b/include/asm-generic/fb.h
> @@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info)
>  }
>  #endif
>  
> +/*
> + * I/O helpers for the framebuffer. Prefer these functions over their
> + * regular counterparts. The regular I/O functions provide in-order
> + * access and swap bytes to/from little-endian ordering. Neither is
> + * required for framebuffers. Instead, the helpers read and write
> + * raw framebuffer data. Independent operations can be reordered for
> + * improved performance.
> + */
> +
> +#ifndef fb_readb
> +static inline u8 fb_readb(const volatile void __iomem *addr)
> +{
> +	return __raw_readb(addr);
> +}
> +#define fb_readb fb_readb
> +#endif

When we need to provide an architecture specific variant the
#ifndef foo
...
#define foo foo
can be added. Right now it is just noise as no architectures provide
their own variants.

But I am missing something somewhere as I cannot see how this builds.
asm-generic now provide the fb_read/fb_write helpers.
But for example sparc has an architecture specifc fb.h so it will not
use the asm-generic variant. So I wonder how sparc get hold of the
asm-generic fb.h file?

Maybe it is obvious, but I miss it.

	Sam

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Arnd Bergmann]

On Tue, May 2, 2023, at 15:02, Thomas Zimmermann wrote:
> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
> in the architecture's <asm/fb.h> header file or the generic one.
>
> The common case has been the use of regular I/O functions, such as
> __raw_readb() or memset_io(). A few architectures used plain system-
> memory reads and writes. Sparc used helpers for its SBus.
>
> The architectures that used special cases provide the same code in
> their __raw_*() I/O helpers. So the patch replaces this code with the
> __raw_*() functions and moves it to <asm-generic/fb.h> for all
> architectures.
>
> v3:
> 	* implement all architectures with generic helpers
> 	* support reordering and native byte order (Geert, Arnd)

This looks good for the read/write helpers, but I'm a little
worried about the memset and memcpy functions, since they do
change behavior on some architectures:

- on sparc64, fb_mem{set,cpy} uses ASI_PHYS_BYPASS_EC_E (like __raw_readb)
  while mem{set_,cpy_from,cpy_to} uses ASI_PHYS_BYPASS_EC_E_L (like readb)
  I don't know the effect of that, but it seems intentional

- on loongarch and csky, the _io variants avoid unaligned access,
  while the normal memcpy/memset is probably broken, so your
  patch is a bugfix

- on ia64, the _io variants use bytewise access and avoid any longer
  loads and stores, so your patch probably makes things slower.

It's probably safe to deal with all the above by either adding
architecture specific overrides to the current version, or
by doing the semantic changes before the move to asm/fb.h, but
one way or the other I'd prefer this to be separate from the
consolidation patch that should not have any changes in behavior.

     Arnd

Re: [PATCH v3 6/6] fbdev: Rename fb_mem*() helpers

[Sam Ravnborg]

Hi Thomas.

On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote:
> Update the names of the fb_mem*() helpers to be consistent with their
> regular counterparts. Hence, fb_memset() now becomes fb_memset_io(),
> fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb()
> becomes fb_memcpy_toio(). No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
...
>  
> -#ifndef fb_memcpy_fromfb
> -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
> +#ifndef fb_memcpy_fromio
> +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
>  {
>  	memcpy_fromio(to, from, n);
>  }
> -#define fb_memcpy_fromfb fb_memcpy_fromfb
> +#define fb_memcpy_fromio fb_memcpy_fromio
>  #endif
>  
> -#ifndef fb_memcpy_tofb
> -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
> +#ifndef fb_memcpy_toio
> +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
>  {
>  	memcpy_toio(to, from, n);
>  }
> -#define fb_memcpy_tofb fb_memcpy_tofb
> +#define fb_memcpy_toio fb_memcpy_toio
>  #endif
>  
>  #ifndef fb_memset
> -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
> +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
>  {
>  	memset_io(addr, c, n);
>  }
> -#define fb_memset fb_memset
> +#define fb_memset fb_memset_io

The static inlines wrappers does not provide any value, and could be replaced by
direct calls to memcpy_fromio(), memcpy_toio(), memset_io().

If you decide to keep the wrappers I will not hold you back, so the
patch has my:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

But I prefer the direct calls without the wrappers....

	Sam

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3462 bytes --]

Hi Sam

Am 02.05.23 um 22:03 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Tue, May 02, 2023 at 03:02:22PM +0200, Thomas Zimmermann wrote:
>> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
>> in the architecture's <asm/fb.h> header file or the generic one.
> 
> In reality they are now all implemented in the generic one.
> 
>>
>> The common case has been the use of regular I/O functions, such as
>> __raw_readb() or memset_io(). A few architectures used plain system-
>> memory reads and writes. Sparc used helpers for its SBus.
>>
>> The architectures that used special cases provide the same code in
>> their __raw_*() I/O helpers. So the patch replaces this code with the
>> __raw_*() functions and moves it to <asm-generic/fb.h> for all
>> architectures.
> Which is also documented here.

The first paragraph documents the design intention, the other one the 
implementation. But I agree that it's not well phrased. I'll see if I 
can improve the text.

> 
>>
>> v3:
>> 	* implement all architectures with generic helpers
>> 	* support reordering and native byte order (Geert, Arnd)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   include/asm-generic/fb.h | 101 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/fb.h       |  53 --------------------
>>   2 files changed, 101 insertions(+), 53 deletions(-)
>>
>> diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
>> index 6922dd248c51..0540eccdbeca 100644
>> --- a/include/asm-generic/fb.h
>> +++ b/include/asm-generic/fb.h
>> @@ -31,4 +31,105 @@ static inline int fb_is_primary_device(struct fb_info *info)
>>   }
>>   #endif
>>   
>> +/*
>> + * I/O helpers for the framebuffer. Prefer these functions over their
>> + * regular counterparts. The regular I/O functions provide in-order
>> + * access and swap bytes to/from little-endian ordering. Neither is
>> + * required for framebuffers. Instead, the helpers read and write
>> + * raw framebuffer data. Independent operations can be reordered for
>> + * improved performance.
>> + */
>> +
>> +#ifndef fb_readb
>> +static inline u8 fb_readb(const volatile void __iomem *addr)
>> +{
>> +	return __raw_readb(addr);
>> +}
>> +#define fb_readb fb_readb
>> +#endif
> 
> When we need to provide an architecture specific variant the
> #ifndef foo
> ...
> #define foo foo
> can be added. Right now it is just noise as no architectures provide
> their own variants.

Given Arnd's comments, this might change. It also makes sense from a 
design POV.

> 
> But I am missing something somewhere as I cannot see how this builds.
> asm-generic now provide the fb_read/fb_write helpers.
> But for example sparc has an architecture specifc fb.h so it will not
> use the asm-generic variant. So I wonder how sparc get hold of the
> asm-generic fb.h file?

All architecture's <asm/fb.h> files include <asm-generic/fb.h>, so that 
they all get the interfaces which they don't define themselves. For 
Sparc, this is at [1].

Best regards
Thomas


[1] 
https://cgit.freedesktop.org/drm/drm-tip/tree/arch/sparc/include/asm/fb.h#n19

> 
> Maybe it is obvious, but I miss it.
> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2427 bytes --]

Hi

Am 02.05.23 um 21:54 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Tue, May 02, 2023 at 03:02:21PM +0200, Thomas Zimmermann wrote:
>> Fbdev's main header file, <linux/fb.h>, includes <asm/io.h> to get
>> declarations for I/O helper functions. From these declarations, it
>> later defines framebuffer I/O helpers, such as fb_{read,write}[bwlq]()
>> or fb_memset().
>>
>> The framebuffer I/O helpers depend on the system architecture and
>> will therefore be moved into <asm/fb.h>. Prepare this change by first
>> adding an include statement for <linux/io.h> to <asm-generic/fb.h>.
>> Include <asm/fb.h> in all source files that use the framebuffer I/O
>> helpers, so that they still get the necessary I/O functions.
>>
> ...
>>
>> diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
>> index 60a96fdb5dd8..fd38e8a073b8 100644
>> --- a/drivers/video/fbdev/arkfb.c
>> +++ b/drivers/video/fbdev/arkfb.c
>> @@ -27,6 +27,8 @@
>>   #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
>>   #include <video/vga.h>
>>   
>> +#include <asm/fb.h>
> 
> When we have a header like linux/fb.h - it is my understanding that it is
> preferred to include that file, and not the asm/fb.h variant.
> 
> This is assuming the linux/fb.h contains the generic stuff, and includes
> asm/fb.h for the architecture specific parts.
> 
> So drivers will include linux/fb.h and then they automatically get the
> architecture specific parts from asm/fb.h.
> 
> In other words, drivers are not supposed to include asm/fb.h, if
> linux.fb.h exists - and linux/fb.h shall include the asm/fb.h.
> 
> If the above holds true, then it is wrong and not needed to add asm/fb.h
> as seen above.
> 
> 
> There are countless examples where the above are not followed,
> but to my best understanding the above it the preferred way to do it.

Where did youher this? I only know about this in the case of asm/io.h 
vs. linux/io.h.

If that's the case, we should put those helpers into a new header file, 
because one of the motivations here is to remove <asm/io.h> from 
<linux/fb.h>.

Best regards
Thomas

> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Am 02.05.23 um 21:54 schrieb Sam Ravnborg:
>> On Tue, May 02, 2023 at 03:02:21PM +0200, Thomas Zimmermann wrote:

[...]

>>>   #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
>>>   #include <video/vga.h>
>>>   
>>> +#include <asm/fb.h>
>> 
>> When we have a header like linux/fb.h - it is my understanding that it is
>> preferred to include that file, and not the asm/fb.h variant.
>> 
>> This is assuming the linux/fb.h contains the generic stuff, and includes
>> asm/fb.h for the architecture specific parts.
>> 
>> So drivers will include linux/fb.h and then they automatically get the
>> architecture specific parts from asm/fb.h.
>> 
>> In other words, drivers are not supposed to include asm/fb.h, if
>> linux.fb.h exists - and linux/fb.h shall include the asm/fb.h.
>> 
>> If the above holds true, then it is wrong and not needed to add asm/fb.h
>> as seen above.
>> 
>> 
>> There are countless examples where the above are not followed,
>> but to my best understanding the above it the preferred way to do it.
>
> Where did youher this? I only know about this in the case of asm/io.h 
> vs. linux/io.h.
>

I understand that's the case too. I believe even checkpatch.pl complains
about it? (not that the script always get right, but just as an example).

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Geert Uytterhoeven]

On Wed, May 3, 2023 at 9:19 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> > Am 02.05.23 um 21:54 schrieb Sam Ravnborg:
> >> On Tue, May 02, 2023 at 03:02:21PM +0200, Thomas Zimmermann wrote:
>
> [...]
>
> >>>   #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
> >>>   #include <video/vga.h>
> >>>
> >>> +#include <asm/fb.h>
> >>
> >> When we have a header like linux/fb.h - it is my understanding that it is
> >> preferred to include that file, and not the asm/fb.h variant.
> >>
> >> This is assuming the linux/fb.h contains the generic stuff, and includes
> >> asm/fb.h for the architecture specific parts.
> >>
> >> So drivers will include linux/fb.h and then they automatically get the
> >> architecture specific parts from asm/fb.h.
> >>
> >> In other words, drivers are not supposed to include asm/fb.h, if
> >> linux.fb.h exists - and linux/fb.h shall include the asm/fb.h.
> >>
> >> If the above holds true, then it is wrong and not needed to add asm/fb.h
> >> as seen above.
> >>
> >>
> >> There are countless examples where the above are not followed,
> >> but to my best understanding the above it the preferred way to do it.
> >
> > Where did youher this? I only know about this in the case of asm/io.h
> > vs. linux/io.h.
> >
>
> I understand that's the case too. I believe even checkpatch.pl complains
> about it? (not that the script always get right, but just as an example).

One more to chime in: in general, drivers should only include <linux/foo.h>.
Including <asm/foo.h> directly is the exception.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1908 bytes --]

Hi

Am 03.05.23 um 09:19 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
>> Am 02.05.23 um 21:54 schrieb Sam Ravnborg:
>>> On Tue, May 02, 2023 at 03:02:21PM +0200, Thomas Zimmermann wrote:
> 
> [...]
> 
>>>>    #include <linux/console.h> /* Why should fb driver call console functions? because console_lock() */
>>>>    #include <video/vga.h>
>>>>    
>>>> +#include <asm/fb.h>
>>>
>>> When we have a header like linux/fb.h - it is my understanding that it is
>>> preferred to include that file, and not the asm/fb.h variant.
>>>
>>> This is assuming the linux/fb.h contains the generic stuff, and includes
>>> asm/fb.h for the architecture specific parts.
>>>
>>> So drivers will include linux/fb.h and then they automatically get the
>>> architecture specific parts from asm/fb.h.
>>>
>>> In other words, drivers are not supposed to include asm/fb.h, if
>>> linux.fb.h exists - and linux/fb.h shall include the asm/fb.h.
>>>
>>> If the above holds true, then it is wrong and not needed to add asm/fb.h
>>> as seen above.
>>>
>>>
>>> There are countless examples where the above are not followed,
>>> but to my best understanding the above it the preferred way to do it.
>>
>> Where did youher this? I only know about this in the case of asm/io.h
>> vs. linux/io.h.
>>
> 
> I understand that's the case too. I believe even checkpatch.pl complains
> about it? (not that the script always get right, but just as an example).

Do you know if that's the general rule? If so, we might want to 
repurpose <asm/fbio.h> for the framebuffer I/O functions.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 6/6] fbdev: Rename fb_mem*() helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2459 bytes --]

Hi

Am 02.05.23 um 22:08 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote:
>> Update the names of the fb_mem*() helpers to be consistent with their
>> regular counterparts. Hence, fb_memset() now becomes fb_memset_io(),
>> fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb()
>> becomes fb_memcpy_toio(). No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> ...
>>   
>> -#ifndef fb_memcpy_fromfb
>> -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
>> +#ifndef fb_memcpy_fromio
>> +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
>>   {
>>   	memcpy_fromio(to, from, n);
>>   }
>> -#define fb_memcpy_fromfb fb_memcpy_fromfb
>> +#define fb_memcpy_fromio fb_memcpy_fromio
>>   #endif
>>   
>> -#ifndef fb_memcpy_tofb
>> -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
>> +#ifndef fb_memcpy_toio
>> +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
>>   {
>>   	memcpy_toio(to, from, n);
>>   }
>> -#define fb_memcpy_tofb fb_memcpy_tofb
>> +#define fb_memcpy_toio fb_memcpy_toio
>>   #endif
>>   
>>   #ifndef fb_memset
>> -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
>> +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
>>   {
>>   	memset_io(addr, c, n);
>>   }
>> -#define fb_memset fb_memset
>> +#define fb_memset fb_memset_io
> 
> The static inlines wrappers does not provide any value, and could be replaced by
> direct calls to memcpy_fromio(), memcpy_toio(), memset_io().
> 
> If you decide to keep the wrappers I will not hold you back, so the
> patch has my:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> But I prefer the direct calls without the wrappers....

At first I was also skeptical if those fb_mem*() wrappers are needed. 
But Arnd mentioned that there are subtle differences between the current 
code and Linux' mem*_io() functions. Keeping the wrappers might be needed.

Best regards
Thomas

> 
> 	Sam

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 0/6] fbdev: Move framebuffer I/O helpers to <asm/fb.h>

[Geert Uytterhoeven]

Hi Thomas,

On Tue, May 2, 2023 at 3:02 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> (was: fbdev: Use regular I/O function for framebuffers)
>
> Fbdev provides helpers for framebuffer I/O, such as fb_readl(),
> fb_writel() or fb_memcpy_to_fb(). The implementation of each helper
> depends on the architecture, but they are all equivalent to regular
> I/O functions of similar names. So use regular functions instead and
> move all helpers into <asm-generic/fb.h>
>
> The first patch a simple whitespace cleanup.
>
> Until now, <linux/fb.h> contained an include of <asm/io.h>. As this
> will go away patches 2 to 4 prepare include statements in the various
> drivers. Source files that use regular I/O helpers, such as readl(),
> now include <linux/io.h>. Source files that use framebuffer I/O
> helpers, such as fb_readl(), also include <asm/fb.h>.
>
> Patch 5 replaces the architecture-based if-else branching in
> <linux/fb.h> by helpers in <asm-generic/fb.h>. All helpers use Linux'
> existing I/O functions.
>
> Patch 6 harmonizes naming among fbdev and existing I/O functions.
>
> The patchset has been built for a variety of platforms, such as x86-64,
> arm, aarch64, ppc64, parisc, m64k, mips and sparc.
>
> v3:
>         * add the new helpers in <asm-generic/fb.h>
>         * support reordering and native byte order (Geert, Arnd)

Thanks, this fixes the mangled display I was seeing on ARAnyM
with bpp=16.

BTW, this series seems to have mixed dependencies: the change
to include/asm-generic/fb.h depends on "[PATCH v3 00/19] arch:
Consolidate <asm/fb.h>"[1], but with that applied, I had to manually
fixup drivers/video/fbdev/core/fb_cfb_fops.c.

[1] https://lore.kernel.org/all/20230417125651.25126-1-tzimmermann@suse.de,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 4/6] fbdev: Include <linux/io.h> via <asm/fb.h>

[Arnd Bergmann]

On Wed, May 3, 2023, at 10:12, Thomas Zimmermann wrote:
> Am 03.05.23 um 09:19 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>>>>
>>>> There are countless examples where the above are not followed,
>>>> but to my best understanding the above it the preferred way to do it.
>>>
>>> Where did youher this? I only know about this in the case of asm/io.h
>>> vs. linux/io.h.
>>>
>> 
>> I understand that's the case too. I believe even checkpatch.pl complains
>> about it? (not that the script always get right, but just as an example).
>
> Do you know if that's the general rule? If so, we might want to 
> repurpose <asm/fbio.h> for the framebuffer I/O functions.

It's certainly the general trend across all of the kernel to have
drivers prefer including linux/*.h, and to move stuff from asm/*.h
to linux/*.h as it gets generalized across architectures.

     Arnd

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2336 bytes --]

Hi

Am 02.05.23 um 22:06 schrieb Arnd Bergmann:
> On Tue, May 2, 2023, at 15:02, Thomas Zimmermann wrote:
>> Implement framebuffer I/O helpers, such as fb_read*() and fb_write*(),
>> in the architecture's <asm/fb.h> header file or the generic one.
>>
>> The common case has been the use of regular I/O functions, such as
>> __raw_readb() or memset_io(). A few architectures used plain system-
>> memory reads and writes. Sparc used helpers for its SBus.
>>
>> The architectures that used special cases provide the same code in
>> their __raw_*() I/O helpers. So the patch replaces this code with the
>> __raw_*() functions and moves it to <asm-generic/fb.h> for all
>> architectures.
>>
>> v3:
>> 	* implement all architectures with generic helpers
>> 	* support reordering and native byte order (Geert, Arnd)
> 
> This looks good for the read/write helpers, but I'm a little
> worried about the memset and memcpy functions, since they do
> change behavior on some architectures:
> 
> - on sparc64, fb_mem{set,cpy} uses ASI_PHYS_BYPASS_EC_E (like __raw_readb)
>    while mem{set_,cpy_from,cpy_to} uses ASI_PHYS_BYPASS_EC_E_L (like readb)
>    I don't know the effect of that, but it seems intentional
> 
> - on loongarch and csky, the _io variants avoid unaligned access,
>    while the normal memcpy/memset is probably broken, so your
>    patch is a bugfix
> 
> - on ia64, the _io variants use bytewise access and avoid any longer
>    loads and stores, so your patch probably makes things slower.
> 
> It's probably safe to deal with all the above by either adding
> architecture specific overrides to the current version, or
> by doing the semantic changes before the move to asm/fb.h, but
> one way or the other I'd prefer this to be separate from the
> consolidation patch that should not have any changes in behavior.

I think I'll add architecture overrides that contain the current code, 
even if they contain some force-casting wrt __iomem. If anyone wants to 
fix the issues, they can then address them easily.

Best regards
Thomas

> 
>       Arnd

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Arnd Bergmann]

On Wed, May 3, 2023, at 16:55, Thomas Zimmermann wrote:
> Am 02.05.23 um 22:06 schrieb Arnd Bergmann:

>> It's probably safe to deal with all the above by either adding
>> architecture specific overrides to the current version, or
>> by doing the semantic changes before the move to asm/fb.h, but
>> one way or the other I'd prefer this to be separate from the
>> consolidation patch that should not have any changes in behavior.
>
> I think I'll add architecture overrides that contain the current code, 
> even if they contain some force-casting wrt __iomem. If anyone wants to 
> fix the issues, they can then address them easily.

Ok, sounds good,

     Arnd

Re: [PATCH v3 5/6] fbdev: Move framebuffer I/O helpers into <asm/fb.h>

[Sam Ravnborg]

Hi Thomas,

> > But I am missing something somewhere as I cannot see how this builds.
> > asm-generic now provide the fb_read/fb_write helpers.
> > But for example sparc has an architecture specifc fb.h so it will not
> > use the asm-generic variant. So I wonder how sparc get hold of the
> > asm-generic fb.h file?
> 
> All architecture's <asm/fb.h> files include <asm-generic/fb.h>, so that they
> all get the interfaces which they don't define themselves. For Sparc, this
> is at [1].
> 
> Best regards
> Thomas
> 
> 
> [1]
> https://cgit.freedesktop.org/drm/drm-tip/tree/arch/sparc/include/asm/fb.h#n19
> 
> > 
> > Maybe it is obvious, but I miss it.

OK, it was obvious and I missed it.
I looked at the mainline kernel, and not the drm-tip variant.
Sorry for the noise.

	Sam

Re: [PATCH v3 6/6] fbdev: Rename fb_mem*() helpers

[Sam Ravnborg]

Hi Thomas,

On Wed, May 03, 2023 at 10:15:46AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 02.05.23 um 22:08 schrieb Sam Ravnborg:
> > Hi Thomas.
> > 
> > On Tue, May 02, 2023 at 03:02:23PM +0200, Thomas Zimmermann wrote:
> > > Update the names of the fb_mem*() helpers to be consistent with their
> > > regular counterparts. Hence, fb_memset() now becomes fb_memset_io(),
> > > fb_memcpy_fromfb() now becomes fb_memcpy_fromio() and fb_memcpy_tofb()
> > > becomes fb_memcpy_toio(). No functional changes.
> > > 
> > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > ---
> > ...
> > > -#ifndef fb_memcpy_fromfb
> > > -static inline void fb_memcpy_fromfb(void *to, const volatile void __iomem *from, size_t n)
> > > +#ifndef fb_memcpy_fromio
> > > +static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
> > >   {
> > >   	memcpy_fromio(to, from, n);
> > >   }
> > > -#define fb_memcpy_fromfb fb_memcpy_fromfb
> > > +#define fb_memcpy_fromio fb_memcpy_fromio
> > >   #endif
> > > -#ifndef fb_memcpy_tofb
> > > -static inline void fb_memcpy_tofb(volatile void __iomem *to, const void *from, size_t n)
> > > +#ifndef fb_memcpy_toio
> > > +static inline void fb_memcpy_toio(volatile void __iomem *to, const void *from, size_t n)
> > >   {
> > >   	memcpy_toio(to, from, n);
> > >   }
> > > -#define fb_memcpy_tofb fb_memcpy_tofb
> > > +#define fb_memcpy_toio fb_memcpy_toio
> > >   #endif
> > >   #ifndef fb_memset
> > > -static inline void fb_memset(volatile void __iomem *addr, int c, size_t n)
> > > +static inline void fb_memset_io(volatile void __iomem *addr, int c, size_t n)
> > >   {
> > >   	memset_io(addr, c, n);
> > >   }
> > > -#define fb_memset fb_memset
> > > +#define fb_memset fb_memset_io
> > 
> > The static inlines wrappers does not provide any value, and could be replaced by
> > direct calls to memcpy_fromio(), memcpy_toio(), memset_io().
> > 
> > If you decide to keep the wrappers I will not hold you back, so the
> > patch has my:
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> > But I prefer the direct calls without the wrappers....
> 
> At first I was also skeptical if those fb_mem*() wrappers are needed. But
> Arnd mentioned that there are subtle differences between the current code
> and Linux' mem*_io() functions. Keeping the wrappers might be needed.
Saw the dialog, and agree that keeping current behaviour is the way to
go for now even if this is more code and wrappers.

	Sam