Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support

[Mark Brown]

On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:

This looks mostly good, there's a few odd things and missing use of
framework features.

> Qualcomm Universal Peripheral (QUP) core is an AHB slave that
> provides a common data path (an output FIFO and an input FIFO)
> for serial peripheral interface (SPI) mini-core. SPI in master mode
> support up to 50MHz, up to four chip selects, and a programmable
> data path from 4 bits to 32 bits; MODE0..3 protocols

The grammar in this and the Kconfig text is a bit garbled, might want to
give it a once over (support -> supports for example).

> +static void spi_qup_deassert_cs(struct spi_qup *controller,
> +				struct spi_qup_device *chip)
> +{


> +	if (chip->mode & SPI_CS_HIGH)
> +		iocontol &= ~mask;
> +	else
> +		iocontol |= mask;

Implement a set_cs() operation and let the core worry about all this
for you as well as saving two implementations.

> +		word = 0;
> +		for (idx = 0; idx < controller->bytes_per_word &&
> +		     controller->tx_bytes < xfer->len; idx++,
> +		     controller->tx_bytes++) {
> +
> +			if (!tx_buf)
> +				continue;

Do you need to set the _MUST_TX flag?

> +	qup_err = readl_relaxed(controller->base + QUP_ERROR_FLAGS);
> +	spi_err = readl_relaxed(controller->base + SPI_ERROR_FLAGS);
> +	opflags = readl_relaxed(controller->base + QUP_OPERATIONAL);
> +
> +	writel_relaxed(qup_err, controller->base + QUP_ERROR_FLAGS);
> +	writel_relaxed(spi_err, controller->base + SPI_ERROR_FLAGS);
> +	writel_relaxed(opflags, controller->base + QUP_OPERATIONAL);
> +
> +	if (!xfer)
> +		return IRQ_HANDLED;

Are you sure?  It seems wrong to just ignore interrupts, some comments
would help explain why.

> +static int spi_qup_transfer_do(struct spi_qup *controller,
> +			       struct spi_qup_device *chip,
> +			       struct spi_transfer *xfer)

This looks like a transfer_one() function, please use the framework
features where you can.

> +	if (controller->speed_hz != chip->speed_hz) {
> +		ret = clk_set_rate(controller->cclk, chip->speed_hz);
> +		if (ret) {
> +			dev_err(controller->dev, "fail to set frequency %d",
> +				chip->speed_hz);
> +			return -EIO;
> +		}
> +	}

Is calling into the clock framework really so expensive that we need to
avoid doing it?  You also shouldn't be interacting with the hardware in
setup().

> +	if (chip->bits_per_word <= 8)
> +		controller->bytes_per_word = 1;
> +	else if (chip->bits_per_word <= 16)
> +		controller->bytes_per_word = 2;
> +	else
> +		controller->bytes_per_word = 4;

This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.

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

This entire function can be removed, the core can do it for you.

> +	if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> +		max_freq = 19200000;
> +
> +	if (!max_freq) {
> +		dev_err(dev, "invalid clock frequency %d\n", max_freq);
> +		return -ENXIO;
> +	}
> +
> +	ret = clk_set_rate(cclk, max_freq);
> +	if (ret)
> +		dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);

You set the clock rate per transfer so why bother setting it here,
perhaps we support the rate the devices request but not this maximum
rate?

> +	master->num_chipselect = SPI_NUM_CHIPSELECTS;
> +	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);

Are you *sure* the device supports anything other than whole bytes?

> +	ret = devm_spi_register_master(dev, master);
> +	if (!ret) {
> +		pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_active(dev);
> +		pm_runtime_enable(dev);
> +		return ret;
> +	}

This is really unclearly written, the success case looks like error
handling.

> +#ifdef CONFIG_PM_RUNTIME
> +static int spi_qup_pm_suspend_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	disable_irq(controller->irq);

Why do you need to disable the interrupt?  Will the hardware generate
spurious interrupts, if so some documentation is in order.

> +static int spi_qup_pm_resume_runtime(struct device *device)
> +{
> +	struct spi_master *master = dev_get_drvdata(device);
> +	struct spi_qup *controller = spi_master_get_devdata(master);
> +
> +	clk_prepare_enable(controller->cclk);
> +	clk_prepare_enable(controller->iclk);
> +	enable_irq(controller->irq);

No error checking here...

Attachment: signature.asc
Description: Digital signature