Re: [PATCH v4 2/2] spi: add driver for J-Core SPI controller

[Mark Brown]

On Sun, Apr 03, 2016 at 05:12:45AM +0000, Rich Felker wrote:

> +config SPI_JCORE
> +	tristate "J-Core SPI Master"
> +	depends on OF
> +

An architecture or SoC dependency with || COMPILE_TEST would be useful
for avoiding cluttering Kconfig for other users.  Though as this is in a
FPGA it's perhaps likely people will pick this up for other FPGAs so
perhaps a comment to that effect if it seems likely.

> +static void jcore_spi_baudrate(struct jcore_spi *hw, int speed)
> +{
> +	if (speed == hw->speed_hz) return;
> +	hw->speed_hz = speed;
> +	if (speed >= hw->clock_freq/2)

Coding style, spaces around /.

> +	return count<len ? -EREMOTEIO : 0;

Again here, and please also write this as a normal if statement so it's
easier to read.

> +	/* The SPI clock rate controlled via a configurable clock divider
> +	 * which is applied to the reference clock. A 50 MHz reference is
> +	 * most suitable for obtaining standard SPI clock rates, but some
> +	 * designs may have a different reference clock, and the DT must
> +	 * make the driver aware so that it can properly program the
> +	 * requested rate. If omitted, 50 MHz is assumed. */
> +	clock_freq = 50000000;
> +	of_property_read_u32(node, "clock-frequency", &clock_freq);
> +	hw->clock_freq = clock_freq;

Why are you not using the clock API for this?  Just require a clock and
use clk_get_rate() to find out what rate it is.

> +	pdev->dev.dma_mask = 0;

Why are we doing this?  There's no DMA code...

> +	dev_info(&pdev->dev, "base %p, noirq\n", hw->base);

This is just adding noise to the boot, just remove it - it's useful to
log information we get from the silicon but things like the base address
don't really add anything and end up cluttering (and slowing) the boot
when everything does it.

Attachment: signature.asc
Description: PGP signature