(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
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 <[email protected]>
---
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
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
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)
Thomas Zimmermann <[email protected]> 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
On Wed, May 3, 2023 at 9:19 AM Javier Martinez Canillas
<[email protected]> wrote:
> Thomas Zimmermann <[email protected]> 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 -- [email protected]
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
Hi
Am 03.05.23 um 09:19 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <[email protected]> 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)
Hi Thomas,
On Tue, May 2, 2023 at 3:02 PM Thomas Zimmermann <[email protected]> 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/[email protected],
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, May 3, 2023, at 10:12, Thomas Zimmermann wrote:
> Am 03.05.23 um 09:19 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <[email protected]> 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