Hmmm. It seems like this patch got dropped on the floor without any comments. Here's my review....
On Tue, Jun 2, 2009 at 1:12 PM, Richard Ršöjfors <[email protected]> wrote: > Here is a patch where the xilinx_spi is splitted into three parts, an OF and > a platform driver and generic part. > > The generic part now also works on X86 and support for the Xilinx SPI IP DS570 > > Signed-off-by: Richard Röjfors <[email protected]> In general I'm not opposed to the change, but I've got concerns about the form and some of the details. First, it is an invasive patch, primarily due to the addition of accesor macros. I'd like to see the creation of the accessors split into a separate patch from the platform/of_platform bus binding changes so that review is easier. More comments below. > --- > Index: linux-2.6.30-rc7/drivers/spi/Kconfig > =================================================================== > --- linux-2.6.30-rc7/drivers/spi/Kconfig (revision 861) > +++ linux-2.6.30-rc7/drivers/spi/Kconfig (revision 869) > @@ -211,8 +211,8 @@ > SPI driver for Toshiba TXx9 MIPS SoCs > > config SPI_XILINX > - tristate "Xilinx SPI controller" > - depends on XILINX_VIRTEX && EXPERIMENTAL > + tristate "Xilinx SPI controller common module" > + depends on EXPERIMENTAL Drop the change to the description (see below) > select SPI_BITBANG > help > This exposes the SPI controller IP from the Xilinx EDK. > @@ -220,6 +220,25 @@ > See the "OPB Serial Peripheral Interface (SPI) (v1.00e)" > Product Specification document (DS464) for hardware details. > > +config SPI_XILINX_OF > + tristate "Xilinx SPI controller OF device" > + depends on SPI_XILINX && XILINX_VIRTEX > + help > + This exposes the SPI controller IP from the Xilinx EDK. > + > + See the "OPB Serial Peripheral Interface (SPI) (v1.00e)" > + Product Specification document (DS464) for hardware details. > + > +config SPI_XILINX_PLTFM > + tristate "Xilinx SPI controller platform device" > + depends on SPI_XILINX > + help > + This exposes the SPI controller IP from the Xilinx EDK. > + > + See the "OPB Serial Peripheral Interface (SPI) (v1.00e)" > + Product Specification document (DS464) for hardware details. Personally, I'd rather these 2 options not be user-visable and instead auto-enable the OF binding when the of_bus is enabled, and auto-enable the platform bus binding when platform bus is enabled. > Index: linux-2.6.30-rc7/drivers/spi/xilinx_spi.c > =================================================================== > --- linux-2.6.30-rc7/drivers/spi/xilinx_spi.c (revision 861) > +++ linux-2.6.30-rc7/drivers/spi/xilinx_spi.c (revision 869) > @@ -14,22 +14,79 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/interrupt.h> > -#include <linux/platform_device.h> > > -#include <linux/of_platform.h> > -#include <linux/of_device.h> > -#include <linux/of_spi.h> > - > #include <linux/spi/spi.h> > #include <linux/spi/spi_bitbang.h> > #include <linux/io.h> > > -#define XILINX_SPI_NAME "xilinx_spi" > +#include "xilinx_spi.h" > > +struct xilinx_spi { > + /* bitbang has to be first */ > + struct spi_bitbang bitbang; > + struct completion done; > + struct resource mem; /* phys mem */ > + void __iomem *regs; /* virt. address of the control registers */ > + u32 irq; > + u8 *rx_ptr; /* pointer in the Tx buffer */ > + const u8 *tx_ptr; /* pointer in the Rx buffer */ > + int remaining_bytes; /* the number of bytes left to transfer */ > + /* offset to the XSPI regs, these might vary... */ > + u8 cr_offset; > + u8 sr_offset; > + u8 txd_offset; > + u8 rxd_offset; > + u8 ssr_offset; > + u8 bits_per_word; > + u8 model; > +}; Why did the structure definition get moved? > +#ifndef CONFIG_PPC > +#define in_8(addr) ioread8(addr) > +#define in_be16(addr) ioread16(addr) > +#define in_be32(addr) ioread32(addr) > + > +#define out_8(addr, b) iowrite8(b, addr) > +#define out_be16(addr, w) iowrite16(w, addr) > +#define out_be32(addr, l) iowrite32(l, addr) > +#endif Oh, ugly. If in_*/out_* are no longer appropriate, then don't define them to the little endian variants. Remove the calls to in_/out_* and use the ioread/iowrite calls instead. In fact, even for powerpc the driver could probably be modified to use io{read,write}{8,16be,32be). > + > +#define XSPI_WRITE8(xspi, offs, val) \ > + do { \ > + if (xspi->model == XILINX_SPI_MODEL_DS464) \ > + out_8(xspi->regs + offs, (val) & 0xff); \ > + else \ > + XSPI_WRITE32(xspi, offs, val); \ > + } while (0) > + > +#define XSPI_WRITE16(xspi, offs, val) \ > + do { \ > + if (xspi->model == XILINX_SPI_MODEL_DS464) \ > + out_be16(xspi->regs + offs, (val) & 0xffff); \ > + else \ > + XSPI_WRITE32(xspi, offs, val); \ > + } while (0) > + > +#define XSPI_WRITE32(xspi, offs, val) \ > + out_be32(xspi->regs + offs, val) > + > +#define XSPI_READ8(xspi, offs) \ > + (xspi->model == XILINX_SPI_MODEL_DS464) ? \ > + in_8(xspi->regs + offs) : XSPI_READ32(xspi, offs) > + > +#define XSPI_READ16(xspi, offs) \ > + (xspi->model == XILINX_SPI_MODEL_DS464) ? \ > + in_be16(xspi->regs + offs) : XSPI_READ32(xspi, offs) > + > +#define XSPI_READ32(xspi, offs) \ > + in_be32(xspi->regs + offs) inlines please. Don't use #define macros. In fact, I'd consider setting up an IO access ops table which would allow the correct accessors to be set up at probe time and would eliminate all the xspi->model checks from above. > + > + > /* Register definitions as per "OPB Serial Peripheral Interface (SPI) > (v1.00e) > * Product Specification", DS464 > */ > -#define XSPI_CR_OFFSET 0x62 /* 16-bit Control Register */ > +#define XSPI_CR_OFFSET_DS464 0x62 /* 16-bit Control Register */ > +#define XSPI_CR_OFFSET_DS570 0x60 > > #define XSPI_CR_ENABLE 0x02 > #define XSPI_CR_MASTER_MODE 0x04 > @@ -40,8 +97,10 @@ > #define XSPI_CR_RXFIFO_RESET 0x40 > #define XSPI_CR_MANUAL_SSELECT 0x80 > #define XSPI_CR_TRANS_INHIBIT 0x100 > +#define XSPI_CR_LSB_FIRST 0x200 > > -#define XSPI_SR_OFFSET 0x67 /* 8-bit Status Register */ > +#define XSPI_SR_OFFSET_DS464 0x67 /* 8-bit Status Register */ > +#define XSPI_SR_OFFSET_DS570 0x64 > > #define XSPI_SR_RX_EMPTY_MASK 0x01 /* Receive FIFO is empty */ > #define XSPI_SR_RX_FULL_MASK 0x02 /* Receive FIFO is full */ > @@ -49,10 +108,13 @@ > #define XSPI_SR_TX_FULL_MASK 0x08 /* Transmit FIFO is full */ > #define XSPI_SR_MODE_FAULT_MASK 0x10 /* Mode fault error */ > > -#define XSPI_TXD_OFFSET 0x6b /* 8-bit Data Transmit > Register */ > -#define XSPI_RXD_OFFSET 0x6f /* 8-bit Data Receive > Register */ > +#define XSPI_TXD_OFFSET_DS464 0x6b /* 8-bit Data Transmit Register */ > +#define XSPI_TXD_OFFSET_DS570 0x68 > +#define XSPI_RXD_OFFSET_DS464 0x6f /* 8-bit Data Receive Register */ > +#define XSPI_RXD_OFFSET_DS570 0x6C > > -#define XSPI_SSR_OFFSET 0x70 /* 32-bit Slave Select > Register */ > +#define XSPI_SSR_OFFSET_DS464 0x70 /* 32-bit Slave Select Register */ > +#define XSPI_SSR_OFFSET_DS570 0x70 It looks like you need to access the registers on 32 bit boundaries, and the current ppc code happens to do a few 16 and 8 bit acccesses. I haven't looked at the data sheet, but I wouldn't be surprised if the 8 and 16 bit registers can also be cleanly access with 32 bit transfers. In which case if you changed all access to be 32 bit aligned, then your patch could be a good deal simpler, and you wouldn't need to mess about with all the DS464/DS570 stuff. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july _______________________________________________ spi-devel-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/spi-devel-general
