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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160728191153.GL11806@sirena.org.uk>
Date:	Thu, 28 Jul 2016 20:11:53 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Rich Felker <dalias@...c.org>
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-spi@...r.kernel.org, linux-sh@...r.kernel.org,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v4 2/2] spi: add driver for J-Core SPI controller

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.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)