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

[Felipe Balbi]

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: signature.asc
Description: Digital signature