Re: [PATCH 2/2] spi: add driver for STM32 SPI controller

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature