Hi Yuhang, > >> > >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > >> index 004b10f..3f6dc93 100644 > >> --- a/drivers/spi/spi.c > >> +++ b/drivers/spi/spi.c > >> @@ -868,6 +868,14 @@ static void of_register_spi_devices(struct > spi_master > >> *master) > >> spi->mode |= SPI_CS_HIGH; > >> if (of_find_property(nc, "spi-3wire", NULL)) > >> spi->mode |= SPI_3WIRE; > >> + if (of_find_property(nc, "spi-tx-dual", NULL)) > >> + spi->mode |= SPI_TX_DUAL; > >> + if (of_find_property(nc, "spi-tx-quad", NULL)) > >> + spi->mode |= SPI_TX_QUAD; > >> + if (of_find_property(nc, "spi-rx-dual", NULL)) > >> + spi->mode |= SPI_RX_DUAL; > >> + if (of_find_property(nc, "spi-rx-quad", NULL)) > >> + spi->mode |= SPI_RX_QUAD; > >> > > [Pekon] I think its cleaner to use similar parameters for channel-width > > info everywhere. Like you have added separate fields 'tx_nbits' and > > 'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ? > > And also in 'spi_device'. This can be like below: > > > > - SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI) > > - SPI_4WIRE : tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI) > > - DUAL_SPI : tx_nbit = 2, rx_nbit = 2 > > - QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4 > > This way 'mode' field can be explicitly left for other purposes like CPHA, > CPOL. > > OR custom modes (like modified SPI mode in Freescale controllers). > > > > But if you are touching any existing configuration (like SPI_3WIRE as > > proposed above), Then please send it as independent patch/proposal, > > which can be Acked by individual SPI driver maintainers. > > > Adding the similar members like 'tx_nbits' and 'rx_nbits' is my > initial thought. But in different struct, that members meaning may be > a little different. > In spi_master, the member is what transfer mode master can support. > In spi_device, the member is what mode the slave will use. > In spi_transfer, the member is the transfer wires needed. > 1: adding the similar members everywhere may looks a little redundant. > What is more, in spi_master and spi_device, it is more like a mode and > in spi_device it is a transfer function due to the mode. > example: > DUAL mode in spi_device, in spi_transfer tx_nbits or rx_nbits can be > set to 1 or 2. > 2: the member in spi_master has to describe all the transfer > mode(single/dual/quad), because the member means the mode that master > support, so the member has to be seperated into several bits to cover > all the single dual and quad and then check it to spi_device. This > also seems similar to what the existed spi_master->mode_bit and > spi_device->mode do. > so added in mode may looks better. > [Pekon]: Just review following scenarios .. _Case-1_: I have a controller which supports - Quad & Dual modes only for reads, - Only Single SPI for writes. This means though bit (spi_master.mode & QUAD_MODE) is set, but still if MTD layer does a SPI WRITE transfer using tx_nbits=4, I should get error..
_Case-2_: Some flash devices also have similar constrains. Example: check Spansion flash S25FL128S datasheet, "section 10.1 command summary". This device supports - Single/Dual/Quad READS, - but only SINGLE & QUAD page-programs (WRITES). So, if you have only one field ('mode') for storing capability of device or master, you cannot resolve above scenarios. This is where I realized that we need to have separate tx_nbits and rx_nbits in spi_device and spi_master also. (This is contrary to my own earliest posts where I proposed reusing mode bits for keeping channel-width info). > > [Pekon]: I think it is correct to assume all these SPI modes as mutually > > exclusive from of_property() point of view? > > Instead of defining separate of_properties(), you can define a single > > of_property() , something like 'max-tx-nbits' and 'max-rx-nbits' > > which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE). > > > Yes, seems better. I will correct it. > > >> /* Device speed */ > >> prop = of_get_property(nc, "spi-max-frequency", &len); > >> @@ -1316,6 +1324,12 @@ int spi_setup(struct spi_device *spi) > >> /* help drivers fail *cleanly* when they need options > >> * that aren't supported with their current master > >> */ > >> + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) > >> || > >> + ((spi->mode & SPI_RX_DUAL) && (spi->mode & > >> SPI_RX_QUAD))) { > >> + dev_err(&spi->dev, > >> + "setup: can not select dual and quad at the same time\n"); > >> + return -EINVAL; > >> + } > > [Pekon]: this check would be not be required if you have single > > of_property() for all modes. > > > > > >> bad_bits = spi->mode & ~spi->master->mode_bits; > >> if (bad_bits) { > >> dev_err(&spi->dev, "setup: unsupported mode bits %x\n", > >> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c > >> index 2e0655d..0b776de 100644 > >> --- a/drivers/spi/spidev.c > >> +++ b/drivers/spi/spidev.c > >> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS); > >> | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \ > >> | SPI_NO_CS | SPI_READY) > >> > >> +#define SPI_EXTMODE_MASK (SPI_MODE_MASK | SPI_TX_DUAL \ > >> + | SPI_TX_QUAD | SPI_RX_DUAL | > >> SPI_RX_QUAD) > >> + > >> struct spidev_data { > >> dev_t devt; > >> spinlock_t spi_lock; > >> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data > *spidev, > >> k_tmp->bits_per_word = u_tmp->bits_per_word; > >> k_tmp->delay_usecs = u_tmp->delay_usecs; > >> k_tmp->speed_hz = u_tmp->speed_hz; > >> + k_tmp->tx_nbits = u_tmp->tx_nbits; > >> + k_tmp->rx_nbits = u_tmp->rx_nbits; > >> #ifdef VERBOSE > >> dev_dbg(&spidev->spi->dev, > >> " xfer len %zd %s%s%s%dbits %u usec %uHz\n", > >> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, > >> unsigned long arg) > >> case SPI_IOC_RD_MAX_SPEED_HZ: > >> retval = __put_user(spi->max_speed_hz, (__u32 __user > >> *)arg); > >> break; > >> - > >> + case SPI_IOC_EXTRD_MODE: > >> + retval = __put_user(spi->mode & SPI_EXTMODE_MASK, > >> + (__u16 __user *)arg); > >> + break; > >> /* write requests */ > >> case SPI_IOC_WR_MODE: > >> retval = __get_user(tmp, (u8 __user *)arg); > >> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd, > >> unsigned long arg) > >> dev_dbg(&spi->dev, "%d Hz (max)\n", tmp); > >> } > >> break; > >> + case SPI_IOC_EXTWR_MODE: > >> + retval = __get_user(tmp, (u16 __user *)arg); > >> + if (retval == 0) { > >> + u16 save = spi->mode; > >> + > >> + if (tmp & ~SPI_EXTMODE_MASK) { > >> + retval = -EINVAL; > >> + break; > >> + } > >> + > >> + tmp |= spi->mode & ~SPI_EXTMODE_MASK; > >> + spi->mode = (u16)tmp; > >> + retval = spi_setup(spi); > >> + if (retval < 0) > >> + spi->mode = save; > >> + else > >> + dev_dbg(&spi->dev, "spi mode %02x\n", > >> tmp); > >> + } > >> + break; > >> > >> default: > >> /* segmented and/or full-duplex I/O request */ > >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > >> index 38c2b92..222e49e 100644 > >> --- a/include/linux/spi/spi.h > >> +++ b/include/linux/spi/spi.h > >> @@ -74,7 +74,7 @@ struct spi_device { > >> struct spi_master *master; > >> u32 max_speed_hz; > >> u8 chip_select; > >> - u8 mode; > >> + u16 mode; > >> #define SPI_CPHA 0x01 /* clock phase */ > >> #define SPI_CPOL 0x02 /* clock polarity */ > >> #define SPI_MODE_0 (0|0) /* (original > >> MicroWire) */ > >> @@ -87,6 +87,10 @@ struct spi_device { > >> #define SPI_LOOP 0x20 /* loopback mode */ > >> #define SPI_NO_CS 0x40 /* 1 dev/bus, no > >> chipselect */ > >> #define SPI_READY 0x80 /* slave pulls low to > >> pause */ > >> +#define SPI_TX_DUAL 0x100 /* transmit with 2 > >> wires */ > >> +#define SPI_TX_QUAD 0x200 /* transmit with 4 > >> wires */ > >> +#define SPI_RX_DUAL 0x400 /* receive with 2 > >> wires */ > >> +#define SPI_RX_QUAD 0x800 /* receive with 4 > >> wires */ > >> u8 bits_per_word; > >> int irq; > >> void *controller_state; > >> @@ -437,6 +441,8 @@ extern struct spi_master > >> *spi_busnum_to_master(u16 busnum); > >> * @rx_buf: data to be read (dma-safe memory), or NULL > >> * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped > >> * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped > >> + * @tx_nbits: number of bits used for writting > >> + * @rx_nbits: number of bits used for reading > >> * @len: size of rx and tx buffers (in bytes) > >> * @speed_hz: Select a speed other than the device default for this > >> * transfer. If 0 the default (from @spi_device) is used. > >> @@ -491,6 +497,11 @@ extern struct spi_master > >> *spi_busnum_to_master(u16 busnum); > >> * by the results of previous messages and where the whole transaction > >> * ends when the chipselect goes intactive. > >> * > >> + * When SPI can transfer in 1x,2x or 4x. It can get this tranfer > >> information > >> + * from device through @tx_nbits and @rx_nbits. In Bi-direction, these > >> + * two should both be set. User can set transfer mode with > >> SPI_NBITS_SINGLE(1x) > >> + * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these > three > >> transfer. > >> + * > >> * The code that submits an spi_message (and its spi_transfers) > >> * to the lower layers is responsible for managing its memory. > >> * Zero-initialize every field you don't set up explicitly, to > >> @@ -511,6 +522,11 @@ struct spi_transfer { > >> dma_addr_t rx_dma; > >> > >> unsigned cs_change:1; > >> + u8 tx_nbits; > >> + u8 rx_nbits; > >> +#define SPI_NBITS_SINGLE 0x0; /* 1bit transfer */ > >> +#define SPI_NBITS_DUAL 0x01; /* 2bits transfer */ > >> +#define SPI_NBITS_QUAD 0x02; /* 4bits transfer */ > > > > [Pekon]: I don't think it's a good place for #defines. Plz keep it out of > > struct > body. > > > >> u8 bits_per_word; > >> u16 delay_usecs; > >> u32 speed_hz; > >> @@ -858,7 +874,7 @@ struct spi_board_info { > >> /* mode becomes spi_device.mode, and is essential for chips > >> * where the default of SPI_CS_HIGH = 0 is wrong. > >> */ > >> - u8 mode; > >> + u16 mode; > >> > >> /* ... may need additional spi_device chip config data here. > >> * avoid stuff protocol drivers can set; but include stuff > > > > with regards, pekon ------------------------------------------------------------------------------ See everything from the browser to the database with AppDynamics Get end-to-end visibility with application monitoring from AppDynamics Isolate bottlenecks and diagnose root cause in seconds. Start your free trial of AppDynamics Pro today! http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk _______________________________________________ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general