Messages in this thread | | | Subject | Re: [PATCH 2/4] Add SUNIX Multi-I/O card device driver | From | "Enrico Weigelt, metux IT consult" <> | Date | Mon, 11 Mar 2019 20:27:43 +0100 |
| |
On 11.03.19 16:05, Arnd Bergmann wrote:
Hi,
> Enrico has already replied with the most important comments. Here> are just some more high-level thoughts from me: I'll try to make things a bit more clear to newcomers:
> * We want the functionality to be present in subsystem specific> standalone drivers, i.e. drivers/tty/serial/, drivers/parport/, drivers/gpio> etc, as Enrico said That means we'd have completely separate drivers for the uart, parport, gpio, etc. These might share some common code (eg. some board config data which is used by all drivers), in a separate module.
> If this is not possible, what you may need to do here is to have a> small driver in drivers/mfd/ that handles the PCI function and creates> platform_device instances that the individual drivers can bind to.
'bind to' here means, the mfd driver will prepare the platform data (platform specific config struct) for the actual drivers and creates instances of them (see platform_device_register*() functions). The kernel will then load the corresponding modules (if not compiled-in) and calls these drivers with the platform data passed in to it's probe() callback.
An example could be my recent gpio-amd-fch and pcengines-apuv2 drivers. Here, the gpio-amd-fch drives the gpio's in the SoC, while the pcengines-apuv2 driver one is just binding drivers together - the actual config (register addressed, etc) is in the pcengines-apu2 driver:
See: drivers/gpio/gpio-amd-fch.c drivers/platform/x86/pcengines-apuv2.c
Maybe it would be a good start, spliiting out the gpio stuff into it's own driver. If the gpio subdevice can't be probed individually (eg. has it's own pci function), you could do it the way I did it w/ gpio-amd-fch and pcengines-apuv2.
> * do not implement your own ioctl handlers. If you need a custom> interface for something hardware specific, split that out into> a separate patch, so the base support can be reviewed> independently. You really should try to get around w/o adding any special ioctl()s or other userland interfaces. If you *really* believe you can't do that, please let's talk about this first. Such cases usually indicate a missing generic functionality in one of the subsystems.
Let me stress some very important point here (which also I frequently have to explain to my clients): in Linux, the kernel is *the* "hardware abstraction layer". Applications can just use generic APIs and never care about the actual underlying hardware - the kernel just presents everything by a uniform interface. That's why drivers really just dock into the corresponding subsystems and not introduce their own APIs.
There're only few exceptions from that, eg. DRI subsystem, because these devices tend to be so specific, that it just doesn't make sense to create a uniform interface (unless we'd want to move entire gallium into the kernel ;-)).
From what I can see in your patches, your cards provide pretty standard uart, parport, gpio.
> * If the functions (in particular serial and parport) have a similar> register layout to existing drivers, try to reuse the existing code> and extend it, rather than duplicating the implementation. Yes, please have a closer look into the corresponding subsystems, which generic drivers already exist, that might fit to your device.
For example, in gpio, we've got a generic driver for devices that map all the direction flags into one, and the line statii into another register (see bgpio_*() functions). We're yet lacking another generic one that puts everything for one line into one register (and then has an array of those) - that's still on my todo list.
--mtx
-- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
| |