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

[Enrico Weigelt, metux IT consult]

On 13.03.19 14:31, Morris Ku wrote:

Hi,

> +why isn't that in ./drivers/tty/serial/sunix/ ?
> +
> driver support SUNIX Character Devices,
> serial ports and parallel ports,so we suggest 
> that in /drivers/char. 

Well, this seems to be a composite device. so it should be actually
different drivers (initialized by one compound driver) - all of the
subdevices I'm seeing here have their own subsystems, none of them
being a plain chardev. Therefore it probably should go to drivers/mfd
(multi function devices). If you split out the subdevice drivers
(which I'd recommend), these should go to the corresponding subsystem
directories (eg. drivers/tty/serial/ for the serial subdevice).

> +please use full name: SUNIX
> +
> 
> Ok, i'll fix in next verion.

Oh, by the way: SUNIX is the company name ? So, there's probably some
device/product name. I'd put that into the config name, too.

Eg. if the device is called "FANCYIO", then the config symbol would
be SUNIX_FANCYIO.

> +why exactly do you introduce driver-specific ioctls ?
> +
> ioctl functions support SUNIX specific serial,parellel and GPIO 
> ,need to use specific ioctol function to get related inforomation.

Which information, exactly, that aren't supported by the corresponding
subsystems ?

To make it clear: the individual functions of this card should go into
the corresponding subsystem. So, you'd have a serial driver, a parallel
driver, a gpio driver - all living in the corresponding subsystems.
NOT: one driver (more precisely: one chardev) to rule them all.

Note: in Linux we wanna use generic APIs where we can. So, if somebody
wants to use a GPIOs, he goes by the GPIO subsystem - no matter which
hardware it actually is - no hw specific devices/ioctl.

> +what is "ACS"
> +
> RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit 
> data when receive data is going.

See struct uart_port and corresponding helpers (drivers/tty/serial),
it already has infrastructure for that. It also does all the buffering
stuff for you.

> SUNIX Multi-I/O card combaine serial ports,parallel ports and GPIO,
> therefore, the define support SUNIX specific gpio interface. 

See above: don't introduce your own specific interfaces - use the
generic ones. Seriously, that's the way it's going on Linux.

> +> +     char                    *dma_buf;
> +
> +why not void * ?
> +
> i am not sure what you mean ?

Is it really correct that the dma_buf has to be char* ?
(and even w/o signed/unsigned attribute).

For opaque memory buffers, we usually use void*.

> +> +// snx_devtable.c
> +> +extern PCI_BOARD snx_pci_board_conf[];
> +<snip>
> +
> +why all these extern functions ?
> +
> function definition in multiple .c files.

it's not a function, but an array of structs.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@xxxxxxxxx -- +49-151-27565287