Message-ID: <1392049766.2853.43.camel@iivanov-dev>
Date:	Mon, 10 Feb 2014 18:29:26 +0200
From:	"Ivan T. Ivanov" <iivanov@...sol.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, linux-spi@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, Alok Chauhan <alokc@...eaurora.org>,
	Gilad Avidov <gavidov@...eaurora.org>,
	Kiran Gunda <kgunda@...eaurora.org>,
	Sagar Dharia <sdharia@...eaurora.org>
Subject: Re: [PATCH 2/2] spi: Add Qualcomm QUP SPI controller support


Hi,

On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote: 
> 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).

Ok

> 
> > +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.

Nice. Will us it.

> 
> > +		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.

Of course this should never happen. This was left from initial stage
of the implementation.

> 
> > +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.

Sure, will do. Somehow I have missed this.

> 
> > +	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?  

Probably not. But why to call it if the frequency is the same.


> You also shouldn't be interacting with the hardware in
> setup().

This is internal hw-setup, not master::setup() or I am
missing something?

> 
> > +	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.

SPI API didn't validate struct spi_transfer::len field.

The whole sniped looks like this:

	chip->bits_per_word = spi->bits_per_word;
	if (xfer->bits_per_word)
		chip->bits_per_word = xfer->bits_per_word;

	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;

	if (controller->bytes_per_word > xfer->len ||
	    xfer->len % controller->bytes_per_word != 0){
		/* No partial transfers */
		dev_err(controller->dev, "invalid len %d for %d bits\n",
			xfer->len, chip->bits_per_word);
		return -EIO;
	}

	n_words = xfer->len / controller->bytes_per_word;

'bytes_per_word' have to be power of 2. This is my understanding of
struct spi_transfer description. So I am discarding all transfers with
'len' non multiple of word size.


> 
> > +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.

Sure, will use it.

> 
> > +	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,

Only if differs from the current one.

> perhaps we support the rate the devices request but not this maximum
> rate?

Thats why it is just a warning. I will see how to handle this better.

> 
> > +	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?

Yep. I see them on the scope.

> 
> > +	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.

I suppose that if use a goto, I will have to do it consistently. 
Will rework it.

> 
> > +#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.

I don't know. Just extra protection? I will have t o find how to 
test this.

> 
> > +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...

Will fix.

Thanks,
Ivan


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists