Messages in this thread |  | | Date | Thu, 20 Feb 2014 00:28:49 +0900 | From | Mark Brown <> | Subject | Re: [PATH v2 2/2] spi: Add Qualcomm QUP SPI controller support |
| |
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. [unhandled content-type:application/pgp-signature] |  |