Message ID | 1375095194-7093-1-git-send-email-wangyuhang2014@gmail.com |
---|---|
State | New, archived |
Headers | show |
Hello, > > Hi, > > modify two things. Hope you are tracking the Patch versions ? Good to include it in mail-subject. > 1: > >> @@ -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 >> 8) & 0x03) == 0x03 || > >> + ((spi->mode >> 10) & 0x03) == 0x03) { > >> + dev_err(&spi->dev, > >> + "setup: can not select dual and quad at the same time\n") > >> + return -EINVAL; > >> + } > >> + return -EINVAL; > >> + } > > >This code won't work if the constants you added for mode flags change > >value. More importantly, anyone searching the code for SPI_TX_DUAL, > >etc. won't find this code. > > so use the certain macro to replace. > > 2: > >You need to make sure that existing userspace binaries can run > >unmodified, changing the types will change the layout of the ioctl > >arguments. This may mean that you need to add new ioctls if you need to > >change types. > > I recovered the SPI_IOC_RD_LSB_FIRST ioctl. > Add new ioctl to fit u16 mode. > In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > --- > drivers/spi/spi.c | 14 ++++++++++++++ > drivers/spi/spidev.c | 29 ++++++++++++++++++++++++++++- > include/linux/spi/spi.h | 20 ++++++++++++++++++-- > include/uapi/linux/spi/spidev.h | 17 ++++++++++++++++- > 4 files changed, 76 insertions(+), 4 deletions(-) > > 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. [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). > /* 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
Le Mon, 29 Jul 2013 11:53:14 +0100, wangyuhang <wangyuhang2014@gmail.com> a écrit : > Hi, > > modify two things. > 1: > >> @@ -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 >> 8) & 0x03) == 0x03 || > >> + ((spi->mode >> 10) & 0x03) == 0x03) { > >> + dev_err(&spi->dev, > >> + "setup: can not select dual and quad at the same > >> time\n") > >> + return -EINVAL; > >> + } > >> + return -EINVAL; > >> + } > > >This code won't work if the constants you added for mode flags change > >value. More importantly, anyone searching the code for SPI_TX_DUAL, > >etc. won't find this code. > > so use the certain macro to replace. > > 2: > >You need to make sure that existing userspace binaries can run > >unmodified, changing the types will change the layout of the ioctl > >arguments. This may mean that you need to add new ioctls if you > >need to change types. > > I recovered the SPI_IOC_RD_LSB_FIRST ioctl. > Add new ioctl to fit u16 mode. > In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > --- > /* IOCTL commands */ > @@ -54,6 +61,8 @@ > * @tx_buf: Holds pointer to userspace buffer with transmit data, or > null. > * If no data is provided, zeroes are shifted out. > * @rx_buf: Holds pointer to userspace buffer for receive data, or > null. > + * @tx_nbits: number of bits used for writting. > + * @rx_nbits: number of bits used for reading. > * @len: Length of tx and rx buffers, in bytes. > * @speed_hz: Temporary override of the device's bitrate. > * @bits_per_word: Temporary override of the device's wordsize. > @@ -85,6 +94,8 @@ > struct spi_ioc_transfer { > __u64 tx_buf; > __u64 rx_buf; > + __u8 tx_nbits; > + __u8 rx_nbits; > > __u32 len; > __u32 speed_hz; You still break old userspace ABI. You need to create a new structure, if you need to add fields. Matthieu
Hi, Pekon 2013/7/29 Gupta, Pekon <pekon@ti.com>: > Hello, > >> >> Hi, >> >> modify two things. > Hope you are tracking the Patch versions ? Good to include it in mail-subject. > Ok, got it. >> 1: >> >> @@ -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 >> 8) & 0x03) == 0x03 || >> >> + ((spi->mode >> 10) & 0x03) == 0x03) { >> >> + dev_err(&spi->dev, >> >> + "setup: can not select dual and quad at the same time\n") >> >> + return -EINVAL; >> >> + } >> >> + return -EINVAL; >> >> + } >> >> >This code won't work if the constants you added for mode flags change >> >value. More importantly, anyone searching the code for SPI_TX_DUAL, >> >etc. won't find this code. >> >> so use the certain macro to replace. >> >> 2: >> >You need to make sure that existing userspace binaries can run >> >unmodified, changing the types will change the layout of the ioctl >> >arguments. This may mean that you need to add new ioctls if you need to >> >change types. >> >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl. >> Add new ioctl to fit u16 mode. >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. >> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> >> --- >> drivers/spi/spi.c | 14 ++++++++++++++ >> drivers/spi/spidev.c | 29 ++++++++++++++++++++++++++++- >> include/linux/spi/spi.h | 20 ++++++++++++++++++-- >> include/uapi/linux/spi/spidev.h | 17 ++++++++++++++++- >> 4 files changed, 76 insertions(+), 4 deletions(-) >> >> 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]: 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
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
Hi, 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>: > Le Mon, 29 Jul 2013 11:53:14 +0100, > wangyuhang <wangyuhang2014@gmail.com> a écrit : > >> Hi, >> >> modify two things. >> 1: >> >> @@ -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 >> 8) & 0x03) == 0x03 || >> >> + ((spi->mode >> 10) & 0x03) == 0x03) { >> >> + dev_err(&spi->dev, >> >> + "setup: can not select dual and quad at the same >> >> time\n") >> >> + return -EINVAL; >> >> + } >> >> + return -EINVAL; >> >> + } >> >> >This code won't work if the constants you added for mode flags change >> >value. More importantly, anyone searching the code for SPI_TX_DUAL, >> >etc. won't find this code. >> >> so use the certain macro to replace. >> >> 2: >> >You need to make sure that existing userspace binaries can run >> >unmodified, changing the types will change the layout of the ioctl >> >arguments. This may mean that you need to add new ioctls if you >> >need to change types. >> >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl. >> Add new ioctl to fit u16 mode. >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. >> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> >> --- > >> /* IOCTL commands */ >> @@ -54,6 +61,8 @@ >> * @tx_buf: Holds pointer to userspace buffer with transmit data, or >> null. >> * If no data is provided, zeroes are shifted out. >> * @rx_buf: Holds pointer to userspace buffer for receive data, or >> null. >> + * @tx_nbits: number of bits used for writting. >> + * @rx_nbits: number of bits used for reading. >> * @len: Length of tx and rx buffers, in bytes. >> * @speed_hz: Temporary override of the device's bitrate. >> * @bits_per_word: Temporary override of the device's wordsize. >> @@ -85,6 +94,8 @@ >> struct spi_ioc_transfer { >> __u64 tx_buf; >> __u64 rx_buf; >> + __u8 tx_nbits; >> + __u8 rx_nbits; >> >> __u32 len; >> __u32 speed_hz; > You still break old userspace ABI. > > You need to create a new structure, if you need to add fields. > I dont think adding the members will cause the program in userspace any errors. User do not have to set these members. Thanks
Hi, Pekon 2013/7/29 Gupta, Pekon <pekon@ti.com>: > 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). > > > To the above two scenarios, it is necessary to modify the slave driver(m25p80.c) what about this: 1,set spi_master->mode | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD; 2,set spi_device->mode | SPI_TX_QUAD | SPI_RX_QUAD(if use 4x read and 4x write) through DT. 3,add your own config and code in slave driver, example m25p80 read and write. if (spi->mode | QUAD) cmd and addr: tx_nbits=1; data: tx_nbits = 4; because no standard for dual and quad to serial flash, so no general code for all flash. I just think to provide a spi interface to let flash can set its own transfer bits. But now we have to correct the slave driver based on ourselves. So if you modify your slave driver correctly, case1 and 2 won't happen. If you use tx_nbits or something similar in spi_master and spi_device, But the procedure may be same to above. Best regards. >> > [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
Le Mon, 29 Jul 2013 15:25:51 +0100, yuhang wang <wangyuhang2014@gmail.com> a écrit : > Hi, > > 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>: > > Le Mon, 29 Jul 2013 11:53:14 +0100, > > wangyuhang <wangyuhang2014@gmail.com> a écrit : > > > >> Hi, > >> > >> modify two things. > >> 1: > >> >> @@ -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 >> 8) & 0x03) == 0x03 || > >> >> + ((spi->mode >> 10) & 0x03) == 0x03) { > >> >> + dev_err(&spi->dev, > >> >> + "setup: can not select dual and quad at the same > >> >> time\n") > >> >> + return -EINVAL; > >> >> + } > >> >> + return -EINVAL; > >> >> + } > >> > >> >This code won't work if the constants you added for mode flags > >> >change value. More importantly, anyone searching the code for > >> >SPI_TX_DUAL, etc. won't find this code. > >> > >> so use the certain macro to replace. > >> > >> 2: > >> >You need to make sure that existing userspace binaries can run > >> >unmodified, changing the types will change the layout of the ioctl > >> >arguments. This may mean that you need to add new ioctls if you > >> >need to change types. > >> > >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl. > >> Add new ioctl to fit u16 mode. > >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. > >> > >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > >> --- > > > >> /* IOCTL commands */ > >> @@ -54,6 +61,8 @@ > >> * @tx_buf: Holds pointer to userspace buffer with transmit data, > >> or null. > >> * If no data is provided, zeroes are shifted out. > >> * @rx_buf: Holds pointer to userspace buffer for receive data, or > >> null. > >> + * @tx_nbits: number of bits used for writting. > >> + * @rx_nbits: number of bits used for reading. > >> * @len: Length of tx and rx buffers, in bytes. > >> * @speed_hz: Temporary override of the device's bitrate. > >> * @bits_per_word: Temporary override of the device's wordsize. > >> @@ -85,6 +94,8 @@ > >> struct spi_ioc_transfer { > >> __u64 tx_buf; > >> __u64 rx_buf; > >> + __u8 tx_nbits; > >> + __u8 rx_nbits; > >> > >> __u32 len; > >> __u32 speed_hz; > > You still break old userspace ABI. > > > > You need to create a new structure, if you need to add fields. > > > I dont think adding the members will cause the program in userspace > any errors. User do not have to set these members. Did you try to run on your patch kernel a program build without the patch ? if I build my program with : > >> struct spi_ioc_transfer { > >> __u64 tx_buf; > >> __u64 rx_buf; > >> > >> __u32 len; > >> __u32 speed_hz; Now I run it on a new kernel. The kernel won't parse correctly my structure : len will go in tx_nbits, rx_nbits and len. Keep in mind that it is binary interface, not source interface. You should not have to rebuild your binary. Matthieu
On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote: >> >> 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' ? There are I think a total of five places that channel width can be used in the Linux SPI stack. a) In the master driver (spi_master) b) In a slave driver (spi_device) c) In a spi_message d) In a spi_transfer e) In the OF device tree, for DT nodes of a and/or b, as they appear in the dt. For a, all the widths supported would be specified. In b, c, and d, only the width to be used would be specified. So, there is some difference here, as we need a method that allows multiple modes to be specified for a but not for the others. Since some slaves will want to send a single spi message with multiple transfers of different widths (e.g., single bit read command followed by quad bit data xfer) it's necessary to specify the width in d. With the widths specified at the lowest level, it is not actually necessary to supply the width for C or B. While it's not necessary to specify the width for B, doing so would be consistent with other fields that can be specified in the spi_transfer and in the spi_device, like bits_per_word and speed_hz. It's worth pointing out that none of the mode bits from the spi_device can be specified in in a spi_transfer. Currently, if you want to change anything in mode you must call spi_setup() on the device. Since width can be changed on a transfer by transfer basis, maybe that is a good reason to say it doesn't belong in mode. If the default width is in the spi_device, then a spi_transfer needs a width state to mean "unspecified". I.e., the read width can be one, two, four, (eight), or unspecified bits. Unspecified gets changed to the default from the spi_device, whereas one bit wide means one bit wide. Allowing the spi_device to have a width means the width can be checked against the master's supported widths at spi_setup() time, like the other mode bits. But, since width can be set on a transfer by transfer basis and each transfer is not checked in the spi core, this doesn't really do the master driver much good. The master driver still needs to check each transfer to see if the width is allowed. I don't see the point of putting the width in C. Current a spi_message can't change any of the settings from the spi_device or supply a default for the the settings in a spi_transfer, and I don't see why width is any different here. For the OF device tree, I don't think the master's node should have anything about the widths supported. There is nothing about any of the other spi master capabilities in the DT. The master driver supplies this information. For the OF slave device node, it's not clear it needs to be there either. The current existing use cases are for memories that only use dual and quad for certain parts of certain commands. Setting the width for the slave overall doesn't make sense. But, the memory might have four data lines connected on the board or it might just have one. That does seem like information about board layout that belongs in the device tree. So there does need to be done "enable quad mode" property. However, is this a SPI property or a property of the memory device's driver? Turning on quad mode support isn't the same that as setting the clock polarity. Clock polarity it a SPI property supplied to spi_setup. Quad mode means the driver should use the quad read command and the appropriate spi_transfers for using it.
Hi, Yes, you are right. I will correct it. Thanks 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>: > Le Mon, 29 Jul 2013 15:25:51 +0100, > yuhang wang <wangyuhang2014@gmail.com> a écrit : > >> Hi, >> >> 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>: >> > Le Mon, 29 Jul 2013 11:53:14 +0100, >> > wangyuhang <wangyuhang2014@gmail.com> a écrit : >> > >> >> Hi, >> >> >> >> modify two things. >> >> 1: >> >> >> @@ -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 >> 8) & 0x03) == 0x03 || >> >> >> + ((spi->mode >> 10) & 0x03) == 0x03) { >> >> >> + dev_err(&spi->dev, >> >> >> + "setup: can not select dual and quad at the same >> >> >> time\n") >> >> >> + return -EINVAL; >> >> >> + } >> >> >> + return -EINVAL; >> >> >> + } >> >> >> >> >This code won't work if the constants you added for mode flags >> >> >change value. More importantly, anyone searching the code for >> >> >SPI_TX_DUAL, etc. won't find this code. >> >> >> >> so use the certain macro to replace. >> >> >> >> 2: >> >> >You need to make sure that existing userspace binaries can run >> >> >unmodified, changing the types will change the layout of the ioctl >> >> >arguments. This may mean that you need to add new ioctls if you >> >> >need to change types. >> >> >> >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl. >> >> Add new ioctl to fit u16 mode. >> >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way. >> >> >> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> >> >> --- >> > >> >> /* IOCTL commands */ >> >> @@ -54,6 +61,8 @@ >> >> * @tx_buf: Holds pointer to userspace buffer with transmit data, >> >> or null. >> >> * If no data is provided, zeroes are shifted out. >> >> * @rx_buf: Holds pointer to userspace buffer for receive data, or >> >> null. >> >> + * @tx_nbits: number of bits used for writting. >> >> + * @rx_nbits: number of bits used for reading. >> >> * @len: Length of tx and rx buffers, in bytes. >> >> * @speed_hz: Temporary override of the device's bitrate. >> >> * @bits_per_word: Temporary override of the device's wordsize. >> >> @@ -85,6 +94,8 @@ >> >> struct spi_ioc_transfer { >> >> __u64 tx_buf; >> >> __u64 rx_buf; >> >> + __u8 tx_nbits; >> >> + __u8 rx_nbits; >> >> >> >> __u32 len; >> >> __u32 speed_hz; >> > You still break old userspace ABI. >> > >> > You need to create a new structure, if you need to add fields. >> > >> I dont think adding the members will cause the program in userspace >> any errors. User do not have to set these members. > Did you try to run on your patch kernel a program build without the > patch ? > > > if I build my program with : > >> >> struct spi_ioc_transfer { >> >> __u64 tx_buf; >> >> __u64 rx_buf; >> >> >> >> __u32 len; >> >> __u32 speed_hz; > > Now I run it on a new kernel. > The kernel won't parse correctly my structure : len will go in > tx_nbits, rx_nbits and len. > > Keep in mind that it is binary interface, not source interface. You > should not have to rebuild your binary. > > Matthieu
Hi,Trent Thanks for so many details. 2013/7/30 Trent Piepho <tpiepho@gmail.com>: > On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote: >>> >> 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' ? > > There are I think a total of five places that channel width can be > used in the Linux SPI stack. > > a) In the master driver (spi_master) > b) In a slave driver (spi_device) > c) In a spi_message > d) In a spi_transfer > e) In the OF device tree, for DT nodes of a and/or b, as they appear in the dt. > > For a, all the widths supported would be specified. In b, c, and d, > only the width to be used would be specified. So, there is some > difference here, as we need a method that allows multiple modes to be > specified for a but not for the others. > > Since some slaves will want to send a single spi message with multiple > transfers of different widths (e.g., single bit read command followed > by quad bit data xfer) it's necessary to specify the width in d. With > the widths specified at the lowest level, it is not actually necessary > to supply the width for C or B. > > While it's not necessary to specify the width for B, doing so would be > consistent with other fields that can be specified in the spi_transfer > and in the spi_device, like bits_per_word and speed_hz. It's worth > pointing out that none of the mode bits from the spi_device can be > specified in in a spi_transfer. Currently, if you want to change > anything in mode you must call spi_setup() on the device. Since width > can be changed on a transfer by transfer basis, maybe that is a good > reason to say it doesn't belong in mode. > what you said is ok. But b) and d) are not the same. d) will be set based on b), but not equal to d)=b)(bits_per_word and speed_hz). So don't you think that b) is more like a transfer mode and d) is the detailed functions related to the mode. Also b) should be set in spi_setup(), only d) width can be changed by transfer, but b) should not be modifyed. But none of the mode bits from the spi_device can be specified in in a spi_transfer, that is a point. Is that necessary to add members like "transfer_mode" in spi_master and spi_device which do the similar operation to "mode"? > If the default width is in the spi_device, then a spi_transfer needs a > width state to mean "unspecified". I.e., the read width can be one, > two, four, (eight), or unspecified bits. Unspecified gets changed to > the default from the spi_device, whereas one bit wide means one bit > wide. > what do you mean by default, is that without init value? Do you want to use bit by bit in spi_transfer? > Allowing the spi_device to have a width means the width can be checked > against the master's supported widths at spi_setup() time, like the > other mode bits. But, since width can be set on a transfer by > transfer basis and each transfer is not checked in the spi core, this > doesn't really do the master driver much good. The master driver > still needs to check each transfer to see if the width is allowed. > agree. so do it in __spi_async() and to make sure that spi_transfer <= spi_device. > I don't see the point of putting the width in C. Current a > spi_message can't change any of the settings from the spi_device or > supply a default for the the settings in a spi_transfer, and I don't > see why width is any different here. > > For the OF device tree, I don't think the master's node should have > anything about the widths supported. There is nothing about any of > the other spi master capabilities in the DT. The master driver > supplies this information. > > For the OF slave device node, it's not clear it needs to be there > either. The current existing use cases are for memories that only use > dual and quad for certain parts of certain commands. Setting the > width for the slave overall doesn't make sense. > > But, the memory might have four data lines connected on the board or > it might just have one. That does seem like information about board > layout that belongs in the device tree. So there does need to be done > "enable quad mode" property. However, is this a SPI property or a > property of the memory device's driver? Turning on quad mode support > isn't the same that as setting the clock polarity. Clock polarity it > a SPI property supplied to spi_setup. Quad mode means the driver > should use the quad read command and the appropriate spi_transfers for > using it. How to set DT depend on whether to seperate the dual and quad from mode. Best regards.
> > Hello, > > 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 > > @@ -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. One more feedback, Its better you define SPI_NBITS_xx matching the actual number of wires used. This would leave places for some micro-wire and 3-wire SPI, and would be more readable. +#define SPI_NBITS_SINGLE_HALF_DUPLEX 0x0; /* 1bit transfer microwire*/ +#define SPI_NBITS_SINGLE_FULL_DUPLEX 0x01; /* 1bit transfer normal SPI*/ +#define SPI_NBITS_DUAL 2; /* 2bits transfer */ +#define SPI_NBITS_QUAD 4; /* 4bits transfer */ with regards, pekon
> > Hi,Trent > > Thanks for so many details. > 2013/7/30 Trent Piepho <tpiepho@gmail.com>: > > On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote: > >>> >> 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' ? > > > > There are I think a total of five places that channel width can be > > used in the Linux SPI stack. > > > > a) In the master driver (spi_master) > > b) In a slave driver (spi_device) > > c) In a spi_message > > d) In a spi_transfer > > e) In the OF device tree, for DT nodes of a and/or b, as they appear in the > dt. > > [Pekon]: Just to be aligned here.. (a) master_driver (spi_master) == like spi/spi-omap2-mcspi.c .. correct ? (b) slave_driver (spi_device) == like mtd/devices/m25p80.c .. correct ? > > For a, all the widths supported would be specified. In b, c, and d, > > only the width to be used would be specified. So, there is some > > difference here, as we need a method that allows multiple modes to be > > specified for a but not for the others. > > > > Since some slaves will want to send a single spi message with multiple > > transfers of different widths (e.g., single bit read command followed > > by quad bit data xfer) it's necessary to specify the width in d. With > > the widths specified at the lowest level, it is not actually necessary > > to supply the width for C or B. > > [Pekon]: Agreed.. This proposal is better of having.. (b) spi_device->mode = SPI_RX_QUAD | SPI_TX_SINGLE; (c) spi_transfer.tx_nbits = 4; spi_transfer.rx_nbits = 4; > > While it's not necessary to specify the width for B, doing so would be > > consistent with other fields that can be specified in the spi_transfer > > and in the spi_device, like bits_per_word and speed_hz. It's worth > > pointing out that none of the mode bits from the spi_device can be > > specified in in a spi_transfer. Currently, if you want to change > > anything in mode you must call spi_setup() on the device. Since width > > can be changed on a transfer by transfer basis, maybe that is a good > > reason to say it doesn't belong in mode. > > > what you said is ok. But b) and d) are not the same. d) will be set > based on b), but not equal to d)=b)(bits_per_word and speed_hz). So > don't you think that b) is more like a transfer mode and d) is the > detailed functions related to the mode. Also b) should be set in > spi_setup(), only d) width can be changed by transfer, but b) should > not be modifyed. But none of the mode bits from the spi_device can be > specified in in a spi_transfer, that is a point. Is that necessary to > add members like "transfer_mode" in spi_master and spi_device which do > the similar operation to "mode"? > [Pekon]: I did not fully understand your comments, But following is my understanding of DT parsing. There should two level of DT binding (a) For SPI controller: - describes the capabilities of H/W engine on SoC - goes into spi_master->mode (b) For SPI devices: - describes capabilities of each device on board (includes board wire connections. (single controller can be connected to many SPI devices) - goes into spi_device->mode, by matching common compatibilities between spi_master and spi_device_n This is important for where multiple SPI devices are connected to one controller which supports all modes (SINGLE, DUAL, QUAD) But; - /dev/spi0 can have all 4 data-outputs connected on board so can support QUAD mode - /dev/spi1 has only 2 data-outputs connected on board so can support only SINGLE | DUAL So, you have defined DT bindings for (b) only. You need to add DT binding for (a) also. > > If the default width is in the spi_device, then a spi_transfer needs a > > width state to mean "unspecified". I.e., the read width can be one, > > two, four, (eight), or unspecified bits. Unspecified gets changed to > > the default from the spi_device, whereas one bit wide means one bit > > wide. > > > what do you mean by default, is that without init value? Do you want > to use bit by bit in spi_transfer? > [Pekon]: So default here means common basic capability which all devices are expected to meet at-least. And I think this is SPI_SINGLE_FULL_DUPLEX mode. > > Allowing the spi_device to have a width means the width can be checked > > against the master's supported widths at spi_setup() time, like the > > other mode bits. But, since width can be set on a transfer by > > transfer basis and each transfer is not checked in the spi core, this > > doesn't really do the master driver much good. The master driver > > still needs to check each transfer to see if the width is allowed. > > > agree. so do it in __spi_async() and to make sure that spi_transfer <= > spi_device. > > > I don't see the point of putting the width in C. Current a > > spi_message can't change any of the settings from the spi_device or > > supply a default for the the settings in a spi_transfer, and I don't > > see why width is any different here. > > [Pekon]: Agreed.. if you are already adding tx_nbits & rx_nbits in spi_transfer, then you don't need to add same in spi_message. (at-least not for now). > > For the OF device tree, I don't think the master's node should have > > anything about the widths supported. There is nothing about any of > > the other spi master capabilities in the DT. The master driver > > supplies this information. > > [Pekon]: Nope not correct. See example above of having multiple SPI devices connected to same controller. > > For the OF slave device node, it's not clear it needs to be there > > either. The current existing use cases are for memories that only use > > dual and quad for certain parts of certain commands. Setting the > > width for the slave overall doesn't make sense. > > > > But, the memory might have four data lines connected on the board or > > it might just have one. That does seem like information about board > > layout that belongs in the device tree. So there does need to be done > > "enable quad mode" property. However, is this a SPI property or a > > property of the memory device's driver? Turning on quad mode support > > isn't the same that as setting the clock polarity. Clock polarity it > > a SPI property supplied to spi_setup. Quad mode means the driver > > should use the quad read command and the appropriate spi_transfers for > > using it. > How to set DT depend on whether to seperate the dual and quad from > mode. > [Pekon] you need separate DT binding for controller and child devices. And then separate for Tx and Rx in each of them. Also, I suggest you add devicetree-discuss maillist when you update the Device-tree documentation for same. I havn't seen any traffic on next version of this patch? Did I miss, or you are working on it? with regards, pekon
Hi, Pekon 2013/8/7 Gupta, Pekon <pekon@ti.com>: >> >> Hello, >> > 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 >> > @@ -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. > One more feedback, Its better you define SPI_NBITS_xx matching > the actual number of wires used. This would leave places for some > micro-wire and 3-wire SPI, and would be more readable. > +#define SPI_NBITS_SINGLE_HALF_DUPLEX 0x0; /* 1bit transfer microwire*/ > +#define SPI_NBITS_SINGLE_FULL_DUPLEX 0x01; /* 1bit transfer normal SPI*/ > +#define SPI_NBITS_DUAL 2; /* 2bits transfer */ > +#define SPI_NBITS_QUAD 4; /* 4bits transfer */ > Totally three points. 1. There are still other positions which have macro defined in struct in spi.h. That means the macro is belonged to the member above. So to make it consistent, define it in @spi_transfer seems OK. 2. I don't think that SPI_NBITS_SINGLE_HALF_DUPLEX is necessary. tx_nbits and rx_nbits in @spi_transfer should be passed to spi master driver and the master driver will set the certain mode depend on the info in @spi_transfer. So there is no need to let the master driver know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI info from spi_device->mode. 3. I agree to match the actual number of wires used. To make it 0x01, 0x02 and 0x04. Thanks. Best regards.
> 2. I don't think that SPI_NBITS_SINGLE_HALF_DUPLEX is necessary. > tx_nbits and rx_nbits in @spi_transfer should be passed to spi master > driver and the master driver will set the certain mode depend on the > info in @spi_transfer. So there is no need to let the master driver > know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI > info from spi_device->mode. I should have been more explicit here.. it would be good it you replace 'spi_device->mode & SPI_3_WIRE' with your above mode & tx_nbit, rx_nbits so that the channel width information becomes consistent for all modes. All mode bit flags in same octet look better. with regards, pekon
Hi, Pekon 2013/8/8 Gupta, Pekon <pekon@ti.com>: >> >> Hi,Trent >> >> Thanks for so many details. >> 2013/7/30 Trent Piepho <tpiepho@gmail.com>: >> > On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote: >> >>> >> 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' ? >> > >> > There are I think a total of five places that channel width can be >> > used in the Linux SPI stack. >> > >> > a) In the master driver (spi_master) >> > b) In a slave driver (spi_device) >> > c) In a spi_message >> > d) In a spi_transfer >> > e) In the OF device tree, for DT nodes of a and/or b, as they appear in the >> dt. >> > > [Pekon]: Just to be aligned here.. > (a) master_driver (spi_master) == like spi/spi-omap2-mcspi.c .. correct ? > (b) slave_driver (spi_device) == like mtd/devices/m25p80.c .. correct ? > Yes, that's it. >> > For a, all the widths supported would be specified. In b, c, and d, >> > only the width to be used would be specified. So, there is some >> > difference here, as we need a method that allows multiple modes to be >> > specified for a but not for the others. >> > >> > Since some slaves will want to send a single spi message with multiple >> > transfers of different widths (e.g., single bit read command followed >> > by quad bit data xfer) it's necessary to specify the width in d. With >> > the widths specified at the lowest level, it is not actually necessary >> > to supply the width for C or B. >> > > [Pekon]: Agreed.. This proposal is better of having.. > (b) spi_device->mode = SPI_RX_QUAD | SPI_TX_SINGLE; > (c) spi_transfer.tx_nbits = 4; spi_transfer.rx_nbits = 4; > >> > While it's not necessary to specify the width for B, doing so would be >> > consistent with other fields that can be specified in the spi_transfer >> > and in the spi_device, like bits_per_word and speed_hz. It's worth >> > pointing out that none of the mode bits from the spi_device can be >> > specified in in a spi_transfer. Currently, if you want to change >> > anything in mode you must call spi_setup() on the device. Since width >> > can be changed on a transfer by transfer basis, maybe that is a good >> > reason to say it doesn't belong in mode. >> > >> what you said is ok. But b) and d) are not the same. d) will be set >> based on b), but not equal to d)=b)(bits_per_word and speed_hz). So >> don't you think that b) is more like a transfer mode and d) is the >> detailed functions related to the mode. Also b) should be set in >> spi_setup(), only d) width can be changed by transfer, but b) should >> not be modifyed. But none of the mode bits from the spi_device can be >> specified in in a spi_transfer, that is a point. Is that necessary to >> add members like "transfer_mode" in spi_master and spi_device which do >> the similar operation to "mode"? >> > [Pekon]: I did not fully understand your comments, But following > is my understanding of DT parsing. > There should two level of DT binding > (a) For SPI controller: > - describes the capabilities of H/W engine on SoC > - goes into spi_master->mode > (b) For SPI devices: > - describes capabilities of each device on board (includes board > wire connections. (single controller can be connected to many SPI devices) > - goes into spi_device->mode, by matching common compatibilities > between spi_master and spi_device_n > > This is important for where multiple SPI devices are connected to > one controller which supports all modes (SINGLE, DUAL, QUAD) But; > - /dev/spi0 can have all 4 data-outputs connected on board > so can support QUAD mode > - /dev/spi1 has only 2 data-outputs connected on board > so can support only SINGLE | DUAL > > So, you have defined DT bindings for (b) only. > You need to add DT binding for (a) also. > well, if the final scheme is to make dual and quad in mode, I agree with your point. >> > If the default width is in the spi_device, then a spi_transfer needs a >> > width state to mean "unspecified". I.e., the read width can be one, >> > two, four, (eight), or unspecified bits. Unspecified gets changed to >> > the default from the spi_device, whereas one bit wide means one bit >> > wide. >> > >> what do you mean by default, is that without init value? Do you want >> to use bit by bit in spi_transfer? >> > [Pekon]: So default here means common basic capability which all > devices are expected to meet at-least. > And I think this is SPI_SINGLE_FULL_DUPLEX mode. > If so, I still can not figure out what Trent means. Is that if spi_device select the default mode, then we need to give spi_transfer(tx_nbits and rx_nbits) a init value to make it not unspecified? >> > Allowing the spi_device to have a width means the width can be checked >> > against the master's supported widths at spi_setup() time, like the >> > other mode bits. But, since width can be set on a transfer by >> > transfer basis and each transfer is not checked in the spi core, this >> > doesn't really do the master driver much good. The master driver >> > still needs to check each transfer to see if the width is allowed. >> > >> agree. so do it in __spi_async() and to make sure that spi_transfer <= >> spi_device. >> >> > I don't see the point of putting the width in C. Current a >> > spi_message can't change any of the settings from the spi_device or >> > supply a default for the the settings in a spi_transfer, and I don't >> > see why width is any different here. >> > > [Pekon]: Agreed.. if you are already adding tx_nbits & rx_nbits in > spi_transfer, then you don't need to add same in spi_message. > (at-least not for now). > >> > For the OF device tree, I don't think the master's node should have >> > anything about the widths supported. There is nothing about any of >> > the other spi master capabilities in the DT. The master driver >> > supplies this information. >> > > [Pekon]: Nope not correct. See example above of having multiple > SPI devices connected to same controller. > >> > For the OF slave device node, it's not clear it needs to be there >> > either. The current existing use cases are for memories that only use >> > dual and quad for certain parts of certain commands. Setting the >> > width for the slave overall doesn't make sense. >> > >> > But, the memory might have four data lines connected on the board or >> > it might just have one. That does seem like information about board >> > layout that belongs in the device tree. So there does need to be done >> > "enable quad mode" property. However, is this a SPI property or a >> > property of the memory device's driver? Turning on quad mode support >> > isn't the same that as setting the clock polarity. Clock polarity it >> > a SPI property supplied to spi_setup. Quad mode means the driver >> > should use the quad read command and the appropriate spi_transfers for >> > using it. >> How to set DT depend on whether to seperate the dual and quad from >> mode. >> > [Pekon] you need separate DT binding for controller and child devices. > And then separate for Tx and Rx in each of them. > Also, I suggest you add devicetree-discuss maillist when you update the > Device-tree documentation for same. > Just as you said above, master node describe the total data wires on SOC, slave node describe the detailed capability selected, which is logically right. But to master, the drivers have already supplied this infomation in probe, so do we still need to add in device tree and provide another method in master driver to get the info from device tree? > I havn't seen any traffic on next version of this patch? > Did I miss, or you are working on it? > No, because of other jobs so didn't fix the patch in time, sorry for that. Even now I still in puzzled that whether to make quad in mode or add a new member in all the related struct. My trend is the previous scheme. However they both have strong and weak points. If add in mode: [strong points]: The existed operation such as compare master mode and device mode, DT interface, mode set in master driver and so on which will be a benifit to make the quad code clear and smooth. [weak points] But to expand mode will make spidev not back compatible and the modified spidev code to fix the compatible problems looks a little wierd. Also another point from Trent, to make it consistent between spi_transfer and spi_device. just like: spi_transfer->speed_hz depend on spi_device->speed_hz; spi_transfer->bits_per_word depends on spi_device->bits_per_word; So is that add the similar member as below looks better? spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode. if add new member: The previous strong points and weak points will become exchanged. I still hope some suggestions to make one scheme extremely powerful and I will provide the patch this week. Best regards.
> > [Pekon] you need separate DT binding for controller and child devices. > > And then separate for Tx and Rx in each of them. > > Also, I suggest you add devicetree-discuss maillist when you update the > > Device-tree documentation for same. > > > Just as you said above, master node describe the total data wires on > SOC, slave node describe the detailed capability selected, which is > logically right. But to master, the drivers have already supplied this > infomation in probe, so do we still need to add in device tree and > provide another method in master driver to get the info from device > tree? > [Pekon]: Yes, for controller the controller_driver should parse or fill in this info in the spi_master->mode. But, still you need to add check while probing spi_device DT that only common supported compatibilities get into spi_device. Example: if spi_master->mode == SPI_TX_DUAL only and of_spi_device() returns SPI_TX_QUAD, then you should return back with error showing mis-match in controller and device capabilities. So, spi_device capabilites should always be sub-set of spi_master capabilities. > > I havn't seen any traffic on next version of this patch? > > Did I miss, or you are working on it? > > > No, because of other jobs so didn't fix the patch in time, sorry for > that. Even now I still in puzzled that whether to make quad in mode or > add a new member in all the related struct. My trend is the previous > scheme. However they both have strong and weak points. > If add in mode: > [strong points]: > The existed operation such as compare master mode and device mode, DT > interface, mode set in master driver and so on which will be a benifit > to make the quad code clear and smooth. > [weak points] > But to expand mode will make spidev not back compatible and the > modified spidev code to fix the compatible problems looks a little > wierd. > Also another point from Trent, to make it consistent between > spi_transfer and spi_device. just like: > spi_transfer->speed_hz depend on spi_device->speed_hz; > spi_transfer->bits_per_word depends on spi_device->bits_per_word; > So is that add the similar member as below looks better? > spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode. > if add new member: > The previous strong points and weak points will become exchanged. > I still hope some suggestions to make one scheme extremely powerful > and I will provide the patch this week. [Pekon]: I'm also inclined to your suggestion of previous scheme, but we don't break the binary compatibility with userspace applications. (as Matt pointed out earlier). But I think its more of an upgrade|extension of of IOCTL than re-defining it. And as SPI is evolving we need to make these upgrades in our interface to adapt to newer SPI. But its upto maintainers to accept that. So, I suggest you send the spidev related patch separately, so that there could be more discussion on that. with regards, pekon
Hi, Pekon 2013/8/9 Gupta, Pekon <pekon@ti.com>: >> 2. I don't think that SPI_NBITS_SINGLE_HALF_DUPLEX is necessary. >> tx_nbits and rx_nbits in @spi_transfer should be passed to spi master >> driver and the master driver will set the certain mode depend on the >> info in @spi_transfer. So there is no need to let the master driver >> know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI >> info from spi_device->mode. > I should have been more explicit here.. it would be good it you replace > 'spi_device->mode & SPI_3_WIRE' with your above mode & tx_nbit, rx_nbits > so that the channel width information becomes consistent for all modes. > All mode bit flags in same octet look better. > If still use mode in spi_master and spi_device, I don't think it is possible to del SPI_3_WIRE. But we should make sure if SPI_3_WIRE is selected by slave, then dual and quad should not be set again. 2013/8/9 Gupta, Pekon <pekon@ti.com>: >> > [Pekon] you need separate DT binding for controller and child devices. >> > And then separate for Tx and Rx in each of them. >> > Also, I suggest you add devicetree-discuss maillist when you update the >> > Device-tree documentation for same. >> > >> Just as you said above, master node describe the total data wires on >> SOC, slave node describe the detailed capability selected, which is >> logically right. But to master, the drivers have already supplied this >> infomation in probe, so do we still need to add in device tree and >> provide another method in master driver to get the info from device >> tree? >> > [Pekon]: Yes, for controller the controller_driver should parse or fill in > this info in the spi_master->mode. > But, still you need to add check while probing spi_device DT that > only common supported compatibilities get into spi_device. > Example: if spi_master->mode == SPI_TX_DUAL only > and of_spi_device() returns SPI_TX_QUAD, then you should return > back with error showing mis-match in controller and device capabilities. > So, spi_device capabilites should always be sub-set of spi_master capabilities. > Yes, it is necessary to check. But I don't think it is a good way to fill in spi_master->mode from master node of DT. Master driver developer should set dual or quad mode in spi_master->mode directly just like SPI_CPHA and SPI_CPOL. To keep consistent, dual and quad should also not appear in master node. >> > I havn't seen any traffic on next version of this patch? >> > Did I miss, or you are working on it? >> > >> No, because of other jobs so didn't fix the patch in time, sorry for >> that. Even now I still in puzzled that whether to make quad in mode or >> add a new member in all the related struct. My trend is the previous >> scheme. However they both have strong and weak points. > >> If add in mode: >> [strong points]: >> The existed operation such as compare master mode and device mode, DT >> interface, mode set in master driver and so on which will be a benifit >> to make the quad code clear and smooth. >> [weak points] >> But to expand mode will make spidev not back compatible and the >> modified spidev code to fix the compatible problems looks a little >> wierd. >> Also another point from Trent, to make it consistent between >> spi_transfer and spi_device. just like: >> spi_transfer->speed_hz depend on spi_device->speed_hz; >> spi_transfer->bits_per_word depends on spi_device->bits_per_word; >> So is that add the similar member as below looks better? >> spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode. > >> if add new member: >> The previous strong points and weak points will become exchanged. > >> I still hope some suggestions to make one scheme extremely powerful >> and I will provide the patch this week. > > [Pekon]: I'm also inclined to your suggestion of previous scheme, > but we don't break the binary compatibility with userspace applications. > (as Matt pointed out earlier). But I think its more of an upgrade|extension of > of IOCTL than re-defining it. And as SPI is evolving we need to make these > upgrades in our interface to adapt to newer SPI. > But its upto maintainers to accept that. So, I suggest you send the spidev > related patch separately, so that there could be more discussion on that. > > OK, I will separate the patch. Thanks. Best regards
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; /* 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; + } 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 */ 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 diff --git a/include/uapi/linux/spi/spidev.h b/include/uapi/linux/spi/spidev.h index 52d9ed0..4d47e8a 100644 --- a/include/uapi/linux/spi/spidev.h +++ b/include/uapi/linux/spi/spidev.h @@ -42,7 +42,14 @@ #define SPI_LOOP 0x20 #define SPI_NO_CS 0x40 #define SPI_READY 0x80 - +#define SPI_TX_DUAL 0x100 +#define SPI_TX_QUAD 0x200 +#define SPI_RX_DUAL 0x400 +#define SPI_RX_QUAD 0x800 + +#define SPI_NBITS_SINGLE 0x0; /* 1bit transfer */ +#define SPI_NBITS_DUAL 0x01; /* 2bits transfer */ +#define SPI_NBITS_QUAD 0x02; /* 4bits transfer */ /*---------------------------------------------------------------------------*/ /* IOCTL commands */ @@ -54,6 +61,8 @@ * @tx_buf: Holds pointer to userspace buffer with transmit data, or null. * If no data is provided, zeroes are shifted out. * @rx_buf: Holds pointer to userspace buffer for receive data, or null. + * @tx_nbits: number of bits used for writting. + * @rx_nbits: number of bits used for reading. * @len: Length of tx and rx buffers, in bytes. * @speed_hz: Temporary override of the device's bitrate. * @bits_per_word: Temporary override of the device's wordsize. @@ -85,6 +94,8 @@ struct spi_ioc_transfer { __u64 tx_buf; __u64 rx_buf; + __u8 tx_nbits; + __u8 rx_nbits; __u32 len; __u32 speed_hz; @@ -126,6 +137,10 @@ struct spi_ioc_transfer { #define SPI_IOC_RD_MAX_SPEED_HZ _IOR(SPI_IOC_MAGIC, 4, __u32) #define SPI_IOC_WR_MAX_SPEED_HZ _IOW(SPI_IOC_MAGIC, 4, __u32) +/* Read / Write of SPI mode (including SPI DUAL/QUAD) */ +#define SPI_IOC_EXTRD_MODE _IOR(SPI_IOC_MAGIC, 5, __u16) +#define SPI_IOC_EXTWR_MODE _IOW(SPI_IOC_MAGIC, 5, __u16) + #endif /* SPIDEV_H */