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

From: Feng Tang <feng.tang@intel.com>
To: David Brownell <david-b@pacbell.net>
Cc: Greg KH <greg@kroah.com>,
	Grant Likely <grant.likely@secretlab.ca>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@suse.de>,
	spi-devel-list <spi-devel-general@lists.sourceforge.net>,
	"linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
	"alan@lxorguk.ukuu.org.uk" <alan@lxorguk.ukuu.org.uk>
Subject: Re: [RFC][PATCH v3] serial: spi: add spi-uart driver for Maxim 3110
Date: Thu, 25 Feb 2010 15:44:11 +0800	[thread overview]
Message-ID: <20100225154411.6d1ec4d0@feng-i7> (raw)
In-Reply-To: <201002242043.15231.david-b@pacbell.net>

On Thu, 25 Feb 2010 12:43:14 +0800
David Brownell <david-b@pacbell.net> wrote:

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

Good point about the DW controller specific data, I'll remove them.
For those "bits_per_word" setting, I think we can put it here instead of
the board initialization code, as many types of boards can leverage the
setting here as it only works in 16b mode.

Thanks,
Feng

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