Re: [PATCH v5 3/8] spi: bcm-qspi: Add BSPI spi-nor flash controller driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

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

> +	bcm_qspi_bspi_lr_start(qspi);
> +	/* Must flush previous writes before starting BSPI operation */
> +	mb();
> +	if (!wait_for_completion_timeout(&qspi->bspi_done, timeo)) {

The comment suggests that the memory barrier is misplaced or that
there's something missing?

> +		if (retry--)
> +			goto retry;

Please write loops using normal C looping constructs rather than goto
statements, it makes life a lot easier.  I'm not convinced that the loop
is safe as is with the completion reinitializaiton and so on, we should
at least be turning off the hardware before we reinitialize the
completion to avoid races with slow hardware.

> +	io_width = msg->data_nbits ? msg->data_nbits : SPI_NBITS_SINGLE;

Normal if statements please.

> +static irqreturn_t bcm_qspi_bspi_lr_err_l2_isr(int irq, void *dev_id)
> +{
> +	struct bcm_qspi_dev_id *qspi_dev_id = dev_id;
> +	struct bcm_qspi *qspi = qspi_dev_id->dev;
> +
> +	dev_dbg(&qspi->pdev->dev, "BSPI INT error status %x\n",
> +		qspi_dev_id->irqp->mask);

I'd expect this to be dev_err() or something, but then the message
claims this an error status and it doesn't seem to actually read that
from the hardware.

> +#ifdef  QSPI_INT_DEBUG
> +	/* this interrupt is for debug purposes only, dont request irq */
> +	{
> +		.irq_name = "spi_lr_overread",
> +		.irq_handler = bcm_qspi_bspi_lr_err_l2_isr,
> +		.mask = INTR_BSPI_LR_OVERREAD_MASK,
> +	},
> +#endif

Why not?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]