[PATCH 06/10 V2] spi: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Mark Brown,

Thanks for the review!

> On Mon, Jul 23, 2012 at 10:40:48PM +0200, Marek Vasut wrote:
> > This is slightly reworked version of the SPI driver.
> > Support for DT has been added and it's been converted
> > to queued API.
> 
> Looks reasonable overall.
> 
> > +	bits_per_word = dev->bits_per_word;
> > +	if (t && t->bits_per_word)
> > +		bits_per_word = t->bits_per_word;
> > +
> > +	if (bits_per_word != 8) {
> > +		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
> > +					__func__, bits_per_word);
> > +		return -EINVAL;
> > +	}
> > 
> > +	if (dev->max_speed_hz)
> > +		hz = dev->max_speed_hz;
> > +	if (t && t->speed_hz)
> > +		hz = t->speed_hz;
> > +	if (hz == 0) {
> > +		dev_err(&dev->dev, "Cannot continue with zero clock\n");
> > +		return -EINVAL;
> > +	}
> 
> These two blocks use a different style (the first does the first assign
> unconditionally, the second uses initialisation with declaration).  I
> prefer the first style but YMMV and it doesn't matter.
> 
> For the missing clock rate might it make sense to just use whatever the
> clock happens to come out as?

Hm, the max_speed_hz should be the cap for the transfer speed. I will change the 
function to this:

hz = dev->max_speed_hz;
if (t && t->speed_hz)
        hz = min(hz, t->speed_hz);

> > +static void mxs_spi_cleanup(struct spi_device *dev)
> > +{
> > +	return;
> > +}
> 
> Empty functions are generally a warning sign...  why do we need this
> one?

Good catch, thanks!

> > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > +{
> > 
> > +	clk_disable_unprepare(ssp->clk);
> 
> It'd be nice to only keep the clocks enabled while doing transfers but
> again totally non-essential.

Hm, this is spread across mxs. Shawn, is there any plan for PM implementation 
for MXS ?

Best regards,
Marek Vasut