[spi-devel-general] [PATCH] Simple driver for Xilinx SPI controler.

[David Brownell]

On Thursday 07 June 2007, Andrei Konovalov wrote:

> >> +/* Simple macros to get the code more readable */
> >> +#define xspi_in16(addr)		in_be16((u16 __iomem *)(addr))
> >> +#define xspi_in32(addr)		in_be32((u32 __iomem *)(addr))
> >> +#define xspi_out16(addr, value)	out_be16((u16 __iomem *)(addr), (value))
> >> +#define xspi_out32(addr, value)	out_be32((u32 __iomem *)(addr), (value))
> > 
> > I'm rather used to seeing I/O addressses passed around as "void __iomem *"
> > so those sorts of cast are not needed...  :)
> 
> I've decided to make the base_address (u8 __iomem *) to make it easier to
> add the register offsets to the base. Like that:

Adding register offsets works with "void __iomem *" too ...


> > You should not need an abort primitive.  ...
> 
> I am paranoid enough not to rely 100% on the spi_device's and the spi_bitbang
> doing all that :)
> Agreed, xspi_abort_transfer is bad name here. But I still would like to
> leave these two writes:
> 
> +	/* Deselect the slave on the SPI bus */
> +	xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
> +	/* Disable the transmitter */
> +	xspi_out16(regs_base + XSPI_CR_OFFSET,
> +		   XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT);
> 
> in xilinx_spi_remove(). Just in case :)

What happens when you add a BUG_ON to catch the device still being active?

Really, you need to rely on the rest of the system working correctly.



> > I take it you can't support SPI_CS_HIGH??
> 
> There is no clear indication in the SPI controller docs that SPI_CS_HIGH
> would work. I suspect it would as we don't use the ability of the controller
> to generate the chip selects automatically (the auto mode doesn't support SPI_CS_HIGH).
> But it is more safe to advertise SPI_CS_HIGH is not supported.

If you don't explicitly support it, it's unlikely to work.  :)

- Dave