| Message ID | 1376216117-5864-1-git-send-email-wangyuhang2014@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Mark, Wang, On Sunday 11 August 2013 03:45 PM, wangyuhang wrote: > fix the previous patch some mistake below: > 1. DT in slave node, use "spi-tx-nbits =<1/2/4>" in place of using > "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the > previous way to get the property in @of_register_spi_devices(). > 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL > SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. > 3. Add the following check > (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the > single, dual and quad. > (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode > example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set > to QUAD(SPI_NBITS_QUAD) > (3)if "@spi_device->mode& SPI_3WIRE", then tx/rx_nbits should be in > single(SPI_NBITS_SINGLE) > > Signed-off-by: wangyuhang<wangyuhang2014@gmail.com> > --- > drivers/spi/spi.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 22 ++++++++++- > 2 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 004b10f..78f257a 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master) > if (of_find_property(nc, "spi-3wire", NULL)) > spi->mode |= SPI_3WIRE; > > + /* Device DUAL/QUAD mode */ > + prop = of_get_property(nc, "spi-tx-nbits",&len); > + if (!prop || len< sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; > + } > + switch(be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_TX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_TX_QUAD; > + break; > + default: > + dev_err(&master->dev, "spi-tx-nbits value is not supported\n"); > + spi_dev_put(spi); > + continue; > + } > + > + prop = of_get_property(nc, "spi-rx-nbits",&len); > + if (!prop || len< sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; > + } > + switch(be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_RX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_RX_QUAD; > + break; > + default: > + dev_err(&master->dev, "spi-rx-nbits value is not supported\n"); > + spi_dev_put(spi); > + continue; > + } > + > /* Device speed */ > prop = of_get_property(nc, "spi-max-frequency",&len); > if (!prop || len< sizeof(*prop)) { > @@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi) > unsigned bad_bits; > int status = 0; > > + /* check mode to prevent that DUAL and QUAD set at the same time > + */ > + 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; > + } > + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden > + */ > + if ((spi->mode& SPI_3WIRE)&& (spi->mode& > + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > /* help drivers fail *cleanly* when they need options > * that aren't supported with their current master > */ > @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) > /** > * Set transfer bits_per_word and max speed as spi device default if > * it is not set for this transfer. > + * Set transfer tx_nbits and rx_nbits as single transfer default > + * (SPI_NBITS_SINGLE) if it is not set for this transfer. > */ > list_for_each_entry(xfer,&message->transfers, transfer_list) { > if (!xfer->bits_per_word) > xfer->bits_per_word = spi->bits_per_word; > if (!xfer->speed_hz) > xfer->speed_hz = spi->max_speed_hz; > + if (xfer->tx_buf&& !xfer->tx_nbits) > + xfer->tx_nbits = SPI_NBITS_SINGLE; > + if (xfer->rx_buf&& !xfer->rx_nbits) > + xfer->rx_nbits = SPI_NBITS_SINGLE; > + /* check transfer tx/rx_nbits: > + * 1. keep the value is not out of single, dual and quad > + * 2. keep tx/rx_nbits is contained by mode in spi_device > + * 3. if SPI_3WIRE, tx/rx_nbits should be in single > + */ Sorry for pitchin in a bit late on this. I was trying to use this for my use case today. I realise this check should be under... > + if (xfer->tx_nbits != SPI_NBITS_SINGLE&& > + xfer->tx_nbits != SPI_NBITS_DUAL&& > + xfer->tx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_DUAL)&& > + !(spi->mode& (SPI_TX_DUAL | SPI_TX_QUAD))) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_QUAD)&& > + !(spi->mode& SPI_TX_QUAD)) > + return -EINVAL; > + if ((spi->mode& SPI_3WIRE)&& > + (xfer->tx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; if (xfer->tx_buf) { ...... } and > + /* check transfer rx_nbits */ > + if (xfer->rx_nbits != SPI_NBITS_SINGLE&& > + xfer->rx_nbits != SPI_NBITS_DUAL&& > + xfer->rx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_DUAL)&& > + !(spi->mode& (SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_QUAD)&& > + !(spi->mode& SPI_RX_QUAD)) > + return -EINVAL; > + if ((spi->mode& SPI_3WIRE)&& > + (xfer->rx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; this should be under if (xfer->rx_buf) { ..... } > } > > message->spi = spi; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 38c2b92..3dc0a86 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,10 @@ 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. If 0 the default > + * (SPI_NBITS_SINGLE) is used. > + * @rx_nbits: number of bits used for reading. If 0 the default > + * (SPI_NBITS_SINGLE) is used. > * @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 +499,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 +524,11 @@ struct spi_transfer { > dma_addr_t rx_dma; > > unsigned cs_change:1; > + u8 tx_nbits; > + u8 rx_nbits; > +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ > +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ > +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ > u8 bits_per_word; > u16 delay_usecs; > u32 speed_hz; > @@ -858,7 +876,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 ------------------------------------------------------------------------------ Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today! http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
On Thursday 22 August 2013 06:51 PM, Mark Brown wrote: > On Thu, Aug 22, 2013 at 06:05:11PM +0530, Sourav Poddar wrote: >> On Sunday 11 August 2013 03:45 PM, wangyuhang wrote: >>> + /* check transfer tx/rx_nbits: >>> + * 1. keep the value is not out of single, dual and quad >>> + * 2. keep tx/rx_nbits is contained by mode in spi_device >>> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >>> + */ >> Sorry for pitchin in a bit late on this. >> I was trying to use this for my use case today. I realise this check >> should be under... > Can you send a patch for this please? Yes, I will send it. ------------------------------------------------------------------------------ Introducing Performance Central, a new site from SourceForge and AppDynamics. Performance Central is your source for news, insights, analysis and resources for efficient Application Performance Management. Visit us today! http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk
On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: > fix the previous patch some mistake below: This isn't a very good commit message. "the previous patch" will have no meaning in the kernel git repo. The rest of the message only describes changes since a previous version of the patch and not the actual patch in full. > > + /* Device DUAL/QUAD mode */ > + prop = of_get_property(nc, "spi-tx-nbits", &len); Why not use of_property_read_u32() here? > + if (!prop || len < sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; So if no spi-tx-nbits property is supplied, the device is rejected and the loop continues to the next device entry? This means ALL EXISTING DEVICE TREES with SPI devices will be rejected, since none of them have this new property! Was this patch tested at all with any system before being accepted? > + /* check mode to prevent that DUAL and QUAD set at the same time > + */ > + 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; This test can be done with fewer operations as: if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { > + } > + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden > + */ > + if ((spi->mode & SPI_3WIRE) && (spi->mode & > + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; No dev_err message for this possibility? What's different than the previous check that does produce a message? > /* help drivers fail *cleanly* when they need options > * that aren't supported with their current master > */ Following this comment is the code: bad_bits = spi->mode & ~spi->master->mode_bits; if (bad_bits) { dev_err(&spi->dev, "setup: unsupported mode bits %x\n", bad_bits); return -EINVAL; } Won't this always trigger for anything that sets one of the dual or quad bits? > @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) > /** > * Set transfer bits_per_word and max speed as spi device default if > * it is not set for this transfer. > + * Set transfer tx_nbits and rx_nbits as single transfer default > + * (SPI_NBITS_SINGLE) if it is not set for this transfer. > */ > list_for_each_entry(xfer, &message->transfers, transfer_list) { > if (!xfer->bits_per_word) > xfer->bits_per_word = spi->bits_per_word; > if (!xfer->speed_hz) > xfer->speed_hz = spi->max_speed_hz; > + if (xfer->tx_buf && !xfer->tx_nbits) > + xfer->tx_nbits = SPI_NBITS_SINGLE; > + if (xfer->rx_buf && !xfer->rx_nbits) > + xfer->rx_nbits = SPI_NBITS_SINGLE; > + /* check transfer tx/rx_nbits: > + * 1. keep the value is not out of single, dual and quad > + * 2. keep tx/rx_nbits is contained by mode in spi_device > + * 3. if SPI_3WIRE, tx/rx_nbits should be in single > + */ > + if (xfer->tx_nbits != SPI_NBITS_SINGLE && > + xfer->tx_nbits != SPI_NBITS_DUAL && > + xfer->tx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && > + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && > + !(spi->mode & SPI_TX_QUAD)) > + return -EINVAL; > + if ((spi->mode & SPI_3WIRE) && > + (xfer->tx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; > + /* check transfer rx_nbits */ > + if (xfer->rx_nbits != SPI_NBITS_SINGLE && > + xfer->rx_nbits != SPI_NBITS_DUAL && > + xfer->rx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && > + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && > + !(spi->mode & SPI_RX_QUAD)) > + return -EINVAL; > + if ((spi->mode & SPI_3WIRE) && > + (xfer->rx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; > } What a lot of code to check the transfer bits. It really needs this much? > @@ -511,6 +524,11 @@ struct spi_transfer { > dma_addr_t rx_dma; > > unsigned cs_change:1; > + u8 tx_nbits; > + u8 rx_nbits; > +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ > +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ > +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ These fields increase the size of a spi_transfer by 4 bytes. If you used bitfields instead it wouldn't increase the size at all since there are still 7 bits left after cs_change. ------------------------------------------------------------------------------ Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save! http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
2013/9/4 Trent Piepho <tpiepho@gmail.com>: > On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: >> fix the previous patch some mistake below: > > This isn't a very good commit message. "the previous patch" will have > no meaning in the kernel git repo. The rest of the message only > describes changes since a previous version of the patch and not the > actual patch in full. > >> >> + /* Device DUAL/QUAD mode */ >> + prop = of_get_property(nc, "spi-tx-nbits", &len); > > Why not use of_property_read_u32() here? > >> + if (!prop || len < sizeof(*prop)) { >> + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", >> + nc->full_name); >> + spi_dev_put(spi); >> + continue; > > So if no spi-tx-nbits property is supplied, the device is rejected and > the loop continues to the next device entry? This means ALL EXISTING > DEVICE TREES with SPI devices will be rejected, since none of them > have this new property! Was this patch tested at all with any system > before being accepted? > This part has been fixed in patch[commit id:a822e99c70f448c4068ea85bb195dac0b2eb3afe] >> + /* check mode to prevent that DUAL and QUAD set at the same time >> + */ >> + 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; > > This test can be done with fewer operations as: > if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || > (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { > > >> + } >> + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden >> + */ >> + if ((spi->mode & SPI_3WIRE) && (spi->mode & >> + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) >> + return -EINVAL; > > No dev_err message for this possibility? What's different than the > previous check that does produce a message? > OK, should be added. Thanks. >> /* help drivers fail *cleanly* when they need options >> * that aren't supported with their current master >> */ > Following this comment is the code: > bad_bits = spi->mode & ~spi->master->mode_bits; > if (bad_bits) { > dev_err(&spi->dev, "setup: unsupported mode bits %x\n", > bad_bits); > return -EINVAL; > } > > Won't this always trigger for anything that sets one of the dual or quad bits? > I can't get your idea clearly. Please provide more infomation. >> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) >> /** >> * Set transfer bits_per_word and max speed as spi device default if >> * it is not set for this transfer. >> + * Set transfer tx_nbits and rx_nbits as single transfer default >> + * (SPI_NBITS_SINGLE) if it is not set for this transfer. >> */ >> list_for_each_entry(xfer, &message->transfers, transfer_list) { >> if (!xfer->bits_per_word) >> xfer->bits_per_word = spi->bits_per_word; >> if (!xfer->speed_hz) >> xfer->speed_hz = spi->max_speed_hz; >> + if (xfer->tx_buf && !xfer->tx_nbits) >> + xfer->tx_nbits = SPI_NBITS_SINGLE; >> + if (xfer->rx_buf && !xfer->rx_nbits) >> + xfer->rx_nbits = SPI_NBITS_SINGLE; >> + /* check transfer tx/rx_nbits: >> + * 1. keep the value is not out of single, dual and quad >> + * 2. keep tx/rx_nbits is contained by mode in spi_device >> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >> + */ >> + if (xfer->tx_nbits != SPI_NBITS_SINGLE && >> + xfer->tx_nbits != SPI_NBITS_DUAL && >> + xfer->tx_nbits != SPI_NBITS_QUAD) >> + return -EINVAL; >> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && >> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) >> + return -EINVAL; >> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && >> + !(spi->mode & SPI_TX_QUAD)) >> + return -EINVAL; >> + if ((spi->mode & SPI_3WIRE) && >> + (xfer->tx_nbits != SPI_NBITS_SINGLE)) >> + return -EINVAL; >> + /* check transfer rx_nbits */ >> + if (xfer->rx_nbits != SPI_NBITS_SINGLE && >> + xfer->rx_nbits != SPI_NBITS_DUAL && >> + xfer->rx_nbits != SPI_NBITS_QUAD) >> + return -EINVAL; >> + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && >> + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) >> + return -EINVAL; >> + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && >> + !(spi->mode & SPI_RX_QUAD)) >> + return -EINVAL; >> + if ((spi->mode & SPI_3WIRE) && >> + (xfer->rx_nbits != SPI_NBITS_SINGLE)) >> + return -EINVAL; >> } > > What a lot of code to check the transfer bits. It really needs this much? > This part is also corrected in patch [commit id:db90a44177ac39fc22b2da5235b231fccdd4c673]. >> @@ -511,6 +524,11 @@ struct spi_transfer { >> dma_addr_t rx_dma; >> >> unsigned cs_change:1; >> + u8 tx_nbits; >> + u8 rx_nbits; >> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ > > These fields increase the size of a spi_transfer by 4 bytes. If you > used bitfields instead it wouldn't increase the size at all since > there are still 7 bits left after cs_change. Yes, it will increase the size by 4 bytes, but using bitfields here will make it logically weird. Bitfields may mean that really have the restriction of number of bits. If just for saving memory, then what about the other members such as bits_per_world, delay_usecs. ------------------------------------------------------------------------------ Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more! Discover the easy way to master current and previous Microsoft technologies and advance your career. Get an incredible 1,500+ hours of step-by-step tutorial videos with LearnDevNow. Subscribe today and save! http://pubads.g.doubleclick.net/gampad/clk?id=58040911&iu=/4140/ostg.clktrk
On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote: >>> dma_addr_t rx_dma; >>> >>> unsigned cs_change:1; >>> + u8 tx_nbits; >>> + u8 rx_nbits; >>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ >> >> These fields increase the size of a spi_transfer by 4 bytes. If you >> used bitfields instead it wouldn't increase the size at all since >> there are still 7 bits left after cs_change. > Yes, it will increase the size by 4 bytes, but using bitfields here > will make it logically weird. Bitfields may mean that really have the > restriction of number of bits. If just for saving memory, then what > about the other members such as bits_per_world, delay_usecs. Rather than just talk about it, I'll make patches to do it. Then it will be clear. On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote: > 2013/9/4 Trent Piepho <tpiepho@gmail.com>: >> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: >>> fix the previous patch some mistake below: >> >> This isn't a very good commit message. "the previous patch" will have >> no meaning in the kernel git repo. The rest of the message only >> describes changes since a previous version of the patch and not the >> actual patch in full. >> >>> >>> + /* Device DUAL/QUAD mode */ >>> + prop = of_get_property(nc, "spi-tx-nbits", &len); >> >> Why not use of_property_read_u32() here? >> >>> + if (!prop || len < sizeof(*prop)) { >>> + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", >>> + nc->full_name); >>> + spi_dev_put(spi); >>> + continue; >> >> So if no spi-tx-nbits property is supplied, the device is rejected and >> the loop continues to the next device entry? This means ALL EXISTING >> DEVICE TREES with SPI devices will be rejected, since none of them >> have this new property! Was this patch tested at all with any system >> before being accepted? >> > This part has been fixed in patch[commit > id:a822e99c70f448c4068ea85bb195dac0b2eb3afe] > >>> + /* check mode to prevent that DUAL and QUAD set at the same time >>> + */ >>> + 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; >> >> This test can be done with fewer operations as: >> if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || >> (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { >> >> >>> + } >>> + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden >>> + */ >>> + if ((spi->mode & SPI_3WIRE) && (spi->mode & >>> + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) >>> + return -EINVAL; >> >> No dev_err message for this possibility? What's different than the >> previous check that does produce a message? >> > OK, should be added. Thanks. > >>> /* help drivers fail *cleanly* when they need options >>> * that aren't supported with their current master >>> */ >> Following this comment is the code: >> bad_bits = spi->mode & ~spi->master->mode_bits; >> if (bad_bits) { >> dev_err(&spi->dev, "setup: unsupported mode bits %x\n", >> bad_bits); >> return -EINVAL; >> } >> >> Won't this always trigger for anything that sets one of the dual or quad bits? >> > I can't get your idea clearly. Please provide more infomation. > >>> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) >>> /** >>> * Set transfer bits_per_word and max speed as spi device default if >>> * it is not set for this transfer. >>> + * Set transfer tx_nbits and rx_nbits as single transfer default >>> + * (SPI_NBITS_SINGLE) if it is not set for this transfer. >>> */ >>> list_for_each_entry(xfer, &message->transfers, transfer_list) { >>> if (!xfer->bits_per_word) >>> xfer->bits_per_word = spi->bits_per_word; >>> if (!xfer->speed_hz) >>> xfer->speed_hz = spi->max_speed_hz; >>> + if (xfer->tx_buf && !xfer->tx_nbits) >>> + xfer->tx_nbits = SPI_NBITS_SINGLE; >>> + if (xfer->rx_buf && !xfer->rx_nbits) >>> + xfer->rx_nbits = SPI_NBITS_SINGLE; >>> + /* check transfer tx/rx_nbits: >>> + * 1. keep the value is not out of single, dual and quad >>> + * 2. keep tx/rx_nbits is contained by mode in spi_device >>> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >>> + */ >>> + if (xfer->tx_nbits != SPI_NBITS_SINGLE && >>> + xfer->tx_nbits != SPI_NBITS_DUAL && >>> + xfer->tx_nbits != SPI_NBITS_QUAD) >>> + return -EINVAL; >>> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && >>> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) >>> + return -EINVAL; >>> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && >>> + !(spi->mode & SPI_TX_QUAD)) >>> + return -EINVAL; >>> + if ((spi->mode & SPI_3WIRE) && >>> + (xfer->tx_nbits != SPI_NBITS_SINGLE)) >>> + return -EINVAL; >>> + /* check transfer rx_nbits */ >>> + if (xfer->rx_nbits != SPI_NBITS_SINGLE && >>> + xfer->rx_nbits != SPI_NBITS_DUAL && >>> + xfer->rx_nbits != SPI_NBITS_QUAD) >>> + return -EINVAL; >>> + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && >>> + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) >>> + return -EINVAL; >>> + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && >>> + !(spi->mode & SPI_RX_QUAD)) >>> + return -EINVAL; >>> + if ((spi->mode & SPI_3WIRE) && >>> + (xfer->rx_nbits != SPI_NBITS_SINGLE)) >>> + return -EINVAL; >>> } >> >> What a lot of code to check the transfer bits. It really needs this much? >> > This part is also corrected in patch [commit > id:db90a44177ac39fc22b2da5235b231fccdd4c673]. > >>> @@ -511,6 +524,11 @@ struct spi_transfer { >>> dma_addr_t rx_dma; >>> >>> unsigned cs_change:1; >>> + u8 tx_nbits; >>> + u8 rx_nbits; >>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ >> >> These fields increase the size of a spi_transfer by 4 bytes. If you >> used bitfields instead it wouldn't increase the size at all since >> there are still 7 bits left after cs_change. > Yes, it will increase the size by 4 bytes, but using bitfields here > will make it logically weird. Bitfields may mean that really have the > restriction of number of bits. If just for saving memory, then what > about the other members such as bits_per_world, delay_usecs. ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 004b10f..78f257a 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master) if (of_find_property(nc, "spi-3wire", NULL)) spi->mode |= SPI_3WIRE; + /* Device DUAL/QUAD mode */ + prop = of_get_property(nc, "spi-tx-nbits", &len); + if (!prop || len < sizeof(*prop)) { + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", + nc->full_name); + spi_dev_put(spi); + continue; + } + switch(be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_TX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_TX_QUAD; + break; + default: + dev_err(&master->dev, "spi-tx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; + } + + prop = of_get_property(nc, "spi-rx-nbits", &len); + if (!prop || len < sizeof(*prop)) { + dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n", + nc->full_name); + spi_dev_put(spi); + continue; + } + switch(be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_RX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_RX_QUAD; + break; + default: + dev_err(&master->dev, "spi-rx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; + } + /* Device speed */ prop = of_get_property(nc, "spi-max-frequency", &len); if (!prop || len < sizeof(*prop)) { @@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi) unsigned bad_bits; int status = 0; + /* check mode to prevent that DUAL and QUAD set at the same time + */ + 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; + } + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden + */ + if ((spi->mode & SPI_3WIRE) && (spi->mode & + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) + return -EINVAL; /* help drivers fail *cleanly* when they need options * that aren't supported with their current master */ @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) /** * Set transfer bits_per_word and max speed as spi device default if * it is not set for this transfer. + * Set transfer tx_nbits and rx_nbits as single transfer default + * (SPI_NBITS_SINGLE) if it is not set for this transfer. */ list_for_each_entry(xfer, &message->transfers, transfer_list) { if (!xfer->bits_per_word) xfer->bits_per_word = spi->bits_per_word; if (!xfer->speed_hz) xfer->speed_hz = spi->max_speed_hz; + if (xfer->tx_buf && !xfer->tx_nbits) + xfer->tx_nbits = SPI_NBITS_SINGLE; + if (xfer->rx_buf && !xfer->rx_nbits) + xfer->rx_nbits = SPI_NBITS_SINGLE; + /* check transfer tx/rx_nbits: + * 1. keep the value is not out of single, dual and quad + * 2. keep tx/rx_nbits is contained by mode in spi_device + * 3. if SPI_3WIRE, tx/rx_nbits should be in single + */ + if (xfer->tx_nbits != SPI_NBITS_SINGLE && + xfer->tx_nbits != SPI_NBITS_DUAL && + xfer->tx_nbits != SPI_NBITS_QUAD) + return -EINVAL; + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) + return -EINVAL; + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && + !(spi->mode & SPI_TX_QUAD)) + return -EINVAL; + if ((spi->mode & SPI_3WIRE) && + (xfer->tx_nbits != SPI_NBITS_SINGLE)) + return -EINVAL; + /* check transfer rx_nbits */ + if (xfer->rx_nbits != SPI_NBITS_SINGLE && + xfer->rx_nbits != SPI_NBITS_DUAL && + xfer->rx_nbits != SPI_NBITS_QUAD) + return -EINVAL; + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) + return -EINVAL; + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && + !(spi->mode & SPI_RX_QUAD)) + return -EINVAL; + if ((spi->mode & SPI_3WIRE) && + (xfer->rx_nbits != SPI_NBITS_SINGLE)) + return -EINVAL; } message->spi = spi; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 38c2b92..3dc0a86 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,10 @@ 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. If 0 the default + * (SPI_NBITS_SINGLE) is used. + * @rx_nbits: number of bits used for reading. If 0 the default + * (SPI_NBITS_SINGLE) is used. * @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 +499,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 +524,11 @@ struct spi_transfer { dma_addr_t rx_dma; unsigned cs_change:1; + u8 tx_nbits; + u8 rx_nbits; +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ u8 bits_per_word; u16 delay_usecs; u32 speed_hz; @@ -858,7 +876,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
fix the previous patch some mistake below: 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the previous way to get the property in @of_register_spi_devices(). 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. 3. Add the following check (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the single, dual and quad. (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set to QUAD(SPI_NBITS_QUAD) (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be in single(SPI_NBITS_SINGLE) Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> --- drivers/spi/spi.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/spi/spi.h | 22 ++++++++++- 2 files changed, 116 insertions(+), 2 deletions(-)