On Mon, Feb 10, 2014 at 06:29:26PM +0200, Ivan T. Ivanov wrote:

On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 

quoted

quoted

+	if (!xfer)
+		return IRQ_HANDLED;

quoted

Are you sure?  It seems wrong to just ignore interrupts, some comments
would help explain why.

Of course this should never happen. This was left from initial stage
of the implementation.

OK, so reporting them as errors seems better then.

quoted

quoted

+	if (controller->speed_hz != chip->speed_hz) {
+		ret = clk_set_rate(controller->cclk, chip->speed_hz);
+		if (ret) {
+			dev_err(controller->dev, "fail to set frequency %d",
+				chip->speed_hz);
+			return -EIO;
+		}
+	}

quoted

Is calling into the clock framework really so expensive that we need to
avoid doing it?  

Probably not. But why to call it if the frequency is the same.

It's less code that could go wrong and the check is already there in the
clock framework hopefully.

quoted

You also shouldn't be interacting with the hardware in
setup().

This is internal hw-setup, not master::setup() or I am
missing something?

The naming could probably be clearer then - config or something.

quoted

quoted

+	if (chip->bits_per_word <= 8)
+		controller->bytes_per_word = 1;
+	else if (chip->bits_per_word <= 16)
+		controller->bytes_per_word = 2;
+	else
+		controller->bytes_per_word = 4;

quoted

This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.

SPI API didn't validate struct spi_transfer::len field.

It's supposed to; if the validation is incomplete then that should be
fixed.