Re: [RFC PATCH] spi: add driver for the SiFive SPI controller

[Mark Brown]

On Mon, Nov 12, 2018 at 03:27:36PM +0100, Emil Renner Berthing wrote:

> I know the discussions about the sifive devicetree compatible
> strings haven't come to a conclusion, so I'm sending this as
> an RFC to get some feedback on the rest of the code.

I've not seen any of these discussions or earlier versions of this
driver so I've no idea what's going on here :(

> +Optional properties:
> +- sifive,fifo-depth		: Depth of hardware queues; defaults to 8
> +- sifive,max-bits-per-word	: Maximum bits per word; defaults to 8
> +

If the hardware isn't fixed yet making these enumerable from the
hardware would be good...

> @@ -0,0 +1,442 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SiFive SPI controller driver (master mode only)
> + *

Please make the entire comment a C++ one to make this look more
intentinal.

> +/* for consistency we need this symbol */
> +#ifdef REG_FMT
> +#undef REG_FMT
> +#endif

We do?  For consistency with what?

> +static void sifive_spi_init(struct sifive_spi *spi)
> +{

> +	/* Set CS/SCK Delays and Inactive Time to defaults */
> +
> +	/* Exit specialized memory-mapped SPI flash mode */

...or not?

> +	/* Set frame format */
> +	cr = FMT_LEN(t->bits_per_word);
> +	switch (mode) {
> +	case SPI_NBITS_QUAD:
> +		cr |= FMT_PROTO_QUAD;
> +		break;

Some namespacing on the driver #defines would be a bit safer against the
possibility of collision with future changes in headers.

> +static void sifive_spi_wait(struct sifive_spi *spi, u32 bit, int poll)
> +{
> +	if (poll) {
> +		u32 cr;
> +		do cr = sifive_spi_read(spi, REG_IP);
> +		while (!(cr & bit));

Please add some braces, indentation or something to make it more clear
that the read is part of a do/while loop - right now it's not
immediately obvious that this is correct.

> +static int sifive_spi_transfer_one(struct spi_master *master,
> +		struct spi_device *device, struct spi_transfer *t)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(master);
> +	int poll = sifive_spi_prep_transfer(spi, device, t);
> +
> +	sifive_spi_execute(spi, t, poll);
> +

Why not just inline the execute function here?  It's the only caller
AFAICT.

> +static void sifive_spi_set_cs(struct spi_device *device, bool is_high)
> +{
> +	struct sifive_spi *spi = spi_master_get_devdata(device->master);
> +
> +	/* Reverse polarity is handled by SCMR/CPOL. Not inverted CS. */
> +	if (device->mode & SPI_CS_HIGH)
> +		is_high = !is_high;

spi_set_cs() will handle CS_HIGH for you.

> +	master->bits_per_word_mask = SPI_BPW_MASK(8);

I thought the device supported other bits per word values?

> +	/* If mmc_spi sees a dma_mask, it starts using dma mapped buffers.
> +	 * Probably it should rely on the SPI core auto mapping instead.
> +	 */
> +	pdev->dev.dma_mask = NULL;

If this is a problem please fix it in the MMC core, don't bodge it like
this.

> +static const struct of_device_id sifive_spi_of_match[] = {
> +	{ .compatible = "sifive,spi0", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sifive_spi_of_match);

spi0 is a *weird* compatible name.

Attachment: signature.asc
Description: PGP signature