Re: [PATCH 2/4] Add SUNIX Multi-I/O card device driver

[Enrico Weigelt, metux IT consult]

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@xxxxxxxxx -- +49-151-27565287