On Thu, Feb 13, 2014 at 06:21:38PM +0200, Ivan T. Ivanov wrote:
> 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 supports up to 50MHz, up to four chip selects, programmable
> data path from 4 bits to 32 bits and numerous protocol variants.
I've applied this since it mostly looks good but there's a few small
outstanding issues, please submit incremental patches fixing them.
> +#define SPI_CONFIG 0x0300
> +#define SPI_IO_CONTROL 0x0304
> +#define SPI_ERROR_FLAGS 0x0308
> +#define SPI_ERROR_FLAGS_EN 0x030c
You've got lots of defines that are just plain SPI_ which is looking for
namespace collisions - please prefix them.
> + if (spi->chip_select >= spi->master->num_chipselect) {
> + dev_err(controller->dev, "invalid chip_select %d\n",
> + spi->chip_select);
> + return -EINVAL;
> + }
The core does this prior to allowing the slave to be registered at all.
> +
> + if (spi->max_speed_hz > controller->max_speed_hz) {
> + dev_err(controller->dev, "invalid max_speed_hz %d\n",
> + spi->max_speed_hz);
> + return -EINVAL;
> + }
The core will check this for you if you set master->max_speed_hz.
> +#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);
> + u32 config;
> +
> + /* Enable clocks auto gaiting */
> + config = readl(controller->base + QUP_CONFIG);
> + config |= QUP_CLOCK_AUTO_GATE;
> + writel_relaxed(config, controller->base + QUP_CONFIG);
> + return 0;
> +}
Why not just enable this all the time? I'd have expected some clock API
calls here similar to those in the main suspend and resume paths.
> +MODULE_VERSION("0.4");
Don't bother doing this, nobody will ever keep updating the version
number - the kernel version should be enough.
Attachment:
signature.asc
Description: Digital signature
