On Fri, Aug 09, 2013 at 08:51:27PM +0200, John Crispin wrote:
Looks fairly good, a few things below but most of them are just using
the core to do things instead of open coding them in the driver rather
than anything substantial.
> +#ifdef DEBUG
> +#define spi_debug(args...) printk(args)
> +#else
> +#define spi_debug(args...)
> +#endif
This shouldn't be driver specific if it's useful, though really it looks
like the driver should just be using dev_dbg() and friends.
> +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 mask)
> +{
> + void __iomem *addr = rs->base + reg;
> + u32 val;
> +
> + val = ioread32(addr);
> + val |= mask;
> + iowrite32(val, addr);
> +}
Is this always called with a suitable lock held?
> + if (bits_per_word != 8) {
You should be setting bits_per_word_mask in the master structure, then
you don't need to check for this.
> +static inline int rt2880_spi_wait_till_ready(struct rt2880_spi *rs)
> +{
> + int i;
> +
> + for (i = 0; i < RALINK_SPI_WAIT_RDY_MAX_LOOP; i++) {
> + u32 status;
> +
> + status = rt2880_spi_read(rs, RAMIPS_SPI_STAT);
> + if ((status & SPISTAT_BUSY) == 0)
> + return 0;
> +
> + udelay(1);
> + }
A cpu_relax() here would be nice.
> +static unsigned int
> +rt2880_spi_write_read(struct spi_device *spi, struct spi_transfer *xfer)
> +{
> + struct rt2880_spi *rs = spidev_to_rt2880_spi(spi);
> + unsigned count = 0;
> + u8 *rx = xfer->rx_buf;
> + const u8 *tx = xfer->tx_buf;
> + int err;
> +
> + spi_debug("%s(%d): %s %s\n", __func__, xfer->len,
> + (tx != NULL) ? "tx" : " ",
> + (rx != NULL) ? "rx" : " ");
> +
> + if (tx) {
> + for (count = 0; count < xfer->len; count++) {
> + rt2880_spi_write(rs, RAMIPS_SPI_DATA, tx[count]);
> + rt2880_spi_setbits(rs, RAMIPS_SPI_CTL, SPICTL_STARTWR);
> + err = rt2880_spi_wait_till_ready(rs);
> + if (err) {
> + dev_err(&spi->dev, "TX failed, err=%d\n", err);
> + goto out;
> + }
> + }
> + }
> +
> + if (rx) {
There is presumably a maximum transfer size here from the FIFO that is
holding the data?
> + if (bits_per_word != 8) {
> + dev_err(&spi->dev,
> + "message rejected: invalid transfer bits_per_word (%d bits)\n",
> + bits_per_word);
Like I say set bits_per_word_mask...
> + if (t->speed_hz && t->speed_hz < (rs->sys_freq / 128)) {
> + dev_err(&spi->dev,
> + "message rejected: device min speed (%d Hz) exceeds required transfer speed (%d Hz)\n",
> + (rs->sys_freq / 128), t->speed_hz);
> + status = -EIO;
> + goto msg_done;
> + }
Set min_speed_hz in the spi_master and the core will check this for you.
> + if (spi->max_speed_hz < (rs->sys_freq / 128)) {
> + dev_err(&spi->dev, "setup: requested speed is too low %d Hz\n",
> + spi->max_speed_hz);
> + return -EINVAL;
> + }
> +
> + if (spi->bits_per_word != 0 && spi->bits_per_word != 8) {
> + dev_err(&spi->dev,
> + "setup: requested bits per words - os wrong %d bpw\n",
> + spi->bits_per_word);
> + return -EINVAL;
> + }
Again the core can do this for you.
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_request_and_ioremap(&pdev->dev, r);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
devm_ioremap_resource().
> + status = clk_enable(clk);
> + if (status)
> + return status;
clk_prepare_enable(), and it'd be nice to use runtime PM and enable the
clock only when doing transfers though that's not essential.
> +static int rt2880_spi_remove(struct platform_device *pdev)
> +{
> + struct spi_master *master;
> + struct rt2880_spi *rs;
> +
> + master = dev_get_drvdata(&pdev->dev);
> + rs = spi_master_get_devdata(master);
> +
> + clk_disable(rs->clk);
> + clk_put(rs->clk);
No clk_put if you've used devm_clk_get().
Attachment:
signature.asc
Description: Digital signature
