Re: [PATCH] nios2: Add Altera SPI master driver

On Sat, Jul 30, 2011 at 10:18:23PM +0200, franck.jullien@gmail.com wrote:

Please use dev_info and friends for driver output.

+	return 0;
+}
+
+
+static unsigned int altera_spi_xchg_single(struct altera_spi *altera_spi, unsigned int data)
+{
+	struct nios_spi *nios_spi = altera_spi->regs;
+
+	while (!(readl(&nios_spi->status) & NIOS_SPI_TRDY));
+	writel(data, &nios_spi->txdata);
+
+	while (!(readl(&nios_spi->status) & NIOS_SPI_RRDY));
+
+	return readl(&nios_spi->rxdata);
+}
+
+/*
+ * When using SPI_CS_HIGH devices, only one device is allowed to be
+ * connected to the Altera SPI master. This limitation is due to the
+ * emulation of an active high CS by writing 0 to the slaveselect register
+ * (this produce a '1' to all CS pins).
+ */
+static void altera_spi_chipselect(struct spi_device *spi, int is_active)
+{
+	struct altera_spi *altera_spi = container_of(spi->master, struct altera_spi, master);
+	struct nios_spi *nios_spi = altera_spi->regs;
+	uint32_t tmp;
+
+	if (spi->mode & SPI_CS_HIGH) {
+		switch (is_active) {
+		case CS_INACTIVE:
+			writel(1 << spi->chip_select, &nios_spi->slaveselect);
+			tmp = readl(&nios_spi->control);
+			writel(tmp | NIOS_SPI_SSO, &nios_spi->control);
+			break;
+
+		case CS_ACTIVE:
+			tmp = readw(&nios_spi->control);
+			writew(tmp & ~NIOS_SPI_SSO, &nios_spi->control);
+			writel(0, &nios_spi->slaveselect);
+			break;
+		}
+	} else {
+		switch (is_active) {
+		case CS_INACTIVE:
+			tmp = readw(&nios_spi->control);
+			writew(tmp & ~NIOS_SPI_SSO, &nios_spi->control);
+			break;
+
+		case CS_ACTIVE:
+			writel(1 << spi->chip_select, &nios_spi->slaveselect);
+			tmp = readl(&nios_spi->control);
+			writel(tmp | NIOS_SPI_SSO, &nios_spi->control);
+			break;
+		}
+	}
+}

Better to split this code into two functions
(altera_spi_cs_active/inactive)

+
+static int altera_spi_transfer(struct spi_device *spi, struct spi_message *mesg)
+{
+	struct altera_spi *altera_spi = container_of(spi->master, struct altera_spi, master);
+	struct nios_spi *nios_spi = altera_spi->regs;
+	struct spi_transfer	*t = NULL;
+	int word_len =  spi->bits_per_word;
+
+	u32 txval;
+	u32 rxval;
+
+	altera_spi_chipselect(spi, 1);
+
+	word_len = spi->bits_per_word;
+	mesg->actual_length = 0;
+
+	list_for_each_entry(t, &mesg->transfers, transfer_list) {
+
+		if (word_len <= 8) {
+			const u8 *txbuf = t->tx_buf;
+			u8 *rxbuf = t->rx_buf;
+			int i = 0;
+
+			while (i < t->len) {
+				txval = txbuf ? txbuf[i] : 0;
+				rxval = altera_spi_xchg_single(altera_spi, txval);
+				if (rxbuf)
+					rxbuf[i] = rxval;
+				i++;
+				mesg->actual_length++;
+			}
+		} else if (word_len <= 16) {
+			const u16 *txbuf = t->tx_buf;
+			u16 *rxbuf = t->rx_buf;
+			int i = 0;
+
+			while (i < t->len >> 1) {
+				txval = txbuf ? txbuf[i] : 0;
+				rxval = altera_spi_xchg_single(altera_spi, txval);
+				if (rxbuf)
+					rxbuf[i] = rxval;
+				i++;
+				mesg->actual_length += 2;
+			}
+		} else if (word_len <= 32) {
+			const u32 *txbuf = t->tx_buf;
+			u32 *rxbuf = t->rx_buf;
+			int i = 0;
+
+			while (i < t->len >> 2) {
+				txval = txbuf ? txbuf[i] : 0;
+				rxval = altera_spi_xchg_single(altera_spi, txval);
+				if (rxbuf)
+					rxbuf[i] = rxval;
+				i++;
+				mesg->actual_length += 4;
+			}

Can you please put the code inside this loop into a seperate function?
I can imagine that at some point we want to introduce a helper function
to the spi core which loops around the transfers of each message and does
the chipselect handling. Having this code in a seperate function now
makes it easier to convert to the helper later.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox