Re: [PATCH v5] serial: spi: add spi-uart driver for Maxim 3110

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, Mar 2, 2010 at 7:57 PM, Feng Tang <feng.tang@intel.com> wrote:
> Hi All,
>
> Here is a driver for Maxim 3110 SPI-UART device, please help to review.
>
> It has been validated with Designware SPI controller (drivers/spi: dw_spi.c &
> dw_spi_pci.c). It supports polling and IRQ mode, supports batch read, and
> provides a console.
>
> change log:
>  since v4:
>        * Add explanation why not using DMA
>        * Fix a bug in max3110_read_multi()
>  since v3:
>        * Remove the Designware controller related stuff
>        * Remove some initialization code which should be done in the platform
>          setup code
>        * Add a missing change for drivers/serial/Makefile
>  since v2:
>        * Address comments from Andrew Morton
>        * Use test/set_bit to avoid race condition for SMP/preempt case
>        * Fix bug related with endian order
>  since v1:
>        * Address comments from Alan Cox
>        * Use a "use_irq" module parameter to runtime check whether
>          to use IRQ mode
>        * Add the suspend/resume implementation
[...]
> + *
> + * 3. This driver doesn't support work with a spi cotnroller in DMA mode, and
> + *    the reason for not using DMA is:
> + *    a) Maxim 3110 is a low speed UART device, whose tx/rx buffer are very few,
> + *    and using DMA may be over-killing when working as a system console(imaging
> + *    we edit a text file on it)
> + *    b) Handling only one 16b word transfer will be very common, but non-32b
> + *    aligned DMA operation is not supported by all kinds of DMA controllers
> + *
> + *    Some spi controller drivers like Pxa2xx, Designware have option for device
> + *    driver to chose to work in poll or DMA mode. And platform driver which
> + *    enumerates Max3110 device should leverage this option to not use DMA.

Between this, reading through the other comments, and the existence of
the max3100 driver, I'm less and less comfortable with this driver.
First, if this driver gets merged, then it is quite likely that
neither you or the 3100 author will get around to merging them.
Second, as others have pointed out, this driver is making assumptions
about the SPI bus that it has no business making.  The fact that it
doesn't work with DMA isn't a design choice, it is a bug and whether
or not it is a low speed uart device is completely beside the point.

I'm generally not involved with the serial device drivers, but FWIW,
I'm no longer in favor of this driver getting merged.  If you really
want to get it merged, then I recommend asking Greg to pick it up into
staging so that there is some imperative to fix it up and merge it
with the max3100 driver.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html