On 10/14/2013 05:18 PM, Igor Grinberg wrote:
diff --git a/drivers/spi/omap3_spi.c b/drivers/spi/omap3_spi.c index 6bac245..add98e1 100644 --- a/drivers/spi/omap3_spi.c +++ b/drivers/spi/omap3_spi.c @@ -20,7 +20,7 @@ #include <asm/io.h> #include "omap3_spi.h"
-#define WORD_LEN 8 +#define SPI_DEFAULT_WORDLEN 8;
As already pointed out, the semicolon should not be there...
Will fix
#define SPI_WAIT_TIMEOUT 3000000;
While at this, can you please, send a separate patch to fix the above semicolon?
Will do
static void spi_reset(struct omap3_spi_slave *ds) @@ -128,10 +128,32 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, ds->regs = regs; ds->freq = max_hz; ds->mode = mode;
- ds->wordlen = SPI_DEFAULT_WORDLEN;
While we know, OMAP SPI controller supports this, I think many other controllers can also support this feature and thus their drivers can benefit from this option.
Wouldn't it be better to add the wordlen field to the generic slave structure instead of the OMAP specific structure?
Makes sense. Will move wordlen to struct spi_slave.
return &ds->slave; }
+int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen) +{
- struct omap3_spi_slave *ds;
- int chconf;
- if (wordlen < 4 || wordlen > 32) {
printf("SPI error: unsupported wordlen %d\n", wordlen);return -1;- }
- ds = to_omap3_spi(slave);
- ds->wordlen = wordlen;
- chconf = readl(&ds->regs->channel[ds->slave.cs].chconf);
- chconf &= ~OMAP3_MCSPI_CHCONF_WL_MASK;
- chconf |= (wordlen - 1) << 7;
- omap3_spi_write_chconf(ds, chconf);
I would recommend to update the controller just before the transaction takes place, e.g. in spi_xfer() function, so the spi_set_wordlen() function will only update the slave structure and will not touch the hardware. Thus spi_set_wordlen() can be placed in the generic SPI code.
Yes that is better. Will do.
-int omap3_spi_write(struct spi_slave *slave, unsigned int len, const u8 *txp, +int omap3_spi_write(struct spi_slave *slave, unsigned int len, const void *txp, unsigned long flags) { struct omap3_spi_slave *ds = to_omap3_spi(slave); @@ -250,7 +272,13 @@ int omap3_spi_write(struct spi_slave *slave, unsigned int len, const u8 *txp, } } /* Write the data */
writel(txp[i], &ds->regs->channel[ds->slave.cs].tx);
unsigned int *tx = &ds->regs->channel[ds->slave.cs].tx;if (ds->wordlen > 16)writel(((u32 *)txp)[i], tx);else if (ds->wordlen > 8)writel(((u16 *)txp)[i], tx);elsewritel(((u8 *)txp)[i], tx);Can we always have u32 for the txp and use masks ans shifts instead of the ugly castings?
Working with masks and shifts will basically do the same thing that the casts do, but in a more complicated way. The best way to get rid of the casts will be to store SPI words in u32 arrays to begin with, regardless of the size of SPI word. Unfortunately, this is actually a difficult change:
If we change omap3_spi implementation to work with u32 arrays we also have to change cmd_spi.c, which stores data in byte arrays, and then we will be forced to change all the other U-Boot SPI implementations.
Due to the scope of this change I think it should be done in another patch series.