On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote:
> On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
>> #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
>> /* Support PCI only */
>> static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
>> {
>> - return inl(uhci->io_addr + reg);
>> + return UHCI_IN(inl(uhci->io_addr + reg));
>> }
>>
>> static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
>> {
>> - outl(val, uhci->io_addr + reg);
>> + UHCI_OUT(outl(val, uhci->io_addr + reg));
>
> I'm confused now.
>
> So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
>
> But if it isn't, then these are just no-ops that do nothing? So then
> the driver will fail to work? Why have these stubs at all?
>
> Why not just not build the driver at all if this option is not enabled?
If I remember correctly, the problem here is the lack of
abstractions in the uhci driver, it instead supports all
combinations of on-chip non-PCI devices using readb()/writeb()
and PCI devices using inb()/outb() in a shared codebase.
A particularly tricky combination is a kernel that supports on-chip
UHCI as well as CONFIG_USB_PCI (for EHCI/XHCI) but does not support
I/O ports because of platform limitations. The trick is to come up
with a set of changes that doesn't have to rewrite the entire logic
but also doesn't add an obscene number of #ifdef checks.
That said, there is a minor problem with the empty definition
+#define UHCI_OUT(x)
I think this should be "do { } while (0)" to avoid warnings
about empty if/else blocks.
Arnd
