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

[David Brownell <david-b@pacbell.net>]

On Tuesday 23 February 2010, Feng Tang wrote:
> 
> +config SERIAL_MAX3110
> +       tristate "SPI UART driver for Max3110"
> +       select SERIAL_CORE
> +       select SERIAL_CORE_CONSOLE

Shouldn't that depend on SPI_MASTER?  As it stands, you're
permitting it to build on systems that you *know* don't have
a prayer of running this code ... or often, even finishing
the build.


> +       help
> +         This is the UART protocol driver for MAX3110 device
> +
> +config MAX3110_DESIGNWARE

Having this depend on a specific underlying SPI master controller
is not a good thing.  It should instead be written so that it
runs correctly on *any* controller which exports the standard SPI
programming interface.



> +       boolean "Enable Max3110 to work with Designware controller"
> +       default y
> +       depends on SERIAL_MAX3110
> +

That is, stuff like this, from your max3110 driver:

> +
> +#ifdef CONFIG_MAX3110_DESIGNWARE
> +static struct dw_spi_chip spi_uart = {
> +       .poll_mode = 1,
> +       .enable_dma = 0,
> +       .type = SPI_FRF_SPI,
> +};
> +#endif

Is completely irrelevant for other hardware ... or else
(as with DMA, which should be "enabled by default") is
relevant, but shouldn't be parameterized.


> +       spi->mode = SPI_MODE_0;
> +       spi->bits_per_word = 16;
> +#ifdef CONFIG_MAX3110_DESIGNWARE
> +       spi->controller_data = &spi_uart;
> +#endif

That all looks fishy too.  SPI_MODE should have
been set up as part of device creation.  Ditto
any spi->controller_data ... normally, that all
gets set up as part of board-specific setup.

Normally one goes to a lot of effort to keep
such board-specific code out of drivers.

- Dave

--
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