On Thu, May 07, 2015 at 07:13:11AM +0000, Barry Song wrote: This all looks mostly fine, there are a few comments below but they're all coding style things rather than anything else so should be easy enough to address. > +#define SIRFSOC_SPI_FIFO_LEVEL_MASK(s) ((s->spi_type == SIRF_REAL_SPI) ? \ > + 0xFF : ((s->is_atlas7_usp == 1) ? \ > + 0x1FF : 0x7F)) This pattern isn't very legible and isn't going to scale if there's more types added (eg, some new variant with a bigger FIFO). Can you make these static inline functions with switch and if statements instead? It looks like they should all fit into that pattern. It'd probably help make the patch easier to read if the changes to use these functions were added as separate patches - add the functions and convert everything to use them, then in a separate patch add the options for the new variants. > + .spi_dummy_delay_ctrl = 0x144, > +}; > +struct sirf_spi_register sirf_usp_spi = { Can we have blank lines between the structs please? Also the structs should be declared as static const so they're not in the global namespace. > + if (sspi->spi_type == SIRF_REAL_SPI) { > + u32 regval = readl(sspi->base + spi_reg->spi_ctrl); > + > + writel(regval, sspi->base + spi_reg->spi_ctrl); ... > + } else { > + u32 regval = readl(sspi->base + > + spi_reg->usp_pin_io_data); Please write this as a switch statement so it's easier to handle an new variants. Similarly for any other things that handle variants with if statements.
Attachment:
signature.asc
Description: Digital signature