Re: [RFC PATCH 1/2] spi: Add support for Zynq QSPI controller

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jul 10, 2014 at 02:20:06PM +0530, Harini Katakam wrote:

> This patch adds support for QSPI controller used by Zynq.

The driver looks pretty clean but there are a couple of issues below,
including a little bit more of the flash specifics.

> +static void zynq_qspi_chipselect(struct spi_device *qspi, bool is_high)
> +{
> +	struct zynq_qspi *xqspi = spi_master_get_devdata(qspi->master);
> +	u32 config_reg;
> +
> +	config_reg = zynq_qspi_read(xqspi, ZYNQ_QSPI_CONFIG_OFFSET);
> +
> +	/* Select upper/lower page before asserting CS */
> +	if (xqspi->is_stacked) {
> +		u32 lqspi_cfg_reg;

Like with the dual and quad mode stuff this looks very much like it's
specific to flash rather than something that applies to a generic SPI
driver.  However it does look like it's a generic SPI device which could
be used in other applications which makes things a bit tricky.  We don't
have a really good answer for this right now unfortunately, probably we
need some sort of special interface between the SPI and flash subsystems
to allow flash to use the flash specific stuff.

For use as a generic SPI device what I'd suggest is stripping out the
flash specifics, merging the rest of the support and then considering
the flash specifics separately.

> +/**
> + * zynq_prepare_transfer_hardware - Prepares hardware for transfer.
> + * @master:	Pointer to the spi_master structure which provides
> + *		information about the controller.
> + *
> + * This function enables SPI master controller.
> + *
> + * Return:	Always 0
> + */
> +static int zynq_prepare_transfer_hardware(struct spi_master *master)
> +{
> +	struct zynq_qspi *xqspi = spi_master_get_devdata(master);
> +
> +	zynq_qspi_config_clock_mode(master->cur_msg->spi);

The clock mode needs to be (and is) configured per transfer so I'd
expect it's possible to remove this call.

> +	ret = clk_prepare_enable(xqspi->refclk);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable device clock.\n");

It's better to display the error code.

> +		clk_disable(xqspi->pclk);

This needs to be disable_unprepare().

> +static SIMPLE_DEV_PM_OPS(zynq_qspi_dev_pm_ops, zynq_qspi_suspend,
> +			 zynq_qspi_resume);

It would be better to also implement runtime PM support to disable the
clocks while the device is idle as well, that will save a small amount
of power while the device isn't doing anything.

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