Re: [PATCH] SPI: Add driver for Cadence SPI controller

[Mark Brown <broonie@kernel.org>]

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

On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:

> +	bits_per_word = transfer ?
> +			transfer->bits_per_word : spi->bits_per_word;

This would be a lot more legible without the ternery operator.

> +	if (bits_per_word != 8) {
> +		dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
> +			__func__, spi->bits_per_word);
> +		return -EINVAL;
> +	}

The core will check this for you.

> +static int cdns_spi_setup(struct spi_device *spi)
> +{
> +	if (!spi->max_speed_hz)
> +		return -EINVAL;
> +
> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;

The core does this for you.

> +	return cdns_spi_setup_transfer(spi, NULL);
> +}

It's not clear to me why this has been split into a separate function
and the function will write to the hardware which you're not allowed to
do in setup() if it might affect an ongoing transfer.

> +	intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET);
> +	cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status);
> +
> +	if (intr_status & CDNS_SPI_IXR_MODF_MASK) {
> +		/* Indicate that transfer is completed, the SPI subsystem will
> +		 * identify the error as the remaining bytes to be
> +		 * transferred is non-zero
> +		 */
> +		cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
> +			       CDNS_SPI_IXR_DEFAULT_MASK);
> +		complete(&xspi->done);
> +	} else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) {

What happens if both interrupts are flagged at the same time?

> +		if (xspi->remaining_bytes) {
> +			/* There is more data to send */
> +			cdns_spi_fill_tx_fifo(xspi);
> +		} else {
> +			/* Transfer is completed */
> +			cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
> +				       CDNS_SPI_IXR_DEFAULT_MASK);
> +			complete(&xspi->done);
> +		}
> +	}

I see from further up the file that there are error interrupt states
which might be flagged but these are just being ignored.

> +
> +	return IRQ_HANDLED;

This should only be returned if an interrupt was actually handled - if
the line is shared in some system this will break.

> +	cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET,
> +		       CDNS_SPI_IXR_DEFAULT_MASK);
> +
> +	ret = wait_for_completion_interruptible_timeout(&xspi->done,
> +							CDNS_SPI_TIMEOUT);
> +	if (ret < 1) {

If you return a positive value from transfer_one() the core will wait
for you.

> +static int cdns_prepare_transfer_hardware(struct spi_master *master)
> +{
> +	struct cdns_spi *xspi = spi_master_get_devdata(master);
> +
> +	if (xspi->driver_state != CDNS_SPI_DRIVER_STATE_READY)
> +		return -EINVAL;
> +
> +	cdns_spi_write(xspi->regs + CDNS_SPI_ER_OFFSET,
> +		       CDNS_SPI_ER_ENABLE_MASK);
> +
> +	return 0;
> +}

You probably want to enable the clocks here (and disable them when
unpreparing too).

> +static int cdns_transfer_one_message(struct spi_master *master,
> +				     struct spi_message *msg)
> +{

Just implement transfer_one() and provide a set_cs() operation and most
of this code will go away.

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "irq number is negative\n");
> +		goto remove_master;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq,
> +			       0, pdev->name, xspi);
> +	if (ret != 0) {
> +		ret = -ENXIO;
> +		dev_err(&pdev->dev, "request_irq failed\n");
> +		goto remove_master;
> +	}

I'd expect to see something that initialises the hardware prior to
requesting the interrupt, you don't know what state the hardware is in
when the driver takes control.

> +	init_completion(&xspi->done);

This needs to be done prior to the interrupt as well.

> +	ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select",
> +				   &master->num_chipselect);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "couldn't determine num-chip-select\n");
> +		goto clk_dis_all;
> +	}

Is there no reasonable default?

> +	/* Set to default valid value */
> +	xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4;

The driver should set max_speed_hz as well.

> +	dev_info(&pdev->dev, "at 0x%08X mapped to 0x%08X, irq=%d\n",
> +		 res->start, (u32 __force)xspi->regs, irq);

No need for this, it's just noisy.

> +static int __maybe_unused cdns_spi_suspend(struct device *dev)
> +{

This needs to call spi_master_suspend() as well (and similarly on
resume).

> +	cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
> +		       CDNS_SPI_IXR_DEFAULT_MASK);
> +	complete(&xspi->done);
> +
> +	ctrl_reg = cdns_spi_read(xspi->regs + CDNS_SPI_CR_OFFSET);
> +	ctrl_reg |= CDNS_SPI_CR_SSCTRL_MASK;
> +	cdns_spi_write(xspi->regs + CDNS_SPI_CR_OFFSET, ctrl_reg);

Don't do this, the spi_master_suspend() call will quiesce transfers (or
if open coding it should be doing something similar rather than just
breaking any ongoing transfer).

> +	clk_disable(xspi->ref_clk);
> +	clk_disable(xspi->pclk);

Why not unprepare?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]