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

Dear Mark Brown,

Thanks for the review!

On Mon, Jul 23, 2012 at 10:40:48PM +0200, Marek Vasut wrote:

quoted

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.

quoted

+	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);

quoted

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

quoted

+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