Hi Christian, Thanks for taking time to review the driver. Please see my answers inline - Feng On Wed, 17 Mar 2010 01:22:46 +0800 christian pellegrin <chripell@xxxxxxxxx> wrote: > 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 I don't want to reinvent the wheel, as I have many more tasks to handle :) My 3110 driver started to work in 2008(surely very basic at that time) before your driver is posted on public. And the reasons I still posted driver here are: 1. It provides a console 2. It use buffer read/write to meet our performance goal, max3110 doesn't have good FIFO, but spi controller have and we can leverage that for performance If we can cooperate to make current 3100 driver meet our expectation, I'm more than happy to drop my 3110 one. I'd rather submit some bug fix for it than posting 6 or more versions of code :) > 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. It's not me push it too much, but I was pushed too much :) We can't make the decision how the HW is used on all kinds of platforms. For ours, we heavily rely on it as a sole console, and basic requirement is: the datasheet say it supports 230400~300 baud rate, then driver guy need make it work as it claims, like copying 500 bytes string somewhere and paste it on this console in 230400/600 rate. For the kernel early boot phase debug, I actually had another 3110 early_printk patch which can work right after the kernel starts > > 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. Can't agree more, all I want is just a upstream driver which meets the basic requirement. > > > + * 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. Yes, checking T bit is a good idea for single word transfer model, but it doesn't help when using buffer read/write > > > + * > > + * 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. Yes, like Grant and others have mentioned, I plan to use spi->irq as the condition, platform code which initialize the board_info should clear the irq to 0 if they won't use IRQ for 3110 if (spi->irq) request_irq(spi->irq,...); else start_read_thread; > > > > + 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. Overkill is just the word I have used when replying comments on my earlier version :) I didn't use kmalloc/kfree in my earlier submission, but other developers mentioned the buffer allocated here need be DMA safe, as 3110 may work with many kinds of SPI controller, some of them only supports DMA mode. > > 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? The function is called only in driver's own main thread and reader thread context, 512 should be safe enough. Also the reason I don't use kzalloc/kfree is the same as you mentioned: performance > > > + 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. Why this can't work? the test/set_bit are atomic operations. This bit check is not exclusive, it just try to save some waking up, though waking a already running thread doesn't cost too much as mentioned by Andrew -- 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
