linux-kernel - Re: [PATCH 2/2] spi: add driver for STM32 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: <20170621151300.s74lbymyisvde2zo@sirena.org.uk>
Date:   Wed, 21 Jun 2017 16:13:00 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Amelie Delaunay <amelie.delaunay@...com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] spi: add driver for STM32 SPI controller

On Wed, Jun 21, 2017 at 04:32:06PM +0200, Amelie Delaunay wrote:

A few minor stylistic things but overall this looks really nice, please
send followup patches fixing these style things.

> +	/* Determine the first power of 2 greater than or equal to div */
> +	mbrdiv = (div & (div - 1)) ? fls(div) : fls(div) - 1;

Please write normal conditional statements, it makes things much easier
to read.

> +static bool stm32_spi_can_dma(struct spi_master *master,
> +			      struct spi_device *spi_dev,
> +			      struct spi_transfer *transfer)
> +{
> +	struct stm32_spi *spi = spi_master_get_devdata(master);
> +
> +	dev_dbg(spi->dev, "%s: %s\n", __func__,
> +		(!!(transfer->len > spi->fifo_size)) ? "true" : "false");
> +
> +	return !!(transfer->len > spi->fifo_size);

This !! is redundant, you're converting a boolean value into a boolean
value.

> +	buswidth = (spi->cur_bpw <= 8) ? DMA_SLAVE_BUSWIDTH_1_BYTE :
> +		   (spi->cur_bpw <= 16) ? DMA_SLAVE_BUSWIDTH_2_BYTES :
> +		   DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	/* Valid for DMA Half or Full Fifo threshold */
> +	maxburst = (spi->cur_fthlv == 2) ? 1 : spi->cur_fthlv;

Again, please use normal conditional statements - people have to read
things.

> +static int stm32_spi_suspend(struct device *dev)
> +{
> +	struct spi_master *master = dev_get_drvdata(dev);
> +	struct stm32_spi *spi = spi_master_get_devdata(master);
> +	int ret;
> +
> +	ret = spi_master_suspend(master);
> +	if (ret)
> +		return ret;
> +
> +	clk_disable_unprepare(spi->clk);

It'd be good to also have the clock disabled by runtime PM, that will
save a little more power.  There's support for enabling and disabling
the device in the core so it should just be adding callbacks.  Not
essential though.

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