[PATCH v5 2/8] spi: bcm-qspi: Add Broadcom MSPI driver

[Mark Brown]

On Fri, Jul 29, 2016 at 06:13:07PM -0400, Kamal Dasu wrote:

> +static int bcm_qspi_transfer_one(struct spi_master *master,

> +		if (qspi->next_udelay) {
> +			udelay(qspi->next_udelay);
> +			qspi->next_udelay = 0;
> +		}

Why is the driver manually implementing delays in a transfer_one()
function?  The core will add any required delays between transfers, this
will result in us delaying twice.

> +		read_from_hw(qspi, slots);
> +		if (qspi->cs_change) {
> +			usleep_range(10, 20);
> +			qspi->cs_change = 0;
> +		}

Similarly here, the core will do chip select changes.

> +	if (spi_transfer_is_last(master, trans))
> +		hw_stop(qspi);

The only thing I can see that looks like it might make a difference here
is this hw_stop() operation but all that does is update an internal
state machine so there's no limitation on it needing to be done before
the above operations that I can see.

> +	ret = clk_prepare_enable(qspi->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to prepare clock\n");
> +		goto err2;

Please use named labels rather than numbered, it makes life a lot easier
if anything gets changed.

> +EXPORT_SYMBOL_GPL(bcm_qspi_probe);

This needs some explanation or to be added along with the user.

> diff --git a/drivers/spi/spi-brcmstb-qspi.c b/drivers/spi/spi-brcmstb-qspi.c
> new file mode 100644
> index 0000000..c7df92e
> --- /dev/null
> +++ b/drivers/spi/spi-brcmstb-qspi.c

This should really be a separate patch, it's a separate driver and very
surprising to find it here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20160816/39d2af08/attachment-0001.sig>