On Thu, Mar 4, 2010 at 8:22 AM,
<spi-devel-general-request@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi All,
>
Hi, first of all let me apologize that I haven't see your previous
versions of the driver. I completely missed them. I am the author of
the max3100 driver. As far as I can see the max3110 is just a max3100
with the TTL to EIA level converter. So, first of all, may I ask why
you did start a new driver from scratch? I'm quite happy with the
current one, the only problem I see is that it uses worqueues which
are scheduled like normal processes and so under load their latency is
*big*. I wanted to move to threaded interrupts (otherwise you have to
use some ugly tricks to give workqueue threads real-time scheduling
class) but was waiting for my beta testers^H^H^H^H^H^H^H^H^H^H^H^H^H
customers to move to modern kernel that support them. Anyway the patch
is trivial and I can provide it if there is some interest. A quick
glance at your patch shows you are using the same sched_other threads
so you will face the same problems. I saw you added console capability
which isn't present in current driver, but it's easy to add (I didn't
implement it initially because I don't see a good idea using the
max3100 for initial bring-up of an embedded system because it relies
on the SPI subsystem, worqueues, scheduler and such complicated
things). Anyway keep in mind that the MAX3100 is rather low-end UART
so don't push it too much.
I'm more than happy to work with you to have just one perfect (or
almost perfect) driver instead of two of them half-done. See some
comments inline.
> + * 1. From Max3110 spec, the Rx FIFO has 8 words, while the Tx FIFO only has
> + * 1 word. If SPI master controller doesn't support sclk frequency change,
> + * then the char need be sent out one by one with some delay
I don't think this is a good idea to change speed. Just check the T
bit to see when the TX buffer is empty and send at the maximum
available speed. So the usage of the SPI bus is better.
> + *
> + * 2. Currently only RX availabe interrrupt is used
yeah, this is one of the reasons why MAX31x0 sucks: when you change
configuration of interrupts the RX buffer is cleared. So you cannot
activate/deactivate TX empty interrupt as needed.
> +static int use_irq = 1;
> +module_param(use_irq, int, 0444);
> +MODULE_PARM_DESC(use_irq, "Whether using Max3110's IRQ capability");
I don't think it's a good idea using an UART in polling mode, it just
kills the system. Just pretend this is not possible otherwise some
hardware guys will pester us to do this just to save an electric wire.
> + memset(&x, 0, sizeof x);
> + x.len = len;
> + x.tx_buf = txbuf;
> + x.rx_buf = rxbuf;
why don't initialize this like:
struct spi_transfer x = {
.tx_buf = txbuf,
.rx_buf = rxbuf,
.len = len,
};
> + buf = kmalloc(8, GFP_KERNEL | GFP_DMA);
you do kmalloc and kfree in routines that are called in quite tight
loops: I guess it's an overkill for performance.
> +static unsigned int serial_m3110_tx_empty(struct uart_port *port)
> +{
> + return 1;
> +}
> +
> +static void serial_m3110_stop_tx(struct uart_port *port)
> +{
> + return;
> +}
> +
> +static void serial_m3110_stop_rx(struct uart_port *port)
> +{
> + return;
> +}
are you sure it's sane to just ignore these from higher level?
> + u8 recv_buf[512], *pbuf;
is this sane to allocate 512 byte on the stack?
> + wait_event_interruptible(*wq,
> + max->flags || kthread_should_stop());
> + test_and_set_bit(0, &max->mthread_up);
I guess you are using mthread_up to avoid awakening the main thread
too many times. But this makes sense? Anyway because the awakening and
the setting of the bit is not atomic it won't work anyway.
> +/* Don't handle hw handshaking */
> +static unsigned int serial_m3110_get_mctrl(struct uart_port *port)
> +{
> + return TIOCM_DSR | TIOCM_CAR | TIOCM_DSR;
> +}
> +
> +static void serial_m3110_set_mctrl(struct uart_port *port, unsigned int mctrl)
> +{
> +}
> +
this is quite bad because the MAX3100 is rather slow.
Bye!
--
Christian Pellegrin, see http://www.evolware.org/chri/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
