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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
