Re: [PATCH 2/2] SPI: ralink: add Ralink SoC spi driver

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().