Re: [PATCHv2] drivers: spi: Add qspi flash controller

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 1730 bytes --]

On Tue, Jul 02, 2013 at 12:04:32PM +0100, Mark Brown wrote:
> On Tue, Jul 02, 2013 at 01:43:38PM +0300, Felipe Balbi wrote:
> > On Tue, Jul 02, 2013 at 11:17:18AM +0100, Mark Brown wrote:
> 
> > > +	/* setup command reg */
> > > +	qspi->cmd = 0;
> > > +	qspi->cmd |= QSPI_WLEN(8);
> 
> > Sourav hardcodes wordlenght to 8-bits, and yet he enables 8, 16 and
> > 32-bits per word.
> 
> Yeah, that's what I noticed (well first off I noticed that there were no
> constraints on bits per word at all).
> 
> > > +	qspi->cmd |= QSPI_EN_CS(0);
> 
> > he's also hardcoding the chipselect line which should be take from
> > m->spi->chip_select
> 
> This one *can* be OK if the driver only accepts one chip select (though
> obviously supporting more is better).  I'd really only done a fairly

this controller has 6 chip selects IIRC

> high level review for framework stuff so hadn't got that far yet.
> 
> One thing I really want to get round to doing with the SPI core is
> providing an easy to pick up GPIO chip select as standard 

that should be fairly simple I guess. Just lack of time, I'm assuming ?

Complex will be to support up to 128-bits per word as this particular
controller supports. In fact, this controller is, IMO, overly flexible.
You can do 1, 2, 3, 4, 5, 6, ... , 128 bits per word.

The only problem is that the DATA registers which give you access to the
shift register, aren't mapped to consecutive offsets, but that should
still be ok since we will, anyway, be using readl/writel for each
register. Still, for platforms which can provide writeq/readq (not sure
if that's already supported on ARM) we could save a few cycles, no ?

anyway... it is what it is.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]