Em Sáb, 2008-11-22 às 18:19 -0800, David Brownell escreveu: > SPI-specific bits: > > - I'd have to see the patch. The last one I saw still didn't > list a SPI_MASTER dependency for the core MFD module.
The next one will. > - You should make ezx_pcap_write() and ezx_pcap_read() switch > over to spi_write_then_read(), to ensure it's never doing > DMA to/from the stack. I see a byte-order dependency too... I tried spi_write_then_read before, but it didn't work. I supposed it was because it was doing 2 transfers as the second transfer rx_buf always came zeroed. I see that commit f9b90e39cbc5c4d6ef60022fd1f25d541df0aad1 changed it to do a single transfer, so i will try it again. > - For general paranoia, the probe() should abort if pcap.spi is > already set ... and its cleanup path, plus remove(), should > null that pointer. Ok. > - If you're going to mark the probe() as __devinit, then mark > the remove() as __devexit and use __devexit_p() in the driver > struct. Ok. Shouldn't i use __init instead? > Other comments about the pcap2 core: > - Those show_regs/store_regs calls would IMO make more sense > in debugfs than in sysfs. I will just remove those for now. > - The set_vreg() stuff would seem to make more sense in a > regulator subdevice and driver ... but that's more of a > general driver structure thing, and might be fixed later. > > - Andrew seems to always want a comment explaining why the > IRQ handler for I2C and SPI devices needs to queue_work(). > Maybe the threaded IRQ stuff will help there... > > - The mask_event()/unmask_event() stuff looks like you're > more or less reinventing a baby "struct irq_chip", with > register_event() instead of request_irq(). > Re the IRQ stuff, this looks more like what i2c/chips/menelaus.c > did than mfd/twl4030-irq.c ... in general I think it's better to > pursue the latter approach, making genirq handle such stuff. > (Even though it's kind of awkward to use it for I2C or SPI based > interrupt controllers just now.) I didn't know i could use genirq with spi. In fact i was using genirq before, when the driver was using ssp.c. Now, looking at twl4030-irq.c its clear how i should have done this. Thanks!! > - And the ADC sysfs support isn't supporting hwmon models. > (Neither does the twl4030 ADC support, but that's not been > submitted for mainline yet either...) I will look into this after i finish the irq stuff. This is a lot of stuff, thanks for the review. :) -- Daniel Ribeiro ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
