Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] Respond:Add SUNIX-Multi-I/O card device driver
To:     Morris Ku <saumah@gmail.com>, gregkh@linuxfoundation.org
Cc:     morris_ku@sunix.com, linux-kernel@vger.kernel.org, arnd@arndb.de
References: <20190313133118.14506-1-saumah@gmail.com>
From:   "Enrico Weigelt, metux IT consult" <lkml@metux.net>
Organization: metux IT consult
Message-ID: <93cf168f-9282-ba00-11a2-04252c9c93f4@metux.net>
Date:   Wed, 13 Mar 2019 18:36:36 +0100
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20190313133118.14506-1-saumah@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@metux.net -- +49-151-27565287