Hi
Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
d 100644
--- a/drivers/gpu/drm/qxl/Kconfig
+++ b/drivers/gpu/drm/qxl/Kconfig
@@ -2,6 +2,7 @@
config DRM_QXL
tristate "QXL virtual GPU"
depends on DRM && PCI && MMU
+ depends on HAS_IOPORT
Is there a difference between this style (multiple 'depends on') and the
one used for gma500 (&& && &&)?
No, it's the same. Doing it in one line is mainly useful
if you have some '||' as well.
@@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
writeb(val, bochs->mmio + offset);
} else {
+#ifdef CONFIG_HAS_IOPORT
outb(val, ioport);
+#endif
Could you provide empty defines for the out() interfaces at the top of
the file?
That no longer works since there are now __compiletime_error()
versions of these funcitons. However we can do it more nicely like:
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 9b337f948434..034af6e32200 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
return;
- if (bochs->mmio) {
+ if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
int offset = ioport - 0x3c0 + 0x400;
writeb(val, bochs->mmio + offset);
} else {
-#ifdef CONFIG_HAS_IOPORT
outb(val, ioport);
-#endif
}
For all functions with such a pattern, could we use:
bool bochs_uses_mmio(bochs)
{
return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
}
void writeb_func()
{
if (bochs_uses_mmio()) {
writeb()
#if CONFIG_HAS_IOPORT
} else {
outb()
#endif
}
}
u8 readb_func()
{
u8 ret = 0xff
if (bochs_uses_mmio()) {
ret = readb()
#if CONFIG_HAS_IOPORT
} else {
ret = inb()
#endif
}
return ret;
}
?
The code in bochs_uses_mmio() could then also print a drm_warn_once if
we have neither MMIO nor I/O ports.
I'd review a separate series for such a change.
}
@@ -128,16 +126,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
return 0xff;
- if (bochs->mmio) {
+ if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
int offset = ioport - 0x3c0 + 0x400;
return readb(bochs->mmio + offset);
} else {
-#ifdef CONFIG_HAS_IOPORT
return inb(ioport);
-#else
- return 0xff;
-#endif
}
}
@@ -145,32 +139,26 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
{
u16 ret = 0;
- if (bochs->mmio) {
+ if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
int offset = 0x500 + (reg << 1);
ret = readw(bochs->mmio + offset);
} else {
-#ifdef CONFIG_HAS_IOPORT
outw(reg, VBE_DISPI_IOPORT_INDEX);
ret = inw(VBE_DISPI_IOPORT_DATA);
-#else
- ret = 0xffff;
-#endif
}
return ret;
}
static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val)
{
- if (bochs->mmio) {
+ if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
int offset = 0x500 + (reg << 1);
writew(val, bochs->mmio + offset);
} else {
-#ifdef CONFIG_HAS_IOPORT
outw(reg, VBE_DISPI_IOPORT_INDEX);
outw(val, VBE_DISPI_IOPORT_DATA);
-#endif
}
}
And the in() interfaces could be defined to 0xff[ff].
I assume that you don't want to provide such empty macros in the
kernel's io.h header?
That was the original idea many years ago, but Linus rejected
my pull request for it, so Niklas worked through all drivers
individually to add the dependencies instead.
I see.
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)
